Add arm64 support & fixes for running on /usr-merged/Fedora systems #129
Conversation
This adds support for arm64 host systems, removing the previously present build flags & adding some architecture detection in Machine instead. Signed-off-by: Ryan Gonzalez <[email protected]>
This lets callers distinguish the type of error that occurred. Signed-off-by: Ryan Gonzalez <[email protected]>
These were resulting in the creation of /bin & /lib folderr, which broke
the symlinks to /usr/{bin,lib}.
Signed-off-by: Ryan Gonzalez <[email protected]>
Busybox on Fedora is now "statically" linked to musl but still requires the dynamic linker. Signed-off-by: Ryan Gonzalez <[email protected]>
- SSL certs on Fedora is in /etc/pki - gnutls config used by wget is in /etc/crypto-policies Signed-off-by: Ryan Gonzalez <[email protected]>
| /* Amd64 dynamic linker */ | ||
| err = w.CopyFile("/lib64/ld-linux-x86-64.so.2") | ||
| var dynamicLinker string | ||
| if m.arch == Arm64 { |
There was a problem hiding this comment.
would it be worth adding a table of arch vs. path for these, and erroring out if it's not a supported arch (like we do for Qemu path for instance) ?
| @@ -187,6 +197,14 @@ func (b qemuBackend) StartQemu(kvm bool) (bool, error) { | |||
| "-enable-kvm") | |||
| } | |||
|
|
|||
| if m.arch == Arm64 { | |||
There was a problem hiding this comment.
did you test this with both qemu and kvm backends ?
| err = w.CopyFile(libraryDir + "/libc.so.6") | ||
| if err != nil && errors.Is(err, os.ErrNotExist) && m.arch == Arm64 && strings.HasSuffix(libraryDir, "/lib") { |
There was a problem hiding this comment.
would it be worth making the check for arch more clear ?
or even, do we need the arch check here ?
There was a problem hiding this comment.
This needs a more generic fix to get this to work on fedora.
The overall assumption this code makes is that the main library directory matches the real location of the dynamic linker.
This is true for debian where the canonical linker locations are e.g. "/lib/ld-linux-aarch64.so.1" or "/lib64/ld-linux-x86-64.so.2" are symlinks to "
aarch64-linux-gnu/ld-linux-aarch64.so.1" and "/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2" respectively..
It also works for arch as it only seems to use /lib64 on amd64 where it's just a symlink to /usr/lib anyway.
So fedora is the odd one out here in that it uses /usr/lib64 on arm64 as well and having it's dynamic linkers directly in their canonical location.. Which causes the failure on arm64 as the linker in lib but everything else in lib64
TL;DR to cover fedora (and the likes) properly the way we determine libraryDir should change/be expanded, and probably broken out to an helper, rather then patching it up after the fact
| if err != nil { | ||
| return -1, err | ||
| } | ||
|
|
||
| // Fedora's busybox needs musl |
There was a problem hiding this comment.
nit: keep comment style of /* */
also worth saying that muscl is optional here, it's not really clear from a first look :-)
|
It'd also be great to add tests for Fedora on amd64. See go-debos/test-containers#9 for more information. I don't think we can do tests on arm64, but we could at least crossbuild for that arch? |
| //go:build linux && amd64 | ||
| // +build linux,amd64 | ||
| //go:build linux | ||
| // +build linux |
There was a problem hiding this comment.
I'm really happy to see arm64 support. Thank you. Did you try it with Fedora only or did you also give Debian/Arch a try?
Small nit: Personally, I would add the new/supported arch instead of removing the existing restriction.
With the proposed change, one can trivially build binaries that are known broken. Which in itself is poor user experience IMHO.
There was a problem hiding this comment.
I think that's fine as we could also add support for currently unsupported archs ;-) - as long the error is explicit at runtime I am happy.
| @@ -173,7 +183,7 @@ func (b qemuBackend) StartQemu(kvm bool) (bool, error) { | |||
| } | |||
| memory := fmt.Sprintf("%d", m.memory) | |||
| numcpus := fmt.Sprintf("%d", m.numcpus) | |||
| qemuargs := []string{"qemu-system-x86_64", | |||
| qemuargs := []string{qemuBinaryNames[m.arch], | |||
There was a problem hiding this comment.
It'd also be great to get a CI test for this by creating a fedora test container and hooking that into CI: go-debos/test-containers#9
|
Fwiw did #157 based on heavily refactoring your first patch to make it more generic and fully based on a per-architecture configuration rather then in-code exceptions (apart from the qemu binary).. Also compared to this has a working Would still be worth to rebase and do just the fedora specific tweaks |
| // Mounts for gnutls (used by wget). | ||
| if _, err := os.Stat("/etc/crypto-policies"); err == nil { |
There was a problem hiding this comment.
crypto-policies is not just for gnutls it's the centralized policies for all crypto :)
|
given how long this has been stale and now conflicting i'll close it for now; @refi64 please feel free to re-open once you get back to it :) |
For the arm64 support, this uses a very different approach from #30, but still generalizes things enough to add support for other architectures in the future. Tested to build an Apertis image from a Fedora host.