platform: infer CPU variant from arch on non-Linux OSes - #1070
Conversation
d1be9be to
29a245d
Compare
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
I think this is fine in principle, but we shouldn’t have functions with misleading names. That goes both for (newly) getCPUVariantDarwinWindows and for already-misleading getCPUVariantArm (not highlighting that it is Linux-specific).
2731abc to
b963a37
Compare
| func getCPUVariantDarwinWindows(arch string) string { | ||
| func getCPUVariantFromArch(arch string) string { | ||
| // Darwin and Windows only support v7 for ARM32 and v8 for ARM64 and so we can use | ||
| // runtime.GOARCH to determine the variants |
There was a problem hiding this comment.
This comment (or behavior?) also needs updating … somehow.
E.g. https://wiki.freebsd.org/arm suggests that ARMv6 is still supported, and for hypothetical other OSes we have no idea.
I suppose the major concern is that values for the larger-share OSes are not changing; and ARM ≤v6 will go away over time. so I’m not too worried, but, still…
One option might be to make this OS-conditional on 32-bit. Another might be to just punt and document that we don’t know and are guessing.
There was a problem hiding this comment.
Yeah, good point!
Worth noting: v6 is Tier 3 on FreeBSD 14.x and not supported at all on 15+, and v7 is Tier 2 with no packages built for it (https://www.freebsd.org/platforms/).
So in practice there's no FreeBSD user who could actually run a pkg-based podman workload on anything below v7 that we'd be mis-detecting. Happy to still make the 32-bit case OS-conditional for correctness, or just document the assumption in the comment -- which would you prefer?
There was a problem hiding this comment.
Thanks for that context! In that case either is fine — and the comment is simpler.
There was a problem hiding this comment.
Went with the simpler comment-only version, pushed
Signed-off-by: Michael Johnson <buhnux@gmail.com>
b963a37 to
8077629
Compare
2 participants
On FreeBSD, this falls through to a Linux-only /proc/cpuinfo check on arm64 and logs this error immediately, without ever attempting the read:
Couldn't get cpu architecture: getCPUInfo for OS freebsd not implementedI initially considered a narrower
|| os == "freebsd"check, but I believe linux is the only OS this detection method actually works for.