Move verification of include paths to execution phase. Analysis time is
precious.
RELNOTES: None.
PiperOrigin-RevId: 219529252
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 4db107a..36f934c 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
@@ -45,6 +45,7 @@
import com.google.devtools.build.lib.actions.extra.CppCompileInfo;
import com.google.devtools.build.lib.actions.extra.EnvironmentVariable;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -816,6 +817,40 @@
return Iterables.concat(cxxSystemIncludeDirs, ccCompilationContext.getSystemIncludeDirs());
}
+ @VisibleForTesting
+ void verifyActionIncludePaths() throws ActionExecutionException {
+ ImmutableSet<PathFragment> ignoredDirs = ImmutableSet.copyOf(getValidationIgnoredDirs());
+ // We currently do not check the output of:
+ // - getQuoteIncludeDirs(): those only come from includes attributes, and are checked in
+ // CcCommon.getIncludeDirsFromIncludesAttribute().
+ // - getBuiltinIncludeDirs(): while in practice this doesn't happen, bazel can be configured
+ // to use an absolute system root, in which case the builtin include dirs might be absolute.
+
+ Iterable<PathFragment> includePathsToVerify =
+ Iterables.concat(getIncludeDirs(), getSystemIncludeDirs());
+ for (PathFragment includePath : includePathsToVerify) {
+ // includePathsToVerify contains all paths that are added as -isystem directive on the command
+ // line, most of which are added for include directives in the CcCompilationContext and are
+ // thus also in ignoredDirs. The hash lookup prevents this from becoming O(N^2) for these.
+ if (ignoredDirs.contains(includePath)
+ || FileSystemUtils.startsWithAny(includePath, ignoredDirs)) {
+ continue;
+ }
+ // One starting ../ is okay for getting to a sibling repository.
+ if (includePath.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
+ includePath = includePath.relativeTo(Label.EXTERNAL_PATH_PREFIX);
+ }
+ if (includePath.isAbsolute() || includePath.containsUplevelReferences()) {
+ throw new ActionExecutionException(
+ String.format(
+ "The include path '%s' references a path outside of the execution root.",
+ includePath),
+ this,
+ false);
+ }
+ }
+ }
+
/**
* Returns true if an included artifact is declared in a set of allowed include directories. The
* simple case is that the artifact's parent directory is contained in the set, or is empty.
@@ -1031,6 +1066,10 @@
@ThreadCompatible
public ActionResult execute(ActionExecutionContext actionExecutionContext)
throws ActionExecutionException, InterruptedException {
+ if (needsIncludeValidation) {
+ verifyActionIncludePaths();
+ }
+
setModuleFileFlags();
CppCompileActionContext.Reply reply;
ShowIncludesFilter showIncludesFilterForStdout = null;
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 1e22172..9629b55 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
@@ -18,7 +18,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionEnvironment;
import com.google.devtools.build.lib.actions.ActionOwner;
@@ -26,14 +25,12 @@
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
-import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.LinkedHashMap;
@@ -356,10 +353,6 @@
builtinIncludeDirectories,
grepIncludes);
}
-
- if (cppSemantics.needsIncludeValidation()) {
- verifyActionIncludePaths(action, errorCollector);
- }
return action;
}
@@ -408,37 +401,6 @@
return shouldScanIncludes && useHeaderModules();
}
- private void verifyActionIncludePaths(CppCompileAction action, Consumer<String> errorReporter) {
- ImmutableSet<PathFragment> ignoredDirs = ImmutableSet.copyOf(action.getValidationIgnoredDirs());
- // We currently do not check the output of:
- // - getQuoteIncludeDirs(): those only come from includes attributes, and are checked in
- // CcCommon.getIncludeDirsFromIncludesAttribute().
- // - getBuiltinIncludeDirs(): while in practice this doesn't happen, bazel can be configured
- // to use an absolute system root, in which case the builtin include dirs might be absolute.
-
- Iterable<PathFragment> includePathsToVerify =
- Iterables.concat(action.getIncludeDirs(), action.getSystemIncludeDirs());
- for (PathFragment includePath : includePathsToVerify) {
- // includePathsToVerify contains all paths that are added as -isystem directive on the command
- // line, most of which are added for include directives in the CcCompilationContext and are
- // thus also in ignoredDirs. The hash lookup prevents this from becoming O(N^2) for these.
- if (ignoredDirs.contains(includePath)
- || FileSystemUtils.startsWithAny(includePath, ignoredDirs)) {
- continue;
- }
- // One starting ../ is okay for getting to a sibling repository.
- if (includePath.startsWith(Label.EXTERNAL_PATH_PREFIX)) {
- includePath = includePath.relativeTo(Label.EXTERNAL_PATH_PREFIX);
- }
- if (includePath.isAbsolute() || includePath.containsUplevelReferences()) {
- errorReporter.accept(
- String.format(
- "The include path '%s' references a path outside of the execution root.",
- includePath));
- }
- }
- }
-
/**
* Set action name that is used to pick the right action_config and features from {@link
* FeatureConfiguration}. By default the action name is decided from the source filetype.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index f50c2a4..48f091e 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -22,6 +22,7 @@
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FailAction;
import com.google.devtools.build.lib.actions.extra.CppLinkInfo;
@@ -1278,38 +1279,69 @@
@Test
public void testIncludePathsOutsideExecutionRoot() throws Exception {
- checkError(
+ scratchRule(
"root",
"a",
- "The include path '../somewhere' references a path outside of the execution root.",
"cc_library(name='a', srcs=['a.cc'], copts=['-Id/../../somewhere'])");
+ CppCompileAction compileAction = getCppCompileAction("//root:a");
+ try {
+ compileAction.verifyActionIncludePaths();
+ } catch (ActionExecutionException exception) {
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "The include path '../somewhere' references a path outside of the execution root.");
+ }
}
@Test
public void testAbsoluteIncludePathsOutsideExecutionRoot() throws Exception {
- checkError(
+ scratchRule(
"root",
"a",
- "The include path '/somewhere' references a path outside of the execution root.",
"cc_library(name='a', srcs=['a.cc'], copts=['-I/somewhere'])");
+ CppCompileAction compileAction = getCppCompileAction("//root:a");
+ try {
+ compileAction.verifyActionIncludePaths();
+ } catch (ActionExecutionException exception) {
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "The include path '/somewhere' references a path outside of the execution root.");
+ }
}
@Test
public void testSystemIncludePathsOutsideExecutionRoot() throws Exception {
- checkError(
+ scratchRule(
"root",
"a",
- "The include path '../system' references a path outside of the execution root.",
"cc_library(name='a', srcs=['a.cc'], copts=['-isystem../system'])");
+ CppCompileAction compileAction = getCppCompileAction("//root:a");
+ try {
+ compileAction.verifyActionIncludePaths();
+ } catch (ActionExecutionException exception) {
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ "The include path '../system' references a path outside of the execution root.");
+ }
}
@Test
public void testAbsoluteSystemIncludePathsOutsideExecutionRoot() throws Exception {
- checkError(
+ scratchRule(
"root",
"a",
- "The include path '/system' references a path outside of the execution root.",
"cc_library(name='a', srcs=['a.cc'], copts=['-isystem/system'])");
+ CppCompileAction compileAction = getCppCompileAction("//root:a");
+ try {
+ compileAction.verifyActionIncludePaths();
+ } catch (ActionExecutionException exception) {
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("The include path '/system' references a path outside of the execution root.");
+ }
}
/**
