From 0975c5204efd4c5db35a843958fb7b6390121d54 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 20:53:35 +0200 Subject: [PATCH] Make ShowIncludesFilter ignore execroot differences Fixes https://github.com/bazelbuild/bazel/issues/6847 This change is for making including scanning work on Windows for builds with remote caching or remote execution enabled. After this change, the ShowIncludesFilter will look for the first `execroot\` in the output header file paths, then it considers `C:\...\execroot\` as the execroot path. Because execroot path could be different if remote cache is hit, we ignore it and only add the relative path as dependencies. I'm quite unwilling to make this change, because parsing `execroot\\` for execroot is not guaranteed to work always. But considering the only case this could go wrong is when people use an output base that already contains `execroot\\`, which I think should never happen. Closes #6931. Change-Id: Ife2cb91c75f1b5b297851400e672db2b35ff09e0 PiperOrigin-RevId: 225553627 --- .../build/lib/rules/cpp/CppCompileAction.java | 55 +++----- .../lib/rules/cpp/ShowIncludesFilter.java | 130 ++---------------- .../lib/rules/cpp/ShowIncludesFilterTest.java | 25 +--- 3 files changed, 34 insertions(+), 176 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 0e7c72316ad..77907bdc687 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -236,7 +236,7 @@ public class CppCompileAction extends AbstractAction @Nullable Artifact grepIncludes) { super( owner, - createInputsBuilder(mandatoryInputs, inputsForInvalidation).build(), + NestedSetBuilder.fromNestedSet(mandatoryInputs).addAll(inputsForInvalidation).build(), CollectionUtils.asSetWithoutNulls( outputFile, dotdFile == null ? null : dotdFile.artifact(), @@ -274,12 +274,14 @@ public class CppCompileAction extends AbstractAction this.actionName = actionName; this.featureConfiguration = featureConfiguration; this.needsDotdInputPruning = - cppSemantics.needsDotdInputPruning() && !sourceFile.isFileType(CppFileTypes.CPP_MODULE); + !shareable + && cppSemantics.needsDotdInputPruning() + && !sourceFile.isFileType(CppFileTypes.CPP_MODULE); this.needsIncludeValidation = cppSemantics.needsIncludeValidation(); this.includeProcessing = cppSemantics.getIncludeProcessing(); this.actionClassId = actionClassId; this.builtInIncludeDirectories = builtInIncludeDirectories; - this.additionalInputs = null; + this.additionalInputs = discoversInputs() ? null : ImmutableList.of(); this.usedModules = null; this.topLevelModules = null; this.overwrittenVariables = null; @@ -345,7 +347,9 @@ public Iterable getAdditionalInputs() { /** Clears the discovered {@link #additionalInputs}. */ public void clearAdditionalInputs() { - additionalInputs = null; + if (discoversInputs()) { + additionalInputs = null; + } } @Override @@ -937,22 +941,16 @@ private static boolean isDeclaredIn( } } - /** - * Recalculates this action's live input collection, including sources, middlemen. - * - *

Can only be called if {@link #discoversInputs}, and must be called after execution in that - * case. - */ + /** Recalculates this action's live input collection, including sources, middlemen. */ @VisibleForTesting // productionVisibility = Visibility.PRIVATE @ThreadCompatible - final void updateActionInputs(NestedSet discoveredInputs) { - Preconditions.checkState( - discoversInputs(), "Can't call if not discovering inputs: %s %s", discoveredInputs, this); + public final void updateActionInputs(NestedSet discoveredInputs) { + NestedSetBuilder inputs = NestedSetBuilder.stableOrder(); try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.ACTION_UPDATE, describe())) { - super.updateInputs( - createInputsBuilder(mandatoryInputs, inputsForInvalidation) - .addTransitive(discoveredInputs) - .build()); + inputs.addTransitive(mandatoryInputs); + inputs.addAll(inputsForInvalidation); + inputs.addTransitive(discoveredInputs); + super.updateInputs(inputs.build()); } } @@ -1116,7 +1114,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) setModuleFileFlags(); CppCompileActionContext.Reply reply; - if (!shouldScanDotdFiles()) { + if (discoversInputs() && !shouldScanDotdFiles()) { updateActionInputs(NestedSetBuilder.wrap(Order.STABLE_ORDER, additionalInputs)); } @@ -1211,17 +1209,9 @@ public void close() throws IOException { } reply = null; // Clear in-memory .d files early. - if (discoversInputs()) { - // Post-execute "include scanning", which modifies the action inputs to match what the compile - // action actually used by incorporating the results of .d file parsing. - updateActionInputs(discoveredInputs); - } else { - Preconditions.checkState( - discoveredInputs.isEmpty(), - "Discovered inputs without discovering inputs? %s %s", - discoveredInputs, - this); - } + // Post-execute "include scanning", which modifies the action inputs to match what the compile + // action actually used by incorporating the results of .d file parsing. + updateActionInputs(discoveredInputs); // hdrs_check: This cannot be switched off for C++ build actions, // because doing so would allow for incorrect builds. @@ -1497,13 +1487,6 @@ private static ActionLookupData lookupDataFromModule( module)); } - private static NestedSetBuilder createInputsBuilder( - NestedSet mandatoryInputs, Iterable inputsForInvalidation) { - return NestedSetBuilder.stableOrder() - .addTransitive(mandatoryInputs) - .addAll(inputsForInvalidation); - } - /** * A reference to a .d file. There are two modes: * diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java index a3767652812..efa3e9a6c86 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.vfs.Path; import java.io.ByteArrayOutputStream; import java.io.FilterOutputStream; @@ -25,7 +24,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.List; /** * A Class for filtering the output of /showIncludes from MSVC compiler. @@ -56,98 +54,7 @@ public static class FilterShowIncludesOutputStream extends FilterOutputStream { private final ByteArrayOutputStream buffer = new ByteArrayOutputStream(4096); private final Collection dependencies = new ArrayList<>(); private static final int NEWLINE = '\n'; - // "Note: including file:" in 14 languages, - // cl.exe will print different prefix according to the locale configured for MSVC. - private static final List SHOW_INCLUDES_PREFIXES = - ImmutableList.of( - new String( - new byte[] { - 78, 111, 116, 101, 58, 32, 105, 110, 99, 108, 117, 100, 105, 110, 103, 32, 102, - 105, 108, 101, 58 - }, - StandardCharsets.UTF_8), // English - new String( - new byte[] { - -26, -77, -88, -26, -124, -113, 58, 32, -27, -116, -123, -27, -112, -85, -26, -86, - -108, -26, -95, -120, 58 - }, - StandardCharsets.UTF_8), // Traditional Chinese - new String( - new byte[] { - 80, 111, 122, 110, -61, -95, 109, 107, 97, 58, 32, 86, -60, -115, 101, 116, 110, - -60, -101, 32, 115, 111, 117, 98, 111, 114, 117, 58 - }, - StandardCharsets.UTF_8), // Czech - new String( - new byte[] { - 72, 105, 110, 119, 101, 105, 115, 58, 32, 69, 105, 110, 108, 101, 115, 101, 110, - 32, 100, 101, 114, 32, 68, 97, 116, 101, 105, 58 - }, - StandardCharsets.UTF_8), // German - new String( - new byte[] { - 82, 101, 109, 97, 114, 113, 117, 101, -62, -96, 58, 32, 105, 110, 99, 108, 117, - 115, 105, 111, 110, 32, 100, 117, 32, 102, 105, 99, 104, 105, 101, 114, -62, -96, - 58 - }, - StandardCharsets.UTF_8), // French - new String( - new byte[] { - 78, 111, 116, 97, 58, 32, 102, 105, 108, 101, 32, 105, 110, 99, 108, 117, 115, 111 - }, - StandardCharsets.UTF_8), // Italian - new String( - new byte[] { - -29, -125, -95, -29, -125, -94, 58, 32, -29, -126, -92, -29, -125, -77, -29, -126, - -81, -29, -125, -85, -29, -125, -68, -29, -125, -119, 32, -29, -125, -107, -29, - -126, -95, -29, -126, -92, -29, -125, -85, 58 - }, - StandardCharsets.UTF_8), // Janpanese - new String( - new byte[] { - -20, -80, -72, -22, -77, -96, 58, 32, -19, -113, -84, -19, -107, -88, 32, -19, - -116, -116, -20, -99, -68, 58 - }, - StandardCharsets.UTF_8), // Korean - new String( - new byte[] { - 85, 119, 97, 103, 97, 58, 32, 119, 32, 116, 121, 109, 32, 112, 108, 105, 107, 117, - 58 - }, - StandardCharsets.UTF_8), // Polish - new String( - new byte[] { - 79, 98, 115, 101, 114, 118, 97, -61, -89, -61, -93, 111, 58, 32, 105, 110, 99, - 108, 117, 105, 110, 100, 111, 32, 97, 114, 113, 117, 105, 118, 111, 58 - }, - StandardCharsets.UTF_8), // Portuguese - new String( - new byte[] { - -48, -97, -47, -128, -48, -72, -48, -68, -48, -75, -47, -121, -48, -80, -48, -67, - -48, -72, -48, -75, 58, 32, -48, -78, -48, -70, -48, -69, -47, -114, -47, -121, - -48, -75, -48, -67, -48, -72, -48, -75, 32, -47, -124, -48, -80, -48, -71, -48, - -69, -48, -80, 58 - }, - StandardCharsets.UTF_8), // Russian - new String( - new byte[] { - 78, 111, 116, 58, 32, 101, 107, 108, 101, 110, 101, 110, 32, 100, 111, 115, 121, - 97, 58 - }, - StandardCharsets.UTF_8), // Turkish - new String( - new byte[] { - -26, -77, -88, -26, -124, -113, 58, 32, -27, -116, -123, -27, -112, -85, -26, - -106, -121, -28, -69, -74, 58 - }, - StandardCharsets.UTF_8), // Simplified Chinese - new String( - new byte[] { - 78, 111, 116, 97, 58, 32, 105, 110, 99, 108, 117, 115, 105, -61, -77, 110, 32, - 100, 101, 108, 32, 97, 114, 99, 104, 105, 118, 111, 58 - }, - StandardCharsets.UTF_8) // Spanish - ); + private static final String SHOW_INCLUDES_PREFIX = "Note: including file:"; private final String sourceFileName; private final String execRootSuffix; @@ -163,21 +70,14 @@ public void write(int b) throws IOException { buffer.write(b); if (b == NEWLINE) { String line = buffer.toString(StandardCharsets.UTF_8.name()); - boolean prefixMatched = false; - for (String prefix : SHOW_INCLUDES_PREFIXES) { - if (line.startsWith(prefix)) { - line = line.substring(prefix.length()).trim(); - int index = line.indexOf(execRootSuffix); - if (index != -1) { - line = line.substring(index + execRootSuffix.length()); - } - dependencies.add(line); - prefixMatched = true; - break; + if (line.startsWith(SHOW_INCLUDES_PREFIX)) { + line = line.substring(SHOW_INCLUDES_PREFIX.length()).trim(); + int index = line.indexOf(execRootSuffix); + if (index != -1) { + line = line.substring(index + execRootSuffix.length()); } - } - // cl.exe also prints out the file name unconditionally, we need to also filter it out. - if (!prefixMatched && !line.trim().equals(sourceFileName)) { + dependencies.add(line); + } else if (!line.trim().equals(sourceFileName)) { buffer.writeTo(out); } buffer.reset(); @@ -187,19 +87,9 @@ public void write(int b) throws IOException { @Override public void flush() throws IOException { String line = buffer.toString(StandardCharsets.UTF_8.name()); - - // If this line starts or could start with a prefix. - boolean startingWithAnyPrefix = false; - for (String prefix : SHOW_INCLUDES_PREFIXES) { - if (line.startsWith(prefix) || prefix.startsWith(line)) { - startingWithAnyPrefix = true; - break; - } - } - - if (!startingWithAnyPrefix - // If this line starts or could start with the source file name. + if (!line.startsWith(SHOW_INCLUDES_PREFIX) && !line.startsWith(sourceFileName) + && !SHOW_INCLUDES_PREFIX.startsWith(line) && !sourceFileName.startsWith(line)) { buffer.writeTo(out); buffer.reset(); diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java index 6bfaf66058a..9b4ba63a29c 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java @@ -17,7 +17,6 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.ByteArrayOutputStream; @@ -39,10 +38,10 @@ public class ShowIncludesFilterTest { @Before public void setUpOutputStreams() throws IOException { - showIncludesFilter = new ShowIncludesFilter("foo.cpp"); + showIncludesFilter = new ShowIncludesFilter("foo.cpp", "__main__"); output = new ByteArrayOutputStream(); filterOutputStream = showIncludesFilter.getFilteredOutputStream(output); - fs = new InMemoryFileSystem(DigestHashFunction.SHA256); + fs = new InMemoryFileSystem(); fs.getPath("/out").createDirectory(); } @@ -94,31 +93,17 @@ public void testMatchAllOfNotePrefix() throws IOException { } @Test - // Regression tests for https://github.com/bazelbuild/bazel/issues/9172 - public void testFindHeaderFromAbsolutePathUnderExecrootBase() throws IOException { + public void testMatchAllOfNotePrefixWithAbsolutePath() throws IOException { // "Note: including file:" is the prefix filterOutputStream.write( - getBytes("Note: including file: C:\\tmp\\xxxx\\execroot\\__main__\\foo\\bar\\bar.h")); + getBytes("Note: including file: C:\\tmp\\xxxx\\execroot\\__main__\\bar.h")); filterOutputStream.flush(); // flush to output should not work, waiting for newline assertThat(output.toString()).isEmpty(); filterOutputStream.write(getBytes("\n")); // It's a match, output should be filtered, dependency on bar.h should be found. assertThat(output.toString()).isEmpty(); - assertThat(showIncludesFilter.getDependencies()).contains("..\\__main__\\foo\\bar\\bar.h"); - } - - @Test - public void testFindHeaderFromAbsolutePathOutsideExecroot() throws IOException { - // "Note: including file:" is the prefix - filterOutputStream.write(getBytes("Note: including file: C:\\system\\foo\\bar\\bar.h")); - filterOutputStream.flush(); - // flush to output should not work, waiting for newline - assertThat(output.toString()).isEmpty(); - filterOutputStream.write(getBytes("\n")); - // It's a match, output should be filtered, dependency on bar.h should be found. - assertThat(output.toString()).isEmpty(); - assertThat(showIncludesFilter.getDependencies()).contains("C:\\system\\foo\\bar\\bar.h"); + assertThat(showIncludesFilter.getDependencies()).contains("bar.h"); } @Test