-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
all: Alpine Linux ships modified Go binaries #62053
Comments
Change https://go.dev/cl/520057 mentions this issue: |
Change https://go.dev/cl/520056 mentions this issue: |
Change https://go.dev/cl/520055 mentions this issue: |
There are also several patches in the Alpine port working around test failures that, to my knowledge, were never filed as issues in the Go project. 😞 I would really appreciate it if folks would at least report these kinds of issues here so that we can fix them for everybody, rather than adding a one-off patch specific to one distribution. |
Hmm. On a closer look, I did find a few related issues:
|
For completeness, one of the patches does have a corresponding issue explicitly mentioning the impact on Alpine: (But it appears that patch was deleted as of Go 1.20?) |
I don't understand the rationale given in https://git.alpinelinux.org/aports/commit/community/go/tests-unset-GCCGO.patch?id=a10e9a5e48507198e26a8cf19709e4059da4c79f, but I suspect that it may be working around test failures when cross-compiling, since we have a lot of other gccgo tests that need to skip in that circumstance. Alternatively, that may just be a stale patch working around #53815. I can't fine any issue filed against the Go project for this patch, so it's hard to be sure. Either way, adding this skip should make the test more robust. For #62053. Change-Id: I44dbe9a5a24c0e2d3f22fbe6ca995160a36b2606 Reviewed-on: https://go-review.googlesource.com/c/go/+/520056 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Run-TryBot: Bryan Mills <[email protected]>
… syscall.Unshare syscall.Unshare is the sort of system call that may be blocked in a container environment, and experience has shown that different container implementations choose from a variety of different error codes for blocked syscalls. In particular, the patch in https://git.alpinelinux.org/aports/tree/community/go/tests-unshare-enosys.patch seems to suggest that the container environment used to test the Go distribution on Alpine Linux returns ENOSYS instead of EPERM. The existing testenv.SyscallIsNotSupported helper checks for the kinds of error codes we have seen from containers in practice, so let's use that here. For #62053. Updates #29366. Change-Id: Ic6755f7224fcdc0cb8b25dde2d6047ceb5c3ffdf Reviewed-on: https://go-review.googlesource.com/c/go/+/520057 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Run-TryBot: Bryan Mills <[email protected]>
… errors This addresses the failure mode described in https://git.alpinelinux.org/aports/commit/community/go/tests-filter-overflow-gid.patch?id=9851dde0f5d2a5a50f7f3b5323d1b2ff22e1d028, but without special-casing an implementation-specific group ID. For #62053. Change-Id: I70b1046837b8146889fff7085497213349cd2bf0 Reviewed-on: https://go-review.googlesource.com/c/go/+/520055 Reviewed-by: Ian Lance Taylor <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
Generally speaking, we do need to patch Go occasionally as Go releases break every now and then on musl architectures which are AFAIK not covered by your upstream CI (musl-s390x, musl-i386, musl-riscv64, musl-ppc64le, …). See for example #51787, #58385, #52336, #52337, et cetera. Maybe it makes sense to add support for more musl targets to the Golang CI? I will elaborate further on specific questions below.
Unfortunately, there is not really a portable way to detect this AFAIK. For clang/gcc the preferred linker is IIRC determined when clang/gcc itself is compiled so this is not an issue (CC: @ncopa). Prior to 3315066 and #54197 Go used to do this as well I think. From the top of my head, I can't think of an upstreamable solution right now which is also why I never raised this issue upstream.
I wasn't aware that Go ≥1.21.0 binaries are fully statically linked. I only tested this feature with older Go versions (i.e. Go 1.20) and for those, it doesn't work, as you rightly pointed out. However, Alpine is generally wary of software that downloads or relies on pre-built binaries, so this might warrant further discussion on the Alpine side. See also: a prior related discussion regarding Nonetheless, thank you for bringing this to my attention; I will follow up on this! |
Regarding the general comment made by @bcmills:
I always tried to open Go upstream issues for our patches whenever I was of the opinion that the issue could/should be fixed upstream. As such, there are several musl-compatibility issues in the Go bug tracker that have been reported by me, and in the same spirit, I also upstreamed our entire gccgo patchset earlier this year. In fact, I try to do this for every package I maintain downstream. It's simply not true that we are not opening upstream issues for test failures. Also keep in mind that this is a lot of work that downstream packagers are usually not paid for and hence perform in their free time. Furthermore, my experience with Go upstream regarding musl compatibility issues hasn't necessarily been positive. Last year, I was basically told that musl is not a supported platform, which is not exactly encouraging. |
@nmeum in response to #51787 (comment), I don't remember the order of events so I can't say whether that comment was true at the time, but it's not true now - we do have a linux-amd64-alpine builder. You can see it on https://build.golang.org/ |
@nmeum what do you think about the Go linker defaulting on Linux to the dynamic linker of /bin/sh (assuming it is dynamically linked)? That seems like a reasonable way to answer "what is the dynamic linker on this system?" and it would correctly work on Alpine and Ubuntu. |
@nmeum regarding defaults for downloading new toolchains, we completely understand Alpine wanting to set different defaults for GOPROXY, GOSUMDB, and GOTOOLCHAIN, which is why we moved them into go.env: that way they can be changed in "source" instead of in "binary": anyone verifying the Alpine Go builds will see the changed go.env and ideally see unchanged binaries. The 0004 patch takes care of setting GOTOOLCHAIN=local in go.env to set the default, and we do not object to that at all. That's Alpine's prerogative, full stop. (You may want to consider GOTOOLCHAIN=path instead, but I completely understand wanting a default that's not GOTOOLCHAIN=auto.) The 0003 patch, on the other hand, disables downloads even when the user has tried to enable them by explicitly setting GOTOOLCHAIN=auto. That seems very unfortunate. If a user explicitly wants to opt back in, they should be able to do that without having to install a standard Go toolchain first. If the 0003 patch didn't mean to target GOTOOLCHAIN=auto and only wanted to target GOTOOLCHAIN=go1.20 (which does not work on Alpine because Go 1.20 and earlier didn't run on Alpine out of the box), then I would be willing to have the toolchain download check for /etc/alpine-release to detect being on Alpine (or maybe ldd /bin/sh to detect musl) and print a nice error instead of downloading and failing to run those earlier toolchains. |
The patch which forcefully sets See: https://git.alpinelinux.org/aports/commit/?id=f21344fec95c820d9d1945e6bee27678268472f8 |
I suppose that could work on most systems, though there are some edge cases where it doesn't (e.g. statically linked
Originally, it was explicitly intended to prevent users from setting
I am aware that this is the case nowadays, keep in mind though that Alpine supports architectures that are not covered by your CI, see the first paragraph of my previous comment in #62053 (comment). |
One more thing that came to mind regarding the general goal of having “the binaries we post on go.dev/dl match the binaries that other distributions post after building from source”: Alpine builds Go itself with |
It's likely that these deserve other issues depending on what the overall goal is, but I'll note that Alpine/Ubuntu are not the only major distributions to be shipping binaries that aren't going to match those from
Apologizes if these are already known, of course. This isn't exhaustive, just what was easily searchable (mostly via https://pkgs.org/). (Unrelated, but I have also noticed quite a few distros which are still doing things that are no longer useful on modern versions of Go, e.g. prebuilding std and so on; IIRC that stuff all goes into the cache. Also a lot of CC/CXX setting to preset those, which I also think doesn't do anything anymore?) |
This has been a main blocker (at least for me) in using Go packaged in Nixpkgs since compiled binaries (not Go itself but user programs) cannot be reproduced outside of the Nixpkgs environment due to the patches applied to the standard library. See also NixOS/nixpkgs#125198 Perhaps we can add variables that can be set via |
[#187010312](https://www.pivotaltracker.com/story/show/187010312) `golang:latest` is based on debian instead of alpine. Since we were using golang:1.22.0-alpine before we should keep using latest version of alpine `golang:alpine`. This Dockerfile definitely needs Alpine since we perform some APK operations: #957 Also, we must keep in mind that there might be differences in the golang binaries, since Alpine binaries tend to be statically linked against MUSL instead of glibc: golang/go#62053 We might want to keep an eye on this topic to ensure our binaries are always built in a consistent way. Thank you very much @zucchinidev 🥇 for noticing this error and letting me know :) Keep up the good work! Co-authored-by: Andrea Zucchini <[email protected]>
[#187010312](https://www.pivotaltracker.com/story/show/187010312) `golang:latest` is based on debian instead of alpine. Since we were using golang:1.22.0-alpine before we should keep using latest version of alpine `golang:alpine`. This Dockerfile definitely needs Alpine since we perform some APK operations: #957 Also, we must keep in mind that there might be differences in the golang binaries, since Alpine binaries tend to be statically linked against MUSL instead of glibc: golang/go#62053 We might want to keep an eye on this topic to ensure our binaries are always built in a consistent way. Thank you very much @zucchinidev 🥇 for noticing this error and letting me know :) Keep up the good work! Co-authored-by: Andrea Zucchini <[email protected]>
* chore: pin golang image to latest [#187010312](https://www.pivotaltracker.com/story/show/187010312) * fix: pin golang alpine instead of latest [#187010312](https://www.pivotaltracker.com/story/show/187010312) `golang:latest` is based on debian instead of alpine. Since we were using golang:1.22.0-alpine before we should keep using latest version of alpine `golang:alpine`. This Dockerfile definitely needs Alpine since we perform some APK operations: #957 Also, we must keep in mind that there might be differences in the golang binaries, since Alpine binaries tend to be statically linked against MUSL instead of glibc: golang/go#62053 We might want to keep an eye on this topic to ensure our binaries are always built in a consistent way. Thank you very much @zucchinidev 🥇 for noticing this error and letting me know :) Keep up the good work! Co-authored-by: Andrea Zucchini <[email protected]> * fix: allow downloading go toolchains on-the-fly [#187010312](https://www.pivotaltracker.com/story/show/187010312) Golang Docker images come with `GOTOOLCHAIN=local` by default which prevents downloading toolchains on-the-fly. This would cause our CI to fail for a few hours when a new go version is released but golang tags are not yet up-to-date with it. https://github.com/search?q=repo%3Adocker-library%2Fgolang%20GOTOOLCHAIN&type=code --------- Co-authored-by: Andrea Zucchini <[email protected]>
One of the goals of Go 1.21's reproducible builds effort was to have the binaries we post on go.dev/dl match the binaries that other distributions post after building from source. Of course, to make that the case, the other distributions need to not patch the source.
For Go 1.21, we incorporated the various minor fixes that Ubuntu had accumulated, with the effect that Ubuntu's debs, while not exactly the same binaries, are easily reproduced. Specifically, the recipe is to build with GO386=softfloat and then run 'strip' on the binaries. For Go 1.22 it would be nice to make go build -ldflags=-s have the same effect as strip on ELF systems, so that
GO386=softfloat GO_LDFLAGS=-s ./make.bash -distpack
would reproduce the Ubuntu binaries, but even today, we have a trivial reproduction.For Go 1.21, we also arranged that binaries built on Alpine would automatically use musl as the dynamic linker, and we arranged for the Go toolchain itself to not use dynamic binaries, so that the posted binaries would work unmodified on Alpine. Unfortunately, Alpine is still patching the source code of the Go toolchain, and so their binaries do not match the official ones. Specifically, they make two changes that affect the binaries:
0001-cmd-link-prefer-musl-s-over-glibc-s-ld.so-during-dyn.patch changes the preference list for what happens when running on a system with both the glibc and the musl dynamic linker installed. In this case the Go linker has a choice, and the default is to choose glibc, but Alpine wants Go to choose musl.
@nmeum, is there some standard config file on the system, perhaps some file in etc, that indicates that musl should be preferred over glibc? How does the clang or gcc toolchain on Alpine know? If there is some standard file, the Go linker could look at it too in Go 1.22. That would allow removing this patch.
0003-Prevent-Go-from-downloading-other-toolchain-versions.patch is hard-coding
pathOnly = true
, overriding the user's own environment settings. This is unnecessary and limits Go's functionality on Alpine. The commentary says:but this is not true: starting with Go 1.21.0, the toolchain binaries are statically linked pure Go programs, with no references to glibc, and so they should work fine on Alpine. (If this is not true I'd like to fix that.) It's true that downloading older toolchains won't work, but the way toolchain downloads work, automatic downloads only ever fetch newer toolchains. The incompatibility would only arise with explicit versions set in the environment like
It doesn't make sense to disable the entire feature just to avoid problems with this relatively uncommon use case. (Again, the most common use case by far will be downloading a newer toolchain, and those work.)
I understand that Alpine would prefer to set a system default, and we created the go.env for that purpose, and in fact https://git.alpinelinux.org/aports/tree/community/go/0004-go.env-Don-t-switch-Go-toolchain-version-as-directed.patch does exactly that, by changing the default from
GOTOOLCHAIN=auto
toGOTOOLCHAIN=local
. That seems perfectly reasonable as a policy choice, andgo.env
is the place to set policy. Making the change there allows users to override the policy decision when they have different choices, either by setting an explicit environment variable or usinggo env -w
.@nmeum, given the existence of the 0004 patch, can the 0003 patch be removed?
The text was updated successfully, but these errors were encountered: