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

Add a build setting to disable architecture-specific binary output paths. #2249

Conversation

jmillikin-stripe
Copy link
Contributor

This is another attempt to fix #1239 (go_binary output paths containing architecture-specific components). See #1257 for a previous approach that wasn't accepted.

This time I'm using the "Starlark build settings" feature of newer Bazels (https://docs.bazel.build/versions/1.0.0/skylark/config.html), which lets us cleanly plumb the setting from a command-line flag to GoContext. The end result:

$ bazel build //tests/core/go_binary:hello
[...]
Target //tests/core/go_binary:hello up-to-date:
  bazel-bin/tests/core/go_binary/darwin_amd64_stripped/hello

$ bazel build //tests/core/go_binary:hello --@io_bazel_rules_go//go/build_settings:per_mode_binaries=false
Target //tests/core/go_binary:hello up-to-date:
  bazel-bin/tests/core/go_binary/hello

In addition to being set by a flag, Bazel has an in-development mechanism ("configuration transitions") for overriding this on a per-target basis, and having it propagate through the dependency graph. So a target that built a deployable container image could use this to ensure its contained binaries have consistent paths regardless of whether they're written in C++ or Go.

Note: Bazel's option parser doesn't accept flags for build settings defined in external repositories (bazelbuild/bazel#10039). The fix is bazelbuild/bazel#10052 and it should hopefully land in v1.2.

@jayconrod
Copy link
Contributor

We're planning to eventually migrate all parts of the build configuration to build settings (#2219). I'd prefer to have a complete plan for that migration before adding any build settings. Could you comment on that issue to make sure this is investigated?

@jmillikin-stripe
Copy link
Contributor Author

We're planning to eventually migrate all parts of the build configuration to build settings (#2219). I'd prefer to have a complete plan for that migration before adding any build settings. Could you comment on that issue to make sure this is investigated?

Refactoring the entire cross-compilation system to use build settings instead of platforms seems like a big project. Would it be possible to land this without blocking on it?

I'd be fine with naming it experimental_, then you can feel free to change/move/remove it in the future.

@jayconrod
Copy link
Contributor

Refactoring the entire cross-compilation system to use build settings instead of platforms seems like a big project. Would it be possible to land this without blocking on it?

I'd prefer not to merge this. In general, I'd like to have as few settings as possible, since each one adds a dimension to the space of configurations that need to be tested. A large fraction of rules_go bugs are in under-tested combinations of configurations, so I don't want to expand that space further unless a new configuration provides a lot of value.

As part of the design for build setting migration, I want to understand how they affect output paths and caching. I remember at some point a hash of the configuration was part of the output path. If that's still true, we won't need to encode the Go configuration in the output path, and we won't need a setting like this at all.

I'd be fine with naming it experimental_, then you can feel free to change/move/remove it in the future.

I think the best way to maintain this might be as a patch applied on the http_archive rule for io_bazel_rules_go.

Experimental features still have a maintenance cost in testing and avoiding unintentional changes when the surrounding code.

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

Successfully merging this pull request may close these issues.

go_binary output paths contain a spurious os_arch component
3 participants