Make C++ toolchain optional #3390

illicitonion · 2022-12-08T16:01:17Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Currently, if you transition a go_binary, you must have a C++ toolchain
for it even if it's building in pure mode without cgo.

By making this toolchain optional, we push the error from a
toolchain-resolution-time error to an analysis-time error only if a
toolchain is both needed and not found.

This showed up in testing aspect-build/bazel-lib#289

achew22 · 2022-12-08T16:07:07Z

Can you also include a test to confirm we don't regress on this? Thanks!

go/private/context.bzl

fmeum · 2022-12-08T16:16:27Z

@illicitonion Is it possible that you based your commit on a rather old version of master? Rebasing should fix the CI failure.

WORKSPACE

fmeum · 2023-01-30T17:31:24Z

@illicitonion This should be unblocked as soon as Bazel 6.1 is the minimum supported version of rules_go: bazelbuild/bazel@ac47e10

illicitonion · 2023-01-31T10:52:12Z

Amazing, thanks!

LeandroLovisolo · 2023-12-21T03:29:12Z

@illicitonion This should be unblocked as soon as Bazel 6.1 is the minimum supported version of rules_go: bazelbuild/bazel@ac47e10

Bazel 6.1 has been released. Any chance this PR could be merged?

fmeum · 2023-12-21T07:06:12Z

We still support Bazel 5.4.0 though.

@illicitonion Would you be interested in picking this up again and using bazel_features for feature detection?

illicitonion · 2023-12-22T11:34:36Z

Sure, happy to pick this up again!

It looks like bazel_features requires a two-step init (http_archiveing it in, then calling a function from it), but rules_go only has a one-step init (loading go_rules_dependencies and calling it) - is there an existing pattern in rules_go for two-step init? (The problem being: You can't load the function you need to call at the top of the file because the http_archive you're loading it from doesn't exist yet, and you can't load it after the http_archive has been called because you can't load inside a function)

fmeum · 2023-12-22T13:07:15Z

@illicitonion There isn't and adding a second step would be a breaking change. How about adding this feature for Bzlmod only? Then we can look into whether we can support WORKSPACE as a follow-up.

illicitonion · 2023-12-22T17:25:48Z

@fmeum I think we're good to go on this, PTAL!

(Sorry for the horrors of the fake bazel_features :))

fmeum

Looks great, only a few stylistic comments

go/private/context.bzl

go/private/fake_bazel_features.bzl

tests/core/cross/BUILD.bazel

tests/core/starlark/cgo/cgo_test.bzl

fmeum · 2023-12-27T18:46:14Z

@linzhp Could you test this?

tyler-french

Tested these changes at Uber and everything passes

Currently, if you transition a go_binary, you must have a C++ toolchain for it even if it's building in pure mode without cgo. By making this toolchain optional, we push the error from a toolchain-resolution-time error to an analysis-time error only if a toolchain is both _needed_ and not found.

Also switch from @bazel_tools//tools/cpp:current_cc_toolchain to @bazel_tools//tools/cpp:optional_current_cc_toolchain because otherwise we still get a hard error if the toolchain can't be found.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | patch | `v0.44.1` -> `v0.44.2` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.44.2`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.44.2) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.1...v0.44.2) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "7c76d6236b28ff695aa28cf35f95de317a9472fd1fb14ac797c9bf684f09b37c", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.5") #### What's Changed - Make C++ toolchain optional by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3390 - Fix a race detected only if a test times out by [@patrickmscott](https://togithub.com/patrickmscott) in [bazelbuild/rules_go#3808 - registering timeout handler synchronously by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3810 - patch release 0.44.2 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3811 **Full Changelog**: bazelbuild/rules_go@v0.44.1...v0.44.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | patch | `v0.44.1` -> `v0.44.2` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.44.2`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.44.2) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.1...v0.44.2) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "7c76d6236b28ff695aa28cf35f95de317a9472fd1fb14ac797c9bf684f09b37c", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.5") #### What's Changed - Make C++ toolchain optional by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3390 - Fix a race detected only if a test times out by [@patrickmscott](https://togithub.com/patrickmscott) in [bazelbuild/rules_go#3808 - registering timeout handler synchronously by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3810 - patch release 0.44.2 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3811 **Full Changelog**: bazelbuild/rules_go@v0.44.1...v0.44.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/kreempuff/rules_unreal_engine).

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | patch | `v0.44.1` -> `v0.44.2` | --- ### Release Notes <details> <summary>bazelbuild/rules_go (io_bazel_rules_go)</summary> ### [`v0.44.2`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.44.2) [Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.44.1...v0.44.2) #### `WORKSPACE` code load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") http_archive( name = "io_bazel_rules_go", sha256 = "7c76d6236b28ff695aa28cf35f95de317a9472fd1fb14ac797c9bf684f09b37c", urls = [ "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", "https://github.com/bazelbuild/rules_go/releases/download/v0.44.2/rules_go-v0.44.2.zip", ], ) load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") go_rules_dependencies() go_register_toolchains(version = "1.21.5") #### What's Changed - Make C++ toolchain optional by [@illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3390 - Fix a race detected only if a test times out by [@patrickmscott](https://togithub.com/patrickmscott) in [bazelbuild/rules_go#3808 - registering timeout handler synchronously by [@linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3810 - patch release 0.44.2 by [@tyler-french](https://togithub.com/tyler-french) in [bazelbuild/rules_go#3811 **Full Changelog**: bazelbuild/rules_go@v0.44.1...v0.44.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>

fmeum reviewed Dec 8, 2022

View reviewed changes

go/private/context.bzl Show resolved Hide resolved

katre reviewed Dec 8, 2022

View reviewed changes

go/private/context.bzl Show resolved Hide resolved

fmeum reviewed Dec 8, 2022

View reviewed changes

go/private/context.bzl Show resolved Hide resolved

illicitonion force-pushed the optional-cpp-toolchain branch 2 times, most recently from 5da15de to 264ad0c Compare December 8, 2022 17:33

fmeum reviewed Dec 8, 2022

View reviewed changes

WORKSPACE Outdated Show resolved Hide resolved

illicitonion force-pushed the optional-cpp-toolchain branch from a7cf9e9 to 982ada2 Compare December 8, 2022 19:58

fmeum mentioned this pull request Dec 9, 2022

Using optional C++ toolchains while maintaining compatibility with non-platform builds bazelbuild/bazel#16966

Closed

fmeum mentioned this pull request Mar 8, 2023

rules_go requires a cc toolchain #3470

Open

illicitonion force-pushed the optional-cpp-toolchain branch 5 times, most recently from 0232f96 to 09c9158 Compare December 22, 2023 16:50

fmeum approved these changes Dec 22, 2023

View reviewed changes

go/private/context.bzl Outdated Show resolved Hide resolved

go/private/context.bzl Outdated Show resolved Hide resolved

go/private/fake_bazel_features.bzl Outdated Show resolved Hide resolved

fmeum reviewed Dec 22, 2023

View reviewed changes

tests/core/cross/BUILD.bazel Outdated Show resolved Hide resolved

illicitonion force-pushed the optional-cpp-toolchain branch 5 times, most recently from 59e4347 to f36882b Compare December 27, 2023 16:01

fmeum approved these changes Dec 27, 2023

View reviewed changes

tests/core/starlark/cgo/cgo_test.bzl Show resolved Hide resolved

fmeum force-pushed the optional-cpp-toolchain branch from f36882b to f08151c Compare December 28, 2023 08:34

tyler-french approved these changes Dec 29, 2023

View reviewed changes

illicitonion added 5 commits December 30, 2023 08:32

Add tests

1971a0a

Add target that actually uses cgo

59a24d2

Switch to use bazel_features

a3d516d

Also switch from @bazel_tools//tools/cpp:current_cc_toolchain to @bazel_tools//tools/cpp:optional_current_cc_toolchain because otherwise we still get a hard error if the toolchain can't be found.

Review comments

ae43602

fmeum force-pushed the optional-cpp-toolchain branch from f08151c to ae43602 Compare December 30, 2023 07:32

fmeum enabled auto-merge (squash) December 30, 2023 07:32

fmeum merged commit b5d0db5 into bazelbuild:master Dec 30, 2023
2 checks passed

illicitonion deleted the optional-cpp-toolchain branch January 2, 2024 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make C++ toolchain optional #3390

Make C++ toolchain optional #3390

illicitonion commented Dec 8, 2022

achew22 commented Dec 8, 2022

fmeum commented Dec 8, 2022

fmeum commented Jan 30, 2023

illicitonion commented Jan 31, 2023

LeandroLovisolo commented Dec 21, 2023 •

edited

fmeum commented Dec 21, 2023

illicitonion commented Dec 22, 2023

fmeum commented Dec 22, 2023

illicitonion commented Dec 22, 2023

fmeum left a comment

fmeum commented Dec 27, 2023

tyler-french left a comment

Make C++ toolchain optional #3390

Make C++ toolchain optional #3390

Conversation

illicitonion commented Dec 8, 2022

achew22 commented Dec 8, 2022

fmeum commented Dec 8, 2022

fmeum commented Jan 30, 2023

illicitonion commented Jan 31, 2023

LeandroLovisolo commented Dec 21, 2023 • edited

fmeum commented Dec 21, 2023

illicitonion commented Dec 22, 2023

fmeum commented Dec 22, 2023

illicitonion commented Dec 22, 2023

fmeum left a comment

Choose a reason for hiding this comment

fmeum commented Dec 27, 2023

tyler-french left a comment

Choose a reason for hiding this comment

LeandroLovisolo commented Dec 21, 2023 •

edited