-
Notifications
You must be signed in to change notification settings - Fork 651
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
Fix hermetic toolchain linking before Go 1.21 #3692
Conversation
Go toolchain checks which linker flags are available by running the linker on a temporary file. Before Go 1.21, this command was run in a temporary directory, but this has now been fixed upstream. golang/go#59952 This change will not be needed for Go versions 1.21 and above, but will be beneficial for users on lower versions and using a hermetic toolchain. See bazel-contrib/toolchains_llvm#183 for some more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyler-french Could you test this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am noticing some serious degradation in our GoLink
speed when testing. Have you seen anything similar?
I may need to dig a bit deeper into this to see why this might be happening.
I did not do any performance regression testing. But some performance hit may be expected. Go toolchain is now running a small linker command before running the actual link command. Previously, that small linker command was failing trivially. |
Our CI is experiencing a 2x overall build time (without cache) with this change applied, compared to without this change. I would like to do a little more investigation, and see if there's a way we can fix this issue without adding latency to the build. Otherwise, we should guard this feature behind a config flag so that users are not impacted unless they need this feature. |
Which hermetic toolchain do you use in your CI? Maybe I can help with this investigation if I find time. |
It is also very likely that you would hit the increased build time issue when you move to Go 1.21, which has a similar change as in this PR, but built into the toolchain itself. |
I tested it with Go 1.21.1
We use
|
I tried to reproduce with I can not reproduce what you are describing; maybe I am not calling with the right toolchain target. In the example above, the link step took 20 seconds on first invocation (clean /tmp dir), and then 4 seconds for each repeated run (with The issue in your CI could be more specific to the setup you have. |
Closing this because it has been roughly a year since the release of Go 1.21, so it is less needed now. The extra diligence for regression testing makes it not worthwhile. |
Go toolchain checks which linker flags are available by running the
linker on a temporary file. Before Go 1.21, this command was run in a
temporary directory, but this has now been fixed upstream.
golang/go#59952
This change will not be needed for Go versions 1.21 and above, but will
be beneficial for users on lower versions and using a hermetic
toolchain.
See bazel-contrib/toolchains_llvm#183 for some more
context.