Skip to content
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

Why go install std? #407

Closed
rittneje opened this issue Feb 13, 2022 · 6 comments · Fixed by #408
Closed

Why go install std? #407

rittneje opened this issue Feb 13, 2022 · 6 comments · Fixed by #408

Comments

@rittneje
Copy link

The Dockerfile currently runs go install std, because "the official binary release tarballs do".

# pre-compile the standard library, just like the official binary release tarballs do
go install std; \

What exactly is the point of this? The make.bash script will already compile the standard library packages.

It could be useful to precompile with -race, but this is unconditionally skipped right now.

# go install: -race is only supported on linux/amd64, linux/ppc64le, linux/arm64, freebsd/amd64, netbsd/amd64, darwin/amd64 and windows/amd64
# go install -race std; \

@tianon
Copy link
Member

tianon commented Feb 14, 2022

What exactly is the point of this? The make.bash script will already compile the standard library packages.

I don't recall exactly why we added it (I think to prevent Go from needing to immediately compile the stdlib when the first go build is invoked?), but if it's no longer necessary or useful then I think it's probably reasonable to remove. It'd be great to try and verify that somehow, though, instead of blindly just removing it. 🙈

Perhaps we can verify that the before/after of running that command causes no change? I tried building an image locally without that, then running go install std in a new container from that image, and docker diff on that container shows a lot of new files created in /usr/local/go/pkg which I do believe are useful?

It could be useful to precompile with -race, but this is unconditionally skipped right now.

Historically, this was disabled unilaterally on Alpine due to golang/go#14481, but it looks like we could maybe enable it now? I'm hesitant because it should be reasonably easy (and fast) for users to compile for themselves but isn't needed by most use cases for the image (see also #250 (comment)).

@rittneje
Copy link
Author

Those might just be "last updated timestamp" changes. From what I can tell docker diff doesn't distinguish metadata changes from content changes.

@tianon
Copy link
Member

tianon commented Feb 14, 2022

Doh, you're correct - I used tar --create --directory /usr/local/go/pkg --numeric-owner --sort name --mtime=@0 . to create a reproducible tarball of the before/after and they're 100% identical! I guess we can safely skip that bit. 😄

Edit: that was in 1.17, but I just tested with the same result in 1.16

@rittneje
Copy link
Author

I'm hesitant because it should be reasonably easy (and fast) for users to compile for themselves but isn't needed by most use cases for the image

One thing I did want to mention on this point (re: go install -race) is that since it is a docker image, every time you start a fresh container, it would have to recompile the race-detected stdlib. This is different from the experience with a native toolchain, where this would only happen when you upgrade Go versions.

I can understand the hesitancy for Alpine specifically, but what about Debian?

@tianon
Copy link
Member

tianon commented Feb 15, 2022

In Debian, we're able to use Go's published release tarballs for all the architectures which support -race (linux/amd64, linux/ppc64le, linux/arm64), and my understanding is that they thus already include those (they certainly used to - if they don't anymore, that would be an interesting data point).

@rittneje
Copy link
Author

Ah, just checked for linux/amd64, and you are correct. Did not know that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants