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."); + } } /**