diff options
author | hlopko <hlopko@google.com> | 2018-02-08 05:26:53 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-02-08 05:28:55 -0800 |
commit | 943afc78c1170e93e154edd1ef17389bae248e6b (patch) | |
tree | 0933bf969f30849e7b09407095e73a5ac7b53440 /src/main/java/com | |
parent | b116240ea496b95f8846abd76f29416b0bdc9cc9 (diff) |
Introduce -c source_file -o output_file build variables
Prior to this cl CompileCommandLine would (almost) unconditionally emit -c and
-o flags. This cl removes this logic and relies on crosstool to emit these
flags. This is another small step towards platform independent C++ rules.
Memory use is not affected, since the build variables used by this cl are already
exposed, this cl just forces crosstools to use it.
Encore of https://github.com/bazelbuild/bazel/commit/f26e8694ae78599b3e2004e3360eaf3443fa53a6.
RELNOTES: None.
PiperOrigin-RevId: 184981106
Diffstat (limited to 'src/main/java/com')
9 files changed, 111 insertions, 81 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java index e8d5a93d1c..1ac7da832d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.actions; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; import java.util.List; @@ -33,5 +34,6 @@ public interface CommandAction extends Action, ExecutionInfoSpecifier { ImmutableMap<String, String> getEnvironment(); /** Returns inputs to this action, including inputs that may be pruned. */ + @VisibleForTesting // productionVisibility = Visibility.PRIVATE Iterable<Artifact> getPossibleInputsForTesting(); } 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 8d599be1f1..fa2c96e789 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 @@ -119,7 +119,6 @@ public final class CcCommon { private static final ImmutableSet<String> DEFAULT_FEATURES = ImmutableSet.of( CppRuleClasses.DEPENDENCY_FILE, - CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET, CppRuleClasses.RANDOM_SEED, CppRuleClasses.MODULE_MAPS, CppRuleClasses.MODULE_MAP_HOME_CWD, 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 5f2a3d8a52..7eef57f135 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 @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile; import com.google.devtools.build.lib.skyframe.serialization.InjectingObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; -import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.FileSystemProvider; import com.google.devtools.build.lib.vfs.PathFragment; @@ -40,7 +39,6 @@ public final class CompileCommandLine { new CompileCommandLine_AutoCodec(); private final Artifact sourceFile; - private final Artifact outputFile; private final CoptsFilter coptsFilter; private final FeatureConfiguration featureConfiguration; private final PathFragment crosstoolTopPathFragment; @@ -52,7 +50,6 @@ public final class CompileCommandLine { @VisibleForSerialization CompileCommandLine( Artifact sourceFile, - Artifact outputFile, CoptsFilter coptsFilter, FeatureConfiguration featureConfiguration, PathFragment crosstoolTopPathFragment, @@ -60,7 +57,6 @@ public final class CompileCommandLine { String actionName, DotdFile dotdFile) { this.sourceFile = Preconditions.checkNotNull(sourceFile); - this.outputFile = Preconditions.checkNotNull(outputFile); this.coptsFilter = coptsFilter; this.featureConfiguration = Preconditions.checkNotNull(featureConfiguration); this.crosstoolTopPathFragment = crosstoolTopPathFragment; @@ -80,8 +76,12 @@ public final class CompileCommandLine { return featureConfiguration.getEnvironmentVariables(actionName, variables); } - protected List<String> getArgv( - PathFragment outputFile, CcToolchainFeatures.Variables overwrittenVariables) { + /** + * @param overwrittenVariables: Variables that will overwrite original build variables. When null, + * unmodified original variables are used. + */ + protected List<String> getArguments( + @Nullable CcToolchainFeatures.Variables overwrittenVariables) { List<String> commandLine = new ArrayList<>(); // first: The command name. @@ -96,17 +96,6 @@ public final class CompileCommandLine { // second: The compiler options. commandLine.addAll(getCompilerOptions(overwrittenVariables)); - - if (!featureConfiguration.isEnabled(CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET)) { - // third: The file to compile! - commandLine.add("-c"); - commandLine.add(sourceFile.getExecPathString()); - - // finally: The output file. (Prefixed with -o). - commandLine.add("-o"); - commandLine.add(outputFile.getPathString()); - } - return commandLine; } @@ -124,19 +113,6 @@ public final class CompileCommandLine { addFilteredOptions( options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables)); - if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) { - if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) { - options.add("-S"); - } else if (FileType.contains( - outputFile, - CppFileTypes.PREPROCESSED_C, - CppFileTypes.PREPROCESSED_CPP, - CppFileTypes.PIC_PREPROCESSED_C, - CppFileTypes.PIC_PREPROCESSED_CPP)) { - options.add("-E"); - } - } - return options; } @@ -182,19 +158,16 @@ public final class CompileCommandLine { public static Builder builder( Artifact sourceFile, - Artifact outputFile, CoptsFilter coptsFilter, String actionName, PathFragment crosstoolTopPathFragment, DotdFile dotdFile) { - return new Builder( - sourceFile, outputFile, coptsFilter, actionName, crosstoolTopPathFragment, dotdFile); + return new Builder(sourceFile, coptsFilter, actionName, crosstoolTopPathFragment, dotdFile); } /** A builder for a {@link CompileCommandLine}. */ public static final class Builder { private final Artifact sourceFile; - private final Artifact outputFile; private CoptsFilter coptsFilter; private FeatureConfiguration featureConfiguration; private CcToolchainFeatures.Variables variables = Variables.EMPTY; @@ -205,7 +178,6 @@ public final class CompileCommandLine { public CompileCommandLine build() { return new CompileCommandLine( Preconditions.checkNotNull(sourceFile), - Preconditions.checkNotNull(outputFile), Preconditions.checkNotNull(coptsFilter), Preconditions.checkNotNull(featureConfiguration), Preconditions.checkNotNull(crosstoolTopPathFragment), @@ -216,13 +188,11 @@ public final class CompileCommandLine { private Builder( Artifact sourceFile, - Artifact outputFile, CoptsFilter coptsFilter, String actionName, PathFragment crosstoolTopPathFragment, DotdFile dotdFile) { this.sourceFile = sourceFile; - this.outputFile = outputFile; this.coptsFilter = coptsFilter; this.actionName = actionName; this.crosstoolTopPathFragment = crosstoolTopPathFragment; 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 bab2726259..a12e21ddad 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 @@ -70,6 +70,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'preprocess-assemble'", @@ -81,6 +83,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'linkstamp-compile'", @@ -92,6 +96,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c-compile'", @@ -103,6 +109,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-compile'", @@ -114,6 +122,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-header-parsing'", @@ -125,6 +135,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-header-preprocessing'", @@ -136,6 +148,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-module-compile'", @@ -147,6 +161,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", "action_config {", " config_name: 'c++-module-codegen'", @@ -158,6 +174,8 @@ public class CppActionConfigs { " implies: 'user_compile_flags'", " implies: 'sysroot'", " implies: 'unfiltered_compile_flags'", + " implies: 'compiler_input_flags'", + " implies: 'compiler_output_flags'", "}", ifTrue( !existingFeatureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS), @@ -1060,6 +1078,68 @@ public class CppActionConfigs { " flag: '@%{linker_param_file}'", " }", " }", + "}"), + ifTrue( + !existingFeatureNames.contains("compiler_input_flags"), + "feature {", + " name: 'compiler_input_flags'", + " enabled: true", + " flag_set {", + " action: 'assemble'", + " action: 'preprocess-assemble'", + " action: 'c-compile'", + " action: 'c++-compile'", + " action: 'linkstamp-compile'", + " action: 'c++-module-compile'", + " action: 'c++-module-codegen'", + " action: 'objc-compile'", + " action: 'objc++-compile'", + " action: 'c++-header-preprocessing'", + " action: 'c++-header-parsing'", + " action: 'lto-backend'", + " expand_if_all_available: 'source_file'", + " flag_group {", + " flag: '-c'", + " flag: '%{source_file}'", + " }", + " }", + "}"), + ifTrue( + !existingFeatureNames.contains("compiler_output_flags"), + "feature {", + " name: 'compiler_output_flags'", + " enabled: true", + " flag_set {", + " action: 'assemble'", + " action: 'preprocess-assemble'", + " action: 'c-compile'", + " action: 'c++-compile'", + " action: 'linkstamp-compile'", + " action: 'c++-module-compile'", + " action: 'c++-module-codegen'", + " action: 'objc-compile'", + " action: 'objc++-compile'", + " action: 'c++-header-preprocessing'", + " action: 'c++-header-parsing'", + " action: 'lto-backend'", + " flag_group {", + " expand_if_all_available: 'output_object_file'", + " flag: '-o'", + " flag: '%{output_object_file}'", + " }", + " flag_group {", + " expand_if_all_available: 'output_assembly_file'", + " flag: '-S'", + " flag: '-o'", + " flag: '%{output_assembly_file}'", + " }", + " flag_group {", + " expand_if_all_available: 'output_preprocess_file'", + " flag: '-E'", + " flag: '-o'", + " flag: '%{output_preprocess_file}'", + " }", + " }", "}"))); } 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 15cf6a7bfc..91534e580c 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 @@ -322,7 +322,6 @@ public class CppCompileAction extends AbstractAction this.compileCommandLine = CompileCommandLine.builder( sourceFile, - outputFile, coptsFilter, actionName, crosstoolTopPathFragment, @@ -624,7 +623,7 @@ public class CppCompileAction extends AbstractAction @Override public List<String> getCmdlineIncludes() { ImmutableList.Builder<String> cmdlineIncludes = ImmutableList.builder(); - List<String> args = getArgv(); + List<String> args = getArguments(); for (Iterator<String> argi = args.iterator(); argi.hasNext();) { String arg = argi.next(); if (arg.equals("-include") && argi.hasNext()) { @@ -686,21 +685,9 @@ public class CppCompileAction extends AbstractAction return ImmutableMap.copyOf(environment); } - /** - * Returns a new, mutable list of command and arguments (argv) to be passed - * to the gcc subprocess. - */ - public final List<String> getArgv() { - return getArgv(getInternalOutputFile()); - } - - protected final List<String> getArgv(PathFragment outputFile) { - return compileCommandLine.getArgv(outputFile, overwrittenVariables); - } - @Override public List<String> getArguments() { - return getArgv(); + return compileCommandLine.getArguments(overwrittenVariables); } @Override @@ -1050,7 +1037,8 @@ public class CppCompileAction extends AbstractAction public ResourceSet estimateResourceConsumptionLocal() { // We use a local compile, so much of the time is spent waiting for IO, // but there is still significant CPU; hence we estimate 50% cpu usage. - return ResourceSet.createWithRamCpuIo(/*memoryMb=*/200, /*cpuUsage=*/0.5, /*ioUsage=*/0.0); + return ResourceSet.createWithRamCpuIo( + /* memoryMb= */ 200, /* cpuUsage= */ 0.5, /* ioUsage= */ 0.0); } @Override @@ -1067,10 +1055,10 @@ public class CppCompileAction extends AbstractAction // itself is fully determined by the input source files and module maps. // A better long-term solution would be to make the compiler to find them automatically and // never hand in the .pcm files explicitly on the command line in the first place. - f.addStrings(compileCommandLine.getArgv(getInternalOutputFile(), null)); + f.addStrings(compileCommandLine.getArguments(/* overwrittenVariables= */ null)); /* - * getArgv() above captures all changes which affect the compilation + * getArguments() above captures all changes which affect the compilation * command and hence the contents of the object file. But we need to * also make sure that we reexecute the action if any of the fields * that affect whether validateIncludes() will report an error or warning @@ -1313,9 +1301,9 @@ public class CppCompileAction extends AbstractAction message.append(getProgressMessage()); message.append('\n'); // Outputting one argument per line makes it easier to diff the results. - // The first element in getArgv() is actually the command to execute. + // The first element in getArguments() is actually the command to execute. String legend = " Command: "; - for (String argument : ShellEscaper.escapeAll(getArgv())) { + for (String argument : ShellEscaper.escapeAll(getArguments())) { message.append(legend); message.append(argument); message.append('\n'); 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 7940984e6f..62454204fa 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 @@ -147,13 +147,6 @@ public class CppRuleClasses { public static final String RANDOM_SEED = "random_seed"; /** - * A string constant for the compile_action_flags_in_flag_set feature. This feature is just a - * transitional feature which helps telling whether -c and -o options are already in flag_set of - * action_config in CROSSTOOL file. Once the transition is done, it should be removed. - */ - public static final String COMPILE_ACTION_FLAGS_IN_FLAG_SET = "compile_action_flags_in_flag_set"; - - /** * A string constant for the dependency_file feature. This feature generates the .d file. */ public static final String DEPENDENCY_FILE = "dependency_file"; 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 acdaafde3a..382d71200d 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 @@ -207,14 +207,12 @@ public class FakeCppCompileAction extends CppCompileAction { // line to write to $TEST_TMPDIR instead. final String outputPrefix = "$TEST_TMPDIR/"; String argv = - getArgv(outputFile.getExecPath()) + getArguments() .stream() .map( input -> { String result = ShellEscaper.escapeString(input); - // Once -c and -o options are added into action_config, the argument of - // getArgv(outputFile.getExecPath()) won't be used anymore. There will always be - // -c <tempOutputFile>, but here it has to be outputFile, so we replace it. + // Replace -c <tempOutputFile> so it's -c <outputFile>. if (input.equals(tempOutputFile.getPathString())) { result = outputPrefix + ShellEscaper.escapeString(outputFile.getExecPathString()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java index d0f68c97bd..ed3fa2210f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java @@ -41,17 +41,18 @@ public class SpawnGccStrategy implements CppCompileActionContext { CppCompileAction action, ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { Iterable<Artifact> inputs = Iterables.concat(action.getInputs(), action.getAdditionalInputs()); - Spawn spawn = new SimpleSpawn( - action, - ImmutableList.copyOf(action.getArgv()), - ImmutableMap.copyOf(action.getEnvironment()), - ImmutableMap.copyOf(action.getExecutionInfo()), - EmptyRunfilesSupplier.INSTANCE, - ImmutableList.<Artifact>copyOf(inputs), - /*tools=*/ImmutableList.<Artifact>of(), - /*filesetManifests=*/ImmutableList.<Artifact>of(), - action.getOutputs().asList(), - action.estimateResourceConsumptionLocal()); + Spawn spawn = + new SimpleSpawn( + action, + ImmutableList.copyOf(action.getArguments()), + ImmutableMap.copyOf(action.getEnvironment()), + ImmutableMap.copyOf(action.getExecutionInfo()), + EmptyRunfilesSupplier.INSTANCE, + ImmutableList.copyOf(inputs), + /* tools= */ ImmutableList.of(), + /* filesetManifests= */ ImmutableList.of(), + action.getOutputs().asList(), + action.estimateResourceConsumptionLocal()); List<SpawnResult> spawnResults = actionExecutionContext diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index f768eda0f5..b6bd529655 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -517,7 +517,6 @@ public class CompilationSupport { .add(CppRuleClasses.COMPILE_ALL_MODULES) .add(CppRuleClasses.EXCLUDE_PRIVATE_HEADERS_IN_MODULE_MAPS) .add(CppRuleClasses.ONLY_DOTH_HEADERS_IN_MODULE_MAPS) - .add(CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET) .add(CppRuleClasses.DEPENDENCY_FILE) .add(CppRuleClasses.INCLUDE_PATHS) .add(isHost ? "host" : "nonhost") |