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

go: use configuration transitions instead of aspect #2414

Merged
merged 10 commits into from
Apr 17, 2020

Conversation

jayconrod
Copy link
Contributor

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

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
@achew22
Copy link
Member

achew22 commented Mar 26, 2020

Woohoo! Congratulations on doing this @jayconrod. This is the number one PR of the week as far as I'm concerned!

@jayconrod
Copy link
Contributor Author

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.

@jayconrod
Copy link
Contributor Author

jayconrod commented Apr 3, 2020

Still to do:

  • Get tests passing. I think this will require updating the minimum Bazel version to 2.2.0.
  • Test that build settings work on the command line.
  • Update documentation in modes.rst, core.rst, and README.rst.

@steeve
Copy link
Contributor

steeve commented Apr 6, 2020

So happy to see this!

@seh
Copy link
Contributor

seh commented Apr 6, 2020

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.

@jayconrod
Copy link
Contributor Author

jayconrod commented Apr 6, 2020

@seh At the moment, it's possible to set build tags with --define=gotags=foo,bar. The --define flag was never really meant to be used like this though, so this is undocumented. It will eventually be deprecated and removed. EDIT: it looks like this didn't actually work.

After this change, you'll be able to set build tags either by setting gotags = ["foo", "bar"] on a go_binary rule or by using the --@io_bazel_rules_goo//go/config:tags command line flag. Both would be equivalent.

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.
@jayconrod
Copy link
Contributor Author

I decided to leave gotags out of this change, since I don't want to make it bigger than necessary. The infrastructure is there though, so it should be an easy follow-up. I've milestoned #1351 for the next release.

@jayconrod jayconrod marked this pull request as ready for review April 17, 2020 21:40
@jayconrod
Copy link
Contributor Author

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.

@jayconrod jayconrod merged commit 3edc6d5 into master Apr 17, 2020
@jayconrod jayconrod deleted the feature/transition branch April 17, 2020 21:48
@steeve
Copy link
Contributor

steeve commented Apr 18, 2020

Congratulations! I’ll test it asap!

@jmillikin-stripe
Copy link
Contributor

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?

@pcj
Copy link
Member

pcj commented Apr 20, 2020

Thanks @jayconrod for landing this!

Looking forward to testing this... Any notes to share regarding suggested/reqiured migration steps?

@steeve
Copy link
Contributor

steeve commented Apr 20, 2020

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?

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.

@steeve
Copy link
Contributor

steeve commented Apr 20, 2020

By the way tested this weekend, it works great and opens up so much possibilities!

@jayconrod
Copy link
Contributor Author

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?

@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.

@jayconrod
Copy link
Contributor Author

Looking forward to testing this... Any notes to share regarding suggested/reqiured migration steps?

@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 go_binary or go_test rule API. Just that goos and goarch will now work with select (they previously did not affect what select picked).

--feature flags should still work in this release, but I'll deprecate them before the release. Weird things may happen if they're mixed with the new flags.

Hopefully that's it.

@pcj
Copy link
Member

pcj commented Apr 20, 2020

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 goos = "linux", goarch = "amd64" in the rule, or should one continue to specify --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64?

@steeve
Copy link
Contributor

steeve commented Apr 20, 2020

@pcj

They are equivalent since goos/goarch will trigger a transition on platform.

However, if you want your binary to be always built for a specific goos/goarch tuple, without fiddling with .bazelrc or bazel flags, I'd say put it in the target itself.

@jayconrod
Copy link
Contributor Author

@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 go_binary fundamentally should produce a Linux / amd64 executable, I'd set goos and goarch. That may be true for a Docker container.

It might make sense in the future for go_image itself to do a transition though.

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