Skip to content

Commit

Permalink
Make C++ toolchain optional (#3390)
Browse files Browse the repository at this point in the history
* Make C++ toolchain optional

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.

* Add tests

* Add target that actually uses cgo

* Switch to use bazel_features

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
  • Loading branch information
illicitonion authored Dec 30, 2023
1 parent 0a6311c commit b5d0db5
Show file tree
Hide file tree
Showing 9 changed files with 174 additions and 4 deletions.
24 changes: 24 additions & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,17 @@ tasks:
- tests/core/cgo/generate_imported_dylib.sh
build_targets:
- "//..."
- "--"
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/starlark/cgo/..." # Doesn't work before bazel 6
test_targets:
- "//..."
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/starlark/cgo/..." # Doesn't work before bazel 6
# Bzlmod tests require Bazel 6+
- "-//tests/core/nogo/bzlmod/..."
ubuntu2004:
Expand All @@ -28,6 +37,10 @@ tasks:
- "--config=incompatible"
build_targets:
- "//..."
- "--"
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
test_targets:
- "//..."
debian11_zig_cc:
Expand Down Expand Up @@ -67,6 +80,10 @@ tasks:
- "--host_crosstool_top=@local_config_apple_cc//:toolchain"
build_targets:
- "//..."
- "--"
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
test_flags:
- "--apple_crosstool_top=@local_config_apple_cc//:toolchain"
- "--crosstool_top=@local_config_apple_cc//:toolchain"
Expand All @@ -78,6 +95,10 @@ tasks:
- tests/core/cgo/generate_imported_dylib.sh
build_targets:
- "//..."
- "--"
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
test_flags:
# Some tests depend on this feature being disabled. However, because it's
# enabled by default in the rbe_ubuntu1604 platform, we cannot simply remove
Expand Down Expand Up @@ -243,6 +264,9 @@ tasks:
- "-//tests/legacy/test_chdir:go_default_test"
- "-//tests/legacy/test_rundir:go_default_test"
- "-//tests/legacy/transitive_data:go_default_test"
- "-//tests/core/cross:darwin_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:linux_go_cross_cgo" # Doesn't work before bazel 6
- "-//tests/core/cross:windows_go_cross_cgo" # Doesn't work before bazel 6
test_flags:
- '--action_env=PATH=C:\tools\msys64\usr\bin;C:\tools\msys64\bin;C:\tools\msys64\mingw64\bin;C:\python3\Scripts\;C:\python3;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\GooGet;C:\Program Files\Google\Compute Engine\metadata_scripts;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files\Google\Compute Engine\sysprep;C:\ProgramData\chocolatey\bin;C:\Program Files\Git\cmd;C:\tools\msys64\usr\bin;c:\openjdk\bin;C:\Program Files (x86)\Windows Kits\8.1\Windows Performance Toolkit\;C:\Program Files\CMake\bin;c:\ninja;c:\bazel;c:\buildkite'
# On Windows CI, bazel (bazelisk) needs %LocalAppData% to find the cache directory.
Expand Down
1 change: 1 addition & 0 deletions go/private/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ bzl_library(
"//go/platform:apple",
"//go/private:go_toolchain",
"//go/private/rules:transition",
"@bazel_features//:features.bzl",
"@bazel_skylib//lib:paths",
"@bazel_skylib//rules:common_settings",
"@bazel_tools//tools/build_defs/cc:action_names.bzl",
Expand Down
18 changes: 14 additions & 4 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@bazel_features//:features.bzl", "bazel_features")
load(
"@bazel_tools//tools/cpp:toolchain_utils.bzl",
"find_cpp_toolchain",
Expand Down Expand Up @@ -643,8 +644,11 @@ def _cgo_context_data_impl(ctx):
# toolchain (to be inputs into actions that need it).
# ctx.files._cc_toolchain won't work when cc toolchain resolution
# is switched on.
cc_toolchain = find_cpp_toolchain(ctx)
if cc_toolchain.compiler in _UNSUPPORTED_C_COMPILERS:
if bazel_features.cc.find_cpp_toolchain_has_mandatory_param:
cc_toolchain = find_cpp_toolchain(ctx, mandatory = False)
else:
cc_toolchain = find_cpp_toolchain(ctx)
if not cc_toolchain or cc_toolchain.compiler in _UNSUPPORTED_C_COMPILERS:
return []

feature_configuration = cc_common.configure_features(
Expand Down Expand Up @@ -833,12 +837,18 @@ def _cgo_context_data_impl(ctx):
cgo_context_data = rule(
implementation = _cgo_context_data_impl,
attrs = {
"_cc_toolchain": attr.label(default = "@bazel_tools//tools/cpp:current_cc_toolchain"),
"_cc_toolchain": attr.label(default = "@bazel_tools//tools/cpp:optional_current_cc_toolchain" if bazel_features.cc.find_cpp_toolchain_has_mandatory_param else "@bazel_tools//tools/cpp:current_cc_toolchain"),
"_xcode_config": attr.label(
default = "@bazel_tools//tools/osx:current_xcode_config",
),
},
toolchains = ["@bazel_tools//tools/cpp:toolchain_type"],
toolchains = [
# In pure mode, a C++ toolchain isn't needed when transitioning.
# But if we declare a mandatory toolchain dependency here, a cross-compiling C++ toolchain is required at toolchain resolution time.
# So we make this toolchain dependency optional, so that it's only attempted to be looked up if it's actually needed.
# Optional toolchain support was added in bazel 6.0.0.
config_common.toolchain_type("@bazel_tools//tools/cpp:toolchain_type", mandatory = False) if hasattr(config_common, "toolchain_type") else "@bazel_tools//tools/cpp:toolchain_type",
],
fragments = ["apple", "cpp"],
doc = """Collects information about the C/C++ toolchain. The C/C++ toolchain
is needed to build cgo code, but is generally optional. Rules can't have
Expand Down
2 changes: 2 additions & 0 deletions go/private/mode.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ def _ternary(*values):

def get_mode(ctx, go_toolchain, cgo_context_info, go_config_info):
static = _ternary(go_config_info.static if go_config_info else "off")
if getattr(ctx.attr, "pure", None) == "off" and not cgo_context_info:
fail("{} has pure explicitly set to off, but no C++ toolchain could be found for its platform".format(ctx.label))
pure = _ternary(
"on" if not cgo_context_info else "auto",
go_config_info.pure if go_config_info else "off",
Expand Down
35 changes: 35 additions & 0 deletions go/private/polyfill_bazel_features.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# bazel_features when used from a WORKSPACE rather than bzlmod context requires a two-step set-up (loading bazel_features, then calling a function from inside it).
# rules_go only has one-step set-up, and it would be a breaking change to require a second step.
# Accordingly, we supply a polyfill implementation of bazel_features which is only used when using rules_go from a WORKSPACE file,
# to avoid complicating code in rules_go itself.
# We just implement the checks we've seen we actually need, and hope to delete this completely when we are in a pure-bzlmod world.

_POLYFILL_BAZEL_FEATURES = """bazel_features = struct(
cc = struct(
find_cpp_toolchain_has_mandatory_param = {find_cpp_toolchain_has_mandatory_param},
)
)
"""

def _polyfill_bazel_features_impl(rctx):
# An empty string is treated as a "dev version", which is greater than anything.
bazel_version = native.bazel_version or "999999.999999.999999"
version_parts = bazel_version.split(".")
if len(version_parts) != 3:
fail("invalid Bazel version '{}': got {} dot-separated segments, want 3".format(bazel_version, len(version_parts)))
major_version_int = int(version_parts[0])
minor_version_int = int(version_parts[1])

find_cpp_toolchain_has_mandatory_param = major_version_int > 6 or (major_version_int == 6 and minor_version_int >= 1)

rctx.file("BUILD.bazel", """exports_files(["features.bzl"])
""")
rctx.file("features.bzl", _POLYFILL_BAZEL_FEATURES.format(
find_cpp_toolchain_has_mandatory_param = repr(find_cpp_toolchain_has_mandatory_param),
))

polyfill_bazel_features = repository_rule(
implementation = _polyfill_bazel_features_impl,
# Force reruns on server restarts to keep native.bazel_version up-to-date.
local = True,
)
6 changes: 6 additions & 0 deletions go/private/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# Once nested repositories work, this file should cease to exist.

load("//go/private:common.bzl", "MINIMUM_BAZEL_VERSION")
load("//go/private:polyfill_bazel_features.bzl", "polyfill_bazel_features")
load("//go/private/skylib/lib:versions.bzl", "versions")
load("//go/private:nogo.bzl", "DEFAULT_NOGO", "go_register_nogo")
load("//proto:gogo.bzl", "gogo_special_proto")
Expand Down Expand Up @@ -284,6 +285,11 @@ def go_rules_dependencies(force = False):
nogo = DEFAULT_NOGO,
)

_maybe(
polyfill_bazel_features,
name = "bazel_features",
)

def _maybe(repo_rule, name, **kwargs):
if name not in native.existing_rules():
repo_rule(name = name, **kwargs)
Expand Down
24 changes: 24 additions & 0 deletions tests/core/cross/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,30 @@ go_cross_binary(
target = ":native_bin",
)

# Because pure = "on" on the underlying target, this doesn't actually need cgo (and won't try to use it).
# This target ensures that (from Bazel 6) we don't require a C++ toolchain if we're not actually going to use cgo.
go_cross_binary(
name = "windows_go_cross_cgo",
platform = "@io_bazel_rules_go//go/toolchain:windows_amd64_cgo",
target = ":native_bin",
)

# Because pure = "on" on the underlying target, this doesn't actually need cgo (and won't try to use it).
# This target ensures that (from Bazel 6) we don't require a C++ toolchain if we're not actually going to use cgo.
go_cross_binary(
name = "linux_go_cross_cgo",
platform = "@io_bazel_rules_go//go/toolchain:linux_amd64_cgo",
target = ":native_bin",
)

# Because pure = "on" on the underlying target, this doesn't actually need cgo (and won't try to use it).
# This target ensures that (from Bazel 6) we don't require a C++ toolchain if we're not actually going to use cgo.
go_cross_binary(
name = "darwin_go_cross_cgo",
platform = "@io_bazel_rules_go//go/toolchain:darwin_amd64_cgo",
target = ":native_bin",
)

go_library(
name = "platform_lib",
srcs = select({
Expand Down
25 changes: 25 additions & 0 deletions tests/core/starlark/cgo/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
load("@io_bazel_rules_go//go/private:common.bzl", "GO_TOOLCHAIN")
load(":cgo_test.bzl", "cgo_test_suite")

constraint_value(
name = "os_does_not_exist",
constraint_setting = "@platforms//os:os",
)

# Make a platform we know won't have a C++ toolchain registered for it.
platform(
name = "platform_has_no_cc_toolchain",
constraint_values = [":os_does_not_exist"],
)

# Make a fake Go toolchain for this platform
toolchain(
name = "fake_go_toolchain",
target_compatible_with = [
":os_does_not_exist",
],
toolchain = "@go_sdk//:go_linux_amd64-impl",
toolchain_type = GO_TOOLCHAIN,
)

cgo_test_suite()
43 changes: 43 additions & 0 deletions tests/core/starlark/cgo/cgo_test.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts")
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_cross_binary")

def _missing_cc_toolchain_explicit_pure_off_test(ctx):
env = analysistest.begin(ctx)

asserts.expect_failure(env, "has pure explicitly set to off, but no C++ toolchain could be found for its platform")

return analysistest.end(env)

missing_cc_toolchain_explicit_pure_off_test = analysistest.make(
_missing_cc_toolchain_explicit_pure_off_test,
expect_failure = True,
config_settings = {
"//command_line_option:extra_toolchains": str(Label("//tests/core/starlark/cgo:fake_go_toolchain")),
},
)

def cgo_test_suite():
go_binary(
name = "cross_impure",
srcs = ["main.go"],
pure = "off",
tags = ["manual"],
)

go_cross_binary(
name = "go_cross_impure_cgo",
platform = ":platform_has_no_cc_toolchain",
target = ":cross_impure",
tags = ["manual"],
)

missing_cc_toolchain_explicit_pure_off_test(
name = "missing_cc_toolchain_explicit_pure_off_test",
target_under_test = ":go_cross_impure_cgo",
)

"""Creates the test targets and test suite for cgo.bzl tests."""
native.test_suite(
name = "cgo_tests",
tests = [":missing_cc_toolchain_explicit_pure_off_test"],
)

0 comments on commit b5d0db5

Please sign in to comment.