-
Notifications
You must be signed in to change notification settings - Fork 654
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
Ignore pre-compiled stdlib only on 1.19 with experiments #3508
Conversation
Does this mean that the stdlib doesn't need to be rebuilt for other experiments? I think I don't fully understand why we need to rebuild in this one particular case but not others. |
There can be flags disabling experiments like "nocoverageredesign", "nounified" in People can also pre-compile stdlib for Go 1.20 with some experiments on or off, which is the same as with the list of flags in I don't object to the idea of not checking |
What happens if we assume the pre-compiled stdlib is fine but it isn't supported? Will it still be built in the correct version, just without us caching it properly? If that's the case we can remove the check for experiments entirely, but the current form of the PR is definitely fine. |
When is a stdlib not supported? |
What I'm wondering is what happens if we do not trigger a build of the standard library (by taking this branch: rules_go/go/private/actions/stdlib.bzl Line 42 in e558f56
|
I think there will be linking errors when the pre-compiled stdlib binary doesn't match the compilation mode for other packages. @sywhang to confirm |
I see, can this happen with Go experiments? In that case not triggering recompilation when it's needed could also result in linker errors. But I assume you confirmed that is not the case for this particular change, so that one seems safe to make. |
1 similar comment
I see, can this happen with Go experiments? In that case not triggering recompilation when it's needed could also result in linker errors. But I assume you confirmed that is not the case for this particular change, so that one seems safe to make. |
It could happen with Go experiments, so if users want to pre-compile stdlib, it's the users' responsibility to give rules_go a pre-compiled binary that matches the Go experiment flags defined at Go sdk rules. We can run The other way to implement this PR is to recompile when Go version is 1.19 or older and |
That sounds good to me! |
I had to expose |
Thanks for this: changing my |
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
#3398 allowed people to use boringcrypto on 1.19 by ignoring the pre-compile stdlib when boringcrypto is request. However, #3401 over generalized the intention by ignore pre-compile stdlib for all experimental flags for all Go versions. In addition, rules_go add a "nocoverageredesign" experimental flag for Go 1.20. This means rules_go always ignores pre-compiled stdlib that users provide, even it's compiled correctly.
This restore the intention of #3398 by only ignoring pre-compiled stdlib on Go 1.19 with experiments. This allows people to add precompiled stdlib on 1.20+ to improve build performance.
Other notes for review