Fix windows params file quoting for MSVC (#31570) Previously the params files passed on the command line would use `\` instead of quoting the file, leading to build failures for windows. This reuses the existing rules_cc feature that is used for linking to determine which params file format should be used. Closes #31570 COPYBARA_INTEGRATE_REVIEW=https://github.com/bazelbuild/bazel/pull/31570 from keith:ks/fix-windows-params-file-quoting-for-msvc f379471a58608144ac762a2747c3063ca2e4c97e PiperOrigin-RevId: 995150479 Change-Id: I5555afc4fefeb230dd42b7a96072b1221060b9e0
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 0c5ec06..e6776f0 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
@@ -1044,7 +1044,9 @@ ParamFileInfo paramFileInfo = null; if (cppConfiguration().useArgsParamsFile()) { paramFileInfo = - ParamFileInfo.builder(ParameterFileType.GCC_QUOTED).setUseAlways(true).build(); + ParamFileInfo.builder(getParameterFileType(featureConfiguration)) + .setUseAlways(true) + .build(); } CommandLineAndParamFileInfo commandLineAndParamFileInfo = new CommandLineAndParamFileInfo(commandLine, paramFileInfo); @@ -1054,6 +1056,12 @@ return StarlarkList.immutableCopyOf(ImmutableList.of(args)); } + static ParameterFileType getParameterFileType(FeatureConfiguration featureConfiguration) { + return featureConfiguration.isEnabled(CppRuleClasses.WINDOWS_QUOTING_FOR_PARAM_FILES) + ? ParameterFileType.WINDOWS + : ParameterFileType.GCC_QUOTED; + } + @Override public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyContext) throws CommandLineExpansionException, InterruptedException { @@ -1388,6 +1396,7 @@ compileCommandLine.getEnvironment(PathMapper.NOOP), executionInfo, getCommandLineKey(), + getParameterFileType(featureConfiguration), ccCompilationContext.getDeclaredIncludeSrcs(), mandatoryInputs, mandatorySpawnInputs, @@ -1406,6 +1415,7 @@ Map<String, String> environmentVariables, Map<String, String> executionInfo, byte[] commandLineKey, + ParameterFileType parameterFileType, NestedSet<Artifact> declaredIncludeSrcs, NestedSet<Artifact> mandatoryInputs, NestedSet<Artifact> mandatorySpawnInputs, @@ -1420,6 +1430,7 @@ fp.addStringMap(environmentVariables); fp.addStringMap(executionInfo); fp.addBytes(commandLineKey); + fp.addString(parameterFileType.toString()); actionKeyContext.addNestedSetToFingerprint(fp, declaredIncludeSrcs); fp.addInt(0); // mark the boundary between input types @@ -1636,8 +1647,7 @@ paramFilePath, paramFileArg, compilerOptions, - // TODO(b/132888308): Support MSVC, which has its own method of escaping strings. - ParameterFileType.GCC_QUOTED); + getParameterFileType(featureConfiguration)); args = compileCommandLine.getArgumentsWithParameterFile(pathMapper, paramFileArg, paramFilePath); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java index 0041962..12ad873 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
@@ -217,6 +217,7 @@ cppCompileActionBuilder.getExecutionInfo(), CppCompileAction.computeCommandLineKey( commandLine.getCompilerOptions(/* overwrittenVariables= */ null, PathMapper.NOOP)), + CppCompileAction.getParameterFileType(cppCompileActionBuilder.getFeatureConfiguration()), cppCompileActionBuilder.getCcCompilationContext().getDeclaredIncludeSrcs(), mandatoryInputs, mandatoryInputs,
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 fbff9ab..abf0945 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
@@ -299,6 +299,9 @@ /** A feature to use gcc quoting for linking param files. */ public static final String GCC_QUOTING_FOR_PARAM_FILES = "gcc_quoting_for_param_files"; + /** A feature to use Windows quoting for compiler and linker param files. */ + public static final String WINDOWS_QUOTING_FOR_PARAM_FILES = "windows_quoting_for_param_files"; + /** * A feature to indicate that this target generates debug symbols for a dSYM file. For Apple * platform only.
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl index 9c9742c..c14ec9f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl
@@ -101,6 +101,7 @@ compiler_param_file = "compiler_param_file", compiler_param_file_on_demand = "compiler_param_file_on_demand", gcc_quoting_for_param_files = "gcc_quoting_for_param_files", + windows_quoting_for_param_files = "windows_quoting_for_param_files", objcopy_embed_flags = "objcopy_embed_flags", ld_embed_flags = "ld_embed_flags", opt = "opt", @@ -915,6 +916,11 @@ enabled = True, ) +_windows_quoting_for_param_files_feature = feature( + name = _FEATURE_NAMES.windows_quoting_for_param_files, + enabled = True, +) + _static_link_cpp_runtimes_feature = feature( name = _FEATURE_NAMES.static_link_cpp_runtimes, enabled = True, @@ -1449,6 +1455,7 @@ _FEATURE_NAMES.compiler_param_file: _compiler_param_file_feature, _FEATURE_NAMES.compiler_param_file_on_demand: _compiler_param_file_on_demand_feature, _FEATURE_NAMES.gcc_quoting_for_param_files: _gcc_quoting_for_param_files_feature, + _FEATURE_NAMES.windows_quoting_for_param_files: _windows_quoting_for_param_files_feature, _FEATURE_NAMES.module_maps: _module_maps_feature, _FEATURE_NAMES.static_link_cpp_runtimes: _static_link_cpp_runtimes_feature, _FEATURE_NAMES.simple_compile_feature: _simple_compile_feature,
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index 6a878a0..4ae4ced 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD
@@ -105,9 +105,11 @@ deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:parameter_file", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_core", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:run_environment_info", + "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/rules/cpp",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java index f995c84..3bfcd6f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -16,11 +16,13 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.baseArtifactNames; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.baseNamesOf; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.AnalysisUtils; @@ -28,6 +30,7 @@ import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; +import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; @@ -41,6 +44,7 @@ import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; +import java.io.ByteArrayOutputStream; import java.util.List; import org.junit.Before; import org.junit.Ignore; @@ -1146,6 +1150,75 @@ } @Test + public void testCompilationParameterFileWindowsQuoting() throws Exception { + assertCompilationParameterFileQuoting( + CppRuleClasses.COMPILER_PARAM_FILE, /* windowsQuoting= */ true); + } + + @Test + public void testCompilationParameterFileOnDemandWindowsQuoting() throws Exception { + assertCompilationParameterFileQuoting( + CppRuleClasses.COMPILER_PARAM_FILE_ON_DEMAND, /* windowsQuoting= */ true); + } + + @Test + public void testCompilationParameterFileGccQuoting() throws Exception { + assertCompilationParameterFileQuoting( + CppRuleClasses.COMPILER_PARAM_FILE, /* windowsQuoting= */ false); + } + + @Test + public void testCompilationParameterFileQuotingAffectsActionKey() throws Exception { + assertCompilationParameterFileQuoting( + CppRuleClasses.COMPILER_PARAM_FILE, /* windowsQuoting= */ true); + CppCompileAction windowsAction = getCppCompileAction("//a:foo bar"); + String windowsKey = windowsAction.getKey(actionKeyContext, /* inputMetadataProvider= */ null); + + useConfiguration( + "--platforms=" + TestConstants.PLATFORM_LABEL, + "--features=-" + CppRuleClasses.WINDOWS_QUOTING_FOR_PARAM_FILES); + CppCompileAction gccAction = getCppCompileAction("//a:foo bar"); + + assertThat(gccAction.getArguments()).isEqualTo(windowsAction.getArguments()); + assertThat(gccAction.getKey(actionKeyContext, /* inputMetadataProvider= */ null)) + .isNotEqualTo(windowsKey); + } + + private void assertCompilationParameterFileQuoting( + String paramFileFeature, boolean windowsQuoting) throws Exception { + CcToolchainConfig.Builder config = CcToolchainConfig.builder(); + if (windowsQuoting) { + config.withFeatures(paramFileFeature, CppRuleClasses.WINDOWS_QUOTING_FOR_PARAM_FILES); + } else { + config.withFeatures(paramFileFeature); + } + AnalysisMock.get().ccSupport().setupCcToolchainConfig(mockToolsConfig, config); + scratch.file( + "a/BUILD", + """ + load("@rules_cc//cc:cc_library.bzl", "cc_library") + cc_library(name = "foo bar", srcs = ["source with spaces.cc"]) + """); + useConfiguration( + "--platforms=" + TestConstants.PLATFORM_LABEL, + "--min_param_file_size=0", + "--experimental_use_cpp_compile_action_args_params_file"); + CppCompileAction action = getCppCompileAction("//a:foo bar"); + ByteArrayOutputStream out = new ByteArrayOutputStream(); + action.getArgumentsForExecute(PathMapper.NOOP).paramFileActionInput().writeTo(out); + + String source = "a/source with spaces.cc"; + String object = action.getPrimaryOutput().getExecPathString(); + assertThat(out.toString(UTF_8).lines().toList()) + .containsAtLeast( + windowsQuoting ? "\"" + source + "\"" : source.replace(" ", "\\ "), + windowsQuoting ? "\"" + object + "\"" : object.replace(" ", "\\ ")); + Args args = (Args) action.getStarlarkArgs().get(0); + assertThat(args.getParamFileInfo().getFileType()) + .isEqualTo(windowsQuoting ? ParameterFileType.WINDOWS : ParameterFileType.GCC_QUOTED); + } + + @Test public void testCppCompileActionArgvIgnoreParamFile() throws Exception { AnalysisMock.get() .ccSupport()