From 9b6cf941461769becc06e30d4fd0c2007066abbd Mon Sep 17 00:00:00 2001 From: kush Date: Wed, 6 Sep 2017 20:49:57 +0200 Subject: Internal change PiperOrigin-RevId: 167751263 --- .../devtools/build/lib/rules/cpp/CcCommon.java | 65 +++---- .../build/lib/rules/cpp/CcLibraryHelper.java | 22 ++- .../build/lib/rules/cpp/CcToolchainFeatures.java | 35 +--- .../build/lib/rules/cpp/CcToolchainProvider.java | 6 +- .../build/lib/rules/cpp/CompileCommandLine.java | 75 ++++---- .../build/lib/rules/cpp/CppActionConfigs.java | 191 +++++++-------------- .../build/lib/rules/cpp/CppCompileAction.java | 7 +- .../lib/rules/cpp/CppCompileActionBuilder.java | 50 +++++- .../build/lib/rules/cpp/CppConfiguration.java | 75 +------- .../devtools/build/lib/rules/cpp/CppModel.java | 146 ++++------------ .../build/lib/rules/cpp/CppRuleClasses.java | 11 +- .../devtools/build/lib/rules/cpp/CppSemantics.java | 6 +- .../build/lib/rules/cpp/FakeCppCompileAction.java | 2 + .../build/lib/rules/objc/ObjcCppSemantics.java | 6 +- 14 files changed, 245 insertions(+), 452 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/rules') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index 47aa40a0ec..4856b0a1e6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.base.Joiner; -import com.google.common.base.Predicate; -import com.google.common.base.Predicates; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -165,15 +163,12 @@ public final class CcCommon { } } - if (!getCoptsFilter(ruleContext).apply("-Wno-future-warnings")) { - ruleContext.attributeWarning( - "nocopts", - String.format( - "Regular expression '%s' is too general; for example, it matches " - + "'-Wno-future-warnings'. Thus it might *re-enable* compiler warnings we wish " - + "to disable globally. To disable all compiler warnings, add '-w' to copts " - + "instead", - Preconditions.checkNotNull(getNoCoptsPattern(ruleContext)))); + Pattern nocopts = getNoCopts(ruleContext); + if (nocopts != null && nocopts.matcher("-Wno-future-warnings").matches()) { + ruleContext.attributeWarning("nocopts", + "Regular expression '" + nocopts.pattern() + "' is too general; for example, it matches " + + "'-Wno-future-warnings'. Thus it might *re-enable* compiler warnings we wish to " + + "disable globally. To disable all compiler warnings, add '-w' to copts instead"); } return ImmutableList.builder() @@ -354,36 +349,27 @@ public final class CcCommon { return ImmutableList.copyOf(CppHelper.expandMakeVariables(ruleContext, "copts", unexpanded)); } - /** Returns copts filter built from the make variable expanded nocopts attribute. */ - Predicate getCoptsFilter() { - return getCoptsFilter(ruleContext); + Pattern getNoCopts() { + return getNoCopts(ruleContext); } - /** @see CcCommon#getCoptsFilter() */ - private static Predicate getCoptsFilter(RuleContext ruleContext) { - Pattern noCoptsPattern = getNoCoptsPattern(ruleContext); - if (noCoptsPattern == null) { - return Predicates.alwaysTrue(); - } - return flag -> !noCoptsPattern.matcher(flag).matches(); - } - - @Nullable - private static Pattern getNoCoptsPattern(RuleContext ruleContext) { - if (!ruleContext.getRule().isAttrDefined(NO_COPTS_ATTRIBUTE, Type.STRING)) { - return null; - } - String nocoptsAttr = - ruleContext.expandMakeVariables( - NO_COPTS_ATTRIBUTE, ruleContext.attributes().get(NO_COPTS_ATTRIBUTE, Type.STRING)); - try { - return Pattern.compile(nocoptsAttr); - } catch (PatternSyntaxException e) { - ruleContext.attributeError( - NO_COPTS_ATTRIBUTE, - "invalid regular expression '" + nocoptsAttr + "': " + e.getMessage()); - return null; + /** + * Returns nocopts pattern built from the make variable expanded nocopts + * attribute. + */ + private static Pattern getNoCopts(RuleContext ruleContext) { + Pattern nocopts = null; + if (ruleContext.getRule().isAttrDefined(NO_COPTS_ATTRIBUTE, Type.STRING)) { + String nocoptsAttr = ruleContext.expandMakeVariables(NO_COPTS_ATTRIBUTE, + ruleContext.attributes().get(NO_COPTS_ATTRIBUTE, Type.STRING)); + try { + nocopts = Pattern.compile(nocoptsAttr); + } catch (PatternSyntaxException e) { + ruleContext.attributeError(NO_COPTS_ATTRIBUTE, + "invalid regular expression '" + nocoptsAttr + "': " + e.getMessage()); + } } + return nocopts; } // TODO(bazel-team): calculating nocopts every time is not very efficient, @@ -395,7 +381,8 @@ public final class CcCommon { * otherwise. */ static boolean noCoptsMatches(String option, RuleContext ruleContext) { - return !getCoptsFilter(ruleContext).apply(option); + Pattern nocopts = getNoCopts(ruleContext); + return nocopts == null ? false : nocopts.matcher(option).matches(); } private static final String DEFINES_ATTRIBUTE = "defines"; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java index 02c7c26002..a7ffab3456 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java @@ -19,7 +19,6 @@ import static java.util.stream.Collectors.toCollection; import com.google.common.base.Function; import com.google.common.base.Optional; -import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -68,6 +67,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.regex.Pattern; import javax.annotation.Nullable; /** @@ -266,7 +266,7 @@ public final class CcLibraryHelper { private final List nonCodeLinkerInputs = new ArrayList<>(); private ImmutableList copts = ImmutableList.of(); private final List linkopts = new ArrayList<>(); - private Predicate coptsFilter = Predicates.alwaysTrue(); + @Nullable private Pattern nocopts; private final Set defines = new LinkedHashSet<>(); private final List deps = new ArrayList<>(); private final List depContexts = new ArrayList<>(); @@ -378,7 +378,7 @@ public final class CcLibraryHelper { addLooseIncludeDirs(common.getLooseIncludeDirs()); addNonCodeLinkerInputs(common.getLinkerScripts()); addSystemIncludeDirs(common.getSystemIncludeDirs()); - setCoptsFilter(common.getCoptsFilter()); + setNoCopts(common.getNoCopts()); setHeadersCheckingMode(semantics.determineHeadersCheckingMode(ruleContext)); return this; } @@ -640,9 +640,11 @@ public final class CcLibraryHelper { return this; } - /** Sets a pattern that is used to filter copts; set to {@code null} for no filtering. */ - public CcLibraryHelper setCoptsFilter(Predicate coptsFilter) { - this.coptsFilter = Preconditions.checkNotNull(coptsFilter); + /** + * Sets a pattern that is used to filter copts; set to {@code null} for no filtering. + */ + public CcLibraryHelper setNoCopts(@Nullable Pattern nocopts) { + this.nocopts = nocopts; return this; } @@ -1148,7 +1150,12 @@ public final class CcLibraryHelper { */ private CppModel initializeCppModel() { return new CppModel( - ruleContext, semantics, ccToolchain, fdoSupport, configuration, copts, coptsFilter) + ruleContext, + semantics, + ccToolchain, + fdoSupport, + configuration, + copts) .addCompilationUnitSources(compilationUnitSources) .setLinkTargetType(linkType) .setNeverLink(neverlink) @@ -1159,6 +1166,7 @@ public final class CcLibraryHelper { // Note: this doesn't actually save the temps, it just makes the CppModel use the // configurations --save_temps setting to decide whether to actually save the temps. .setSaveTemps(true) + .setNoCopts(nocopts) .setDynamicLibrary(dynamicLibrary) .addLinkopts(linkopts) .setFeatureConfiguration(featureConfiguration) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java index 383e64fd3b..705dc80a26 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java @@ -35,7 +35,6 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariableValue; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; import java.io.IOException; @@ -1413,13 +1412,6 @@ public class CcToolchainFeatures implements Serializable { return this; } - /** Overrides a variable to expands {@code name} to {@code value} instead. */ - public Builder overrideStringSequenceVariable(String name, ImmutableList value) { - Preconditions.checkNotNull(value, "Cannot set null as a value for variable '%s'", name); - variablesMap.put(name, new StringSequence(value)); - return this; - } - /** * Add a sequence variable that expands {@code name} to {@code values}. * @@ -1516,8 +1508,10 @@ public class CcToolchainFeatures implements Serializable { return this; } - /** @return a new {@Variables} object. */ - public Variables build() { + /** + * @return a new {@Variables} object. + */ + Variables build() { return new Variables( ImmutableMap.copyOf(variablesMap), ImmutableMap.copyOf(stringVariablesMap)); } @@ -1738,27 +1732,6 @@ public class CcToolchainFeatures implements Serializable { return commandLine; } - /** @return the flags expanded for the given {@code action} in per-feature buckets. */ - public ImmutableList>> getPerFeatureExpansions( - String action, Variables variables) { - ImmutableList.Builder>> perFeatureExpansions = - ImmutableList.builder(); - if (actionIsConfigured(action)) { - List commandLine = new ArrayList<>(); - ActionConfig actionConfig = actionConfigByActionName.get(action); - actionConfig.expandCommandLine(variables, enabledFeatureNames, commandLine); - perFeatureExpansions.add(Pair.of(actionConfig.getName(), commandLine)); - } - - for (Feature feature : enabledFeatures) { - List commandLine = new ArrayList<>(); - feature.expandCommandLine(action, variables, enabledFeatureNames, commandLine); - perFeatureExpansions.add(Pair.of(feature.getName(), commandLine)); - } - - return perFeatureExpansions.build(); - } - /** @return the environment variables (key/value pairs) for the given {@code action}. */ ImmutableMap getEnvironmentVariables(String action, Variables variables) { ImmutableMap.Builder envBuilder = ImmutableMap.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java index 6526877565..4a7cc2d2d4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java @@ -354,12 +354,8 @@ public final class CcToolchainProvider extends ToolchainInfo { "Returns the default list of options which cannot be filtered by BUILD " + "rules. These should be appended to the command line after filtering." ) - public ImmutableList getUnfilteredCompilerOptionsWithSysroot(Iterable features) { - return cppConfiguration.getUnfilteredCompilerOptions(features, sysroot); - } - public ImmutableList getUnfilteredCompilerOptions(Iterable features) { - return cppConfiguration.getUnfilteredCompilerOptions(features, null); + return cppConfiguration.getUnfilteredCompilerOptions(features, sysroot); } @SkylarkCallable( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java index db43758289..3e1eaf69c4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.rules.cpp; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; @@ -23,10 +22,10 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfig import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile; import com.google.devtools.build.lib.util.FileType; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -38,6 +37,7 @@ public final class CompileCommandLine { private final Artifact outputFile; private final Label sourceLabel; private final Predicate coptsFilter; + private final Collection features; private final FeatureConfiguration featureConfiguration; private final CcToolchainFeatures.Variables variables; private final String actionName; @@ -50,6 +50,7 @@ public final class CompileCommandLine { Artifact outputFile, Label sourceLabel, Predicate coptsFilter, + Collection features, FeatureConfiguration featureConfiguration, CppConfiguration cppConfiguration, CcToolchainFeatures.Variables variables, @@ -60,6 +61,7 @@ public final class CompileCommandLine { this.outputFile = Preconditions.checkNotNull(outputFile); this.sourceLabel = Preconditions.checkNotNull(sourceLabel); this.coptsFilter = coptsFilter; + this.features = Preconditions.checkNotNull(features); this.featureConfiguration = Preconditions.checkNotNull(featureConfiguration); this.cppConfiguration = Preconditions.checkNotNull(cppConfiguration); this.variables = variables; @@ -109,10 +111,34 @@ public final class CompileCommandLine { return commandLine; } + private boolean isObjcCompile(String actionName) { + return (actionName.equals(CppCompileAction.OBJC_COMPILE) + || actionName.equals(CppCompileAction.OBJCPP_COMPILE)); + } + public List getCompilerOptions( @Nullable CcToolchainFeatures.Variables overwrittenVariables) { List options = new ArrayList<>(); + CppConfiguration toolchain = cppConfiguration; + + addFilteredOptions(options, toolchain.getCompilerOptions(features)); + String sourceFilename = sourceFile.getExecPathString(); + if (CppFileTypes.C_SOURCE.matches(sourceFilename)) { + addFilteredOptions(options, toolchain.getCOptions()); + } + if (CppFileTypes.CPP_SOURCE.matches(sourceFilename) + || CppFileTypes.CPP_HEADER.matches(sourceFilename) + || CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename) + || CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) { + addFilteredOptions(options, toolchain.getCxxOptions(features)); + } + + // TODO(bazel-team): This needs to be before adding getUnfilteredCompilerOptions() and after + // adding the warning flags until all toolchains are migrated; currently toolchains use the + // unfiltered compiler options to inject include paths, which is superseded by the feature + // configuration; on the other hand toolchains switch off warnings for the layering check + // that will be re-added by the feature flags. CcToolchainFeatures.Variables updatedVariables = variables; if (variables != null && overwrittenVariables != null) { CcToolchainFeatures.Variables.Builder variablesBuilder = @@ -121,14 +147,18 @@ public final class CompileCommandLine { variablesBuilder.addAndOverwriteAll(overwrittenVariables); updatedVariables = variablesBuilder.build(); } - addFilteredOptions( - options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables)); + addFilteredOptions(options, featureConfiguration.getCommandLine(actionName, updatedVariables)); + // Unfiltered compiler options contain system include paths. These must be added after + // the user provided options, otherwise users adding include paths will not pick up their + // own include paths first. if (isObjcCompile(actionName)) { PathFragment sysroot = cppProvider.getSysroot(); if (sysroot != null) { - options.add(cppConfiguration.getSysrootCompilerOption(sysroot)); + options.add(toolchain.getSysrootCompilerOption(sysroot)); } + } else { + options.addAll(cppProvider.getUnfilteredCompilerOptions(features)); } // Add the options of --per_file_copt, if the label or the base name of the source file @@ -156,22 +186,9 @@ public final class CompileCommandLine { return options; } - private boolean isObjcCompile(String actionName) { - return (actionName.equals(CppCompileAction.OBJC_COMPILE) - || actionName.equals(CppCompileAction.OBJCPP_COMPILE)); - } - // For each option in 'in', add it to 'out' unless it is matched by the 'coptsFilter' regexp. - private void addFilteredOptions( - List out, List>> expandedFeatures) { - for (Pair> pair : expandedFeatures) { - if (pair.getFirst().equals(CppRuleClasses.UNFILTERED_COMPILE_FLAGS_FEATURE_NAME)) { - out.addAll(pair.getSecond()); - continue; - } - - pair.getSecond().stream().filter(coptsFilter).forEachOrdered(out::add); - } + private void addFilteredOptions(List out, List in) { + in.stream().filter(coptsFilter).forEachOrdered(out::add); } public Artifact getSourceFile() { @@ -194,8 +211,8 @@ public final class CompileCommandLine { * explicit attribute, not using platform-dependent garbage bag that copts is). */ public ImmutableList getCopts() { - if (variables.isAvailable(CppModel.USER_COMPILE_FLAGS_VARIABLE_NAME)) { - return Variables.toStringList(variables, CppModel.USER_COMPILE_FLAGS_VARIABLE_NAME); + if (variables.isAvailable(CppModel.COPTS_VARIABLE_VALUE)) { + return Variables.toStringList(variables, CppModel.COPTS_VARIABLE_VALUE); } else { return ImmutableList.of(); } @@ -206,6 +223,7 @@ public final class CompileCommandLine { Artifact outputFile, Label sourceLabel, Predicate coptsFilter, + ImmutableList features, String actionName, CppConfiguration cppConfiguration, DotdFile dotdFile, @@ -215,6 +233,7 @@ public final class CompileCommandLine { outputFile, sourceLabel, coptsFilter, + features, actionName, cppConfiguration, dotdFile, @@ -226,7 +245,8 @@ public final class CompileCommandLine { private final Artifact sourceFile; private final Artifact outputFile; private final Label sourceLabel; - private Predicate coptsFilter; + private final Predicate coptsFilter; + private final Collection features; private FeatureConfiguration featureConfiguration; private CcToolchainFeatures.Variables variables = Variables.EMPTY; private final String actionName; @@ -240,6 +260,7 @@ public final class CompileCommandLine { Preconditions.checkNotNull(outputFile), Preconditions.checkNotNull(sourceLabel), Preconditions.checkNotNull(coptsFilter), + Preconditions.checkNotNull(features), Preconditions.checkNotNull(featureConfiguration), Preconditions.checkNotNull(cppConfiguration), Preconditions.checkNotNull(variables), @@ -253,6 +274,7 @@ public final class CompileCommandLine { Artifact outputFile, Label sourceLabel, Predicate coptsFilter, + Collection features, String actionName, CppConfiguration cppConfiguration, DotdFile dotdFile, @@ -261,6 +283,7 @@ public final class CompileCommandLine { this.outputFile = outputFile; this.sourceLabel = sourceLabel; this.coptsFilter = coptsFilter; + this.features = features; this.actionName = actionName; this.cppConfiguration = cppConfiguration; this.dotdFile = dotdFile; @@ -277,11 +300,5 @@ public final class CompileCommandLine { this.variables = variables; return this; } - - @VisibleForTesting - Builder setCoptsFilter(Predicate filter) { - this.coptsFilter = Preconditions.checkNotNull(filter); - return this; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index 958b26f7f6..8ed59a2fe0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -15,7 +15,7 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; +import java.util.Set; /** * A helper class for creating action_configs for the c++ actions. @@ -33,7 +33,7 @@ public class CppActionConfigs { public static String getCppActionConfigs( CppPlatform platform, - ImmutableSet existingFeatureNames, + Set features, String gccToolPath, String cppLinkDynamicLibraryToolPath, String arToolPath, @@ -41,8 +41,7 @@ public class CppActionConfigs { boolean supportsEmbeddedRuntimes, boolean supportsInterfaceSharedLibraries) { String cppDynamicLibraryLinkerTool = ""; - if (!existingFeatureNames.contains("dynamic_library_linker_tool") - && supportsInterfaceSharedLibraries) { + if (!features.contains("dynamic_library_linker_tool") && supportsInterfaceSharedLibraries) { cppDynamicLibraryLinkerTool = "" + "feature {" @@ -66,9 +65,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'preprocess-assemble'", @@ -76,9 +73,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c-compile'", @@ -86,9 +81,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c++-compile'", @@ -96,9 +89,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c++-header-parsing'", @@ -106,9 +97,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c++-header-preprocessing'", @@ -116,9 +105,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c++-module-compile'", @@ -126,9 +113,7 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", "action_config {", " config_name: 'c++-module-codegen'", @@ -136,33 +121,8 @@ public class CppActionConfigs { " tool {", " tool_path: '" + gccToolPath + "'", " }", - " implies: 'legacy_compile_flags'", - " implies: 'user_compile_flags'", - " implies: 'unfiltered_compile_flags'", + " implies: 'copts'", "}", - ifTrue( - !existingFeatureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS), - "feature {", - " name: 'legacy_compile_flags'", - " enabled: true", - " flag_set {", - " expand_if_all_available: 'legacy_compile_flags'", - " action: 'assemble'", - " action: 'preprocess-assemble'", - " action: 'c-compile'", - " action: 'c++-compile'", - " action: 'c++-header-parsing'", - " action: 'c++-header-preprocessing'", - " action: 'c++-module-compile'", - " action: 'c++-module-codegen'", - " action: 'lto-backend'", - " action: 'clif-match'", - " flag_group {", - " iterate_over: 'legacy_compile_flags'", - " flag: '%{legacy_compile_flags}'", - " }", - " }", - "}"), // Gcc options: // -MD turns on .d file output as a side-effect (doesn't imply -E) // -MM[D] enables user includes only, not system includes @@ -173,7 +133,7 @@ public class CppActionConfigs { // This combination gets user and system includes with specified name: // -MD -MF ifTrue( - !existingFeatureNames.contains(CppRuleClasses.DEPENDENCY_FILE), + !features.contains(CppRuleClasses.DEPENDENCY_FILE), "feature {", " name: 'dependency_file'", " flag_set {", @@ -202,7 +162,7 @@ public class CppActionConfigs { // any value which differs for all translation units; we use the // path to the object file. ifTrue( - !existingFeatureNames.contains(CppRuleClasses.RANDOM_SEED), + !features.contains(CppRuleClasses.RANDOM_SEED), "feature {", " name: 'random_seed'", " flag_set {", @@ -215,7 +175,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.PIC), + !features.contains(CppRuleClasses.PIC), "feature {", " name: 'pic'", " flag_set {", @@ -232,7 +192,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.PER_OBJECT_DEBUG_INFO), + !features.contains(CppRuleClasses.PER_OBJECT_DEBUG_INFO), "feature {", " name: 'per_object_debug_info'", " flag_set {", @@ -248,7 +208,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.PREPROCESSOR_DEFINES), + !features.contains(CppRuleClasses.PREPROCESSOR_DEFINES), "feature {", " name: 'preprocessor_defines'", " flag_set {", @@ -266,7 +226,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.INCLUDE_PATHS), + !features.contains(CppRuleClasses.INCLUDE_PATHS), "feature {", " name: 'include_paths'", " flag_set {", @@ -296,7 +256,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.FDO_INSTRUMENT), + !features.contains(CppRuleClasses.FDO_INSTRUMENT), "feature {", " name: 'fdo_instrument'", " provides: 'profile'", @@ -313,7 +273,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.FDO_OPTIMIZE), + !features.contains(CppRuleClasses.FDO_OPTIMIZE), "feature {", " name: 'fdo_optimize'", " provides: 'profile'", @@ -330,7 +290,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.AUTOFDO), + !features.contains(CppRuleClasses.AUTOFDO), "feature {", " name: 'autofdo'", " provides: 'profile'", @@ -345,7 +305,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.LIPO), + !features.contains(CppRuleClasses.LIPO), "feature {", " name: 'lipo'", " requires { feature: 'autofdo' }", @@ -458,7 +418,7 @@ public class CppActionConfigs { // follow right after build_interface_libraries. cppDynamicLibraryLinkerTool), ifTrue( - !existingFeatureNames.contains("symbol_counts"), + !features.contains("symbol_counts"), "feature {", " name: 'symbol_counts'", " flag_set {", @@ -471,7 +431,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("shared_flag"), + !features.contains("shared_flag"), "feature {", " name: 'shared_flag'", " flag_set {", @@ -482,7 +442,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("linkstamps"), + !features.contains("linkstamps"), "feature {", " name: 'linkstamps'", " flag_set {", @@ -496,7 +456,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("output_execpath_flags"), + !features.contains("output_execpath_flags"), "feature {", " name: 'output_execpath_flags'", " flag_set {", @@ -509,7 +469,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("output_execpath_flags_executable"), + !features.contains("output_execpath_flags_executable"), "feature {", " name: 'output_execpath_flags_executable'", " flag_set {", @@ -538,7 +498,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("runtime_library_search_directories"), + !features.contains("runtime_library_search_directories"), "feature {", " name: 'runtime_library_search_directories',", " flag_set {", @@ -563,7 +523,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("library_search_directories"), + !features.contains("library_search_directories"), "feature {", " name: 'library_search_directories'", " flag_set {", @@ -577,7 +537,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("archiver_flags"), + !features.contains("archiver_flags"), "feature {", " name: 'archiver_flags'", " flag_set {", @@ -598,7 +558,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("libraries_to_link"), + !features.contains("libraries_to_link"), "feature {", " name: 'libraries_to_link'", " flag_set {", @@ -759,7 +719,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("force_pic_flags"), + !features.contains("force_pic_flags"), "feature {", " name: 'force_pic_flags'", " flag_set {", @@ -771,7 +731,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("legacy_link_flags"), + !features.contains("legacy_link_flags"), "feature {", " name: 'legacy_link_flags'", " flag_set {", @@ -785,7 +745,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("fission_support"), + !features.contains("fission_support"), "feature {", " name: 'fission_support'", " flag_set {", @@ -799,7 +759,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("strip_debug_symbols"), + !features.contains("strip_debug_symbols"), "feature {", " name: 'strip_debug_symbols'", " flag_set {", @@ -813,7 +773,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains("linker_param_file"), + !features.contains("linker_param_file"), "feature {", " name: 'linker_param_file'", " flag_set {", @@ -836,7 +796,7 @@ public class CppActionConfigs { " }", "}"), ifTrue( - !existingFeatureNames.contains(CppRuleClasses.COVERAGE), + !features.contains(CppRuleClasses.COVERAGE), "feature {", " name: 'coverage'", "}", @@ -899,6 +859,28 @@ public class CppActionConfigs { " feature: 'coverage'", " }", "}"), + ifTrue( + !features.contains(CppRuleClasses.COPTS), + "feature {", + " name: 'copts'", + " flag_set {", + " expand_if_all_available: 'copts'", + " action: 'assemble'", + " action: 'preprocess-assemble'", + " action: 'c-compile'", + " action: 'c++-compile'", + " action: 'c++-header-parsing'", + " action: 'c++-header-preprocessing'", + " action: 'c++-module-compile'", + " action: 'c++-module-codegen'", + " action: 'lto-backend'", + " action: 'clif-match'", + " flag_group {", + " iterate_over: 'copts'", + " flag: '%{copts}'", + " }", + " }", + "}"), "action_config {", " config_name: 'strip'", " action_name: 'strip'", @@ -941,63 +923,6 @@ public class CppActionConfigs { "}")); } - public static String getFeaturesToAppearLastInToolchain( - ImmutableSet existingFeatureNames) { - return Joiner.on("\n") - .join( - ImmutableList.of( - ifTrue( - !existingFeatureNames.contains("user_compile_flags"), - "feature {", - " name: 'user_compile_flags'", - " enabled: true", - " flag_set {", - " expand_if_all_available: 'user_compile_flags'", - " action: 'assemble'", - " action: 'preprocess-assemble'", - " action: 'c-compile'", - " action: 'c++-compile'", - " action: 'c++-header-parsing'", - " action: 'c++-header-preprocessing'", - " action: 'c++-module-compile'", - " action: 'c++-module-codegen'", - " action: 'lto-backend'", - " action: 'clif-match'", - " flag_group {", - " iterate_over: 'user_compile_flags'", - " flag: '%{user_compile_flags}'", - " }", - " }", - "}"), - // unfiltered_compile_flags contain system include paths. These must be added - // after the user provided options (present in legacy_compile_flags build - // variable above), otherwise users adding include paths will not pick up their own - // include paths first. - ifTrue( - !existingFeatureNames.contains("unfiltered_compile_flags"), - "feature {", - " name: 'unfiltered_compile_flags'", - " enabled: true", - " flag_set {", - " expand_if_all_available: 'unfiltered_compile_flags'", - " action: 'assemble'", - " action: 'preprocess-assemble'", - " action: 'c-compile'", - " action: 'c++-compile'", - " action: 'c++-header-parsing'", - " action: 'c++-header-preprocessing'", - " action: 'c++-module-compile'", - " action: 'c++-module-codegen'", - " action: 'lto-backend'", - " action: 'clif-match'", - " flag_group {", - " iterate_over: 'unfiltered_compile_flags'", - " flag: '%{unfiltered_compile_flags}'", - " }", - " }", - "}"))); - } - private static String ifLinux(CppPlatform platform, String... lines) { return ifTrue(platform == CppPlatform.LINUX, lines); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index a8d3a38bbb..ed46336d53 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -250,6 +250,7 @@ public class CppCompileAction extends AbstractAction * * @param owner the owner of the action, usually the configured target that emitted it * @param allInputs the list of all action inputs. + * @param features TODO(bazel-team): Add parameter description. * @param featureConfiguration TODO(bazel-team): Add parameter description. * @param variables TODO(bazel-team): Add parameter description. * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must contain @@ -284,6 +285,9 @@ public class CppCompileAction extends AbstractAction protected CppCompileAction( ActionOwner owner, NestedSet allInputs, + // TODO(bazel-team): Eventually we will remove 'features'; all functionality in 'features' + // will be provided by 'featureConfiguration'. + ImmutableList features, FeatureConfiguration featureConfiguration, CcToolchainFeatures.Variables variables, Artifact sourceFile, @@ -348,6 +352,7 @@ public class CppCompileAction extends AbstractAction outputFile, sourceLabel, coptsFilter, + features, actionName, cppConfiguration, dotdFile, @@ -797,7 +802,7 @@ public class CppCompileAction extends AbstractAction */ @VisibleForTesting public List getCompilerOptions() { - return compileCommandLine.getCompilerOptions(/* overwrittenVariables= */ null); + return compileCommandLine.getCompilerOptions(/*updatedVariables=*/ null); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java index 1db66cabc7..6020ae906b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java @@ -38,11 +38,14 @@ import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.function.Consumer; +import java.util.regex.Pattern; /** * Builder class to construct C++ compile actions. @@ -52,6 +55,7 @@ public class CppCompileActionBuilder { private final ActionOwner owner; private final BuildConfiguration configuration; + private final List features = new ArrayList<>(); private CcToolchainFeatures.FeatureConfiguration featureConfiguration; private CcToolchainFeatures.Variables variables = Variables.EMPTY; private Artifact sourceFile; @@ -66,7 +70,7 @@ public class CppCompileActionBuilder { private Artifact gcnoFile; private CppCompilationContext context = CppCompilationContext.EMPTY; private final List pluginOpts = new ArrayList<>(); - private Predicate coptsFilter = Predicates.alwaysTrue(); + private final List nocopts = new ArrayList<>(); private ImmutableList extraSystemIncludePrefixes = ImmutableList.of(); private boolean usePic; private boolean allowUsingHeaderModules; @@ -97,6 +101,7 @@ public class CppCompileActionBuilder { sourceLabel, ruleContext.getConfiguration(), getLipoScannableMap(ruleContext), + ruleContext.getFeatures(), ccToolchain); } @@ -111,6 +116,7 @@ public class CppCompileActionBuilder { sourceLabel, configuration, getLipoScannableMap(ruleContext), + ruleContext.getFeatures(), ccToolchain); } @@ -120,12 +126,14 @@ public class CppCompileActionBuilder { Label sourceLabel, BuildConfiguration configuration, Map lipoScannableMap, + Set features, CcToolchainProvider ccToolchain) { this.owner = actionOwner; this.sourceLabel = sourceLabel; this.configuration = configuration; this.cppConfiguration = configuration.getFragment(CppConfiguration.class); this.lipoScannableMap = ImmutableMap.copyOf(lipoScannableMap); + this.features.addAll(features); this.mandatoryInputsBuilder = NestedSetBuilder.stableOrder(); this.allowUsingHeaderModules = true; this.localShellEnvironment = configuration.getLocalShellEnvironment(); @@ -153,6 +161,7 @@ public class CppCompileActionBuilder { */ public CppCompileActionBuilder(CppCompileActionBuilder other) { this.owner = other.owner; + this.features.addAll(other.features); this.featureConfiguration = other.featureConfiguration; this.sourceFile = other.sourceFile; this.sourceLabel = other.sourceLabel; @@ -167,7 +176,7 @@ public class CppCompileActionBuilder { this.gcnoFile = other.gcnoFile; this.context = other.context; this.pluginOpts.addAll(other.pluginOpts); - this.coptsFilter = other.coptsFilter; + this.nocopts.addAll(other.nocopts); this.extraSystemIncludePrefixes = ImmutableList.copyOf(other.extraSystemIncludePrefixes); this.specialInputsHandler = other.specialInputsHandler; this.actionClassId = other.actionClassId; @@ -207,6 +216,23 @@ public class CppCompileActionBuilder { return mandatoryInputsBuilder.build(); } + private static Predicate getNocoptPredicate(Collection patterns) { + final ImmutableList finalPatterns = ImmutableList.copyOf(patterns); + if (finalPatterns.isEmpty()) { + return Predicates.alwaysTrue(); + } else { + return option -> { + for (Pattern pattern : finalPatterns) { + if (pattern.matcher(option).matches()) { + return false; + } + } + + return true; + }; + } + } + private Iterable getLipoScannables(NestedSet realMandatoryInputs) { boolean fake = tempOutputFile != null; @@ -359,6 +385,7 @@ public class CppCompileActionBuilder { new FakeCppCompileAction( owner, allInputs, + ImmutableList.copyOf(features), featureConfiguration, variables, sourceFile, @@ -376,7 +403,7 @@ public class CppCompileActionBuilder { cppConfiguration, context, actionContext, - coptsFilter, + getNocoptPredicate(nocopts), getLipoScannables(realMandatoryInputs), cppSemantics, ccToolchain, @@ -386,6 +413,7 @@ public class CppCompileActionBuilder { new CppCompileAction( owner, allInputs, + ImmutableList.copyOf(features), featureConfiguration, variables, sourceFile, @@ -406,7 +434,7 @@ public class CppCompileActionBuilder { cppConfiguration, context, actionContext, - coptsFilter, + getNocoptPredicate(nocopts), specialInputsHandler, getLipoScannables(realMandatoryInputs), additionalIncludeFiles.build(), @@ -507,8 +535,10 @@ public class CppCompileActionBuilder { return this; } - /** Returns the build variables to be used for the action. */ - public CcToolchainFeatures.Variables getVariables() { + /** + * Returns the build variables to be used for the action. + */ + CcToolchainFeatures.Variables getVariables() { return variables; } @@ -644,6 +674,11 @@ public class CppCompileActionBuilder { return this; } + public CppCompileActionBuilder addNocopts(Pattern nocopts) { + this.nocopts.add(nocopts); + return this; + } + public CppCompileActionBuilder setContext(CppCompilationContext context) { this.context = context; return this; @@ -679,7 +714,4 @@ public class CppCompileActionBuilder { return ccToolchain; } - public void setCoptsFilter(Predicate coptsFilter) { - this.coptsFilter = Preconditions.checkNotNull(coptsFilter); - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index 539883bb72..a36781773f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -55,7 +55,6 @@ import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CTool import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LinkingModeFlags; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.ToolPath; -import com.google.protobuf.Descriptors.FieldDescriptor; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; import java.io.Serializable; @@ -632,13 +631,12 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } } - ImmutableSet featureNames = - toolchain - .getFeatureList() - .stream() - .map(feature -> feature.getName()) - .collect(ImmutableSet.toImmutableSet()); - if (!featureNames.contains(CppRuleClasses.NO_LEGACY_FEATURES)) { + ImmutableSet.Builder featuresBuilder = ImmutableSet.builder(); + for (CToolchain.Feature feature : toolchain.getFeatureList()) { + featuresBuilder.add(feature.getName()); + } + Set features = featuresBuilder.build(); + if (!features.contains(CppRuleClasses.NO_LEGACY_FEATURES)) { try { String gccToolPath = "DUMMY_GCC_TOOL"; String linkerToolPath = "DUMMY_LINKER_TOOL"; @@ -659,28 +657,10 @@ public class CppConfiguration extends BuildConfiguration.Fragment { stripToolPath = tool.getPath(); } } - - // TODO(b/30109612): Remove fragile legacyCompileFlags shuffle once there are no legacy - // crosstools. - // Existing projects depend on flags from legacy toolchain fields appearing first on the - // compile command line. 'legacy_compile_flags' feature contains all these flags, and so it - // needs to appear before other features from {@link CppActionConfigs}. - CToolchain.Feature legacyCompileFlagsFeature = - toolchain - .getFeatureList() - .stream() - .filter(feature -> feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS)) - .findFirst() - .orElse(null); - if (legacyCompileFlagsFeature != null) { - toolchainBuilder.addFeature(legacyCompileFlagsFeature); - toolchain = removeLegacyCompileFlagsFeatureFromToolchain(toolchain); - } - TextFormat.merge( CppActionConfigs.getCppActionConfigs( getTargetLibc().equals("macosx") ? CppPlatform.MAC : CppPlatform.LINUX, - featureNames, + features, gccToolPath, linkerToolPath, arToolPath, @@ -696,34 +676,9 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } toolchainBuilder.mergeFrom(toolchain); - - if (!featureNames.contains(CppRuleClasses.NO_LEGACY_FEATURES)) { - try { - TextFormat.merge( - CppActionConfigs.getFeaturesToAppearLastInToolchain(featureNames), toolchainBuilder); - } catch (ParseException e) { - // Can only happen if we change the proto definition without changing our - // configuration above. - throw new RuntimeException(e); - } - } return toolchainBuilder.build(); } - private CToolchain removeLegacyCompileFlagsFeatureFromToolchain(CToolchain toolchain) { - FieldDescriptor featuresFieldDescriptor = CToolchain.getDescriptor().findFieldByName("feature"); - return toolchain - .toBuilder() - .setField( - featuresFieldDescriptor, - toolchain - .getFeatureList() - .stream() - .filter(feature -> !feature.getName().equals(CppRuleClasses.LEGACY_COMPILE_FLAGS)) - .collect(ImmutableList.toImmutableList())) - .build(); - } - @VisibleForTesting static CompilationMode importCompilationMode(CrosstoolConfig.CompilationMode mode) { return CompilationMode.valueOf(mode.name()); @@ -2008,22 +1963,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return requestedFeatures.build(); } - public ImmutableList collectLegacyCompileFlags( - String sourceFilename, ImmutableSet features) { - ImmutableList.Builder legacyCompileFlags = ImmutableList.builder(); - legacyCompileFlags.addAll(getCompilerOptions(features)); - if (CppFileTypes.C_SOURCE.matches(sourceFilename)) { - legacyCompileFlags.addAll(getCOptions()); - } - if (CppFileTypes.CPP_SOURCE.matches(sourceFilename) - || CppFileTypes.CPP_HEADER.matches(sourceFilename) - || CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename) - || CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) { - legacyCompileFlags.addAll(getCxxOptions(features)); - } - return legacyCompileFlags.build(); - } - public static PathFragment computeDefaultSysroot(CToolchain toolchain) { PathFragment defaultSysroot = toolchain.getBuiltinSysroot().length() == 0 diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java index 0681bc040b..90e8965b49 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; -import com.google.common.base.Predicate; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -52,6 +50,8 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; +import javax.annotation.Nullable; /** * Representation of a C/C++ compilation. Its purpose is to share the code that creates compilation @@ -121,9 +121,6 @@ public final class CppModel { */ public static final String SYSTEM_INCLUDE_PATHS_VARIABLE_NAME = "system_include_paths"; - /** Name of the build variable for the dependency file path */ - public static final String DEPENDENCY_FILE_VARIABLE_NAME = "dependency_file"; - /** Name of the build variable for the collection of macros defined for preprocessor. */ public static final String PREPROCESSOR_DEFINES_VARIABLE_NAME = "preprocessor_defines"; @@ -140,24 +137,12 @@ public final class CppModel { /** Name of the build variable for the LTO indexing bitcode file. */ public static final String LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME = "lto_indexing_bitcode_file"; + /** Build variable for all user provided copt flags. */ + public static final String COPTS_VARIABLE_VALUE = "copts"; + /** Name of the build variable for stripopts for the strip action. */ public static final String STRIPOPTS_VARIABLE_NAME = "stripopts"; - /** - * Build variable for all flags coming from legacy crosstool fields, such as compiler_flag, - * optional_compiler_flag, cxx_flag, optional_cxx_flag. - */ - public static final String LEGACY_COMPILE_FLAGS_VARIABLE_NAME = "legacy_compile_flags"; - - /** - * Build variable for all flags coming from copt rule attribute, and from --copt, --cxxopt, or - * --conlyopt options. - */ - public static final String USER_COMPILE_FLAGS_VARIABLE_NAME = "user_compile_flags"; - - /** Build variable for flags coming from unfiltered_cxx_flag CROSSTOOL fields. */ - public static final String UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME = "unfiltered_compile_flags"; - private final CppSemantics semantics; private final RuleContext ruleContext; private final BuildConfiguration configuration; @@ -168,7 +153,7 @@ public final class CppModel { private final Set sourceFiles = new LinkedHashSet<>(); private final List mandatoryInputs = new ArrayList<>(); private final ImmutableList copts; - private final Predicate coptsFilter; + @Nullable private Pattern nocopts; private boolean fake; private boolean maySaveTemps; private boolean onlySingleOutput; @@ -187,7 +172,6 @@ public final class CppModel { private final CcToolchainProvider ccToolchain; private final FdoSupportProvider fdoSupport; private String linkedArtifactNameSuffix = ""; - private final ImmutableSet features; public CppModel( RuleContext ruleContext, @@ -195,31 +179,7 @@ public final class CppModel { CcToolchainProvider ccToolchain, FdoSupportProvider fdoSupport, ImmutableList copts) { - this( - ruleContext, - semantics, - ccToolchain, - fdoSupport, - ruleContext.getConfiguration(), - copts, - Predicates.alwaysTrue()); - } - - public CppModel( - RuleContext ruleContext, - CppSemantics semantics, - CcToolchainProvider ccToolchain, - FdoSupportProvider fdoSupport, - ImmutableList copts, - Predicate coptsFilter) { - this( - ruleContext, - semantics, - ccToolchain, - fdoSupport, - ruleContext.getConfiguration(), - copts, - coptsFilter); + this(ruleContext, semantics, ccToolchain, fdoSupport, ruleContext.getConfiguration(), copts); } public CppModel( @@ -228,17 +188,14 @@ public final class CppModel { CcToolchainProvider ccToolchain, FdoSupportProvider fdoSupport, BuildConfiguration configuration, - ImmutableList copts, - Predicate coptsFilter) { + ImmutableList copts) { this.ruleContext = Preconditions.checkNotNull(ruleContext); this.semantics = semantics; this.ccToolchain = Preconditions.checkNotNull(ccToolchain); this.fdoSupport = Preconditions.checkNotNull(fdoSupport); this.configuration = configuration; this.copts = copts; - this.coptsFilter = Preconditions.checkNotNull(coptsFilter); cppConfiguration = ruleContext.getFragment(CppConfiguration.class); - features = ruleContext.getFeatures(); } private Artifact getDwoFile(Artifact outputFile) { @@ -315,6 +272,15 @@ public final class CppModel { return this; } + /** + * Sets the nocopts pattern. This is used to filter out flags from the system defined set of + * flags. By default no filter is applied. + */ + public CppModel setNoCopts(@Nullable Pattern nocopts) { + this.nocopts = nocopts; + return this; + } + /** * Adds the given linkopts to the optional dynamic library link command. */ @@ -466,6 +432,10 @@ public final class CppModel { private CppCompileActionBuilder initializeCompileAction( Artifact sourceArtifact, Label sourceLabel) { CppCompileActionBuilder builder = createCompileActionBuilder(sourceArtifact, sourceLabel); + if (nocopts != null) { + builder.addNocopts(nocopts); + } + builder.setFeatureConfiguration(featureConfiguration); return builder; @@ -493,6 +463,8 @@ public final class CppModel { CcToolchainFeatures.Variables.Builder buildVariables = new CcToolchainFeatures.Variables.Builder(); + // TODO(bazel-team): Pull out string constants for all build variables. + CppCompilationContext builderContext = builder.getContext(); Artifact sourceFile = builder.getSourceFile(); Artifact outputFile = builder.getOutputFile(); @@ -500,19 +472,7 @@ public final class CppModel { buildVariables.addStringVariable(SOURCE_FILE_VARIABLE_NAME, sourceFile.getExecPathString()); buildVariables.addStringVariable(OUTPUT_FILE_VARIABLE_NAME, outputFile.getExecPathString()); - buildVariables.addStringSequenceVariable(USER_COMPILE_FLAGS_VARIABLE_NAME, copts); - - String sourceFilename = sourceFile.getExecPathString(); - buildVariables.addStringSequenceVariable( - LEGACY_COMPILE_FLAGS_VARIABLE_NAME, - cppConfiguration.collectLegacyCompileFlags(sourceFilename, features)); - - if (!CppFileTypes.OBJC_SOURCE.matches(sourceFilename) - && !CppFileTypes.OBJCPP_SOURCE.matches(sourceFilename)) { - buildVariables.addStringSequenceVariable( - UNFILTERED_COMPILE_FLAGS_VARIABLE_NAME, - ccToolchain.getUnfilteredCompilerOptionsWithSysroot(features)); - } + buildVariables.addStringSequenceVariable(COPTS_VARIABLE_VALUE, copts); if (builder.getTempOutputFile() != null) { realOutputFilePath = builder.getTempOutputFile().getPathString(); @@ -535,7 +495,7 @@ public final class CppModel { // Set dependency_file to enable .d file generation. if (dotdFile != null) { buildVariables.addStringVariable( - DEPENDENCY_FILE_VARIABLE_NAME, dotdFile.getSafeExecPath().getPathString()); + "dependency_file", dotdFile.getSafeExecPath().getPathString()); } if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS) && cppModuleMap != null) { @@ -746,11 +706,7 @@ public final class CppModel { builder.getContext().getCppModuleMap(), ImmutableMap.of()); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleContext); env.registerAction(compileAction); Artifact tokenFile = compileAction.getOutputFile(); @@ -812,11 +768,7 @@ public final class CppModel { builder.setDwoFile(dwoFile); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleContext); AnalysisEnvironment env = ruleContext.getAnalysisEnvironment(); env.registerAction(compileAction); @@ -875,13 +827,9 @@ public final class CppModel { /*dwoFile=*/ null, /*ltoIndexingFile=*/ null, builder.getContext().getCppModuleMap(), - /* sourceSpecificBuildVariables= */ ImmutableMap.of()); + /*sourceSpecificBuildVariables=*/ ImmutableMap.of()); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleContext); env.registerAction(compileAction); Artifact tokenFile = compileAction.getOutputFile(); @@ -962,11 +910,7 @@ public final class CppModel { picBuilder.setLtoIndexingFile(ltoIndexingFile); semantics.finalizeCompileActionBuilder( - ruleContext, - picBuilder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, picBuilder, featureConfiguration.getFeatureSpecification()); CppCompileAction picAction = picBuilder.buildOrThrowRuleError(ruleContext); env.registerAction(picAction); directOutputs.add(picAction.getOutputFile()); @@ -1033,11 +977,7 @@ public final class CppModel { builder.setLtoIndexingFile(ltoIndexingFile); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleContext); env.registerAction(compileAction); Artifact objectFile = compileAction.getOutputFile(); @@ -1078,11 +1018,7 @@ public final class CppModel { builder.getContext().getCppModuleMap(), source.getBuildVariables()); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); // Make sure this builder doesn't reference ruleContext outside of analysis phase. CppCompileActionTemplate actionTemplate = new CppCompileActionTemplate( sourceArtifact, @@ -1138,11 +1074,7 @@ public final class CppModel { builder.getContext().getCppModuleMap(), ImmutableMap.of()); semantics.finalizeCompileActionBuilder( - ruleContext, - builder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, builder, featureConfiguration.getFeatureSpecification()); CppCompileAction action = builder.buildOrThrowRuleError(ruleContext); env.registerAction(action); if (addObject) { @@ -1472,11 +1404,7 @@ public final class CppModel { builder.getContext().getCppModuleMap(), ImmutableMap.of()); semantics.finalizeCompileActionBuilder( - ruleContext, - dBuilder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, dBuilder, featureConfiguration.getFeatureSpecification()); CppCompileAction dAction = dBuilder.buildOrThrowRuleError(ruleContext); ruleContext.registerAction(dAction); @@ -1494,11 +1422,7 @@ public final class CppModel { builder.getContext().getCppModuleMap(), ImmutableMap.of()); semantics.finalizeCompileActionBuilder( - ruleContext, - sdBuilder, - featureConfiguration.getFeatureSpecification(), - coptsFilter, - features); + ruleContext, sdBuilder, featureConfiguration.getFeatureSpecification()); CppCompileAction sdAction = sdBuilder.buildOrThrowRuleError(ruleContext); ruleContext.registerAction(sdAction); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index b099854b14..06e4c9b687 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java @@ -176,8 +176,8 @@ public class CppRuleClasses { public static final SafeImplicitOutputsFunction CC_BINARY_DEBUG_PACKAGE = fromTemplates("%{name}.dwp"); - /** Name of the feature that will be exempt from flag filtering when nocopts are used */ - public static final String UNFILTERED_COMPILE_FLAGS_FEATURE_NAME = "unfiltered_compile_flags"; + /** A string constant for the copts feature. */ + public static final String COPTS = "copts"; /** * A string constant for the parse_headers feature. @@ -288,13 +288,6 @@ public class CppRuleClasses { */ public static final String NO_LEGACY_FEATURES = "no_legacy_features"; - /** - * A string constant for the legacy_compile_flags feature. If this feature is present in the - * toolchain, and the toolchain doesn't specify no_legacy_features, bazel will move - * legacy_compile_flags before other features from {@link CppActionConfigs}. - */ - public static final String LEGACY_COMPILE_FLAGS = "legacy_compile_flags"; - /** * A string constant for the feature that makes us build per-object debug info files. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java index 3331cb1e0f..dfdafb6ded 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; -import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -42,9 +40,7 @@ public interface CppSemantics { void finalizeCompileActionBuilder( RuleContext ruleContext, CppCompileActionBuilder actionBuilder, - FeatureSpecification featureSpecification, - Predicate coptsFilter, - ImmutableSet features); + FeatureSpecification featureSpecification); /** * Called before {@link CppCompilationContext}s are finalized. diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java index 3ca810051b..bf6b9fdc70 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java @@ -56,6 +56,7 @@ public class FakeCppCompileAction extends CppCompileAction { FakeCppCompileAction( ActionOwner owner, NestedSet allInputs, + ImmutableList features, FeatureConfiguration featureConfiguration, CcToolchainFeatures.Variables variables, Artifact sourceFile, @@ -81,6 +82,7 @@ public class FakeCppCompileAction extends CppCompileAction { super( owner, allInputs, + features, featureConfiguration, variables, sourceFile, diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java index 54aa46e80b..27f920b103 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java @@ -18,7 +18,6 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.DYNAMIC_FRAM import static com.google.devtools.build.lib.rules.objc.ObjcProvider.HEADER; import static com.google.devtools.build.lib.rules.objc.ObjcProvider.STATIC_FRAMEWORK_FILE; -import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; @@ -98,16 +97,13 @@ public class ObjcCppSemantics implements CppSemantics { public void finalizeCompileActionBuilder( RuleContext ruleContext, CppCompileActionBuilder actionBuilder, - FeatureSpecification featureSpecification, - Predicate coptsFilter, - ImmutableSet features) { + FeatureSpecification featureSpecification) { actionBuilder.setCppConfiguration(ruleContext.getFragment(CppConfiguration.class)); actionBuilder.setActionContext(CppCompileActionContext.class); // Because Bazel does not support include scanning, we need the entire crosstool filegroup, // including header files, as opposed to just the "compile" filegroup. actionBuilder.addTransitiveMandatoryInputs(actionBuilder.getToolchain().getCrosstool()); actionBuilder.setShouldScanIncludes(false); - actionBuilder.setCoptsFilter(coptsFilter); actionBuilder.addTransitiveMandatoryInputs(objcProvider.get(STATIC_FRAMEWORK_FILE)); actionBuilder.addTransitiveMandatoryInputs(objcProvider.get(DYNAMIC_FRAMEWORK_FILE)); -- cgit v1.2.3