From 1b937d288804c21bae727f80cc464ae199c68340 Mon Sep 17 00:00:00 2001 From: janakr Date: Wed, 29 May 2019 13:57:16 -0700 Subject: [PATCH] Put ActionLookupData inside DerivedArtifact, and move ArtifactOwner into SourceArtifact. ActionLookupData is set inside a DerivedArtifact at ConfiguredTarget creation time. In particular, it is now mutable. This means that it is no longer safe to call #getArtifactOwner, #getOwnerLabel, etc. on an artifact if it was created by the configured target that is currently being analyzed. This allows all callers that were indirectly getting the ActionLookupData via retrieving an ActionLookupKey from the graph to just use it directly. For now, don't actually do very much with this new power. In particular, don't change ArtifactFunction to avoid that lookup. In a follow-up, ActionExecutionFunction will stop requesting ArtifactValues for normal output artifacts, instead requesting ActionExecutionValues directly, thus avoiding the memory and CPU of creating ArtifactValues for the vast majority of artifacts. Will also remove ArtifactOwner from ArtifactFactory#getDerivedArtifact in a follow-up. PiperOrigin-RevId: 250560440 --- .../testing/junit/runner/JUnit4Bazel.java | 4 +- .../build/lib/actions/ActionInputHelper.java | 23 +- .../build/lib/actions/ActionLookupValue.java | 47 ---- .../devtools/build/lib/actions/Actions.java | 208 +++++++++------ .../devtools/build/lib/actions/Artifact.java | 237 ++++++++++++------ .../build/lib/actions/ArtifactFactory.java | 4 +- .../lib/actions/BasicActionLookupValue.java | 17 +- .../build/lib/analysis/BuildView.java | 41 +-- .../analysis/CachingAnalysisEnvironment.java | 51 +++- .../build/lib/analysis/ConfiguredAspect.java | 21 +- .../lib/analysis/ConfiguredTargetFactory.java | 38 +-- .../analysis/RuleConfiguredTargetBuilder.java | 9 +- .../analysis/actions/SpawnActionTemplate.java | 9 +- .../MergedConfiguredTarget.java | 6 + .../RuleConfiguredTarget.java | 25 +- .../SkylarkRuleConfiguredTargetUtil.java | 3 + .../test/CoverageReportActionFactory.java | 39 ++- .../coverage/BazelCoverageReportModule.java | 9 +- .../coverage/CoverageReportActionBuilder.java | 4 +- .../build/lib/rules/cpp/CppCompileAction.java | 58 +---- .../rules/cpp/CppCompileActionTemplate.java | 8 +- .../lib/runtime/CriticalPathComputer.java | 5 +- .../lib/skyframe/ActionInputMapHelper.java | 34 ++- .../ActionTemplateExpansionFunction.java | 71 +++++- .../build/lib/skyframe/ArtifactFunction.java | 20 +- .../build/lib/skyframe/AspectValue.java | 2 +- .../skyframe/BuildInfoCollectionFunction.java | 4 +- .../skyframe/ConfiguredTargetFunction.java | 5 +- .../lib/skyframe/CoverageReportFunction.java | 11 +- .../NonRuleConfiguredTargetValue.java | 5 +- .../skyframe/RuleConfiguredTargetValue.java | 10 - .../lib/skyframe/SkyframeActionExecutor.java | 46 ++-- .../build/lib/skyframe/SkyframeExecutor.java | 29 +-- .../lib/skyframe/TestCompletionFunction.java | 58 +---- .../lib/skyframe/WorkspaceStatusValue.java | 3 +- .../java/com/google/devtools/build/lib/BUILD | 1 + .../lib/actions/ArtifactFactoryTest.java | 2 + .../build/lib/actions/ArtifactTest.java | 44 ++-- .../lib/actions/CustomCommandLineTest.java | 18 +- .../actions/ExecutableSymlinkActionTest.java | 2 + .../lib/actions/util/ActionsTestUtil.java | 27 +- .../actions/ParamFileWriteActionTest.java | 7 +- .../actions/SpawnActionTemplateTest.java | 10 +- .../analysis/actions/SymlinkActionTest.java | 2 + .../lib/analysis/util/BuildViewTestCase.java | 72 ++---- .../lib/exec/SpawnInputExpanderTest.java | 21 +- .../AbstractRemoteActionCacheTests.java | 3 - .../lib/rules/android/AndroidLibraryTest.java | 22 +- .../lib/rules/cpp/CppLinkActionTest.java | 19 +- .../lib/rules/cpp/HeaderDiscoveryTest.java | 2 - .../lib/rules/objc/HeaderThinningTest.java | 1 - .../proto/ProtoCompileActionBuilderTest.java | 32 ++- .../skyframe/ActionMetadataHandlerTest.java | 18 +- .../ActionTemplateExpansionFunctionTest.java | 9 +- .../lib/skyframe/ArtifactFunctionTest.java | 59 ++--- .../skyframe/FilesystemValueCheckerTest.java | 27 +- ...ursiveFilesystemTraversalFunctionTest.java | 6 +- .../skyframe/TimestampBuilderTestCase.java | 8 +- .../lib/skyframe/TreeArtifactBuildTest.java | 78 ++++-- .../skyframe/TreeArtifactMetadataTest.java | 5 +- 60 files changed, 848 insertions(+), 811 deletions(-) diff --git a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/JUnit4Bazel.java b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/JUnit4Bazel.java index 5ea00cd9ca5493..2443583b484970 100644 --- a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/JUnit4Bazel.java +++ b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/JUnit4Bazel.java @@ -59,8 +59,8 @@ import org.junit.runner.notification.RunListener; /** - * Utility class to create a JUnit4Runner instance from a {@link Builder}. All required - * dependencies are being injected automatically. + * Utility class to create a JUnit4Runner instance from a {@link Builder}. All required dependencies + * are being injected automatically. */ public final class JUnit4Bazel { private Supplier> topLevelSuiteSupplier; diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index 2d2ffd74457ce8..d3203158e7bb37 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -144,19 +144,9 @@ public static Collection fromPaths(Collection paths) { */ public static TreeFileArtifact treeFileArtifact( Artifact.SpecialArtifact parent, PathFragment relativePath) { - Preconditions.checkState(parent.isTreeArtifact(), - "Given parent %s must be a TreeArtifact", parent); - return new TreeFileArtifact(parent, relativePath); - } - - public static TreeFileArtifact treeFileArtifact( - Artifact.SpecialArtifact parent, PathFragment relativePath, ArtifactOwner artifactOwner) { - Preconditions.checkState(parent.isTreeArtifact(), - "Given parent %s must be a TreeArtifact", parent); - return new TreeFileArtifact( - parent, - relativePath, - artifactOwner); + TreeFileArtifact result = treeFileArtifactWithNoGeneratingActionSet(parent, relativePath); + result.setGeneratingActionKey(parent.getGeneratingActionKey()); + return result; } /** @@ -168,6 +158,13 @@ public static TreeFileArtifact treeFileArtifact( return treeFileArtifact(parent, PathFragment.create(relativePath)); } + public static TreeFileArtifact treeFileArtifactWithNoGeneratingActionSet( + Artifact.SpecialArtifact parent, PathFragment relativePath) { + Preconditions.checkState( + parent.isTreeArtifact(), "Given parent %s must be a TreeArtifact", parent); + return new TreeFileArtifact(parent, relativePath); + } + /** Returns an Iterable of TreeFileArtifacts with the given parent and parent relative paths. */ public static Iterable asTreeFileArtifacts( final Artifact.SpecialArtifact parent, Iterable parentRelativePaths) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java index 6611deb7f2aa54..278d9e8eebddea 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java @@ -15,15 +15,11 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.math.BigInteger; -import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** @@ -38,38 +34,6 @@ protected ActionLookupValue(@Nullable BigInteger nonceVersion) { /** Returns a list of actions registered by this {@link SkyValue}. */ public abstract ImmutableList getActions(); - /** - * Returns a map where keys are artifacts generated by this {@link SkyValue}, and values are - * the index of the action which generates this artifact. - */ - protected abstract ImmutableMap getGeneratingActionIndex(); - - /** - * Returns the index of the action that generates {@code artifact} in this value, or null if this - * value does not have a generating action for this artifact. The index together with the key for - * this {@link ActionLookupValue} uniquely identifies the action. - * - *

Unlike {@link #getAction}, this may be called after action execution. - */ - @Nullable - public Integer getGeneratingActionIndex(Artifact artifact) { - return getGeneratingActionIndex().get(artifact); - } - - /** - * Returns the action that generates {@code artifact}, if known to this value, or null. This - * method should be avoided. Call it only when the action is really needed, and it is known to be - * present. - */ - @Nullable - public ActionAnalysisMetadata getGeneratingActionDangerousReadJavadoc(Artifact artifact) { - Integer actionIndex = getGeneratingActionIndex(artifact); - if (actionIndex == null) { - return null; - } - return getActions().get(actionIndex); - } - /** * Returns the {@link Action} with index {@code index} in this value. Never null. Should only be * called during action execution by {@code ArtifactFunction} and {@code ActionExecutionFunction} @@ -98,17 +62,6 @@ public boolean isActionTemplate(int index) { return getActions().get(index) instanceof ActionTemplate; } - /** To be used only when checking consistency of the action graph -- not by other values. */ - public Map getMapForConsistencyCheck() { - return getMapForConsistencyCheck(getGeneratingActionIndex(), getActions()); - } - - public static Map getMapForConsistencyCheck( - Map generatingActionIndex, - final List actions) { - return Maps.transformValues(generatingActionIndex, actions::get); - } - /** * Returns the number of {@link Action} objects present in this value. */ diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index 4e9fb5efbf4ed0..6a44b935ad8f72 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java @@ -31,9 +31,6 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.SortedMap; -import java.util.TreeMap; -import java.util.function.Function; -import javax.annotation.Nullable; /** * Helper class for actions. @@ -112,80 +109,131 @@ private static boolean artifactsEqualWithoutOwner( } /** - * Finds action conflicts. An action conflict happens if two actions generate the same output - * artifact. Shared actions are tolerated. See {@link #canBeShared} for details. + * Assigns generating action keys to artifacts, and finds action conflicts. An action conflict + * happens if two actions generate the same output artifact. Shared actions are not allowed. See + * {@link #canBeShared} for details. Should be called by a configured target/aspect on the actions + * it owns. Should not be used for "global" checks of multiple configured targets: use {@link + * #findArtifactPrefixConflicts} for that. * - * @param actions a list of actions to check for action conflicts - * @return a structure giving the mapping between artifacts and generating actions, with a level - * of indirection. + * @param actions a list of actions to check for action conflict, all generated by the same + * configured target/aspect. + * @return a structure giving the actions, with a level of indirection. * @throws ActionConflictException iff there are two actions generate the same output */ - public static GeneratingActions findAndThrowActionConflict( - ActionKeyContext actionKeyContext, ImmutableList actions) + public static GeneratingActions assignOwnersAndFindAndThrowActionConflict( + ActionKeyContext actionKeyContext, + ImmutableList actions, + ActionLookupValue.ActionLookupKey actionLookupKey) throws ActionConflictException { - return Actions.maybeFilterSharedActionsAndThrowIfConflict( - actionKeyContext, actions, /*allowSharedAction=*/ false); + return Actions.assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( + actionKeyContext, actions, actionLookupKey, /*allowSharedAction=*/ false); } /** - * Finds action conflicts. An action conflict happens if two actions generate the same output - * artifact. Shared actions are tolerated. See {@link #canBeShared} for details. + * Assigns generating action keys to artifacts and finds action conflicts. An action conflict + * happens if two actions generate the same output artifact. Shared actions are tolerated. See + * {@link #canBeShared} for details. Should be called by a configured target/aspect on the actions + * it owns. Should not be used for "global" checks of multiple configured targets: use {@link + * #findArtifactPrefixConflicts} for that. * - * @param actions a list of actions to check for action conflicts - * @return a structure giving the mapping between artifacts and generating actions, with a level - * of indirection. + * @param actions a list of actions to check for action conflicts, all generated by the same + * configured target/aspect. + * @return a structure giving the actions, with a level of indirection. * @throws ActionConflictException iff there are two unshareable actions generating the same * output */ - public static GeneratingActions filterSharedActionsAndThrowActionConflict( - ActionKeyContext actionKeyContext, ImmutableList actions) + public static GeneratingActions assignOwnersAndFilterSharedActionsAndThrowActionConflict( + ActionKeyContext actionKeyContext, + ImmutableList actions, + ActionLookupValue.ActionLookupKey actionLookupKey) throws ActionConflictException { - return Actions.maybeFilterSharedActionsAndThrowIfConflict( - actionKeyContext, actions, /*allowSharedAction=*/ true); + return Actions.assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( + actionKeyContext, actions, actionLookupKey, /*allowSharedAction=*/ true); } - private static GeneratingActions maybeFilterSharedActionsAndThrowIfConflict( + private static void verifyGeneratingActionKeys( + Artifact.DerivedArtifact output, + ActionLookupData otherKey, + boolean allowSharedAction, + ActionKeyContext actionKeyContext, + ImmutableList actions) + throws ActionConflictException { + ActionLookupData firstKey = output.getGeneratingActionKey(); + Preconditions.checkState( + firstKey.getActionLookupKey().equals(otherKey.getActionLookupKey()), + "Mismatched lookup keys? %s %s %s", + output, + firstKey, + otherKey); + int actionIndex = firstKey.getActionIndex(); + int otherIndex = otherKey.getActionIndex(); + if (actionIndex != otherIndex + && (!allowSharedAction + || !Actions.canBeShared( + actionKeyContext, actions.get(actionIndex), actions.get(otherIndex)))) { + throw new ActionConflictException( + actionKeyContext, output, actions.get(actionIndex), actions.get(otherIndex)); + } + } + + /** + * Checks {@code actions} for conflicts and sets each artifact's generating action key. + * + *

Conflicts can happen in one of two ways: the same artifact can be the output of multiple + * unshareable actions (or shareable actions if {@code allowSharedAction} is false), or two + * artifacts with the same execPath can be the outputs of different unshareable actions. + * + *

Also builds a map of package-relative paths to artifacts, for use by output file configured + * targets when they are retrieving their artifacts from their associated rule. + */ + private static GeneratingActions assignOwnersAndMaybeFilterSharedActionsAndThrowIfConflict( ActionKeyContext actionKeyContext, ImmutableList actions, + ActionLookupValue.ActionLookupKey actionLookupKey, boolean allowSharedAction) throws ActionConflictException { - Map generatingActions = new HashMap<>(); + Map seenArtifacts = new HashMap<>(); + Map artifactsByPackageRelativePath = new HashMap<>(); + // If the action owner has a label, we get the package directory so that we can store package- + // relative paths. + Label owningLabel = actionLookupKey.getLabel(); + PathFragment packageDirectory = + owningLabel != null ? owningLabel.getPackageIdentifier().getSourceRoot() : null; + + // Loop over the actions, looking at all outputs for conflicts. int actionIndex = 0; for (ActionAnalysisMetadata action : actions) { + ActionLookupData generatingActionKey = ActionLookupData.create(actionLookupKey, actionIndex); for (Artifact artifact : action.getOutputs()) { - Integer previousIndex = generatingActions.put(artifact, actionIndex); - if (previousIndex != null && previousIndex != actionIndex) { - if (!allowSharedAction - || !Actions.canBeShared(actionKeyContext, actions.get(previousIndex), action)) { - throw new ActionConflictException( - actionKeyContext, artifact, actions.get(previousIndex), action); + Artifact.DerivedArtifact output = (Artifact.DerivedArtifact) artifact; + // Has an artifact with this execPath been seen before? + Artifact.DerivedArtifact equalOutput = + seenArtifacts.putIfAbsent(output.getExecPath(), output); + if (equalOutput != null) { + // Yes: assert that its generating action and this artifact's are compatible. + verifyGeneratingActionKeys( + equalOutput, generatingActionKey, allowSharedAction, actionKeyContext, actions); + } else { + // No: populate the package-relative path map with this artifact if applicable. + PathFragment rootRelativePath = output.getRootRelativePath(); + if (packageDirectory != null && rootRelativePath.startsWith(packageDirectory)) { + artifactsByPackageRelativePath.put( + rootRelativePath.relativeTo(packageDirectory), output); } } + // Was this output already seen, so it has a generating action key set? + if (!output.hasGeneratingActionKey()) { + // Common case: artifact hasn't been seen before. + output.setGeneratingActionKey(generatingActionKey); + } else { + // Key is already set: verify that the generating action and this action are compatible. + verifyGeneratingActionKeys( + output, generatingActionKey, allowSharedAction, actionKeyContext, actions); + } } actionIndex++; } - return new GeneratingActions(actions, ImmutableMap.copyOf(generatingActions)); - } - - /** - * Finds Artifact prefix conflicts between generated artifacts. An artifact prefix conflict - * happens if one action generates an artifact whose path is a prefix of another artifact's path. - * Those two artifacts cannot exist simultaneously in the output tree. - * - * @param generatingActions a map between generated artifacts and their associated generating - * actions. - * @return a map between actions that generated the conflicting artifacts and their associated - * {@link ArtifactPrefixConflictException}. - */ - public static Map - findArtifactPrefixConflicts(Map generatingActions) { - TreeMap artifactPathMap = new TreeMap<>(comparatorForPrefixConflicts()); - for (Artifact artifact : generatingActions.keySet()) { - artifactPathMap.put(artifact.getExecPath(), artifact); - } - - return findArtifactPrefixConflicts( - new MapBasedImmutableActionGraph(generatingActions), artifactPathMap); + return new GeneratingActions(actions, ImmutableMap.copyOf(artifactsByPackageRelativePath)); } /** @@ -321,52 +369,50 @@ public static String escapeLabel(Label label) { return PATH_ESCAPER.escape(label.getPackageName() + ":" + label.getName()); } - private static class MapBasedImmutableActionGraph implements ActionGraph { - private final Map generatingActions; - - MapBasedImmutableActionGraph( - Map generatingActions) { - this.generatingActions = ImmutableMap.copyOf(generatingActions); - } - - @Nullable - @Override - public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { - return generatingActions.get(artifact); - } - } - - /** Container class for actions and the artifacts they generate. */ + /** + * Container class for actions, ensuring they have already been checked for conflicts and their + * generated artifacts have had owners assigned. + */ public static class GeneratingActions { public static final GeneratingActions EMPTY = new GeneratingActions(ImmutableList.of(), ImmutableMap.of()); private final ImmutableList actions; - private final ImmutableMap generatingActionIndex; + private final ImmutableMap artifactsByPackageRelativePath; private GeneratingActions( ImmutableList actions, - ImmutableMap generatingActionIndex) { + ImmutableMap artifactsByPackageRelativePath) { this.actions = actions; - this.generatingActionIndex = generatingActionIndex; + this.artifactsByPackageRelativePath = artifactsByPackageRelativePath; } - public static GeneratingActions fromSingleAction(ActionAnalysisMetadata action) { + public static GeneratingActions fromSingleAction( + ActionAnalysisMetadata action, ActionLookupValue.ActionLookupKey actionLookupKey) { + Label owningLabel = actionLookupKey.getLabel(); + Map artifactsByPackageRelativePath = new HashMap<>(); + PathFragment packageDirectory = null; + if (owningLabel != null) { + packageDirectory = owningLabel.getPackageIdentifier().getSourceRoot(); + } + ActionLookupData generatingActionKey = ActionLookupData.create(actionLookupKey, 0); + for (Artifact output : action.getOutputs()) { + ((Artifact.DerivedArtifact) output).setGeneratingActionKey(generatingActionKey); + PathFragment rootRelativePath = output.getRootRelativePath(); + if (packageDirectory != null && rootRelativePath.startsWith(packageDirectory)) { + artifactsByPackageRelativePath.put(rootRelativePath.relativeTo(packageDirectory), output); + } + } return new GeneratingActions( - ImmutableList.of(action), - ImmutableMap.copyOf( - action - .getOutputs() - .stream() - .collect(ImmutableMap.toImmutableMap(Function.identity(), (a) -> 0)))); - } - - public ImmutableMap getGeneratingActionIndex() { - return generatingActionIndex; + ImmutableList.of(action), ImmutableMap.copyOf(artifactsByPackageRelativePath)); } public ImmutableList getActions() { return actions; } + + public ImmutableMap getArtifactsByPackageRelativePath() { + return artifactsByPackageRelativePath; + } } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 673143b9d70718..594247d06e9ff5 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import com.google.devtools.build.lib.skylarkbuildapi.FileApi; import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; import com.google.devtools.build.lib.util.FileType; @@ -103,7 +102,6 @@ * */ @Immutable -@AutoCodec public abstract class Artifact implements FileType.HasFileType, ActionInput, @@ -223,45 +221,11 @@ public ImmutableList getFileset(Artifact artifact) { /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */ public static final Predicate MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact(); - private static final Interner ARTIFACT_INTERNER = - BlazeInterners.newWeakInterner(); - private final int hashCode; private final ArtifactRoot root; private final PathFragment execPath; - private final ArtifactOwner owner; - /** - * The {@code rootRelativePath is a few characters shorter than the {@code execPath} for derived - * artifacts, so we save a few bytes by serializing it rather than the {@code execPath}, - * especially when the {@code root} is common to many artifacts and therefore memoized. - */ - @AutoCodec.VisibleForSerialization - @AutoCodec.Instantiator - static Artifact createForSerialization( - ArtifactRoot root, ArtifactOwner owner, PathFragment rootRelativePath) { - if (rootRelativePath == null || rootRelativePath.isAbsolute() != root.getRoot().isAbsolute()) { - throw new IllegalArgumentException( - rootRelativePath - + ": illegal rootRelativePath for " - + rootRelativePath - + " (root: " - + root - + ")"); - } - PathFragment rootExecPath = root.getExecPath(); - if (rootExecPath.isEmpty()) { - return new Artifact.SourceArtifact(root, rootRelativePath, owner); - } else { - return ARTIFACT_INTERNER.intern( - new Artifact.DerivedArtifact(root, rootExecPath.getRelative(rootRelativePath), owner)); - } - } - - private Artifact( - ArtifactRoot root, - PathFragment execPath, - ArtifactOwner owner) { + private Artifact(ArtifactRoot root, PathFragment execPath) { Preconditions.checkNotNull(root); if (execPath.isEmpty()) { throw new IllegalArgumentException( @@ -273,25 +237,102 @@ private Artifact( this.hashCode = execPath.hashCode(); this.root = root; this.execPath = execPath; - this.owner = Preconditions.checkNotNull(owner); } /** An artifact corresponding to a file in the output tree, generated by an {@link Action}. */ + @AutoCodec public static class DerivedArtifact extends Artifact { + private static final Interner INTERNER = BlazeInterners.newWeakInterner(); + private final PathFragment rootRelativePath; + private ActionLookupData generatingActionKey; @VisibleForTesting - public DerivedArtifact(ArtifactRoot root, PathFragment execPath, ArtifactOwner owner) { - super(root, execPath, owner); + public DerivedArtifact(ArtifactRoot root, PathFragment execPath) { + super(root, execPath); Preconditions.checkState( !root.getExecPath().isEmpty(), "Derived root has no exec path: %s, %s", root, execPath); this.rootRelativePath = execPath.relativeTo(root.getExecPath()); } + /** Called when a configured target's actions are being collected. */ + @VisibleForTesting + public void setGeneratingActionKey(ActionLookupData generatingActionKey) { + Preconditions.checkState( + this.generatingActionKey == null, + "Already set generating action key: %s (%s %s)", + this, + this.generatingActionKey, + generatingActionKey); + this.generatingActionKey = Preconditions.checkNotNull(generatingActionKey, this); + } + + boolean hasGeneratingActionKey() { + return this.generatingActionKey != null; + } + + @Override + public boolean hasArtifactOwnerSet() { + return hasGeneratingActionKey(); + } + + /** Can only be called once {@link #setGeneratingActionKey} is called. */ + public ActionLookupData getGeneratingActionKey() { + return Preconditions.checkNotNull(generatingActionKey, this); + } + + @Override + public ActionLookupValue.ActionLookupKey getArtifactOwner() { + return getGeneratingActionKey().getActionLookupKey(); + } + + @Override + public Label getOwnerLabel() { + return getGeneratingActionKey().getLabel(); + } + @Override public PathFragment getRootRelativePath() { return rootRelativePath; } + + @Override + boolean ownersEqual(Artifact other) { + DerivedArtifact that = (DerivedArtifact) other; + if (this.generatingActionKey == null || that.generatingActionKey == null) { + // Happens only intra-analysis of a configured target. Tolerate. + return true; + } + return this.generatingActionKey.equals(that.generatingActionKey); + } + + /** + * The {@code rootRelativePath is a few characters shorter than the {@code execPath}, so we save + * a few bytes by serializing it rather than the {@code execPath}, especially when the {@code + * root} is common to many artifacts and therefore memoized. + */ + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static DerivedArtifact createForSerialization( + ArtifactRoot root, PathFragment rootRelativePath, ActionLookupData generatingActionKey) { + if (rootRelativePath == null + || rootRelativePath.isAbsolute() != root.getRoot().isAbsolute()) { + throw new IllegalArgumentException( + rootRelativePath + + ": illegal rootRelativePath for " + + root + + " (generatingActionKey: " + + generatingActionKey + + ")"); + } + Preconditions.checkState( + !root.isSourceRoot(), "Root not derived: %s %s", root, rootRelativePath); + PathFragment rootExecPath = root.getExecPath(); + DerivedArtifact artifact = + new DerivedArtifact(root, rootExecPath.getRelative(rootRelativePath)); + artifact.setGeneratingActionKey(generatingActionKey); + return INTERNER.intern(artifact); + } } public final Path getPath() { @@ -357,28 +398,36 @@ public String expandToCommandLine() { } /** - * Returns the artifact owner. May be null. + * Returns the artifact's owning label. May be null. Can only be called if {@link + * #hasArtifactOwnerSet} returns true. */ - @Nullable public final Label getOwner() { + @Nullable + public final Label getOwner() { return getOwnerLabel(); } /** - * Gets the {@code ActionLookupKey} of the {@code ConfiguredTarget} that owns this artifact, if it - * was set. Otherwise, this should be a dummy value -- either {@link + * Can only be called if {@link #hasArtifactOwnerSet} returns true. + * + *

Gets the {@code ActionLookupKey} of the {@code ConfiguredTarget} that owns this artifact, if + * it was set. Otherwise, this should be a dummy value -- either {@link * ArtifactOwner.NullArtifactOwner#INSTANCE} or a dummy owner set in tests. Such a dummy value * should only occur for source artifacts if created without specifying the owner, or for special * derived artifacts, such as target completion middleman artifacts, build info artifacts, and the * like. + * + *

For derived artifacts, this can only be called after the configured target that created the + * artifact has finished analyzing. Before that analysis is complete, this artifact should not be + * visible to any other configured target, so there is no need to call this method, since the + * configured target knows what it is. */ - public final ArtifactOwner getArtifactOwner() { - return owner; - } + public abstract ArtifactOwner getArtifactOwner(); - @Override - public Label getOwnerLabel() { - return owner.getLabel(); - } + /** + * Always true for source artifacts. True for derived artifacts after their configured target has + * finished analyzing. + */ + public abstract boolean hasArtifactOwnerSet(); /** * Returns the root beneath which this Artifact resides, if any. This may be one of the @@ -471,18 +520,21 @@ public boolean isConstantMetadata() { * TODO(shahan): move {@link Artifact#getPath} to this subclass. * */ public static final class SourceArtifact extends Artifact { + private final ArtifactOwner owner; + @VisibleForTesting public SourceArtifact(ArtifactRoot root, PathFragment execPath, ArtifactOwner owner) { - super(root, execPath, owner); + super(root, execPath); + this.owner = owner; } /** - * SourceArtifacts are compared without their owners, since owners do not affect behavior, - * unlike with derived artifacts, whose owners determine their generating actions. + * Source artifacts do not consider their owners in equality checks, since their owners are + * purely cosmetic. */ @Override - public boolean equals(Object other) { - return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other); + boolean ownersEqual(Artifact other) { + return true; } @Override @@ -490,8 +542,23 @@ public PathFragment getRootRelativePath() { return getExecPath(); } + @Override + public ArtifactOwner getArtifactOwner() { + return owner; + } + + @Override + public Label getOwnerLabel() { + return owner.getLabel(); + } + + @Override + public boolean hasArtifactOwnerSet() { + return true; + } + boolean differentOwnerOrRoot(ArtifactOwner owner, ArtifactRoot root) { - return !this.getArtifactOwner().equals(owner) || !this.getRoot().equals(root); + return !this.owner.equals(owner) || !this.getRoot().equals(root); } } @@ -519,13 +586,24 @@ public enum SpecialArtifactType { public static final class SpecialArtifact extends DerivedArtifact { private final SpecialArtifactType type; - @VisibleForSerialization - public SpecialArtifact( - ArtifactRoot root, PathFragment execPath, ArtifactOwner owner, SpecialArtifactType type) { - super(root, execPath, owner); + @VisibleForTesting + public SpecialArtifact(ArtifactRoot root, PathFragment execPath, SpecialArtifactType type) { + super(root, execPath); this.type = type; } + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static SpecialArtifact create( + ArtifactRoot root, + PathFragment execPath, + SpecialArtifactType type, + ActionLookupData generatingActionKey) { + SpecialArtifact result = new SpecialArtifact(root, execPath, type); + result.setGeneratingActionKey(generatingActionKey); + return result; + } + @Override public final boolean isFileset() { return type == SpecialArtifactType.FILESET; @@ -584,25 +662,13 @@ public static final class TreeFileArtifact extends DerivedArtifact { /** * Constructs a TreeFileArtifact with the given parent-relative path under the given parent - * TreeArtifact. The {@link ArtifactOwner} of the TreeFileArtifact is the {@link ArtifactOwner} - * of the parent TreeArtifact. + * TreeArtifact. */ @VisibleForTesting - public TreeFileArtifact(SpecialArtifact parent, PathFragment parentRelativePath) { - this(parent, parentRelativePath, parent.getArtifactOwner()); - } - - /** - * Constructs a TreeFileArtifact with the given parent-relative path under the given parent - * TreeArtifact, owned by the given {@code artifactOwner}. - */ - @AutoCodec.Instantiator - TreeFileArtifact( - SpecialArtifact parentTreeArtifact, PathFragment parentRelativePath, ArtifactOwner owner) { + public TreeFileArtifact(SpecialArtifact parentTreeArtifact, PathFragment parentRelativePath) { super( parentTreeArtifact.getRoot(), - parentTreeArtifact.getExecPath().getRelative(parentRelativePath), - owner); + parentTreeArtifact.getExecPath().getRelative(parentRelativePath)); Preconditions.checkArgument( parentTreeArtifact.isTreeArtifact(), "The parent of TreeFileArtifact (parent-relative path: %s) is not a TreeArtifact: %s", @@ -620,6 +686,17 @@ public TreeFileArtifact(SpecialArtifact parent, PathFragment parentRelativePath) this.parentRelativePath = parentRelativePath; } + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static TreeFileArtifact createForSerialization( + SpecialArtifact parentTreeArtifact, + PathFragment parentRelativePath, + ActionLookupData generatingActionKey) { + TreeFileArtifact result = new TreeFileArtifact(parentTreeArtifact, parentRelativePath); + result.setGeneratingActionKey(generatingActionKey); + return result; + } + @Override public SpecialArtifact getParent() { return parentTreeArtifact; @@ -683,7 +760,7 @@ public final String prettyPrint() { @SuppressWarnings("EqualsGetClass") // Distinct classes of Artifact are never equal. @Override - public boolean equals(Object other) { + public final boolean equals(Object other) { if (this == other) { return true; } @@ -694,13 +771,15 @@ public boolean equals(Object other) { return false; } Artifact that = (Artifact) other; - return equalsWithoutOwner(that) && owner.equals(that.owner); + return equalsWithoutOwner(that) && ownersEqual(that); } - public boolean equalsWithoutOwner(Artifact other) { + final boolean equalsWithoutOwner(Artifact other) { return hashCode == other.hashCode && execPath.equals(other.execPath) && root.equals(other.root); } + abstract boolean ownersEqual(Artifact other); + @Override public final int hashCode() { // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java index 3cc5e3b6eb3827..45b0f0273d4b29 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java @@ -325,9 +325,9 @@ private Artifact createArtifact( if (type == null) { return root.isSourceRoot() ? new Artifact.SourceArtifact(root, execPath, owner) - : new Artifact.DerivedArtifact(root, execPath, owner); + : new Artifact.DerivedArtifact(root, execPath); } else { - return new Artifact.SpecialArtifact(root, execPath, owner, type); + return new Artifact.SpecialArtifact(root, execPath, type); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java index 3b4a153b3cd9ea..2e3eef90018947 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java @@ -17,8 +17,6 @@ import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import java.math.BigInteger; import javax.annotation.Nullable; @@ -28,22 +26,18 @@ */ public class BasicActionLookupValue extends ActionLookupValue { protected final ImmutableList actions; - @VisibleForSerialization protected final ImmutableMap generatingActionIndex; protected BasicActionLookupValue( ImmutableList actions, - ImmutableMap generatingActionIndex, @Nullable BigInteger nonceVersion) { super(nonceVersion); this.actions = actions; - this.generatingActionIndex = generatingActionIndex; } @VisibleForTesting public BasicActionLookupValue( Actions.GeneratingActions generatingActions, @Nullable BigInteger nonceVersion) { - this( - generatingActions.getActions(), generatingActions.getGeneratingActionIndex(), nonceVersion); + this(generatingActions.getActions(), nonceVersion); } @Override @@ -51,14 +45,7 @@ public ImmutableList getActions() { return actions; } - @Override - protected ImmutableMap getGeneratingActionIndex() { - return generatingActionIndex; - } - protected ToStringHelper getStringHelper() { - return MoreObjects.toStringHelper(this) - .add("actions", actions) - .add("generatingActionIndex", generatingActionIndex); + return MoreObjects.toStringHelper(this).add("actions", actions); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 9e82277a78634d..96a77454795082 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -29,10 +29,11 @@ import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionGraph; +import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; -import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.PackageRoots; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; @@ -77,7 +78,6 @@ import com.google.devtools.build.lib.syntax.SkylarkImport.SkylarkImportSyntaxException; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.RegexFilter; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; import java.util.ArrayList; import java.util.Collection; @@ -489,10 +489,11 @@ private AnalysisResult createResult( allTargetsToTest, baselineCoverageArtifacts, getArtifactFactory(), + skyframeExecutor.getActionKeyContext(), CoverageReportValue.COVERAGE_REPORT_KEY, loadingResult.getWorkspaceName()); if (actionsWrapper != null) { - ImmutableList actions = actionsWrapper.getActions(); + Actions.GeneratingActions actions = actionsWrapper.getActions(); skyframeExecutor.injectCoverageReportData(actions); actionsWrapper.getCoverageOutputs().forEach(artifactsToOwnerLabelsBuilder::addArtifact); } @@ -516,19 +517,25 @@ private AnalysisResult createResult( @Nullable @Override public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) { - ArtifactOwner artifactOwner = artifact.getArtifactOwner(); - if (artifactOwner instanceof ActionLookupValue.ActionLookupKey) { - SkyKey key = (ActionLookupValue.ActionLookupKey) artifactOwner; - ActionLookupValue val; - try { - val = (ActionLookupValue) graph.getValue(key); - } catch (InterruptedException e) { - throw new IllegalStateException( - "Interruption not expected from this graph: " + key, e); - } - return val == null ? null : val.getGeneratingActionDangerousReadJavadoc(artifact); + if (artifact.isSourceArtifact()) { + return null; + } + ActionLookupData generatingActionKey = + ((Artifact.DerivedArtifact) artifact).getGeneratingActionKey(); + ActionLookupValue val; + try { + val = (ActionLookupValue) graph.getValue(generatingActionKey.getActionLookupKey()); + } catch (InterruptedException e) { + throw new IllegalStateException( + "Interruption not expected from this graph: " + generatingActionKey, e); + } + if (val == null) { + return null; } - return null; + int actionIndex = generatingActionKey.getActionIndex(); + return val.isActionTemplate(actionIndex) + ? val.getActionTemplate(actionIndex) + : val.getAction(actionIndex); } }; return new AnalysisResult( @@ -666,8 +673,8 @@ private ImmutableList filterTransitiveExtraActions( ImmutableList.Builder artifacts = ImmutableList.builder(); // Add to 'artifacts' all extra-actions which were registered by aspects which 'topLevel' // might have injected. - for (Artifact artifact : provider.getTransitiveExtraActionArtifacts()) { - ArtifactOwner owner = artifact.getArtifactOwner(); + for (Artifact.DerivedArtifact artifact : provider.getTransitiveExtraActionArtifacts()) { + ActionLookupValue.ActionLookupKey owner = artifact.getArtifactOwner(); if (owner instanceof AspectKey) { if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) { artifacts.add(artifact); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index d66dee76dd815f..b31208d5ee2e68 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.WorkspaceStatusValue; import com.google.devtools.build.lib.syntax.StarlarkSemantics; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; import java.io.PrintWriter; @@ -76,7 +77,15 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment { private MiddlemanFactory middlemanFactory; private ExtendedEventHandler errorEventListener; private SkyFunction.Environment skyframeEnv; - private Map artifacts; + /** + * Map of artifacts to either themselves or to {@code Pair} if + * --experimental_extended_sanity_checks is enabled. In the latter case, the string will contain + * the stack trace of where the artifact was created. In the former case, we'll construct a + * generic message in case of error. + * + *

The artifact is stored so that we can deduplicate artifacts created multiple times. + */ + private Map artifacts; /** * The list of actions registered by the configured target this analysis environment is @@ -198,12 +207,21 @@ private Map getOrphanArtifactMap() { // The order of the artifacts.entrySet iteration is unspecified - we use a TreeMap here to // guarantee that the return value of this method is deterministic. Map orphanArtifacts = new TreeMap<>(Artifact.EXEC_PATH_COMPARATOR); - for (Map.Entry entry : artifacts.entrySet()) { + for (Map.Entry entry : artifacts.entrySet()) { Artifact a = entry.getKey(); if (!a.isSourceArtifact() && !artifactsWithActions.contains(a)) { - orphanArtifacts.put(a, String.format("%s\n%s", - a.getExecPathString(), // uncovered artifact - entry.getValue())); // origin of creation + Object value = entry.getValue(); + if (value instanceof Artifact) { + value = "No origin, run with --experimental_extended_sanity_checks"; + } else { + value = ((Pair) value).second; + } + orphanArtifacts.put( + a, + String.format( + "%s\n%s", + a.getExecPathString(), // uncovered artifact + value)); // origin of creation } } return orphanArtifacts; @@ -240,14 +258,23 @@ public MiddlemanFactory getMiddlemanFactory() { * sealed (disable()). For performance reasons we only track the originating stacktrace when * running with --experimental_extended_sanity_checks. */ - private Artifact.DerivedArtifact trackArtifactAndOrigin( + @SuppressWarnings("unchecked") // Cast of artifacts map's value to Pair. + private Artifact.DerivedArtifact dedupAndTrackArtifactAndOrigin( Artifact.DerivedArtifact a, @Nullable Throwable e) { - if ((e != null) && !artifacts.containsKey(a)) { + if (artifacts.containsKey(a)) { + Object value = artifacts.get(a); + if (e == null) { + return (Artifact.DerivedArtifact) value; + } else { + return ((Pair) value).first; + } + } + if ((e != null)) { StringWriter sw = new StringWriter(); e.printStackTrace(new PrintWriter(sw)); - artifacts.put(a, sw.toString()); + artifacts.put(a, Pair.of(a, sw.toString())); } else { - artifacts.put(a, "No origin, run with --experimental_extended_sanity_checks"); + artifacts.put(a, a); } return a; } @@ -256,7 +283,7 @@ private Artifact.DerivedArtifact trackArtifactAndOrigin( public Artifact.DerivedArtifact getDerivedArtifact( PathFragment rootRelativePath, ArtifactRoot root) { Preconditions.checkState(enabled); - return trackArtifactAndOrigin( + return dedupAndTrackArtifactAndOrigin( artifactFactory.getDerivedArtifact(rootRelativePath, root, getOwner()), extendedSanityChecks ? new Throwable() : null); } @@ -265,7 +292,7 @@ public Artifact.DerivedArtifact getDerivedArtifact( public SpecialArtifact getTreeArtifact(PathFragment rootRelativePath, ArtifactRoot root) { Preconditions.checkState(enabled); return (SpecialArtifact) - trackArtifactAndOrigin( + dedupAndTrackArtifactAndOrigin( artifactFactory.getTreeArtifact(rootRelativePath, root, getOwner()), extendedSanityChecks ? new Throwable() : null); } @@ -274,7 +301,7 @@ public SpecialArtifact getTreeArtifact(PathFragment rootRelativePath, ArtifactRo public Artifact.DerivedArtifact getFilesetArtifact( PathFragment rootRelativePath, ArtifactRoot root) { Preconditions.checkState(enabled); - return trackArtifactAndOrigin( + return dedupAndTrackArtifactAndOrigin( artifactFactory.getFilesetArtifact(rootRelativePath, root, getOwner()), extendedSanityChecks ? new Throwable() : null); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java index 5c015335726b36..832c1ae96cfbbc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java @@ -64,18 +64,15 @@ public final class ConfiguredAspect { private final AspectDescriptor descriptor; private final ImmutableList actions; - private final ImmutableMap generatingActionIndex; private final TransitiveInfoProviderMap providers; @AutoCodec.VisibleForSerialization ConfiguredAspect( AspectDescriptor descriptor, ImmutableList actions, - ImmutableMap generatingActionIndex, TransitiveInfoProviderMap providers) { this.descriptor = descriptor; this.actions = actions; - this.generatingActionIndex = generatingActionIndex; this.providers = providers; // Initialize every SkylarkApiProvider @@ -105,14 +102,6 @@ public ImmutableList getActions() { return actions; } - /** - * Returns a map where keys are artifacts that are action outputs of this rule, and values are the - * index of the action that generates that artifact. - */ - public ImmutableMap getGeneratingActionIndex() { - return generatingActionIndex; - } - /** Returns the providers created by the aspect. */ public TransitiveInfoProviderMap getProviders() { return providers; @@ -145,15 +134,13 @@ public Object get(String legacyKey) { } public static ConfiguredAspect forAlias(ConfiguredAspect real) { - return new ConfiguredAspect( - real.descriptor, real.getActions(), real.getGeneratingActionIndex(), real.getProviders()); + return new ConfiguredAspect(real.descriptor, real.getActions(), real.getProviders()); } public static ConfiguredAspect forNonapplicableTarget(AspectDescriptor descriptor) { return new ConfiguredAspect( descriptor, ImmutableList.of(), - ImmutableMap.of(), new TransitiveInfoProviderMapBuilder().add().build()); } @@ -289,14 +276,14 @@ public ConfiguredAspect build() throws ActionConflictException { AnalysisEnvironment analysisEnvironment = ruleContext.getAnalysisEnvironment(); GeneratingActions generatingActions = - Actions.filterSharedActionsAndThrowActionConflict( + Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict( analysisEnvironment.getActionKeyContext(), - analysisEnvironment.getRegisteredActions()); + analysisEnvironment.getRegisteredActions(), + ruleContext.getOwner()); return new ConfiguredAspect( descriptor, generatingActions.getActions(), - generatingActions.getGeneratingActionIndex(), providers.build()); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index d8096c6f9f2e31..7f76c220869198 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -24,8 +24,6 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.actions.ArtifactFactory; -import com.google.devtools.build.lib.actions.ArtifactOwner; -import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FailAction; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.DependencyResolver.DependencyKind; @@ -36,6 +34,7 @@ import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget; +import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil; import com.google.devtools.build.lib.analysis.test.AnalysisFailure; import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo; @@ -70,7 +69,6 @@ import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.util.OrderedSetMultimap; -import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -160,29 +158,6 @@ private TransitiveInfoCollection findPrerequisite( return null; } - /** Returns the output artifact for the given file, or null if Skyframe deps are missing. */ - private Artifact getOutputArtifact( - OutputFile outputFile, - BuildConfiguration configuration, - boolean isFileset, - ArtifactFactory artifactFactory) { - Rule rule = outputFile.getAssociatedRule(); - ArtifactRoot root = - rule.hasBinaryOutput() - ? configuration.getBinDirectory(rule.getRepository()) - : configuration.getGenfilesDirectory(rule.getRepository()); - ArtifactOwner owner = ConfiguredTargetKey.of(rule.getLabel(), configuration); - PathFragment rootRelativePath = - outputFile.getLabel().getPackageIdentifier().getSourceRoot().getRelative( - outputFile.getLabel().getName()); - Artifact result = isFileset - ? artifactFactory.getFilesetArtifact(rootRelativePath, root, owner) - : artifactFactory.getDerivedArtifact(rootRelativePath, root, owner); - // The associated rule should have created the artifact. - Preconditions.checkNotNull(result, "no artifact for %s", rootRelativePath); - return result; - } - /** * Invokes the appropriate constructor to create a {@link ConfiguredTarget} instance. * @@ -231,14 +206,15 @@ public final ConfiguredTarget createConfiguredTarget( config, prerequisiteMap.get(DependencyResolver.OUTPUT_FILE_RULE_DEPENDENCY), visibility); - boolean isFileset = outputFile.getGeneratingRule().getRuleClass().equals("Fileset"); - Artifact artifact = getOutputArtifact(outputFile, config, isFileset, artifactFactory); if (analysisEnvironment.getSkyframeEnv().valuesMissing()) { return null; } - TransitiveInfoCollection rule = targetContext.findDirectPrerequisite( - outputFile.getGeneratingRule().getLabel(), config); - return new OutputFileConfiguredTarget(targetContext, outputFile, rule, artifact); + RuleConfiguredTarget rule = + (RuleConfiguredTarget) + targetContext.findDirectPrerequisite( + outputFile.getGeneratingRule().getLabel(), config); + Artifact artifact = rule.getOutputByPackageRelativePath(outputFile.getLabel().getName()); + return new OutputFileConfiguredTarget(targetContext, outputFile, rule, artifact); } else if (target instanceof InputFile) { InputFile inputFile = (InputFile) target; TargetContext targetContext = diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 15f79376bf4fca..1d553fdc388943 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -222,13 +222,16 @@ public ConfiguredTarget build() throws ActionConflictException { } AnalysisEnvironment analysisEnvironment = ruleContext.getAnalysisEnvironment(); - GeneratingActions generatingActions = Actions.filterSharedActionsAndThrowActionConflict( - analysisEnvironment.getActionKeyContext(), analysisEnvironment.getRegisteredActions()); + GeneratingActions generatingActions = + Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict( + analysisEnvironment.getActionKeyContext(), + analysisEnvironment.getRegisteredActions(), + ruleContext.getOwner()); return new RuleConfiguredTarget( ruleContext, providers, generatingActions.getActions(), - generatingActions.getGeneratingActionIndex()); + generatingActions.getArtifactsByPackageRelativePath()); } private NestedSet