From 5854788d6061aee73791760b74b8a0d7de1d31d8 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Thu, 3 Oct 2024 20:28:31 -0700 Subject: [PATCH] Add conlyopts and cxxopts attributes to cc rules The inability to pass C or C++ specific compiler flags to targets that contain a mix of those sources is a common sticking point for new users. These mirror the global `--conlyopt` and `--cxxopt` flags but at the target level. Fixes https://github.com/bazelbuild/bazel/issues/22041 RELNOTES: Add conlyopts and cxxopts attributes to cc rules Closes #23792. PiperOrigin-RevId: 682144094 Change-Id: I0fe8b728c493652d875ce6a6dd2a9989c36b1f24 --- .../lib/rules/cpp/CcCompilationHelper.java | 30 ++++++++++++- .../build/lib/rules/cpp/CcModule.java | 4 ++ .../lib/starlarkbuildapi/cpp/CcModuleApi.java | 14 +++++++ .../starlark/builtins_bzl/common/cc/attrs.bzl | 12 +++++- .../builtins_bzl/common/cc/cc_binary.bzl | 4 +- .../builtins_bzl/common/cc/cc_common.bzl | 4 ++ .../builtins_bzl/common/cc/cc_helper.bzl | 8 ++-- .../builtins_bzl/common/cc/cc_library.bzl | 4 +- .../rules/cpp/CompileBuildVariablesTest.java | 42 +++++++++++++++++++ 9 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index 97f3ece8536118..f1cb9dc44056bd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -271,6 +271,8 @@ public CcCompilationContext getCcCompilationContext() { private final LinkedHashMap compilationUnitSources = new LinkedHashMap<>(); private final LinkedHashMap moduleInterfaceSources = new LinkedHashMap<>(); private ImmutableList copts = ImmutableList.of(); + private ImmutableList conlyopts = ImmutableList.of(); + private ImmutableList cxxopts = ImmutableList.of(); private CoptsFilter coptsFilter = CoptsFilter.alwaysPasses(); private final Set defines = new LinkedHashSet<>(); private final Set localDefines = new LinkedHashSet<>(); @@ -615,6 +617,18 @@ public void setCoptsFilter(CoptsFilter coptsFilter) { this.coptsFilter = Preconditions.checkNotNull(coptsFilter); } + @CanIgnoreReturnValue + public CcCompilationHelper setConlyopts(ImmutableList copts) { + this.conlyopts = Preconditions.checkNotNull(copts); + return this; + } + + @CanIgnoreReturnValue + public CcCompilationHelper setCxxopts(ImmutableList copts) { + this.cxxopts = Preconditions.checkNotNull(copts); + return this; + } + /** * Adds the given defines to the compiler command line of this target as well as its dependent * targets. @@ -1270,9 +1284,21 @@ public static ImmutableList getCoptsFromOptions( private ImmutableList getCopts(Artifact sourceFile, Label sourceLabel) { ImmutableList.Builder coptsList = ImmutableList.builder(); - coptsList.addAll( - getCoptsFromOptions(cppConfiguration, semantics, sourceFile.getExecPathString())); + String sourceFilename = sourceFile.getExecPathString(); + coptsList.addAll(getCoptsFromOptions(cppConfiguration, semantics, sourceFilename)); coptsList.addAll(copts); + + if (CppFileTypes.C_SOURCE.matches(sourceFilename)) { + coptsList.addAll(conlyopts); + } + + if (CppFileTypes.CPP_SOURCE.matches(sourceFilename) + || CppFileTypes.CPP_HEADER.matches(sourceFilename) + || CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename) + || CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) { + coptsList.addAll(cxxopts); + } + if (sourceFile != null && sourceLabel != null) { coptsList.addAll(collectPerFileCopts(sourceFile, sourceLabel)); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index b56c713203904c..ca3ef1601351f4 100755 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -2049,6 +2049,8 @@ public Tuple compile( String includePrefix, String stripIncludePrefix, Sequence userCompileFlags, // expected + Sequence conlyFlags, // expected + Sequence cxxFlags, // expected Sequence ccCompilationContexts, // expected Object implementationCcCompilationContextsObject, String name, @@ -2216,6 +2218,8 @@ public Tuple compile( .setCopts( ImmutableList.copyOf( Sequence.cast(userCompileFlags, String.class, "user_compile_flags"))) + .setConlyopts(ImmutableList.copyOf(Sequence.cast(conlyFlags, String.class, "conly_flags"))) + .setCxxopts(ImmutableList.copyOf(Sequence.cast(cxxFlags, String.class, "cxx_flags"))) .addAdditionalCompilationInputs( Sequence.cast(additionalInputs, Artifact.class, "additional_inputs")) .addAdditionalInputs(nonCompilationAdditionalInputs) diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java index db81e5588a8910..0867415556f026 100755 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java @@ -233,6 +233,18 @@ default void compilerFlagExists() {} positional = false, named = true, defaultValue = "[]"), + @Param( + name = "conly_flags", + doc = "Additional list of compilation options for C compiles.", + positional = false, + named = true, + defaultValue = "[]"), + @Param( + name = "cxx_flags", + doc = "Additional list of compilation options for C++ compiles.", + positional = false, + named = true, + defaultValue = "[]"), @Param( name = "compilation_contexts", doc = "Headers from dependencies used for compilation.", @@ -395,6 +407,8 @@ Tuple compile( String includePrefix, String stripIncludePrefix, Sequence userCompileFlags, // expected + Sequence conlyFlags, // expected + Sequence cxxFlags, // expected Sequence ccCompilationContexts, // expected Object implementationCcCompilationContextsObject, String name, diff --git a/src/main/starlark/builtins_bzl/common/cc/attrs.bzl b/src/main/starlark/builtins_bzl/common/cc/attrs.bzl index f02ab98356b08f..808c5d44d6c24b 100644 --- a/src/main/starlark/builtins_bzl/common/cc/attrs.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/attrs.bzl @@ -163,7 +163,7 @@ is prepended with -D and added to the compile command line for this but not to its dependents. """), "copts": attr.string_list(doc = """ -Add these options to the C++ compilation command. +Add these options to the C/C++ compilation command. Subject to "Make variable" substitution and Bourne shell tokenization.

@@ -178,6 +178,16 @@ Subject to "Make variable" substitution and no_copts_tokenization, Bourne shell tokenization applies only to strings that consist of a single "Make" variable.

+"""), + "conlyopts": attr.string_list(doc = """ +Add these options to the C compilation command. +Subject to "Make variable" substitution and +Bourne shell tokenization. +"""), + "cxxopts": attr.string_list(doc = """ +Add these options to the C++ compilation command. +Subject to "Make variable" substitution and +Bourne shell tokenization. """), "hdrs_check": attr.string( doc = "Deprecated, no-op.", diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl index 40b637694db45b..8203180c2a1ff8 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl @@ -502,7 +502,9 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False): actions = ctx.actions, feature_configuration = feature_configuration, cc_toolchain = cc_toolchain, - user_compile_flags = runtimes_copts + cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions), + user_compile_flags = runtimes_copts + cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "copts"), + conly_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "conlyopts"), + cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"), defines = cc_helper.defines(ctx, additional_make_variable_substitutions), local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps), system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions), diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl index 0a578b88163802..f516506add9605 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl @@ -675,6 +675,8 @@ def _compile( include_prefix = "", strip_include_prefix = "", user_compile_flags = [], + conly_flags = [], + cxx_flags = [], compilation_contexts = [], implementation_compilation_contexts = _UNBOUND, disallow_pic_outputs = False, @@ -760,6 +762,8 @@ def _compile( include_prefix = include_prefix, strip_include_prefix = strip_include_prefix, user_compile_flags = user_compile_flags, + conly_flags = conly_flags, + cxx_flags = cxx_flags, compilation_contexts = compilation_contexts, implementation_compilation_contexts = implementation_compilation_contexts, disallow_pic_outputs = disallow_pic_outputs, diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl index 92b6864f05fc32..5292b08cbfd9e9 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl @@ -924,10 +924,10 @@ def _expand_make_variables_for_copts(ctx, tokenization, unexpanded_tokens, addit tokens.append(_expand(ctx, token, additional_make_variable_substitutions, targets = targets)) return tokens -def _get_copts(ctx, feature_configuration, additional_make_variable_substitutions): - if not hasattr(ctx.attr, "copts"): - fail("could not find rule attribute named: 'copts'") - attribute_copts = ctx.attr.copts +def _get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "copts"): + if not hasattr(ctx.attr, attr): + fail("could not find rule attribute named: '{}'".format(attr)) + attribute_copts = getattr(ctx.attr, attr) tokenization = not (cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "no_copts_tokenization") or "no_copts_tokenization" in ctx.features) expanded_attribute_copts = _expand_make_variables_for_copts(ctx, tokenization, attribute_copts, additional_make_variable_substitutions) return expanded_attribute_copts diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl index 5a68c89be5d18f..a62767520d45cb 100755 --- a/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl @@ -58,7 +58,9 @@ def _cc_library_impl(ctx): name = ctx.label.name, cc_toolchain = cc_toolchain, feature_configuration = feature_configuration, - user_compile_flags = runtimes_copts + cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions), + user_compile_flags = runtimes_copts + cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "copts"), + conly_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "conlyopts"), + cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"), defines = cc_helper.defines(ctx, additional_make_variable_substitutions), local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps + ctx.attr.implementation_deps), system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions), diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java index 241902a3c12afd..28c701109e6af7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java @@ -103,6 +103,48 @@ public void testPresenceOfUserCompileFlags() throws Exception { assertThat(copts).contains("-foo"); } + @Test + public void testPresenceOfConlyFlags() throws Exception { + useConfiguration( + "--conlyopt=-foo", "--cxxopt=-not-passed", "--per_file_copt=//x:bin@-per-file"); + + scratch.file( + "x/BUILD", + "cc_binary(name = 'bin', srcs = ['bin.c'], copts = ['-bar'], conlyopts = ['-baz'], cxxopts" + + " = ['-not-passed'])"); + scratch.file("x/bin.c"); + + CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin"); + + ImmutableList copts = + CcToolchainVariables.toStringList( + variables, CompileBuildVariables.USER_COMPILE_FLAGS.getVariableName(), PathMapper.NOOP); + assertThat(copts) + .containsExactlyElementsIn(ImmutableList.of("-foo", "-bar", "-baz", "-per-file")) + .inOrder(); + } + + @Test + public void testCxxFlagsOrder() throws Exception { + useConfiguration( + "--cxxopt=-foo", "--conlyopt=-not-passed", "--per_file_copt=//x:bin@-per-file"); + + scratch.file( + "x/BUILD", + "cc_binary(name = 'bin', srcs = ['bin.cc'], copts = ['-bar'], cxxopts = ['-baz'], conlyopts" + + " = ['-not-passed'])"); + scratch.file("x/bin.cc"); + + CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin"); + + ImmutableList copts = + CcToolchainVariables.toStringList( + variables, CompileBuildVariables.USER_COMPILE_FLAGS.getVariableName(), PathMapper.NOOP); + assertThat(copts) + .containsExactlyElementsIn(ImmutableList.of("-foo", "-bar", "-baz", "-per-file")) + .inOrder(); + } + @Test public void testPerFileCoptsAreInUserCompileFlags() throws Exception { scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['bin.cc'])");