-
-
Notifications
You must be signed in to change notification settings - Fork 656
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
go: use configuration transitions instead of aspect #2414
Conversation
This changes how the goos, goarch, race, msan, static, and pure attributes of go_binary and go_test are implemented. Previously, the go_binary and go_test rules used an aspect on the deps and embed attributes. If any of the mode attributes listed above were set, the aspect declared additional output files and actions to generate them for the specified configuration. This approach had two significant problems. First, the aspect caused the analysis to be performed twice, wasting time and memory even in the case where no cross-compilation was needed. Second, the aspect was not compatible with conditional dependencies specified with select expressions: aspects run on the configured target graph, but the mode attributes could not affect Bazel's configuration. This change deletes the aspect and implements these attributes using a different mechanism: build settings and configuration transitions. A new package is added //go/config with a build setting for each attribute. Most settings are flags that may be set on the command-line. This is an alternative to --feature flags, which will be deprecated and eventually removed. The target //:go_config gathers build settings relevant to Go and provides a GoContextInfo (private). The target //:go_context_data depends on //:go_config, //:stdlib, //:nogo, and a few other standard dependencies. go_binary and go_test rules no longer need to depend on these targets directly. Go rules should now only need to depend on //:go_context_data and the Go toolchain. Unfortunately, neither //:go_context_data nor //:go_config can be part of the toolchain because the toolchain is always analyzed for the execution configuration, not the target configuration. Because of the simplified dependencies, the go_rule function is no longer needed by most rules. This change makes it possible to deprecate it. Fixes #1351 Fixes #2219 Updates #2302
Woohoo! Congratulations on doing this @jayconrod. This is the number one PR of the week as far as I'm concerned! |
Glad to hear you're looking forward to this! I'm hoping I'll be able to close a lot of cross-compilation issues after this. |
Still to do:
|
So happy to see this! |
Given that #1351 is linked from here, does this change how Go build tags will work with Bazel? It looks like your patch retains the "gotags" attribute. |
@seh After this change, you'll be able to set build tags either by setting I don't think command line flags like this are quite working yet in Bazel, but rules_go shouldn't need to do anything further to enable that in future versions. |
* //tests/core/transition:cmdline_test verifies that race mode can be enabled on the command line with the Starlark flag. * //tests/core/cross:cross_test now depends on binaries that depend on a package with sources determined by a select statement. It will not build if the select is evaluated in the wrong configuration. * Fixes in compilepkg.bzl and test.bzl to ensure tags are used when compiling and generating the test main. * go/tools/bazel_testing now exports Bazel exit codes and StderrExitError has an Unwrap method.
I decided to leave |
I think this is ready to go, so merging now. I'm hoping to ship this with 0.23.0 in early May. Please try it out and let me know if you have issues. |
Congratulations! I’ll test it asap! |
Now that rules_go has been migrated to build settings, does this mean the change in #2249 (a build setting to disable platform-specific output paths) would be acceptable? |
Thanks @jayconrod for landing this! Looking forward to testing this... Any notes to share regarding suggested/reqiured migration steps? |
I even feel like it should be the default? To be consistent with other bazel rules. However when using a transition, paths are much much bigger. |
By the way tested this weekend, it works great and opens up so much possibilities! |
@jmillikin-stripe @steeve Rather than having a build setting control output path prefixes, I think now we can get rid of the path prefixes entirely. I just opened #2448 to track that. |
@pcj I'll include more information in the v0.23.0 release notes when it's ready. There shouldn't be any incompatible change to the
Hopefully that's it. |
Thanks @jayconrod: for the docs (and/or this thread) I'd like to know the recommended strategy for a go_image/go_binary intended for linux container: should one use |
They are equivalent since However, if you want your binary to be always built for a specific |
@pcj Basically, what @steeve said. You should get the same output (not sure if bit for bit identical, but very close at least). If the It might make sense in the future for |
* Add constants for Bazel exit codes. * Add StderrExitError.Unwrap
This changes how the goos, goarch, race, msan, static, and pure
attributes of go_binary and go_test are implemented.
Previously, the go_binary and go_test rules used an aspect on the deps
and embed attributes. If any of the mode attributes listed above were
set, the aspect declared additional output files and actions to
generate them for the specified configuration. This approach had two
significant problems. First, the aspect caused the analysis to be
performed twice, wasting time and memory even in the case
where no cross-compilation was needed. Second, the aspect was not
compatible with conditional dependencies specified with select
expressions: aspects run on the configured target graph, but the mode
attributes could not affect Bazel's configuration.
This change deletes the aspect and implements these attributes using a
different mechanism: build settings and configuration transitions.
A new package is added //go/config with a build setting for each
attribute. Most settings are flags that may be set on the
command-line. This is an alternative to --feature flags, which will be
deprecated and eventually removed.
The target //:go_config gathers build settings relevant to Go and
provides a GoContextInfo (private).
The target //:go_context_data depends on //:go_config, //:stdlib,
//:nogo, and a few other standard dependencies. go_binary and go_test
rules no longer need to depend on these targets directly. Go rules
should now only need to depend on //:go_context_data and the Go
toolchain. Unfortunately, neither //:go_context_data nor //:go_config
can be part of the toolchain because the toolchain is always analyzed
for the execution configuration, not the target configuration.
Because of the simplified dependencies, the go_rule function is no
longer needed by most rules. This change makes it possible to
deprecate it.
Fixes #1351
Fixes #2219
Updates #2302