-
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
cmd/dist: remove precompiled .a files from binary distributions [freeze exception] #47257
Comments
Another reason to do this is that internet speeds vary wildly. To some people, downloading an extra 50MiB is a split second, but to many others it's staring at their screen for an extra minute. I don't have an ETA for FTTH reaching my building in the UK, for example :) CPU speeds and compile times also vary, but I think the lower bound is much less worrying - even with a five-year-old thin laptop, one should be able to build the standard library in 10-20s. |
I think it's pretty important that a Go binary installation work on a system with no C compiler installed. And at least in the past on macOS Go programs would only work if using the cgo version of the net package. So I think we at least need to provide the .a files for the standard library packages that use cgo, which I think is currently net and os/user. |
Does that imply that on macOS it already isn't possible to build a working Go program that depends on If so, that would imply that without a working C compiler it also isn't possible to build programs that depend on both
I think we could resolve that in one of three ways:
|
On further consideration, I don't think option (2) above is viable. There are too many other ways to invalidate cache entries, such as by setting (or not setting) And I believe our builders already invalidate the cache for libraries bundled in the macOS release, because they build using a non-default value for |
I would agree with you a priori, but as I understand the current state of the world, the introduction of the build cache broke this case many releases back, with no complaints (or at least not enough that we've tried to fix it). The .a files that ship do not actually have the right cache keys embedded inside them to be used by essentially any out-of-the-box install of Go. Instead, they get recomputed (in the cache only, not in the install location) the first time you build something. Assuming I am right about that (I have not done the experiment myself), then I think it is OK to just drop the .a files entirely and not worry about the "cgo without C compiler" case anymore. |
@rsc I was under this impression too until looking at #47215 last week. The C compiler version only became part of the cache key in #40042. So this is true for 1.17rc1, but not for 1.16 or lower. If neither CC nor CGO_CFLAGS are explicitly set, the go command will happily use the precompiled (@bcmills pointed out cases where the cache keys don't match on macOS, but they do match on Linux, and that's the platform I'm most worried about, since a lot of folks are building in Docker without installing a C compiler). |
I wonder how viable this is? I know very little about the guts of the But does the cgo version actually need to be written with cgo? On macOS at least, we link dynamically with |
@jayconrod I think you are correct that with our current implementation we could call the macOS libraries directly without having to use cgo. The same is true on AIX and Solaris, for that matter. |
re CGO_ENABLED=0 GOOS=darwin: #12524 (comment) |
CGO_ENABLED=0 is effectively the same as cross-compiled, and same headaches: netdns's go is simply inadequate, at least for use cases where DNS queries are routed based on domain name to different resolvers by the operating system. /etc/resolv.conf does not adequately describe the OS DNS routing behavior; there's no good way for netdns to ever do this correctly. |
I think we can plausibly check in the relevant cgo-generated pieces for the few stdlib packages that need cgo. |
My suggestion here would be to define that |
Agreed that I think the main technical blocker is being able to build For a broad |
Yes, buildmode=shared is dead and has been for a long time. Anyone using it must not be using modules. /cc @mwhudson |
This proposal has been added to the active column of the proposals project |
@mwhudson, do you know of any existing uses of -buildmode=shared and -linkshared anymore? |
I'm not aware of any used for those buildmodes currently, no. Never quite got to the point of them being genuinely useful before our requirements shifted a bit unfortunately. |
Based on the discussion above, this proposal seems like a likely accept. |
Would this removal leave any method of installing .a files aside from (Context: For reproducible builds we often use isolated build environments where the build cache cannot be persisted between builds. Options for reusing build artifacts currently are, in order of ease:
My concern is that this change would remove option 1, without adding any facility for making 2 or 3 less brittle.) |
…r all of std gcimporter.TestImportTypeparamTests still needs to create full export data because it loads lots of source files from GOROOT/test that expect to be able to import arbitrary subsets of the standard library, so we now skip it in short mode. On a clean build cache, this reduces 'go test -short cmd/compile/internal/importer go/internal/gcimporter' on my machine from 21–28s per test to <6s per test. Updates #56967. Updates #47257. Change-Id: I8fd80293ab135e0d2d213529b74e0ca6429cdfc7 Reviewed-on: https://go-review.googlesource.com/c/go/+/454498 Run-TryBot: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
This fixes tests failing on the Android builders on Go 1.20 after CL 454499. Previously the tests were skipped in the 'compile' helper function, but as of that CL they fail before reaching that point due to missing export data for packages in std. Updates golang/go#56967. Updates golang/go#47257. Change-Id: Ief953b6dbc54c8e0b1f71fc18a0d6ab212caf308 Reviewed-on: https://go-review.googlesource.com/c/tools/+/454500 gopls-CI: kokoro <[email protected]> Reviewed-by: Jamal Carvalho <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
Change https://go.dev/cl/461676 mentions this issue: |
Change https://go.dev/cl/462636 mentions this issue: |
Change https://go.dev/cl/462637 mentions this issue: |
Change https://go.dev/cl/462638 mentions this issue: |
…dLib The test attempted to find all stdlib packages by scanning pkg/$GOOS_$GOARCH for .a files and then tried to import all of them. Now that .a files are no longer being placed there, the test is a noop. Fix this by using go list std (and filtering out testonly packages) and trying to import all of those to recreate what the test intended to do. This also removes a dependency on the pkg/$GOOS_$GOARCH directory which will stop being produced by dist in CL 453496. Updates golang/go#47257 Change-Id: Idfa0cbb21093776183ce193eb5363a9727bf77ef Reviewed-on: https://go-review.googlesource.com/c/tools/+/454118 Run-TryBot: Michael Matloob <[email protected]> Reviewed-by: Michael Matloob <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Reviewed-by: Bryan Mills <[email protected]> (cherry picked from commit 0379b73) Reviewed-on: https://go-review.googlesource.com/c/tools/+/462636 Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Russ Cox <[email protected]>
…rt data for packages individually In short tests, also avoid creating export data for all of std. This change applies the same improvements made to the equivalent std packages in CL 454497 and CL 454498. (It may even fix the reverse builders that are currently timing out in x/tools, although I suspect there is still a bit of work to do for those.) Updates golang/go#56967. Updates golang/go#47257. Change-Id: I82e72557da5f917203637513122932c7942a98e9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/454499 Auto-Submit: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Run-TryBot: Bryan Mills <[email protected]> gopls-CI: kokoro <[email protected]> (cherry picked from commit f540ee6) Reviewed-on: https://go-review.googlesource.com/c/tools/+/462637 Reviewed-by: Russ Cox <[email protected]>
…er when 'go build' is not available This fixes tests failing on the Android builders on Go 1.20 after CL 454499. Previously the tests were skipped in the 'compile' helper function, but as of that CL they fail before reaching that point due to missing export data for packages in std. Updates golang/go#56967. Updates golang/go#47257. Change-Id: Ief953b6dbc54c8e0b1f71fc18a0d6ab212caf308 Reviewed-on: https://go-review.googlesource.com/c/tools/+/454500 gopls-CI: kokoro <[email protected]> Reviewed-by: Jamal Carvalho <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]> (cherry picked from commit bdcd082) Reviewed-on: https://go-review.googlesource.com/c/tools/+/462638 Reviewed-by: Russ Cox <[email protected]>
Since packages in "std" no longer have install targets, checking them for staleness is somewhat meaningless: if they are not cached they will be rebuilt anyway, and without installed archives against which we can compare them the staleness check will not detect builder skew. It would still be meaningful to check "cmd" for staleness, but (especially on sharded VM-based builders) that is a fairly expensive operation relative to its benefit. If we are really interested in detecting builder skew and/or build reproducibility, we could instead add a "misc" test (similar to "misc/reboot", or perhaps even a part of that test) that verifies that bootstrapped binaries are reproducible. For #57734. Updates #47257. Updates #56896. Change-Id: I8683ee81aefe8fb59cce9484453df9729bdc587c Reviewed-on: https://go-review.googlesource.com/c/go/+/452775 Run-TryBot: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Austin Clements <[email protected]>
The purpose of building the host toolchain is so that we can use it to build and test the target configuration. The host configuration should already be tested separately (with its own builder), so we do not need to build the parts of that configuration that are not relevant to the task of building and testing the target configuration. Updates #47257. Change-Id: I814778d2d65b1f2887c9419232b5bfd4068f58af Reviewed-on: https://go-review.googlesource.com/c/go/+/461676 Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Austin Clements <[email protected]>
Change https://go.dev/cl/464736 mentions this issue: |
Instead, have the caller pass in an explicit list of the packages (if any) they need. After #47257, a builder running a test does not necessarily have the entire standard library already cached, especially when running tests in sharded mode. testenv.WriteImportcfg used to write an importcfg for the entire standard library — which required rebuilding the entire standard library — even though most tests need only a tiny subset. This reduces the time to test internal/abi with a cold build cache on my workstation from ~16s to ~0.05s. It somewhat increases the time for 'go test go/internal/gcimporter' with a cold cache, from ~43s to ~54s, presumably due to decreased parallelism in rebuilding the standard library and increased overhead in re-resolving the import map. However, 'go test -short' running time remains stable (~5.5s before and after). Fixes #58248. Change-Id: I9be6b61ae6e28b75b53af85207c281bb93b9346f Reviewed-on: https://go-review.googlesource.com/c/go/+/464736 Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Cherry Mui <[email protected]> Reviewed-by: Than McIntosh <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
Since packages in "std" no longer have install targets, checking them for staleness is somewhat meaningless: if they are not cached they will be rebuilt anyway, and without installed archives against which we can compare them the staleness check will not detect builder skew. It would still be meaningful to check "cmd" for staleness, but (especially on sharded VM-based builders) that is a fairly expensive operation relative to its benefit. If we are really interested in detecting builder skew and/or build reproducibility, we could instead add a "misc" test (similar to "misc/reboot", or perhaps even a part of that test) that verifies that bootstrapped binaries are reproducible. For golang#57734. Updates golang#47257. Updates golang#56896. Change-Id: I8683ee81aefe8fb59cce9484453df9729bdc587c Reviewed-on: https://go-review.googlesource.com/c/go/+/452775 Run-TryBot: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Austin Clements <[email protected]>
The purpose of building the host toolchain is so that we can use it to build and test the target configuration. The host configuration should already be tested separately (with its own builder), so we do not need to build the parts of that configuration that are not relevant to the task of building and testing the target configuration. Updates golang#47257. Change-Id: I814778d2d65b1f2887c9419232b5bfd4068f58af Reviewed-on: https://go-review.googlesource.com/c/go/+/461676 Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Austin Clements <[email protected]>
Instead, have the caller pass in an explicit list of the packages (if any) they need. After golang#47257, a builder running a test does not necessarily have the entire standard library already cached, especially when running tests in sharded mode. testenv.WriteImportcfg used to write an importcfg for the entire standard library — which required rebuilding the entire standard library — even though most tests need only a tiny subset. This reduces the time to test internal/abi with a cold build cache on my workstation from ~16s to ~0.05s. It somewhat increases the time for 'go test go/internal/gcimporter' with a cold cache, from ~43s to ~54s, presumably due to decreased parallelism in rebuilding the standard library and increased overhead in re-resolving the import map. However, 'go test -short' running time remains stable (~5.5s before and after). Fixes golang#58248. Change-Id: I9be6b61ae6e28b75b53af85207c281bb93b9346f Reviewed-on: https://go-review.googlesource.com/c/go/+/464736 Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Cherry Mui <[email protected]> Reviewed-by: Than McIntosh <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
…l compile' does not exist Now that .a files are no longer being preinstalled for standard-library packages (#47257), tests that require the export data from those files must have access to 'go list -export' in order to generate and load export data from the Go build cache. `go list -export` does not work without a Go compiler, so assume that tests that need the 'go' command also need the compiler. This may cause the tests currently failing on the android-.*-emu builders to instead be skipped, since the test harness does not copy the compiler to the execution environment. For golang/go#47257. Change-Id: Ie82ab09ac8165a01fc1d3a083b1e86226efc469d Reviewed-on: https://go-review.googlesource.com/c/tools/+/451597 TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Michael Matloob <[email protected]> gopls-CI: kokoro <[email protected]>
1.19.10 includes a change on linked libraries that's messing up our manager binary see golang/go#47257 and golang/go#57328 (issues are closed, but the error is the same)
The downloadable archives at https://golang.org/dl/ currently contain precompiled .a files for all packages in the standard library. Each archive has a set of .a files for one platform, plus another set for
-race
builds.These files take up quite a bit of space, and they're fast to rebuild on demand. We should consider removing them from binary Go distributions.
For example, in 1.17rc1 on darwin/amd64, the whole distribution uncompressed is 435M. The
pkg/darwin_amd64
directory is 97M (22%), and thepkg/darwin_amd64_race
directory is 109M (25%). Compressed as a zip file with default settings, the archive is 135M. Without .a files, it's 86M (63%).After #40042 was fixed, the C compiler version is included in the cache key for each package that uses cgo. That means that if no C compiler is installed on the system, or if a different C compiler version is installed (very common),
go build
and other commands will rebuild packages that depend on cgo instead of using the versions installed in$GOROOT/pkg
. As of 1.17rc1, there are 27 packages instd
that use cgo directly or indirectly, most prominently,net
. The precompiled files for these packages are almost never used unless the installed C compiler exactly matches the version used to build the Go distribution.Note that the fix for #40042 is causing builds to fail on systems without a C compiler installed (#47215), so it may be partially or completely rolled back in 1.17. If we implement this proposal, we'd have the same problem, so we may want to think about changing the default value of
CGO_ENABLED
(#47251).cc @rsc @bcmills @matloob
The text was updated successfully, but these errors were encountered: