From 1b8349f5e26fa776b2213f200442e55f06651c51 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Thu, 19 Dec 2024 08:35:38 -0800 Subject: [PATCH] [tools] Modernize Bazel spelling for selecting compiler flags Deprecate the 'cc' external. Keeping this repository rule intact with bzlmod is certainly possible, but is somewhat of a hassle. Better to use what rules_cc already offers for compiler selection. The command line option --define=DRAKE_WERROR=ON no longer has any effect. (That flag was intended to be internal use only so hopefully nobody will care. It's replacement is now explicitly documented to be internal use only.) Remove some vestigial cc_toolchain diagnostic programs. Also fix some warning Noble GCC spam from FCL, noticed while debugging. --- CMakeLists.txt | 8 + cmake/bazel.rc.in | 3 + tools/BUILD.bazel | 7 - tools/cc_toolchain/BUILD.bazel | 146 +++++++++++++++--- tools/cc_toolchain/bazel.rc | 8 +- tools/lint/library_lint.bzl | 4 +- tools/skylark/drake_cc.bzl | 89 +++++------ tools/skylark/pybind.bzl | 22 +-- tools/ubuntu-noble.bazelrc | 8 + tools/workspace/cc/repository.bzl | 3 + tools/workspace/default.bzl | 1 + .../fcl_internal/package.BUILD.bazel | 5 +- 12 files changed, 211 insertions(+), 93 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 13531879aa47..57ff3e3ff4ac 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -98,6 +98,14 @@ if(C_COMPILER_NAME STREQUAL ccache OR CXX_COMPILER_NAME STREQUAL ccache) ) endif() +# Get the full C++ compiler major version for our cmake/bazel.rc.in. +string(REGEX MATCH "^([0-9]+)" + DRAKE_CC_TOOLCHAIN_COMPILER_MAJOR + "${CMAKE_CXX_COMPILER_VERSION}") +if(NOT DRAKE_CC_TOOLCHAIN_COMPILER_MAJOR) + set(DRAKE_CC_TOOLCHAIN_COMPILER_MAJOR "0") +endif() + # The minimum compiler versions should match those listed in both # doc/_pages/from_source.md and tools/workspace/cc/repository.bzl. set(MINIMUM_APPLE_CLANG_VERSION 14) diff --git a/cmake/bazel.rc.in b/cmake/bazel.rc.in index b8141595f2d3..23eb82ba19bb 100644 --- a/cmake/bazel.rc.in +++ b/cmake/bazel.rc.in @@ -35,6 +35,9 @@ build:MinSizeRel --compilation_mode=opt --copt=-Os --host_copt=-Os build:Release --compilation_mode=opt build:RelWithDebInfo --compilation_mode=opt --fission=no --copt=-g --host_copt=-g +# Pass along the compiler version. +build --@drake//tools/cc_toolchain:compiler_major=@DRAKE_CC_TOOLCHAIN_COMPILER_MAJOR@ + # Match CMAKE_BUILD_TYPE as well as the WITH_FOO customizations. build @BAZEL_CONFIG@ diff --git a/tools/BUILD.bazel b/tools/BUILD.bazel index 560308f73fad..d3b97724873a 100644 --- a/tools/BUILD.bazel +++ b/tools/BUILD.bazel @@ -44,13 +44,6 @@ alias( # === config_setting rules === -# When this is set, a Drake build will promote some warnings to errors. -# See drake/tools/cc_toolchain/bazel.rc for details. -config_setting( - name = "drake_werror", - values = {"define": "DRAKE_WERROR=ON"}, -) - config_setting( name = "with_gurobi", values = {"define": "WITH_GUROBI=ON"}, diff --git a/tools/cc_toolchain/BUILD.bazel b/tools/cc_toolchain/BUILD.bazel index b90fbfea5f70..17e2a24a1aae 100644 --- a/tools/cc_toolchain/BUILD.bazel +++ b/tools/cc_toolchain/BUILD.bazel @@ -1,44 +1,143 @@ +load("@bazel_skylib//lib:selects.bzl", "selects") +load("@bazel_skylib//rules:common_settings.bzl", "int_flag", "string_flag") load("//tools/lint:lint.bzl", "add_lint_tests") load("//tools/skylark:sh.bzl", "sh_binary") package(default_visibility = ["//visibility:public"]) +# This cc_toolchain package provides labels that can be used by `select()` +# statements in our build rules to tweak settings based on which C++ compiler +# is being used. For example uses, see drake/tools/skylark/drake_cc.bzl. + +# When this is set to "error", a Drake build will promote some warnings of our +# choice to errors. See drake/tools/cc_toolchain/bazel.rc for details. This is +# intended for internal use only, not for users to configure. +string_flag( + name = "error_severity", + build_setting_default = "warning", + values = [ + "warning", + "error", + ], +) + config_setting( name = "apple", constraint_values = ["@platforms//os:osx"], ) +config_setting( + name = "linux", + constraint_values = ["@platforms//os:linux"], +) + +config_setting( + name = "error_severity_warning", + flag_values = {":error_severity": "warning"}, +) + +config_setting( + name = "error_severity_error", + flag_values = {":error_severity": "error"}, +) + +selects.config_setting_group( + name = "apple_clang_with_warnings", + match_all = [ + ":apple", + "@rules_cc//cc/compiler:clang", + ":error_severity_warning", + ], +) + +selects.config_setting_group( + name = "apple_clang_with_errors", + match_all = [ + ":apple", + "@rules_cc//cc/compiler:clang", + ":error_severity_error", + ], +) + +selects.config_setting_group( + name = "gcc_with_warnings", + match_all = [ + "@rules_cc//cc/compiler:gcc", + ":error_severity_warning", + ], +) + +selects.config_setting_group( + name = "gcc_with_errors", + match_all = [ + "@rules_cc//cc/compiler:gcc", + ":error_severity_error", + ], +) + +selects.config_setting_group( + name = "linux_clang_with_warnings", + match_all = [ + ":linux", + "@rules_cc//cc/compiler:clang", + ":error_severity_warning", + ], +) + +selects.config_setting_group( + name = "linux_clang_with_errors", + match_all = [ + ":linux", + "@rules_cc//cc/compiler:clang", + ":error_severity_error", + ], +) + config_setting( name = "debug", values = {"compilation_mode": "dbg"}, ) +# In our developer builds, we use this as an optional hint for configuring +# compiler warnings. It's neither necessary (nor typically set) when Drake +# is installed with CMake or used as a Bazel external. This is intended for +# internal use only, not for users to configure. +# +# TODO(jwnimmer-tri) Ideally, rules_cc would provide this as a toolchain +# attribute that we could query, but it doesn't seem to exist there yet. +int_flag( + name = "compiler_major", + build_setting_default = 0, +) + config_setting( - name = "linux", - constraint_values = ["@platforms//os:linux"], + name = "compiler_major_13", + flag_values = {":compiler_major": "13"}, ) -# Capture some bazel cc_toolchain variables into a bash environment file. These -# values reflect the compiler and flags that Bazel's compilation actions will -# use. Note that this might refer to Bazel's compiler wrapper, `cc_wrapper.sh`, -# and thus may only indirectly identify the actual compiler in use. Note also -# that `$(CC)` may refer to the canonicalized location of the compiler; for -# example, on Ubuntu, Clang may be `/usr/lib/llvm-/bin/clang`, -# canonicalized from `/usr/bin/clang-`. -genrule( - name = "capture_cc", - outs = ["capture_cc.env"], - cmd = "cat < '$@'\n" + - "BAZEL_CC=\"$(CC)\"\n" + - "BAZEL_CC_FLAGS=\"$(CC_FLAGS)\"\n" + - "EOF", - toolchains = [ - "@bazel_tools//tools/cpp:cc_flags", - "@bazel_tools//tools/cpp:current_cc_toolchain", +selects.config_setting_group( + name = "gcc_13_with_warnings", + match_all = [ + "@rules_cc//cc/compiler:gcc", + ":compiler_major_13", + ":error_severity_warning", ], - visibility = ["//common:__pkg__"], ) +selects.config_setting_group( + name = "gcc_13_with_errors", + match_all = [ + "@rules_cc//cc/compiler:gcc", + ":compiler_major_13", + ":error_severity_error", + ], +) + +# ***************************************************************************** +# The following sh_binary (and its prerequisite genrule) are used by drake-ci's +# CMake setup logic to sniff out Drake's selected compiler. They are not used +# by Drake's normal Bazel nor CMake builds. + # Capture bazel's action environment variables into a bash environment file. # These values reflect ONLY the command-line (or rcfile) `--action_env` # settings that Bazel uses when running its toolchain configuration, e.g. @@ -53,7 +152,7 @@ genrule( "CC=\"$${CC:-}\"\n" + "CXX=\"$${CXX:-}\"\n" + "EOF", - visibility = ["//common:__pkg__"], + visibility = ["//visibility:private"], ) # Utility script to indicate which compiler is selected by use of @@ -65,9 +164,8 @@ sh_binary( name = "print_compiler_config", srcs = ["print_compiler_config.sh"], args = ["$(location :capture_compiler_config.env)"], - data = [ - ":capture_compiler_config.env", - ], + data = [":capture_compiler_config.env"], + visibility = ["//visibility:private"], ) add_lint_tests() diff --git a/tools/cc_toolchain/bazel.rc b/tools/cc_toolchain/bazel.rc index 8fdcb87b6751..51696774cfb9 100644 --- a/tools/cc_toolchain/bazel.rc +++ b/tools/cc_toolchain/bazel.rc @@ -1,9 +1,9 @@ # Disable ccache due to incompatibility with Bazel. build --action_env=CCACHE_DISABLE=1 -# When compiling with Drake as the main WORKSPACE (i.e., if and only if this -# rcfile is loaded), we enable -Werror by default for Drake's *own* targets, -# but not for our externals. +# When compiling with Drake as the main module (i.e., if and only if this rcfile +# is loaded), we enable -Werror by default for Drake's *own* targets, but not +# for our externals. # # Developers may still disable errors locally by passing --copt=-w on the # command line, or promote *any* warnings even from externals to errors via @@ -11,4 +11,4 @@ build --action_env=CCACHE_DISABLE=1 # # When compiilng Drake as an external package, this rcfile is not loaded and we # won't promote warnings to errors by default. -build --define=DRAKE_WERROR=ON +build --@drake//tools/cc_toolchain:error_severity=error diff --git a/tools/lint/library_lint.bzl b/tools/lint/library_lint.bzl index 79df42ea5a28..936729545be5 100644 --- a/tools/lint/library_lint.bzl +++ b/tools/lint/library_lint.bzl @@ -93,11 +93,9 @@ def library_lint( ]) # Find libraries that are deps of the package_library but shouldn't be. - extra_deps_expression = "deps({}, 1) except ({}) except {}".format( + extra_deps_expression = "deps({}, 1) except ({})".format( package_name + ":" + short_package_name, correct_deps_expression, - # This is fine (it's a dependency of our copt select() statement). - "//tools:drake_werror", ) # Find libraries that should be deps of the package_library but aren't. diff --git a/tools/skylark/drake_cc.bzl b/tools/skylark/drake_cc.bzl index 6b5f53004d42..11db68fd4dd9 100644 --- a/tools/skylark/drake_cc.bzl +++ b/tools/skylark/drake_cc.bzl @@ -1,4 +1,3 @@ -load("@cc//:compiler.bzl", "COMPILER_ID", "COMPILER_VERSION_MAJOR") load("//tools/skylark:cc.bzl", "CcInfo", "cc_binary", "cc_library", "cc_test") load( "//tools/skylark:kwargs.bzl", @@ -43,23 +42,10 @@ CLANG_FLAGS = CXX_FLAGS + [ "-Werror=unqualified-std-cast-call", ] -# The CLANG_VERSION_SPECIFIC_FLAGS will be enabled for all C++ rules in the -# project when building with a Clang compiler of the specified major -# version (excluding the Apple LLVM compiler, see -# APPLECLANG_VERSION_SPECIFIC_FLAGS below). -CLANG_VERSION_SPECIFIC_FLAGS = { -} - # The APPLECLANG_FLAGS will be enabled for all C++ rules in the project when # building with the Apple LLVM compiler. APPLECLANG_FLAGS = CLANG_FLAGS -# The APPLECLANG_VERSION_SPECIFIC_FLAGS will be enabled for all C++ rules in -# the project when building with an Apple LLVM compiler of the specified major -# version. -APPLECLANG_VERSION_SPECIFIC_FLAGS = { -} - # The GCC_FLAGS will be enabled for all C++ rules in the project when # building with gcc. GCC_FLAGS = CXX_FLAGS + [ @@ -79,6 +65,11 @@ GCC_CC_TEST_FLAGS = [ "-Wno-unused-parameter", ] +# The GCC_VERSION_SPECIFIC_FLAGS will be enabled for all C++ rules in the +# project when building with gcc of the specified major version, but only if +# the --@drake//tools/cc_toolchain:compiler_major=NN flag has been set on the +# command line or in an rcfile. (It typically will be except when Drake is used +# as a Bazel external.) GCC_VERSION_SPECIFIC_FLAGS = { 13: [ "-Werror=pessimizing-move", @@ -93,39 +84,49 @@ GCC_VERSION_SPECIFIC_FLAGS = { ], } +def _defang(flags): + """Given a list of copts, demote all -Werror into just plain -W.""" + return [ + x.replace("-Werror=", "-W") + for x in flags + ] + +# The BASE_COPTS are used for all drake_cc_{binary,library,test} rules. +BASE_COPTS = select({ + "//tools/cc_toolchain:apple_clang_with_errors": APPLECLANG_FLAGS, + "//tools/cc_toolchain:apple_clang_with_warnings": _defang(APPLECLANG_FLAGS), # noqa + "//tools/cc_toolchain:gcc_with_errors": GCC_FLAGS, + "//tools/cc_toolchain:gcc_with_warnings": _defang(GCC_FLAGS), + "//tools/cc_toolchain:linux_clang_with_errors": CLANG_FLAGS, + "//tools/cc_toolchain:linux_clang_with_warnings": _defang(CLANG_FLAGS), + "//conditions:default": _defang(CXX_FLAGS), +}) + select(dict([ + ("//tools/cc_toolchain:gcc_{}_with_errors".format(major_ver), flags) + for major_ver, flags in GCC_VERSION_SPECIFIC_FLAGS.items() +] + [ + ("//tools/cc_toolchain:gcc_{}_with_warnings".format(major_ver), _defang(flags)) # noqa + for major_ver, flags in GCC_VERSION_SPECIFIC_FLAGS.items() +] + [ + ("//conditions:default", []), +])) + def _platform_copts(rule_copts, rule_gcc_copts, rule_clang_copts, cc_test = 0): - """Returns both the rule_copts (plus rule_{cc}_copts iff under the - specified compiler), and platform-specific copts. + """Returns the concatenation of Drake's platform-specific BASE_COPTS, + plus the passed-in rule_copts, plus the passed-in plus rule_{cc}_copts iff + building with the specified compiler). - When cc_test=1, the GCC_CC_TEST_FLAGS will be added. It should only be set - to 1 from cc_test rules or rules that are boil down to cc_test rules. + When cc_test=1, the GCC_CC_TEST_FLAGS will also be added. It should only be + used from cc_test rules or rules that are boil down to cc_test rules. """ - if COMPILER_ID == "AppleClang": - result = APPLECLANG_FLAGS + rule_copts + rule_clang_copts - if COMPILER_VERSION_MAJOR in APPLECLANG_VERSION_SPECIFIC_FLAGS: - result += APPLECLANG_VERSION_SPECIFIC_FLAGS[COMPILER_VERSION_MAJOR] - elif COMPILER_ID == "Clang": - result = CLANG_FLAGS + rule_copts + rule_clang_copts - if COMPILER_VERSION_MAJOR in CLANG_VERSION_SPECIFIC_FLAGS: - result += CLANG_VERSION_SPECIFIC_FLAGS[COMPILER_VERSION_MAJOR] - elif COMPILER_ID == "GNU": - extra_gcc_flags = GCC_CC_TEST_FLAGS if cc_test else [] - result = GCC_FLAGS + extra_gcc_flags + rule_copts + rule_gcc_copts - if COMPILER_VERSION_MAJOR in GCC_VERSION_SPECIFIC_FLAGS: - result += GCC_VERSION_SPECIFIC_FLAGS[COMPILER_VERSION_MAJOR] - else: - result = rule_copts - - # We can't handle select() yet. - # TODO(jwnimmer-tri) We should handle select. - if type(result) != "list": - return result - return select({ - "//tools:drake_werror": result, - "//conditions:default": [ - x.replace("-Werror=", "-W") - for x in result - ], + if not any([rule_copts, rule_gcc_copts, rule_clang_copts, cc_test]): + # In the case of no special arguments at all, we can save Bazel the + # hassle of concatenating a bunch of empty stuff. + return BASE_COPTS + test_gcc_copts = GCC_CC_TEST_FLAGS if cc_test else [] + return BASE_COPTS + rule_copts + select({ + "@rules_cc//cc/compiler:gcc": rule_gcc_copts + test_gcc_copts, + "@rules_cc//cc/compiler:clang": rule_clang_copts, + "//conditions:default": [], }) def _check_library_deps_blacklist(name, deps): diff --git a/tools/skylark/pybind.bzl b/tools/skylark/pybind.bzl index 7fb31ed79259..f60aeccca086 100644 --- a/tools/skylark/pybind.bzl +++ b/tools/skylark/pybind.bzl @@ -1,4 +1,3 @@ -load("@cc//:compiler.bzl", "COMPILER_ID") load("@python//:version.bzl", "PYTHON_EXTENSION_SUFFIX") load("//tools/install:install.bzl", "install") load("//tools/skylark:cc.bzl", "CcInfo", "cc_binary") @@ -6,15 +5,18 @@ load("//tools/skylark:drake_cc.bzl", "drake_cc_binary", "drake_cc_googletest") load("//tools/skylark:drake_py.bzl", "drake_py_library", "drake_py_test") load("//tools/skylark:py.bzl", "py_library") -EXTRA_PYBIND_COPTS = [ - # GCC and Clang don't always agree / succeed when inferring storage - # duration (#9600). Workaround it for now. - "-Wno-unused-lambda-capture", - # pybind11's operator overloading (e.g., .def(py::self + py::self)) - # spuriously triggers this warning, so we'll suppress it anytime we're - # compiling pybind11 modules. - "-Wno-self-assign-overloaded", -] if COMPILER_ID.endswith("Clang") else [] +EXTRA_PYBIND_COPTS = select({ + "@rules_cc//cc/compiler:clang": [ + # GCC and Clang don't always agree / succeed when inferring storage + # duration (#9600). Workaround it for now. + "-Wno-unused-lambda-capture", + # pybind11's operator overloading (e.g., .def(py::self + py::self)) + # spuriously triggers this warning, so we'll suppress it anytime we're + # compiling pybind11 modules. + "-Wno-self-assign-overloaded", + ], + "//conditions:default": [], +}) def pybind_py_library( name, diff --git a/tools/ubuntu-noble.bazelrc b/tools/ubuntu-noble.bazelrc index b41be418d8e2..6a2d48f285e0 100644 --- a/tools/ubuntu-noble.bazelrc +++ b/tools/ubuntu-noble.bazelrc @@ -8,3 +8,11 @@ build:clang --host_action_env=CC=clang-15 build:clang --host_action_env=CXX=clang++-15 build --define=UBUNTU_VERSION=24.04 + +# This flag tells our drake_cc.bzl rules which GCC version they should expect to +# see, so that the rules can tweak our warning flags. Note that this is not +# changing which compiler the build will use; it is only telling what to expect. +# +# Note that when building Drake via CMake, our cmake/bazelrc.in overrides this +# value to match the CC and CXX override that CMake has told Drake to use. +build --@drake//tools/cc_toolchain:compiler_major=13 diff --git a/tools/workspace/cc/repository.bzl b/tools/workspace/cc/repository.bzl index c22ddfe57371..2d26e5617b4c 100644 --- a/tools/workspace/cc/repository.bzl +++ b/tools/workspace/cc/repository.bzl @@ -142,6 +142,9 @@ def _impl(repository_ctx): file_content = """# DO NOT EDIT: generated by cc_repository() +print("DRAKE DEPRECATED: The @cc external is deprecated " + + "and will be removed from Drake on or after 2024-05-01.") + COMPILER_ID = "{}" COMPILER_VERSION_MAJOR = {} diff --git a/tools/workspace/default.bzl b/tools/workspace/default.bzl index 4ce1df39d21b..bdc7cf5ca27b 100644 --- a/tools/workspace/default.bzl +++ b/tools/workspace/default.bzl @@ -159,6 +159,7 @@ def add_default_repositories( if "buildifier" not in excludes: buildifier_repository(name = "buildifier", mirrors = mirrors) if "cc" not in excludes: + # Deprecated 2025-04-01. cc_repository(name = "cc") if "ccd_internal" not in excludes: ccd_internal_repository(name = "ccd_internal", mirrors = mirrors) diff --git a/tools/workspace/fcl_internal/package.BUILD.bazel b/tools/workspace/fcl_internal/package.BUILD.bazel index 7172ea50af2b..ad6c17fbb508 100644 --- a/tools/workspace/fcl_internal/package.BUILD.bazel +++ b/tools/workspace/fcl_internal/package.BUILD.bazel @@ -123,7 +123,10 @@ cc_library_vendored( x.replace("include/fcl/", "drake_hdr/fcl/") for x in _HDRS ], - copts = ["-fvisibility=hidden"], + copts = [ + "-fvisibility=hidden", + "-w", + ], defines = [ "FCL_STATIC_DEFINE", ],