Add support to detect ARM variant on Windows#2700
Conversation
jiria
force-pushed
the
jiria/add-windows-arm-support
branch
from
353fe30 to
a6f0596
Compare
|
Thanks for the pointer @estesp , I have amended the commit message based on the guidelines. |
jiria
force-pushed
the
jiria/add-windows-arm-support
branch
2 times, most recently
from
c6c4f4c to
ca985aa
Compare
Codecov Report
@@ Coverage Diff @@
## master #2700 +/- ##
==========================================
- Coverage 43.59% 43.54% -0.05%
==========================================
Files 101 101
Lines 10722 10733 +11
==========================================
Hits 4674 4674
- Misses 5314 5325 +11
Partials 734 734
Continue to review full report at Codecov.
|
|
@jterry75 FYI Please let me know if there is anything else you need from me to complete this PR. |
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
seems reasonable; just one question about the go linkage to runtime.goarm
There was a problem hiding this comment.
Does this linkage require a certain version of Go? I know the Windows ARM support was recently merged, but wasn't sure if this runtime.goarm predates these patches and has always been available when GOARCH == arm
There was a problem hiding this comment.
It seems to me that runtime.goarm has been in runtime/runtime2.go for at least two years.
@jordanrh1 in case he has some additional context.
There was a problem hiding this comment.
Correct, runtime.goarm has been around for a while. However, runtime.goarm does not necessarily match the instruction set of the CPU. GOARM could be less than the version of the CPU.
Windows only supports ARMv7 for arm32 platforms, and ARMv8 for aarch64. So if the architecture is "arm", you know the architecture version is at least v7, and if the architecture is "arm64", you know the architecture version is at least v8. You could use the following logic to avoid runtime.goarm.
switch runtime.GOARCH {
default:
variant = "unknown"
case "arm":
variant = "v7"
case "arm64":
variant = "v8"
}
There was a problem hiding this comment.
Updated based on recommendation of @jordanrh1 , our Go expert.
jiria
force-pushed
the
jiria/add-windows-arm-support
branch
from
ca985aa to
0f729f8
Compare
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
Two fairly minor things to resolve; otherwise looks good
There was a problem hiding this comment.
one final thing (from me): an early return at the end of the GOOS == windows section means the rest (all prior lines) can remain outside of a large if section.
ARM variant detection logic was authored originally with POSIX OS in mind. This change leaves POSIX code path unaltered and adds support to detect ARM variant on Windows by leveraging runtime.goarch. Signed-off-by: Jiri Appl <[email protected]>
jiria
force-pushed
the
jiria/add-windows-arm-support
branch
from
0f729f8 to
e6529f4
Compare
|
LGTM |
|
Thanks @estesp What needs to further happen in order for this PR to be completed? And second question, what needs to happen to get containerd revendored into moby? |
|
LGTM |
5 participants
Opening PR as per #2693.
We are working on enabling support for ARM on Windows. As part of that, we have hit an issue when getCPUVariant gets called. getCPUVariant calls into getCPUInfo which expects Linux OS and as such it fails for Windows. This change uses GOARM global for Windows and Linux path is unaltered.
@estesp FYI