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

pure has no effect when goarch and goos are auto #3033

Open
uhthomas opened this issue Dec 18, 2021 · 9 comments
Open

pure has no effect when goarch and goos are auto #3033

uhthomas opened this issue Dec 18, 2021 · 9 comments

Comments

@uhthomas
Copy link

uhthomas commented Dec 18, 2021

What version of rules_go are you using?

v0.29.0

What version of gazelle are you using?

v0.24.0

What version of Bazel are you using?

4.2.2

Does this issue reproduce with the latest releases of all the above?

Yes.

What operating system and processor architecture are you using?

linux x86_64 and macos x86_64.

Any other potentially useful information about your toolchain?

None.

What did you do?

Configure the pure attribute on a go_binary target where goarch and goos are auto.

What did you expect to see?

It should have an effect.

What did you see instead?

It does not have an effect.


See this repository for a full reproduction and explanation.

@robfig
Copy link
Contributor

robfig commented Dec 18, 2021

Thank you for the description and reproducer! I don't have a lot of knowledge about platform handling, but I have a few questions after taking a quick look -

  • Today, rules_go declares platforms for use with it -- bazel query @io_bazel_rules_go//go/toolchain:*. Is it the goal to replicate their behavior with a non-go-specific platform declaration?

  • It looks like the platforms above^ have an additional constraint specifying cgo on or off. My experience in cross-compiling using the --platforms flag has also been that the right platform must be selected for cgo=on/off.

I suppose the goal is to have a platform declaration be agnostic of cgo? I'm not sure what that would take. I would be happy to merge a fix, if you have an idea what needs fixing.

@uhthomas
Copy link
Author

uhthomas commented Dec 20, 2021

Per _go_transition_impl, it looks like cgo has to be explicitly toggled via cgo_constraint -> internal_cgo_off.

I suspect the documentation for pure is a little misleading as it states: [1]

If auto, pure mode is enabled when no C/C++ toolchain is configured or when cross-compiling.

I don't see any C/C++ toolchain detection going on, and I don't understand why it wouldn't work when not cross-compiling either.

The documentation for building pure binaries shows an example of using pure = "on' with goarch and goos set to auto. [2] which doesn't work.

This behaviour can be observed by providing Bazel with an invalid compiler when trying to build a pure binary. Given the example repository in the original issue:

❯ bazel --noworkspace_rc build --compiler=some_invalid_compiler //:pure_auto_auto
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/thomas/code/github.com/uhthomas/rules_go-issue/.bazelrc
ERROR: /home/thomas/.cache/bazel/_bazel_thomas/a08a8b424dc00c9fd5788b8c33b3f4a4/external/local_config_cc/BUILD:47:19: in cc_toolchain_suite rule @local_config_cc//:toolchain: cc_toolchain_suite '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'k8' and compiler 'some_invalid_compiler'.
ERROR: Analysis of target '//:pure_auto_auto' failed; build aborted: Analysis of target '@local_config_cc//:toolchain' failed
INFO: Elapsed time: 0.411s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)
❯ bazel --noworkspace_rc build --compiler=some_invalid_compiler //:pure_linux_amd64
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/thomas/code/github.com/uhthomas/rules_go-issue/.bazelrc
INFO: Analyzed target //:pure_linux_amd64 (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:pure_linux_amd64 up-to-date:
  bazel-out/k8-fastbuild-ST-05b09dfdd2e5/bin/pure_linux_amd64_/pure_linux_amd64
INFO: Elapsed time: 0.717s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

I imagine the fix would be to configure cgo independently of the platform.

What are your thoughts?

@robfig
Copy link
Contributor

robfig commented Dec 20, 2021

The documentation for building pure binaries shows an example of using pure = "on' with goarch and goos set to auto. [2] which doesn't work.

Maybe there's a misunderstanding.. I think it does work, when using the platforms that rules_go defines. It only doesn't work with the platforms you've defined. For example, this builds successfully:

$ bazel build --platforms=@io_bazel_rules_go//go/toolchain:darwin_amd64_cgo //:pure_auto_auto

@uhthomas
Copy link
Author

This is likely because it's not actually building a pure binary. Providing an invalid C/C++ compiler to Bazel results in a build failure, where it shouldn't.

❯ bazel --noworkspace_rc build --compiler=some_invalid_compiler --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64_cgo //:pure_auto_auto
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/thomas/code/github.com/uhthomas/rules_go-issue/.bazelrc
INFO: Build option --platforms has changed, discarding analysis cache.
ERROR: /home/thomas/.cache/bazel/_bazel_thomas/a08a8b424dc00c9fd5788b8c33b3f4a4/external/local_config_cc/BUILD:47:19: in cc_toolchain_suite rule @local_config_cc//:toolchain: cc_toolchain_suite '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'k8' and compiler 'some_invalid_compiler'.
ERROR: Analysis of target '//:pure_auto_auto' failed; build aborted: Analysis of target '@local_config_cc//:toolchain' failed
INFO: Elapsed time: 3.216s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 7738 targets configured)

However, it does again work when goarch and goos are configured.

❯ bazel --noworkspace_rc build --compiler=some_invalid_compiler --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64_cgo //:pure_linux_amd64
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/thomas/code/github.com/uhthomas/rules_go-issue/.bazelrc
INFO: Analyzed target //:pure_linux_amd64 (0 packages loaded, 229 targets configured).
INFO: Found 1 target...
Target //:pure_linux_amd64 up-to-date:
  bazel-out/k8-fastbuild-ST-05b09dfdd2e5/bin/pure_linux_amd64_/pure_linux_amd64
INFO: Elapsed time: 1.412s, Critical Path: 0.40s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

Regardless, it should not be necessary to specify a platform for the pure attribute to have any effect.

@robfig
Copy link
Contributor

robfig commented Dec 20, 2021

I don't think --compiler=some_invalid_compiler is a valid way to determine whether cgo is enabled or not.

$ rm .bazelrc  # clear platform overrides
$ bazel build -s //:pure_auto_auto
SUBCOMMAND: # //:pure_auto_auto [action 'GoLink pure_auto_auto_/pure_auto_auto', configuration: 143e6f4fd5d0b84b0d24893e96aeb74d37d539818ff5095c7d505e01fe832835, execution platform: @local_config_platform//:host]
(cd /private/var/tmp/_bazel_robfig/dd7db504c79994e6d63c7fab9857b4b1/execroot/some_workspace && \
  exec env - \
    CGO_ENABLED=0 \
    GOARCH=amd64 \
    GOOS=darwin \
    GOPATH='' \
    GOROOT=bazel-out/darwin-fastbuild-ST-ecf9d2d15f38/bin/external/io_bazel_rules_go/stdlib_ \
    GOROOT_FINAL=GOROOT \
  bazel-out/host/bin/external/go_sdk/builder link -sdk external/go_sdk -installsuffix darwin_amd64 -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o bazel-out/darwin-fastbuild-ST-4c64f0b3d5c7/bin/pure_auto_auto_/pure_auto_auto -main bazel-out/darwin-fastbuild-ST-4c64f0b3d5c7/bin/pure_auto_auto.a -p github.com/uhthomas/rules_go-issue -- -linkmode internal '-buildid=redacted')
Target //:pure_auto_auto up-to-date:
  bazel-out/darwin-fastbuild-ST-4c64f0b3d5c7/bin/pure_auto_auto_/pure_auto_auto

$ bazel build --compiler=some_invalid_compiler -s //:pure_auto_auto
ERROR: /private/var/tmp/_bazel_robfig/dd7db504c79994e6d63c7fab9857b4b1/external/local_config_cc/BUILD:41:19: in cc_toolchain_suite rule @local_config_cc//:toolchain: cc_toolchain_suite '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'darwin' and compiler 'some_invalid_compiler'.
ERROR: Analysis of target '//:pure_auto_auto' failed; build aborted: Analysis of target '@local_config_cc//:toolchain' failed

^ Above, CGO_ENABLED=0 is passed when building the program. I can't speak to Bazel's desire to check the cc toolchain when it isn't required. I'm totally prepared to believe that there's an undesirable interaction with platform definitions, and I would love if you could help us correct that. I suspect that these settings pre-dated platforms landing in Bazel.

@uhthomas
Copy link
Author

I see, thank you for your investigation and clarification.

So it seems that pure binaries are correctly built, but the Go toolchain incorrectly depends on a CC toolchain despite being unused. This is because the rules_go transition implementation incorrectly assumes cross-compilation only occurs with goarch and goos.

It looks like there has been some discussion on C++ autodetection before in #2089. I'm not sure it's possible to remove the cc compiler from the dependency graph automatically due to this explicit reference (see Integrating with C++ Rules).

Would you be happy to accept a PR which modifies the rules_go transition implementation to set //go/toolchain:cgo_off when pure = "on"?

The above suggestion doesn't resolve for pure = "auto", but I'm honestly not sure it's even possible at this current point in time.

@robfig
Copy link
Contributor

robfig commented Dec 22, 2021

Sure, that change sounds strictly positive to me. Thanks

@uhthomas
Copy link
Author

uhthomas commented Feb 7, 2022

bazelbuild/bazel#14726 should help with this!

@uhthomas
Copy link
Author

uhthomas commented May 5, 2022

Looks like optional toolchains will make it into the next LTS (bazelbuild/bazel#3601 (comment)). This will then be easy to resolve.

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

No branches or pull requests

2 participants