diff options
5 files changed, 114 insertions, 76 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java index ed68eb5f0e..09dfaa660b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java @@ -106,8 +106,8 @@ public class SymlinkAction extends AbstractAction { return inputPath == null ? getPrimaryInput().getExecPath() : inputPath; } - public Path getOutputPath() { - return getPrimaryOutput().getPath(); + public Path getOutputPath(ActionExecutionContext actionExecutionContext) { + return actionExecutionContext.getInputPath(getPrimaryOutput()); } @Override @@ -120,7 +120,7 @@ public class SymlinkAction extends AbstractAction { srcPath = actionExecutionContext.getExecRoot().getRelative(inputPath); } try { - getOutputPath().createSymbolicLink(srcPath); + getOutputPath(actionExecutionContext).createSymbolicLink(srcPath); } catch (IOException e) { throw new ActionExecutionException("failed to create symbolic link '" + Iterables.getOnlyElement(getOutputs()).prettyPrint() 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 4448eafd0e..66ad1c603c 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 @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; import com.google.devtools.build.lib.actions.ActionStatusMessage; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactResolver; import com.google.devtools.build.lib.actions.CommandAction; import com.google.devtools.build.lib.actions.CommandLineExpansionException; @@ -50,7 +49,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter; @@ -492,8 +490,7 @@ public class CppCompileAction extends AbstractAction initialResult = actionExecutionContext .getContext(CppIncludeScanningContext.class) - .findAdditionalInputs( - this, actionExecutionContext, includeProcessing); + .findAdditionalInputs(this, actionExecutionContext, includeProcessing); } catch (ExecException e) { throw e.toActionExecutionException( "Include scanning of rule '" + getOwner().getLabel() + "'", @@ -522,9 +519,7 @@ public class CppCompileAction extends AbstractAction @Override public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { - actionExecutionContext - .getEventBus() - .post(ActionStatusMessage.analysisStrategy(this)); + actionExecutionContext.getEventBus().post(ActionStatusMessage.analysisStrategy(this)); Iterable<Artifact> initialResult = findAdditionalInputs(actionExecutionContext); @@ -826,36 +821,33 @@ public class CppCompileAction extends AbstractAction } /** - * Enforce that the includes actually visited during the compile were properly - * declared in the rules. + * Enforce that the includes actually visited during the compile were properly declared in the + * rules. * - * <p>The technique is to walk through all of the reported includes that gcc - * emits into the .d file, and verify that they came from acceptable - * relative include directories. This is done in two steps: + * <p>The technique is to walk through all of the reported includes that gcc emits into the .d + * file, and verify that they came from acceptable relative include directories. This is done in + * two steps: * - * <p>First, each included file is stripped of any include path prefix from - * {@code quoteIncludeDirs} to produce an effective relative include dir+name. + * <p>First, each included file is stripped of any include path prefix from {@code + * quoteIncludeDirs} to produce an effective relative include dir+name. * - * <p>Second, the remaining directory is looked up in {@code declaredIncludeDirs}, - * a list of acceptable dirs. This list contains a set of dir fragments that - * have been calculated by the configured target to be allowable for inclusion - * by this source. If no match is found, an error is reported and an exception - * is thrown. + * <p>Second, the remaining directory is looked up in {@code declaredIncludeDirs}, a list of + * acceptable dirs. This list contains a set of dir fragments that have been calculated by the + * configured target to be allowable for inclusion by this source. If no match is found, an error + * is reported and an exception is thrown. * * @throws ActionExecutionException iff there was an undeclared dependency */ @VisibleForTesting public void validateInclusions( - Iterable<Artifact> inputsForValidation, - ArtifactExpander artifactExpander, - EventHandler eventHandler) + ActionExecutionContext actionExecutionContext, Iterable<Artifact> inputsForValidation) throws ActionExecutionException { IncludeProblems errors = new IncludeProblems(); IncludeProblems warnings = new IncludeProblems(); Set<Artifact> allowedIncludes = new HashSet<>(); for (Artifact input : Iterables.concat(mandatoryInputs, prunableInputs)) { if (input.isMiddlemanArtifact() || input.isTreeArtifact()) { - artifactExpander.expand(input, allowedIncludes); + actionExecutionContext.getArtifactExpander().expand(input, allowedIncludes); } allowedIncludes.add(input); } @@ -887,10 +879,11 @@ public class CppCompileAction extends AbstractAction if (FileSystemUtils.startsWithAny(input.getExecPath(), ignoreDirs)) { continue; } - if (!isDeclaredIn(input, declaredIncludeDirs, declaredIncludeSrcs)) { + if (!isDeclaredIn(actionExecutionContext, input, declaredIncludeDirs, declaredIncludeSrcs)) { // This call can never match the declared include sources (they would be matched above). // There are no declared include sources we need to warn about, so use an empty set here. - if (isDeclaredIn(input, warnIncludeDirs, ImmutableSet.<Artifact>of())) { + if (isDeclaredIn( + actionExecutionContext, input, warnIncludeDirs, ImmutableSet.<Artifact>of())) { warnings.add(input.getExecPath().toString()); } else { errors.add(input.getExecPath().toString()); @@ -930,11 +923,11 @@ public class CppCompileAction extends AbstractAction } if (warnings.hasProblems()) { - eventHandler.handle( - Event.warn( - getOwner().getLocation(), - warnings.getMessage(this, getSourceFile())) - .withTag(Label.print(getOwner().getLabel()))); + actionExecutionContext + .getEventHandler() + .handle( + Event.warn(getOwner().getLocation(), warnings.getMessage(this, getSourceFile())) + .withTag(Label.print(getOwner().getLabel()))); } errors.assertProblemFree(this, getSourceFile()); } @@ -945,18 +938,20 @@ public class CppCompileAction extends AbstractAction } /** - * 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. + * 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. * - * <p>This check also supports a wildcard suffix of '**' for the cases where the - * calculations are inexact. + * <p>This check also supports a wildcard suffix of '**' for the cases where the calculations are + * inexact. * - * <p>It also handles unseen non-nested-package subdirs by walking up the path looking - * for matches. + * <p>It also handles unseen non-nested-package subdirs by walking up the path looking for + * matches. */ private static boolean isDeclaredIn( - Artifact input, Set<PathFragment> declaredIncludeDirs, Set<Artifact> declaredIncludeSrcs) { + ActionExecutionContext actionExecutionContext, + Artifact input, + Set<PathFragment> declaredIncludeDirs, + Set<Artifact> declaredIncludeSrcs) { // First check if it's listed in "srcs". If so, then its declared & OK. if (declaredIncludeSrcs.contains(input)) { return true; @@ -982,7 +977,7 @@ public class CppCompileAction extends AbstractAction } // Still not found: see if it is in a subdir of a declared package. Root root = input.getRoot().getRoot(); - for (Path dir = input.getPath().getParentDirectory();;) { + for (Path dir = actionExecutionContext.getInputPath(input).getParentDirectory(); ; ) { if (dir.getRelative(BUILD_PATH_FRAGMENT).exists()) { return false; // Bad: this is a sub-package, not a subdir of a declared package. } @@ -1183,7 +1178,7 @@ public class CppCompileAction extends AbstractAction actionExecutionContext.getVerboseFailures(), this); } - ensureCoverageNotesFilesExist(); + ensureCoverageNotesFilesExist(actionExecutionContext); // This is the .d file scanning part. CppIncludeExtractionContext scanningContext = @@ -1200,7 +1195,8 @@ public class CppCompileAction extends AbstractAction showIncludesFilterForStderr); } else { discoveredInputs = - discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver(), reply); + discoverInputsFromDotdFiles( + actionExecutionContext, execRoot, scanningContext.getArtifactResolver(), reply); } reply = null; // Clear in-memory .d files early. @@ -1212,10 +1208,7 @@ public class CppCompileAction extends AbstractAction // because doing so would allow for incorrect builds. // HeadersCheckingMode.NONE should only be used for ObjC build actions. if (needsIncludeValidation) { - validateInclusions( - discoveredInputs, - actionExecutionContext.getArtifactExpander(), - actionExecutionContext.getEventHandler()); + validateInclusions(actionExecutionContext, discoveredInputs); } return ActionResult.create(spawnResults); } @@ -1250,7 +1243,10 @@ public class CppCompileAction extends AbstractAction @VisibleForTesting public NestedSet<Artifact> discoverInputsFromDotdFiles( - Path execRoot, ArtifactResolver artifactResolver, Reply reply) + ActionExecutionContext actionExecutionContext, + Path execRoot, + ArtifactResolver artifactResolver, + Reply reply) throws ActionExecutionException { if (!needsDotdInputPruning || getDotdFile() == null) { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); @@ -1259,7 +1255,8 @@ public class CppCompileAction extends AbstractAction new HeaderDiscovery.Builder() .setAction(this) .setSourceFile(getSourceFile()) - .setDependencies(processDepset(execRoot, reply).getDependencies()) + .setDependencies( + processDepset(actionExecutionContext, execRoot, reply).getDependencies()) .setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot)) .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap()); @@ -1270,7 +1267,9 @@ public class CppCompileAction extends AbstractAction return discoveryBuilder.build().discoverInputsFromDependencies(execRoot, artifactResolver); } - public DependencySet processDepset(Path execRoot, Reply reply) throws ActionExecutionException { + public DependencySet processDepset( + ActionExecutionContext actionExecutionContext, Path execRoot, Reply reply) + throws ActionExecutionException { try { DotdFile dotdFile = getDotdFile(); Preconditions.checkNotNull(dotdFile); @@ -1281,7 +1280,7 @@ public class CppCompileAction extends AbstractAction if (dotdFile.artifact() != null || reply == null) { Path dotdPath; if (dotdFile.artifact() != null) { - dotdPath = dotdFile.getPath(); + dotdPath = dotdFile.getPath(actionExecutionContext); } else { dotdPath = execRoot.getRelative(dotdFile.getSafeExecPath()); } @@ -1307,20 +1306,23 @@ public class CppCompileAction extends AbstractAction } /** - * Gcc only creates ".gcno" files if the compilation unit is non-empty. - * To ensure that the set of outputs for a CppCompileAction remains consistent - * and doesn't vary dynamically depending on the _contents_ of the input files, - * we create empty ".gcno" files if gcc didn't create them. + * Gcc only creates ".gcno" files if the compilation unit is non-empty. To ensure that the set of + * outputs for a CppCompileAction remains consistent and doesn't vary dynamically depending on the + * _contents_ of the input files, we create empty ".gcno" files if gcc didn't create them. */ - private void ensureCoverageNotesFilesExist() throws ActionExecutionException { + private void ensureCoverageNotesFilesExist(ActionExecutionContext actionExecutionContext) + throws ActionExecutionException { for (Artifact output : getOutputs()) { - if (output.isFileType(CppFileTypes.COVERAGE_NOTES) // ".gcno" - && !output.getPath().exists()) { + if (output.isFileType(CppFileTypes.COVERAGE_NOTES)) { // ".gcno" + Path outputPath = actionExecutionContext.getInputPath(output); + if (outputPath.exists()) { + continue; + } try { - FileSystemUtils.createEmptyFile(output.getPath()); + FileSystemUtils.createEmptyFile(outputPath); } catch (IOException e) { throw new ActionExecutionException( - "Error creating file '" + output.getPath() + "': " + e.getMessage(), e, this, false); + "Error creating file '" + outputPath + "': " + e.getMessage(), e, this, false); } } } @@ -1441,11 +1443,9 @@ public class CppCompileAction extends AbstractAction return execPath == null ? artifact.getExecPath() : execPath; } - /** - * @return the on-disk location of the .d file or null - */ - public Path getPath() { - return artifact.getPath(); + /** @return the on-disk location of the .d file or null */ + public Path getPath(ActionExecutionContext actionExecutionContext) { + return actionExecutionContext.getInputPath(artifact); } } } 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 0a35544599..5528aac3bd 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 @@ -160,7 +160,8 @@ public class FakeCppCompileAction extends CppCompileAction { new HeaderDiscovery.Builder() .setAction(this) .setSourceFile(getSourceFile()) - .setDependencies(processDepset(execRoot, reply).getDependencies()) + .setDependencies( + processDepset(actionExecutionContext, execRoot, reply).getDependencies()) .setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot)) .setAllowedDerivedinputsMap(getAllowedDerivedInputsMap()); @@ -183,16 +184,16 @@ public class FakeCppCompileAction extends CppCompileAction { // listed in the "srcs" of the cc_fake_binary or in the "srcs" of a cc_library that it // depends on. try { - validateInclusions( - discoveredInputs, - actionExecutionContext.getArtifactExpander(), - actionExecutionContext.getEventHandler()); + validateInclusions(actionExecutionContext, discoveredInputs); } catch (ActionExecutionException e) { // TODO(bazel-team): (2009) make this into an error, once most of the current warnings // are fixed. - actionExecutionContext.getEventHandler().handle(Event.warn( - getOwner().getLocation(), - e.getMessage() + ";\n this warning may eventually become an error")); + actionExecutionContext + .getEventHandler() + .handle( + Event.warn( + getOwner().getLocation(), + e.getMessage() + ";\n this warning may eventually become an error")); } updateActionInputs(discoveredInputs); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 14157c260f..7129779d58 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -30,10 +30,13 @@ import com.google.common.collect.Sets; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; +import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactRoot; @@ -44,6 +47,7 @@ import com.google.devtools.build.lib.actions.MutableActionGraph; import com.google.devtools.build.lib.actions.ResourceManager; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -157,6 +161,7 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.TreeMap; import java.util.UUID; import javax.annotation.Nullable; import org.junit.Before; @@ -197,6 +202,8 @@ public abstract class BuildViewTestCase extends FoundationTestCase { private LoadingOptions customLoadingOptions = null; protected BuildConfigurationValue.Key targetConfigKey; + private ActionLogBufferPathGenerator actionLogBufferPathGenerator; + @Before public final void initializeSkyframeExecutor() throws Exception { initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ true); @@ -272,6 +279,9 @@ public abstract class BuildViewTestCase extends FoundationTestCase { setUpSkyframe(); // Also initializes ResourceManager. ResourceManager.instance().setAvailableResources(getStartingResources()); + this.actionLogBufferPathGenerator = + new ActionLogBufferPathGenerator( + directories.getActionConsoleOutputDirectory(getExecRoot())); } public void initializeMockClient() throws IOException { @@ -1989,4 +1999,27 @@ public abstract class BuildViewTestCase extends FoundationTestCase { public Path getExecRoot() { return directories.getExecRoot(ruleClassProvider.getRunfilesPrefix()); } + + /** Creates instances of {@link ActionExecutionContext} consistent with test case. */ + public class ActionExecutionContextBuilder { + private TreeMap<String, String> clientEnv = new TreeMap<>(); + private ArtifactExpander artifactExpander = null; + + public ActionExecutionContextBuilder setArtifactExpander(ArtifactExpander artifactExpander) { + this.artifactExpander = artifactExpander; + return this; + } + + public ActionExecutionContext build() { + return new ActionExecutionContext( + new DummyExecutor(fileSystem, getExecRoot(), reporter), + /*actionInputFileCache=*/ null, + /*actionInputPrefetcher=*/ null, + actionKeyContext, + /*metadataHandler=*/ null, + actionLogBufferPathGenerator.generate(), + clientEnv, + artifactExpander); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java index 8226bc1bc8..061eac34e4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java @@ -1067,7 +1067,10 @@ public class ObjcLibraryTest extends ObjcRuleTestCase { .setList("srcs", "a.m") .write(); CppCompileAction compileAction = (CppCompileAction) compileAction("//lib:lib", "a.o"); - assertThat(compileAction.discoverInputsFromDotdFiles(null, null, null)).isEmpty(); + assertThat( + compileAction.discoverInputsFromDotdFiles( + new ActionExecutionContextBuilder().build(), null, null, null)) + .isEmpty(); } @Test @@ -1702,7 +1705,8 @@ public class ObjcLibraryTest extends ObjcRuleTestCase { createLibraryTargetWriter("//lib:lib").setList("srcs", "a.m").write(); CppCompileAction compileAction = (CppCompileAction) compileAction("//lib:lib", "a.o"); try { - compileAction.discoverInputsFromDotdFiles(null, null, null); + compileAction.discoverInputsFromDotdFiles( + new ActionExecutionContextBuilder().build(), null, null, null); fail("Expected ActionExecutionException"); } catch (ActionExecutionException expected) { assertThat(expected).hasMessageThat().contains("error while parsing .d file"); |