Make Artifact#equals take the owner into account for derived artifacts.
Derived artifacts' owners are important because they are used to determine the artifact's generating action. Source artifacts' owners are not used in this way, so I left them alone.
This allows us to get rid of most uses of ArtifactSkyKey. We may be able to delete it entirely in a follow-up.
PiperOrigin-RevId: 199836436
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 4336db0..3311ca3 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
@@ -17,7 +17,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
import com.google.common.escape.Escaper;
import com.google.common.escape.Escapers;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
@@ -25,6 +24,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.OsPathPolicy;
import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Collection;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
@@ -75,15 +75,39 @@
}
// Don't bother to check input and output counts first; the expected result for these tests is
// to always be true (i.e., that this method returns true).
- if (!Iterables.elementsEqual(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
+ if (!artifactsEqualWithoutOwner(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
return false;
}
- if (!Iterables.elementsEqual(actionA.getOutputs(), actionB.getOutputs())) {
+ if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) {
return false;
}
return true;
}
+ private static boolean artifactsEqualWithoutOwner(
+ Iterable<Artifact> iterable1, Iterable<Artifact> iterable2) {
+ if (iterable1 instanceof Collection && iterable2 instanceof Collection) {
+ Collection<?> collection1 = (Collection<?>) iterable1;
+ Collection<?> collection2 = (Collection<?>) iterable2;
+ if (collection1.size() != collection2.size()) {
+ return false;
+ }
+ }
+ Iterator<Artifact> iterator1 = iterable1.iterator();
+ Iterator<Artifact> iterator2 = iterable2.iterator();
+ while (iterator1.hasNext()) {
+ if (!iterator2.hasNext()) {
+ return false;
+ }
+ Artifact artifact1 = iterator1.next();
+ Artifact artifact2 = iterator2.next();
+ if (!artifact1.equalsWithoutOwner(artifact2)) {
+ return false;
+ }
+ }
+ return !iterator2.hasNext();
+ }
+
/**
* Finds action conflicts. An action conflict happens if two actions generate the same output
* artifact. Shared actions are tolerated. See {@link #canBeShared} for details.
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 f18e14f..f91c5cd 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
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.actions;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static java.util.Comparator.comparing;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
@@ -45,6 +44,8 @@
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;
import java.io.IOException;
@@ -107,7 +108,8 @@
ActionInput,
FileApi,
Comparable<Object>,
- CommandLineItem {
+ CommandLineItem,
+ SkyKey {
/** Compares artifact according to their exec paths. Sorts null values first. */
@SuppressWarnings("ReferenceEquality") // "a == b" is an optimization
@@ -124,9 +126,7 @@
}
};
- /** Compares artifacts according to their root relative paths. */
- public static final Comparator<Artifact> ROOT_RELATIVE_PATH_COMPARATOR =
- comparing(Artifact::getRootRelativePath);
+ public static final SkyFunctionName ARTIFACT = SkyFunctionName.create("ARTIFACT");
@Override
public int compareTo(Object o) {
@@ -228,6 +228,9 @@
throw new IllegalArgumentException(
"it is illegal to create an artifact with an empty execPath");
}
+ // The ArtifactOwner is not part of this computation because it is very rare that two Artifacts
+ // have the same execPath and different owners, so a collision is fine there. If this is
+ // changed, OwnerlessArtifactWrapper must also be changed.
this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
this.root = root;
this.execPath = execPath;
@@ -455,6 +458,15 @@
public SourceArtifact asSourceArtifact() {
return this;
}
+
+ /**
+ * SourceArtifacts are compared without their owners, since owners do not affect behavior,
+ * unlike with derived artifacts, whose owners determine their generating actions.
+ */
+ @Override
+ public boolean equals(Object other) {
+ return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other);
+ }
}
/**
@@ -637,36 +649,26 @@
}
@Override
- public final boolean equals(Object other) {
+ public boolean equals(Object other) {
if (!(other instanceof Artifact)) {
return false;
}
if (!getClass().equals(other.getClass())) {
return false;
}
- // We don't bother to check root in the equivalence relation, because we
- // assume that no root is an ancestor of another one.
Artifact that = (Artifact) other;
- return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root);
+ return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner());
}
- /**
- * Compare equality including Artifact owner equality, a notable difference compared to the
- * {@link #equals(Object)} method of {@link Artifact}.
- */
- public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) {
- if (first != null) {
- return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
- } else {
- return second == null;
- }
+ public boolean equalsWithoutOwner(Artifact other) {
+ return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root);
}
@Override
public final int hashCode() {
- // This is just path.hashCode(). We cache a copy in the Artifact object to reduce LLC misses
- // during operations which build a HashSet out of many Artifacts. This is a slight loss for
- // memory but saves ~1% overall CPU in some real builds.
+ // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact
+ // object to reduce LLC misses during operations which build a HashSet out of many Artifacts.
+ // This is a slight loss for memory but saves ~1% overall CPU in some real builds.
return hashCode;
}
@@ -974,4 +976,33 @@
printer.append("<generated file " + rootRelativePath + ">");
}
}
+
+ /**
+ * A utility class that compares {@link Artifact}s without taking their owners into account.
+ * Should only be used for detecting action conflicts and merging shared action data.
+ */
+ public static class OwnerlessArtifactWrapper {
+ private final Artifact artifact;
+
+ public OwnerlessArtifactWrapper(Artifact artifact) {
+ this.artifact = artifact;
+ }
+
+ @Override
+ public int hashCode() {
+ // Depends on the fact that Artifact#hashCode does not use ArtifactOwner.
+ return artifact.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ return obj instanceof OwnerlessArtifactWrapper
+ && this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact);
+ }
+ }
+
+ @Override
+ public SkyFunctionName functionName() {
+ return ARTIFACT;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
index 703611f..fca7d22 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import java.util.Objects;
import javax.annotation.Nullable;
/**
@@ -111,9 +112,7 @@
if (o instanceof FilesetTraversalParams.DirectTraversalRoot) {
FilesetTraversalParams.DirectTraversalRoot that =
(FilesetTraversalParams.DirectTraversalRoot) o;
- // Careful! We must compare the artifact owners, which the default {@link Artifact#equals()}
- // method does not do. See the comments on {@link ArtifactSkyKey} and http://b/73738481.
- return Artifact.equalWithOwner(this.getOutputArtifact(), that.getOutputArtifact())
+ return Objects.equals(this.getOutputArtifact(), that.getOutputArtifact())
&& (this.getRootPart().equals(that.getRootPart()))
&& (this.getRelativePart().equals(that.getRelativePart()));
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
index f35f963..dadad15 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.actions;
import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;
@@ -24,7 +25,7 @@
@ThreadSafe
public final class MapBasedActionGraph implements MutableActionGraph {
private final ActionKeyContext actionKeyContext;
- private final ConcurrentMultimapWithHeadElement<Artifact, ActionAnalysisMetadata>
+ private final ConcurrentMultimapWithHeadElement<OwnerlessArtifactWrapper, ActionAnalysisMetadata>
generatingActionMap = new ConcurrentMultimapWithHeadElement<>();
public MapBasedActionGraph(ActionKeyContext actionKeyContext) {
@@ -34,17 +35,18 @@
@Override
@Nullable
public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
- return generatingActionMap.get(artifact);
+ return generatingActionMap.get(new OwnerlessArtifactWrapper(artifact));
}
@Override
public void registerAction(ActionAnalysisMetadata action) throws ActionConflictException {
for (Artifact artifact : action.getOutputs()) {
- ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(artifact, action);
+ OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact);
+ ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(wrapper, action);
if (previousAction != null
&& previousAction != action
&& !Actions.canBeShared(actionKeyContext, action, previousAction)) {
- generatingActionMap.remove(artifact, action);
+ generatingActionMap.remove(wrapper, action);
throw new ActionConflictException(actionKeyContext, artifact, previousAction, action);
}
}
@@ -53,8 +55,9 @@
@Override
public void unregisterAction(ActionAnalysisMetadata action) {
for (Artifact artifact : action.getOutputs()) {
- generatingActionMap.remove(artifact, action);
- ActionAnalysisMetadata otherAction = generatingActionMap.get(artifact);
+ OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact);
+ generatingActionMap.remove(wrapper, action);
+ ActionAnalysisMetadata otherAction = generatingActionMap.get(wrapper);
Preconditions.checkState(
otherAction == null
|| (otherAction != action
diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
index 430c373..081fd67 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
@@ -133,8 +133,7 @@
* NestedSet#children}. When that field is an Object[], this is just identity hash code and
* reference equality, but when it is something else (like an Artifact), we will do an actual
* equality comparison. This may make some singleton NestedSets reference-equal where they were
- * not before. This should be ok, but isn't because Artifact does not properly implement equality
- * (it ignores the ArtifactOwner).
+ * not before. This should be ok as long as the contained object properly implements equality.
*
* <p>TODO(janakr): Fix this bug.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
index ae4ced6..8112076 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
@@ -18,6 +18,7 @@
import com.google.common.base.Predicates;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionLookupData;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey;
@@ -31,11 +32,12 @@
*/
public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
@SuppressWarnings("unchecked")
- private static final Predicate<SkyKey> IS_ARTIFACT_OR_ACTION_SKY_KEY = Predicates.or(
- SkyFunctions.isSkyFunction(SkyFunctions.ARTIFACT),
- SkyFunctions.isSkyFunction(SkyFunctions.ACTION_EXECUTION),
- SkyFunctions.isSkyFunction(SkyFunctions.TARGET_COMPLETION),
- SkyFunctions.isSkyFunction(SkyFunctions.TEST_COMPLETION));
+ private static final Predicate<SkyKey> IS_ARTIFACT_OR_ACTION_SKY_KEY =
+ Predicates.or(
+ SkyFunctions.isSkyFunction(Artifact.ARTIFACT),
+ SkyFunctions.isSkyFunction(SkyFunctions.ACTION_EXECUTION),
+ SkyFunctions.isSkyFunction(SkyFunctions.TARGET_COMPLETION),
+ SkyFunctions.isSkyFunction(SkyFunctions.TEST_COMPLETION));
ActionArtifactCycleReporter(PackageProvider packageProvider) {
super(packageProvider);
@@ -46,9 +48,15 @@
return prettyPrint(key.functionName(), key.argument());
}
- private String prettyPrint(SkyFunctionName skyFunctionName, Object arg) {
+ private static String prettyPrintArtifact(Artifact artifact) {
+ return "file: " + artifact.getRootRelativePathString();
+ }
+
+ private static String prettyPrint(SkyFunctionName skyFunctionName, Object arg) {
if (arg instanceof ArtifactSkyKey) {
- return "file: " + ((ArtifactSkyKey) arg).getArtifact().getRootRelativePathString();
+ return prettyPrintArtifact(((ArtifactSkyKey) arg).getArtifact());
+ } else if (arg instanceof Artifact) {
+ return prettyPrintArtifact(((Artifact) arg));
} else if (arg instanceof ActionLookupData) {
return "action from: " + arg;
} else if (arg instanceof TargetCompletionKey
@@ -67,6 +75,8 @@
Object arg = key.argument();
if (arg instanceof ArtifactSkyKey) {
return ((ArtifactSkyKey) arg).getArtifact().getOwner();
+ } else if (arg instanceof Artifact) {
+ return ((Artifact) arg).getOwner();
} else if (arg instanceof ActionLookupData) {
return ((ActionLookupData) arg).getLabelForErrors();
} else if (arg instanceof TargetCompletionKey
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 8ddb52a..687a787 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -19,10 +19,12 @@
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.Maps;
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
@@ -33,6 +35,8 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
import javax.annotation.Nullable;
/**
@@ -196,4 +200,33 @@
}
return value;
}
+
+ private static <V> ImmutableMap<Artifact, V> transformKeys(
+ ImmutableMap<Artifact, V> data, Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap) {
+ if (data.isEmpty()) {
+ return data;
+ }
+ ImmutableMap.Builder<Artifact, V> result = ImmutableMap.builderWithExpectedSize(data.size());
+ for (Map.Entry<Artifact, V> entry : data.entrySet()) {
+ Artifact transformedArtifact =
+ Preconditions.checkNotNull(
+ newArtifactMap.get(new OwnerlessArtifactWrapper(entry.getKey())), entry);
+ result.put(transformedArtifact, entry.getValue());
+ }
+ return result.build();
+ }
+
+ ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) {
+ Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap =
+ outputs
+ .stream()
+ .collect(Collectors.toMap(OwnerlessArtifactWrapper::new, Function.identity()));
+ // This is only called for shared actions, so we'll almost certainly have to transform all keys
+ // in all sets.
+ return new ActionExecutionValue(
+ transformKeys(artifactData, newArtifactMap),
+ transformKeys(treeArtifactData, newArtifactMap),
+ transformKeys(additionalOutputData, newArtifactMap),
+ outputSymlinks);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 6913091..fbf3ec0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -53,11 +53,11 @@
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws ArtifactFunctionException, InterruptedException {
- ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
- Artifact artifact = artifactSkyKey.getArtifact();
+ Artifact artifact = ArtifactSkyKey.artifact(skyKey);
+ boolean isMandatory = ArtifactSkyKey.isMandatory(skyKey);
if (artifact.isSourceArtifact()) {
try {
- return createSourceValue(artifact, artifactSkyKey.isMandatory(), env);
+ return createSourceValue(artifact, isMandatory, env);
} catch (MissingInputFileException e) {
// The error is not necessarily truly transient, but we mark it as such because we have
// the above side effect of posting an event to the EventBus. Importantly, that event
@@ -279,8 +279,7 @@
throws InterruptedException {
// This artifact aggregates other artifacts. Keep track of them so callers can find them.
ImmutableList.Builder<Pair<Artifact, FileArtifactValue>> inputs = ImmutableList.builder();
- for (Map.Entry<SkyKey, SkyValue> entry :
- env.getValues(ArtifactSkyKey.mandatoryKeys(action.getInputs())).entrySet()) {
+ for (Map.Entry<SkyKey, SkyValue> entry : env.getValues(action.getInputs()).entrySet()) {
Artifact input = ArtifactSkyKey.artifact(entry.getKey());
SkyValue inputValue = entry.getValue();
Preconditions.checkNotNull(inputValue, "%s has null dep %s", artifact, input);
@@ -316,7 +315,7 @@
@Override
public String extractTag(SkyKey skyKey) {
- return Label.print(((ArtifactSkyKey) skyKey.argument()).getArtifact().getOwner());
+ return Label.print(ArtifactSkyKey.artifact(skyKey).getOwner());
}
@VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
index 20d9e97..e63b73c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
@@ -13,135 +13,63 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.base.Function;
import com.google.common.base.Preconditions;
-import com.google.common.collect.Collections2;
import com.google.common.collect.Interner;
-import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
-import java.util.Collection;
/**
- * A {@link SkyKey} coming from an {@link Artifact}. Source artifacts are checked for existence,
- * while output artifacts imply creation of the output file.
+ * A {@link SkyKey} coming from an {@link Artifact} that is not mandatory: absence of the Artifact
+ * does not imply any error.
*
- * <p>There are effectively three kinds of output artifact values corresponding to these keys. The
- * first corresponds to an ordinary artifact {@link FileArtifactValue}. It stores the relevant data
- * for the artifact -- digest/mtime and size. The second corresponds to either an "aggregating"
- * artifact -- the output of an aggregating middleman action -- or a TreeArtifact. It stores the
- * relevant data of all its inputs, as well as a combined digest for itself.
- *
- * <p>Artifacts are compared using just their paths, but in Skyframe, the configured target that
- * owns an artifact must also be part of the comparison. For example, suppose we build //foo:foo in
- * configurationA, yielding artifact foo.out. If we change the configuration to configurationB in
- * such a way that the path to the artifact does not change, requesting foo.out from the graph will
- * result in the value entry for foo.out under configurationA being returned. This would prevent
- * caching the graph in different configurations, and also causes big problems with change pruning,
- * which assumes the invariant that a value's first dependency will always be the same. In this
- * case, the value entry's old dependency on //foo:foo in configurationA would cause it to request
- * (//foo:foo, configurationA) from the graph, causing an undesired re-analysis of (//foo:foo,
- * configurationA).
- *
- * <p>In order to prevent that, instead of just comparing Artifacts, we compare for equality using
- * both the Artifact, and the owner. The effect is functionally that of making Artifact.equals()
- * check the owner, but only within these keys, since outside of Skyframe it is quite crucial that
- * Artifacts with different owners be able to compare equal.
+ * <p>Since {@link Artifact} is already a {@link SkyKey}, this wrapper is only needed when the
+ * {@link Artifact} is not mandatory (discovered during include scanning).
*/
+// TODO(janakr): pull mandatory/non-mandatory handling up to consumers and get rid of this wrapper.
@AutoCodec
public class ArtifactSkyKey implements SkyKey {
private static final Interner<ArtifactSkyKey> INTERNER = BlazeInterners.newWeakInterner();
- private static final Function<Artifact, SkyKey> TO_MANDATORY_KEY =
- artifact -> key(artifact, true);
- private static final Function<ArtifactSkyKey, Artifact> TO_ARTIFACT = ArtifactSkyKey::getArtifact;
- private final Artifact artifact;
- // Always true for derived artifacts.
- private final boolean isMandatory;
- // TODO(janakr): we may want to remove this field in the future. The expensive hash computation
- // is already cached one level down (in the Artifact), so the CPU overhead here may not be
- // worth the memory. However, when running with +CompressedOops, this field is free, so we leave
- // it. When running with -CompressedOops, we might be able to save memory by using polymorphism
- // for isMandatory and dropping this field.
- private int hashCode = 0;
+ private final Artifact.SourceArtifact artifact;
- private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) {
- Preconditions.checkArgument(sourceArtifact.isSourceArtifact());
+ private ArtifactSkyKey(Artifact.SourceArtifact sourceArtifact) {
this.artifact = Preconditions.checkNotNull(sourceArtifact);
- this.isMandatory = mandatory;
- }
-
- /**
- * Constructs an ArtifactSkyKey for a derived artifact. The mandatory attribute is not needed
- * because a derived artifact must be a mandatory input for some action in order to ensure that it
- * is built in the first place. If it fails to build, then that fact is cached in the node, so any
- * action that has it as a non-mandatory input can retrieve that information from the node.
- */
- private ArtifactSkyKey(Artifact derivedArtifact) {
- this.artifact = Preconditions.checkNotNull(derivedArtifact);
- Preconditions.checkArgument(!derivedArtifact.isSourceArtifact(), derivedArtifact);
- this.isMandatory = true; // Unused.
}
@ThreadSafety.ThreadSafe
+ public static SkyKey key(Artifact artifact, boolean isMandatory) {
+ if (isMandatory || !artifact.isSourceArtifact()) {
+ return artifact;
+ }
+ return create(((Artifact.SourceArtifact) artifact));
+ }
+
+ @AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
- public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) {
- return INTERNER.intern(
- artifact.isSourceArtifact()
- ? new ArtifactSkyKey(artifact, isMandatory)
- : new ArtifactSkyKey(artifact));
- }
-
- @ThreadSafety.ThreadSafe
- static Iterable<SkyKey> mandatoryKeys(Iterable<Artifact> artifacts) {
- return Iterables.transform(artifacts, TO_MANDATORY_KEY);
- }
-
- public static Collection<Artifact> artifacts(Collection<? extends ArtifactSkyKey> keys) {
- return Collections2.transform(keys, TO_ARTIFACT);
+ static ArtifactSkyKey create(Artifact.SourceArtifact artifact) {
+ return INTERNER.intern(new ArtifactSkyKey(artifact));
}
public static Artifact artifact(SkyKey key) {
- return TO_ARTIFACT.apply((ArtifactSkyKey) key.argument());
+ return key instanceof Artifact ? (Artifact) key : ((ArtifactSkyKey) key).artifact;
+ }
+
+ public static boolean isMandatory(SkyKey key) {
+ return key instanceof Artifact;
}
@Override
public SkyFunctionName functionName() {
- return SkyFunctions.ARTIFACT;
+ return Artifact.ARTIFACT;
}
@Override
public int hashCode() {
- // We use the hash code caching strategy employed by java.lang.String. There are three subtle
- // things going on here:
- //
- // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
- // Yes, this means that if the hash code is really 0 then we will "recompute" it each time.
- // But this isn't a problem in practice since a hash code of 0 should be rare.
- //
- // (2) Since we have no synchronization, multiple threads can race here thinking there are the
- // first one to compute and cache the hash code.
- //
- // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
- // thread may not be visible by another. Note that we probably don't need to worry about
- // multiple inefficient reads of 'hashCode' on the same thread since it's non-volatile.
- //
- // All three of these issues are benign from a correctness perspective; in the end we have no
- // overhead from synchronization, at the cost of potentially computing the hash code more than
- // once.
- if (hashCode == 0) {
- hashCode = computeHashCode();
- }
- return hashCode;
- }
-
- private int computeHashCode() {
- int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
- return isMandatory ? initialHash : 47 * initialHash + 1;
+ return artifact.hashCode();
}
@Override
@@ -153,24 +81,13 @@
return false;
}
ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that);
- return Artifact.equalWithOwner(artifact, thatArtifactSkyKey.artifact)
- && isMandatory == thatArtifactSkyKey.isMandatory;
+ return artifact.equals(thatArtifactSkyKey.artifact);
}
Artifact getArtifact() {
return artifact;
}
- /**
- * Returns whether the artifact is a mandatory input of its requesting action. May only be called
- * for source artifacts, since a derived artifact must be a mandatory input of some action in
- * order to have been built in the first place.
- */
- public boolean isMandatory() {
- Preconditions.checkState(artifact.isSourceArtifact(), artifact);
- return isMandatory;
- }
-
@Override
public String toString() {
return artifact.prettyPrint() + " " + artifact.getArtifactOwner();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index de70aa1..3916b7b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -302,8 +302,7 @@
Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps =
env.getValuesOrThrow(
- ArtifactSkyKey.mandatoryKeys(
- completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts()),
+ completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts(),
MissingInputFileException.class,
ActionExecutionException.class);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index 0ef296c..0b41400 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -86,7 +86,6 @@
SkyFunctionName.create("BUILD_CONFIGURATION");
public static final SkyFunctionName CONFIGURATION_FRAGMENT =
SkyFunctionName.create("CONFIGURATION_FRAGMENT");
- public static final SkyFunctionName ARTIFACT = SkyFunctionName.create("ARTIFACT");
public static final SkyFunctionName ACTION_EXECUTION = ActionLookupData.NAME;
public static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL =
SkyFunctionName.create("RECURSIVE_DIRECTORY_TRAVERSAL");
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index acc97e0..bdff840 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -50,6 +50,7 @@
import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
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.OwnerlessArtifactWrapper;
import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
import com.google.devtools.build.lib.actions.CachedActionEvent;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
@@ -141,7 +142,8 @@
// We don't want to execute the action again on the second entry to the SkyFunction.
// In both cases, we store the already-computed ActionExecutionValue to avoid having to compute it
// again.
- private ConcurrentMap<Artifact, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>>
+ private ConcurrentMap<
+ OwnerlessArtifactWrapper, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>>
buildActionMap;
// Errors found when examining all actions in the graph are stored here, so that they can be
@@ -388,13 +390,16 @@
}
boolean probeActionExecution(Action action) {
- return buildActionMap.containsKey(action.getPrimaryOutput());
+ return buildActionMap.containsKey(new OwnerlessArtifactWrapper(action.getPrimaryOutput()));
}
private boolean actionReallyExecuted(Action action, ActionLookupData actionLookupData) {
Pair<ActionLookupData, ?> cachedRun =
Preconditions.checkNotNull(
- buildActionMap.get(action.getPrimaryOutput()), "%s %s", action, actionLookupData);
+ buildActionMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())),
+ "%s %s",
+ action,
+ actionLookupData);
return actionLookupData.equals(cachedRun.getFirst());
}
@@ -434,7 +439,8 @@
actionLookupData));
// Check to see if another action is already executing/has executed this value.
Pair<ActionLookupData, FutureTask<ActionExecutionValue>> oldAction =
- buildActionMap.putIfAbsent(primaryOutput, Pair.of(actionLookupData, actionTask));
+ buildActionMap.putIfAbsent(
+ new OwnerlessArtifactWrapper(primaryOutput), Pair.of(actionLookupData, actionTask));
// true if this is a non-shared action or it's shared and to be executed.
boolean isPrimaryActionForTheValue = oldAction == null;
@@ -445,7 +451,10 @@
actionTask = oldAction.second;
}
try {
- return actionTask.get();
+ ActionExecutionValue value = actionTask.get();
+ return isPrimaryActionForTheValue
+ ? value
+ : value.transformForSharedAction(action.getOutputs());
} catch (ExecutionException e) {
Throwables.propagateIfPossible(e.getCause(),
ActionExecutionException.class, InterruptedException.class);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 0de07ff..4d413a9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -503,7 +503,7 @@
map.put(SkyFunctions.TARGET_COMPLETION, CompletionFunction.targetCompletionFunction());
map.put(SkyFunctions.ASPECT_COMPLETION, CompletionFunction.aspectCompletionFunction());
map.put(SkyFunctions.TEST_COMPLETION, new TestCompletionFunction());
- map.put(SkyFunctions.ARTIFACT, new ArtifactFunction());
+ map.put(Artifact.ARTIFACT, new ArtifactFunction());
map.put(
SkyFunctions.BUILD_INFO_COLLECTION,
new BuildInfoCollectionFunction(
@@ -1325,14 +1325,13 @@
resourceManager.resetResourceUsage();
try {
progressReceiver.executionProgressReceiver = executionProgressReceiver;
- Iterable<SkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild);
Iterable<TargetCompletionValue.TargetCompletionKey> targetKeys =
TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext, targetsToTest);
Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext);
Iterable<SkyKey> testKeys =
TestCompletionValue.keys(targetsToTest, topLevelArtifactContext, exclusiveTesting);
return buildDriver.evaluate(
- Iterables.concat(artifactKeys, targetKeys, aspectKeys, testKeys),
+ Iterables.concat(artifactsToBuild, targetKeys, aspectKeys, testKeys),
keepGoing,
numJobs,
reporter);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
index 8e4d7d5..4273e32 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
@@ -142,7 +142,8 @@
}
@Override
- public Map<SkyKey, SkyValue> getValues(Iterable<SkyKey> depKeys) throws InterruptedException {
+ public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys)
+ throws InterruptedException {
preFetch.inform();
try {
return delegate.getValues(depKeys);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
index 4a9946a..cbbe9f9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
@@ -52,7 +52,7 @@
}
}
} else {
- env.getValues(ArtifactSkyKey.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct)));
+ env.getValues(TestProvider.getTestStatusArtifacts(ct));
if (env.valuesMissing()) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java
index e7bda39..712dc47 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractSkyFunctionEnvironment.java
@@ -131,7 +131,8 @@
}
@Override
- public Map<SkyKey, SkyValue> getValues(Iterable<SkyKey> depKeys) throws InterruptedException {
+ public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys)
+ throws InterruptedException {
Map<SkyKey, ValueOrUntypedException> valuesOrExceptions = getValueOrUntypedExceptions(depKeys);
checkValuesAreMissing5(valuesOrExceptions, null, null, null, null, null);
return Collections.unmodifiableMap(
diff --git a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java
index 115fea1..5dc85c0 100644
--- a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java
@@ -160,7 +160,8 @@
}
@Override
- public Map<SkyKey, SkyValue> getValues(Iterable<SkyKey> depKeys) throws InterruptedException {
+ public Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys)
+ throws InterruptedException {
recordDeps(depKeys);
return delegate.getValues(depKeys);
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
index 1ae2916..4201988 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
@@ -205,7 +205,7 @@
* is {@code true}, and, {@code m.get(k) != null} iff the dependency was already evaluated and
* was not in error.
*/
- Map<SkyKey, SkyValue> getValues(Iterable<SkyKey> depKeys) throws InterruptedException;
+ Map<SkyKey, SkyValue> getValues(Iterable<? extends SkyKey> depKeys) throws InterruptedException;
/**
* Similar to {@link #getValues} but allows the caller to specify a set of types that are proper
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
index abbd347..e4bd5b9 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
@@ -411,6 +412,40 @@
assertThat(new Artifact(scratch.file("/newRoot/foo"), root).getRoot()).isEqualTo(root);
}
+ @Test
+ public void hashCodeAndEquals() throws IOException {
+ Path execRoot = scratch.getFileSystem().getPath("/");
+ ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, scratch.dir("/newRoot"));
+ ArtifactOwner firstOwner = () -> Label.parseAbsoluteUnchecked("//bar:bar");
+ ArtifactOwner secondOwner = () -> Label.parseAbsoluteUnchecked("//foo:foo");
+ Artifact derived1 = new Artifact(root, PathFragment.create("newRoot/shared"), firstOwner);
+ Artifact derived2 = new Artifact(root, PathFragment.create("newRoot/shared"), secondOwner);
+ ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(root.getRoot().asPath()));
+ Artifact source1 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), firstOwner);
+ Artifact source2 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), secondOwner);
+ new EqualsTester()
+ .addEqualityGroup(derived1)
+ .addEqualityGroup(derived2)
+ .addEqualityGroup(source1, source2)
+ .testEquals();
+ assertThat(derived1.hashCode()).isEqualTo(derived2.hashCode());
+ assertThat(derived1.hashCode()).isNotEqualTo(source1.hashCode());
+ assertThat(source1.hashCode()).isEqualTo(source2.hashCode());
+ Artifact.OwnerlessArtifactWrapper wrapper1 = new Artifact.OwnerlessArtifactWrapper(derived1);
+ Artifact.OwnerlessArtifactWrapper wrapper2 = new Artifact.OwnerlessArtifactWrapper(derived2);
+ Artifact.OwnerlessArtifactWrapper wrapper3 = new Artifact.OwnerlessArtifactWrapper(source1);
+ Artifact.OwnerlessArtifactWrapper wrapper4 = new Artifact.OwnerlessArtifactWrapper(source2);
+ new EqualsTester()
+ .addEqualityGroup(wrapper1, wrapper2)
+ .addEqualityGroup(wrapper3, wrapper4)
+ .testEquals();
+ Path path1 = derived1.getPath();
+ Path path2 = derived2.getPath();
+ Path path3 = source1.getPath();
+ Path path4 = source2.getPath();
+ new EqualsTester().addEqualityGroup(path1, path2, path3, path4).testEquals();
+ }
+
private Artifact createDirNameArtifact() throws Exception {
return new Artifact(
scratch.file("/aaa/bbb/ccc/ddd"),
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 ec81788..4678cd7 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
@@ -1207,19 +1207,6 @@
/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
- * BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So
- * to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just
- * be "foo.o".
- */
- protected Artifact getBinArtifact(String packageRelativePath, String owner) {
- return getPackageRelativeDerivedArtifact(
- packageRelativePath,
- getConfiguration(owner).getBinDirectory(RepositoryName.MAIN),
- makeConfiguredTargetKey(owner));
- }
-
- /**
- * Gets a derived Artifact for testing in the subdirectory of the {@link
* BuildConfiguration#getBinDirectory} corresponding to the package of {@code owner}. So to
* specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just be
* "foo.o".
@@ -1335,13 +1322,18 @@
packageRelativePath,
getConfiguration(owner)
.getGenfilesDirectory(owner.getLabel().getPackageIdentifier().getRepository()),
- (AspectValue.AspectKey)
- AspectValue.createAspectKey(
- owner.getLabel(),
- getConfiguration(owner),
- new AspectDescriptor(creatingAspectFactory, params),
- getConfiguration(owner))
- .argument());
+ getOwnerForAspect(owner, creatingAspectFactory, params));
+ }
+
+ protected AspectValue.AspectKey getOwnerForAspect(
+ ConfiguredTarget owner, NativeAspectClass creatingAspectFactory, AspectParameters params) {
+ return (AspectValue.AspectKey)
+ AspectValue.createAspectKey(
+ owner.getLabel(),
+ getConfiguration(owner),
+ new AspectDescriptor(creatingAspectFactory, params),
+ getConfiguration(owner))
+ .argument();
}
/**
@@ -1368,11 +1360,11 @@
/**
* Gets a derived Artifact for testing in the subdirectory of the {@link
- * BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}.
- * So to specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should
- * just be "foo.h".
+ * BuildConfiguration#getIncludeDirectory} corresponding to the package of {@code owner}. So to
+ * specify a file foo/foo.o owned by target //foo:foo, {@code packageRelativePath} should just be
+ * "foo.h".
*/
- private Artifact getIncludeArtifact(String packageRelativePath, ArtifactOwner owner) {
+ protected Artifact getIncludeArtifact(String packageRelativePath, ArtifactOwner owner) {
return getPackageRelativeDerivedArtifact(packageRelativePath,
targetConfig.getIncludeDirectory(owner.getLabel().getPackageIdentifier().getRepository()),
owner);
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
index 7ced520..e0c7bc2 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
@@ -411,7 +411,7 @@
" output_to_bindir=0)");
assertThat(getFileConfiguredTarget("//x:bin.out").getArtifact())
- .isEqualTo(getBinArtifact("bin.out", "//x:bin"));
+ .isEqualTo(getBinArtifact("bin.out", getConfiguredTarget("//x:bin")));
assertThat(getFileConfiguredTarget("//x:genfiles.out").getArtifact())
.isEqualTo(getGenfilesArtifact("genfiles.out", "//x:genfiles"));
}
@@ -429,14 +429,16 @@
" cmd=':',",
" output_to_bindir=0)");
+ ConfiguredTarget binCt = getConfiguredTarget("//x:bin");
+ ConfiguredTarget genCt = getConfiguredTarget("//x:genfiles");
assertThat(getFileConfiguredTarget("//x:bin_a.out").getArtifact())
- .isEqualTo(getBinArtifact("bin_a.out", "//x:bin"));
+ .isEqualTo(getBinArtifact("bin_a.out", binCt));
assertThat(getFileConfiguredTarget("//x:bin_b.out").getArtifact())
- .isEqualTo(getBinArtifact("bin_b.out", "//x:bin"));
+ .isEqualTo(getBinArtifact("bin_b.out", binCt));
assertThat(getFileConfiguredTarget("//x:genfiles_a.out").getArtifact())
- .isEqualTo(getGenfilesArtifact("genfiles_a.out", "//x:genfiles"));
+ .isEqualTo(getGenfilesArtifact("genfiles_a.out", genCt));
assertThat(getFileConfiguredTarget("//x:genfiles_b.out").getArtifact())
- .isEqualTo(getGenfilesArtifact("genfiles_b.out", "//x:genfiles"));
+ .isEqualTo(getGenfilesArtifact("genfiles_b.out", genCt));
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java
index 87ecd9e..4f44f85 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java
@@ -1866,6 +1866,8 @@
" manifest = 'AndroidManifest.xml',",
" resource_files = ['res/values/strings.xml'])");
ConfiguredTarget target = getConfiguredTarget("//java/android:test");
+ ConfiguredTarget t1Target = getConfiguredTarget("//java/android:t1");
+ ConfiguredTarget t2Target = getConfiguredTarget("//java/android:t2");
final AndroidLibraryAarInfo provider = target.get(AndroidLibraryAarInfo.PROVIDER);
final Aar test =
@@ -1874,12 +1876,12 @@
getBinArtifact("test_processed_manifest/AndroidManifest.xml", target));
final Aar t1 =
Aar.create(
- getBinArtifact("t1.aar", target),
- getBinArtifact("t1_processed_manifest/AndroidManifest.xml", target));
+ getBinArtifact("t1.aar", t1Target),
+ getBinArtifact("t1_processed_manifest/AndroidManifest.xml", t1Target));
final Aar t2 =
Aar.create(
- getBinArtifact("t2.aar", target),
- getBinArtifact("t2_processed_manifest/AndroidManifest.xml", target));
+ getBinArtifact("t2.aar", t2Target),
+ getBinArtifact("t2_processed_manifest/AndroidManifest.xml", t2Target));
assertThat(provider.getAar()).isEqualTo(test);
assertThat(provider.getTransitiveAars()).containsExactly(test, t1, t2);
@@ -1895,11 +1897,12 @@
" resource_files = ['res/values/strings.xml'])");
ConfiguredTarget target = getConfiguredTarget("//java/android:test");
final AndroidLibraryAarInfo provider = target.get(AndroidLibraryAarInfo.PROVIDER);
+ ConfiguredTarget transitiveTarget = getConfiguredTarget("//java/android:transitive");
final Aar transitive =
Aar.create(
- getBinArtifact("transitive.aar", target),
- getBinArtifact("transitive_processed_manifest/AndroidManifest.xml", target));
+ getBinArtifact("transitive.aar", transitiveTarget),
+ getBinArtifact("transitive_processed_manifest/AndroidManifest.xml", transitiveTarget));
assertThat(provider.getAar()).isNull();
assertThat(provider.getTransitiveAars()).containsExactly(transitive);
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 0f6b4df..a1810f0 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
@@ -469,7 +469,8 @@
useConfiguration();
ConfiguredTarget hello = getConfiguredTarget("//hello:hello");
- Artifact helloObj = getBinArtifact("_objs/hello/hello.obj", "//hello:hello");
+ Artifact helloObj =
+ getBinArtifact("_objs/hello/hello.obj", getConfiguredTarget("//hello:hello"));
CppCompileAction helloObjAction = (CppCompileAction) getGeneratingAction(helloObj);
assertThat(helloObjAction).isNotNull();
@@ -704,23 +705,21 @@
" srcs = ['b.h'],",
" textual_hdrs = ['t.h'],",
")");
- getConfiguredTarget("//module:b");
- Artifact bModuleArtifact = getBinArtifact("_objs/b/b.pic.pcm", "//module:b");
+ ConfiguredTarget moduleB = getConfiguredTarget("//module:b");
+ Artifact bModuleArtifact = getBinArtifact("_objs/b/b.pic.pcm", moduleB);
CppCompileAction bModuleAction = (CppCompileAction) getGeneratingAction(bModuleArtifact);
assertThat(bModuleAction.getIncludeScannerSources()).containsExactly(
getSourceArtifact("module/b.h"), getSourceArtifact("module/t.h"));
- assertThat(bModuleAction.getInputs()).contains(
- getGenfilesArtifactWithNoOwner("module/b.cppmap"));
+ assertThat(bModuleAction.getInputs()).contains(getGenfilesArtifact("b.cppmap", moduleB));
- getConfiguredTarget("//module:a");
- Artifact aObjectArtifact = getBinArtifact("_objs/a/a.pic.o", "//module:a");
+ ConfiguredTarget moduleA = getConfiguredTarget("//module:a");
+ Artifact aObjectArtifact = getBinArtifact("_objs/a/a.pic.o", moduleA);
CppCompileAction aObjectAction = (CppCompileAction) getGeneratingAction(aObjectArtifact);
assertThat(aObjectAction.getIncludeScannerSources()).containsExactly(
getSourceArtifact("module/a.cc"));
assertThat(aObjectAction.getCcCompilationContext().getTransitiveModules(true))
- .contains(getBinArtifact("_objs/b/b.pic.pcm", "//module:b"));
- assertThat(aObjectAction.getInputs()).contains(
- getGenfilesArtifactWithNoOwner("module/b.cppmap"));
+ .contains(getBinArtifact("_objs/b/b.pic.pcm", moduleB));
+ assertThat(aObjectAction.getInputs()).contains(getGenfilesArtifact("b.cppmap", moduleB));
assertNoEvents();
}
@@ -741,11 +740,11 @@
setupPackagesForSourcesWithSameBaseNameTests();
getConfiguredTarget("//foo:lib");
- Artifact a0 = getBinArtifact("_objs/lib/0/a.pic.o", "//foo:lib");
- Artifact a1 = getBinArtifact("_objs/lib/1/a.pic.o", "//foo:lib");
- Artifact a2 = getBinArtifact("_objs/lib/2/a.pic.o", "//foo:lib");
- Artifact a3 = getBinArtifact("_objs/lib/3/A.pic.o", "//foo:lib");
- Artifact b = getBinArtifact("_objs/lib/b.pic.o", "//foo:lib");
+ Artifact a0 = getBinArtifact("_objs/lib/0/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a1 = getBinArtifact("_objs/lib/1/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a2 = getBinArtifact("_objs/lib/2/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a3 = getBinArtifact("_objs/lib/3/A.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact b = getBinArtifact("_objs/lib/b.pic.o", getConfiguredTarget("//foo:lib"));
assertThat(getGeneratingAction(a0)).isNotNull();
assertThat(getGeneratingAction(a1)).isNotNull();
@@ -767,11 +766,11 @@
setupPackagesForSourcesWithSameBaseNameTests();
getConfiguredTarget("//foo:lib");
- Artifact a0 = getBinArtifact("_objs/lib/foo/a.pic.o", "//foo:lib");
- Artifact a1 = getBinArtifact("_objs/lib/foo/subpkg1/a.pic.o", "//foo:lib");
- Artifact a2 = getBinArtifact("_objs/lib/bar/a.pic.o", "//foo:lib");
- Artifact a3 = getBinArtifact("_objs/lib/foo/subpkg2/A.pic.o", "//foo:lib");
- Artifact b = getBinArtifact("_objs/lib/foo/subpkg1/b.pic.o", "//foo:lib");
+ Artifact a0 = getBinArtifact("_objs/lib/foo/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a1 = getBinArtifact("_objs/lib/foo/subpkg1/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a2 = getBinArtifact("_objs/lib/bar/a.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact a3 = getBinArtifact("_objs/lib/foo/subpkg2/A.pic.o", getConfiguredTarget("//foo:lib"));
+ Artifact b = getBinArtifact("_objs/lib/foo/subpkg1/b.pic.o", getConfiguredTarget("//foo:lib"));
assertThat(getGeneratingAction(a0)).isNotNull();
assertThat(getGeneratingAction(a1)).isNotNull();
@@ -862,14 +861,15 @@
setupPackagesForModuleTests(/*useHeaderModules=*/false);
// The //nomodule:f target only depends on non-module targets, thus it should be module-free.
- getConfiguredTarget("//nomodule:f");
- assertThat(getGeneratingAction(getBinArtifact("_objs/f/f.pic.pcm", "//nomodule:f"))).isNull();
- Artifact fObjectArtifact = getBinArtifact("_objs/f/f.pic.o", "//nomodule:f");
+ ConfiguredTarget nomoduleF = getConfiguredTarget("//nomodule:f");
+ ConfiguredTarget nomoduleE = getConfiguredTarget("//nomodule:e");
+ assertThat(getGeneratingAction(getBinArtifact("_objs/f/f.pic.pcm", nomoduleF))).isNull();
+ Artifact fObjectArtifact = getBinArtifact("_objs/f/f.pic.o", nomoduleF);
CppCompileAction fObjectAction = (CppCompileAction) getGeneratingAction(fObjectArtifact);
// Only the module map of f itself itself and the direct dependencies are needed.
- assertThat(getNonSystemModuleMaps(fObjectAction.getInputs())).containsExactly(
- getGenfilesArtifact("f.cppmap", "//nomodule:f"),
- getGenfilesArtifact("e.cppmap", "//nomodule:e"));
+ assertThat(getNonSystemModuleMaps(fObjectAction.getInputs()))
+ .containsExactly(
+ getGenfilesArtifact("f.cppmap", nomoduleF), getGenfilesArtifact("e.cppmap", nomoduleE));
assertThat(getHeaderModules(fObjectAction.getInputs())).isEmpty();
assertThat(fObjectAction.getIncludeScannerSources()).containsExactly(
getSourceArtifact("nomodule/f.cc"));
@@ -877,13 +877,14 @@
// The //nomodule:c target will get the header module for //module:b, which is a direct
// dependency.
- getConfiguredTarget("//nomodule:c");
- assertThat(getGeneratingAction(getBinArtifact("_objs/c/c.pic.pcm", "//nomodule:c"))).isNull();
- Artifact cObjectArtifact = getBinArtifact("_objs/c/c.pic.o", "//nomodule:c");
+ ConfiguredTarget nomoduleC = getConfiguredTarget("//nomodule:c");
+ assertThat(getGeneratingAction(getBinArtifact("_objs/c/c.pic.pcm", nomoduleC))).isNull();
+ Artifact cObjectArtifact = getBinArtifact("_objs/c/c.pic.o", nomoduleC);
CppCompileAction cObjectAction = (CppCompileAction) getGeneratingAction(cObjectArtifact);
- assertThat(getNonSystemModuleMaps(cObjectAction.getInputs())).containsExactly(
- getGenfilesArtifact("b.cppmap", "//module:b"),
- getGenfilesArtifact("c.cppmap", "//nomodule:e"));
+ assertThat(getNonSystemModuleMaps(cObjectAction.getInputs()))
+ .containsExactly(
+ getGenfilesArtifact("b.cppmap", "//module:b"),
+ getGenfilesArtifact("c.cppmap", nomoduleC));
assertThat(getHeaderModules(cObjectAction.getInputs())).isEmpty();
// All headers of transitive dependencies that are built as modules are needed as entry points
// for include scanning.
@@ -895,8 +896,12 @@
// The //nomodule:d target depends on //module:b via one indirection (//nomodule:c).
getConfiguredTarget("//nomodule:d");
- assertThat(getGeneratingAction(getBinArtifact("_objs/d/d.pic.pcm", "//nomodule:d"))).isNull();
- Artifact dObjectArtifact = getBinArtifact("_objs/d/d.pic.o", "//nomodule:d");
+ assertThat(
+ getGeneratingAction(
+ getBinArtifact("_objs/d/d.pic.pcm", getConfiguredTarget("//nomodule:d"))))
+ .isNull();
+ Artifact dObjectArtifact =
+ getBinArtifact("_objs/d/d.pic.o", getConfiguredTarget("//nomodule:d"));
CppCompileAction dObjectAction = (CppCompileAction) getGeneratingAction(dObjectArtifact);
// Module map 'c.cppmap' is needed because it is a direct dependency.
assertThat(getNonSystemModuleMaps(dObjectAction.getInputs())).containsExactly(
@@ -909,21 +914,21 @@
// The //module:j target depends on //module:g via //nomodule:h and on //module:b via
// both //module:g and //nomodule:c.
- getConfiguredTarget("//module:j");
- Artifact jObjectArtifact = getBinArtifact("_objs/j/j.pic.o", "//module:j");
+ ConfiguredTarget moduleJ = getConfiguredTarget("//module:j");
+ Artifact jObjectArtifact = getBinArtifact("_objs/j/j.pic.o", moduleJ);
CppCompileAction jObjectAction = (CppCompileAction) getGeneratingAction(jObjectArtifact);
assertThat(getHeaderModules(jObjectAction.getCcCompilationContext().getTransitiveModules(true)))
.containsExactly(
- getBinArtifact("_objs/b/b.pic.pcm", "//module:b"),
- getBinArtifact("_objs/g/g.pic.pcm", "//module:g"));
+ getBinArtifact("_objs/b/b.pic.pcm", getConfiguredTarget("//module:b")),
+ getBinArtifact("_objs/g/g.pic.pcm", getConfiguredTarget("//module:g")));
assertThat(jObjectAction.getIncludeScannerSources()).containsExactly(
getSourceArtifact("module/j.cc"));
assertThat(jObjectAction.getMainIncludeScannerSource()).isEqualTo(
getSourceArtifact("module/j.cc"));
assertThat(getHeaderModules(jObjectAction.getCcCompilationContext().getTransitiveModules(true)))
.containsExactly(
- getBinArtifact("_objs/b/b.pic.pcm", "//module:b"),
- getBinArtifact("_objs/g/g.pic.pcm", "//module:g"));
+ getBinArtifact("_objs/b/b.pic.pcm", getConfiguredTarget("//module:b")),
+ getBinArtifact("_objs/g/g.pic.pcm", getConfiguredTarget("//module:g")));
}
@Test
@@ -934,34 +939,37 @@
useConfiguration("--cpu=k8");
setupPackagesForModuleTests( /*useHeaderModules=*/true);
- getConfiguredTarget("//nomodule:f");
- Artifact fObjectArtifact = getBinArtifact("_objs/f/f.pic.o", "//nomodule:f");
+ ConfiguredTarget nomoduleF = getConfiguredTarget("//nomodule:f");
+ Artifact fObjectArtifact =
+ getBinArtifact("_objs/f/f.pic.o", getConfiguredTarget("//nomodule:f"));
CppCompileAction fObjectAction = (CppCompileAction) getGeneratingAction(fObjectArtifact);
// Only the module map of f itself itself and the direct dependencies are needed.
assertThat(getNonSystemModuleMaps(fObjectAction.getInputs()))
.containsExactly(
- getGenfilesArtifact("f.cppmap", "//nomodule:f"),
+ getGenfilesArtifact("f.cppmap", nomoduleF),
getGenfilesArtifact("e.cppmap", "//nomodule:e"));
getConfiguredTarget("//nomodule:c");
- Artifact cObjectArtifact = getBinArtifact("_objs/c/c.pic.o", "//nomodule:c");
+ Artifact cObjectArtifact =
+ getBinArtifact("_objs/c/c.pic.o", getConfiguredTarget("//nomodule:c"));
CppCompileAction cObjectAction = (CppCompileAction) getGeneratingAction(cObjectArtifact);
assertThat(getNonSystemModuleMaps(cObjectAction.getInputs()))
.containsExactly(
getGenfilesArtifact("b.cppmap", "//module:b"),
- getGenfilesArtifact("c.cppmap", "//nomodule:e"));
+ getGenfilesArtifact("c.cppmap", "//nomodule:c"));
assertThat(getHeaderModules(cObjectAction.getCcCompilationContext().getTransitiveModules(true)))
- .containsExactly(getBinArtifact("_objs/b/b.pic.pcm", "//module:b"));
+ .containsExactly(getBinArtifact("_objs/b/b.pic.pcm", getConfiguredTarget("//module:b")));
getConfiguredTarget("//nomodule:d");
- Artifact dObjectArtifact = getBinArtifact("_objs/d/d.pic.o", "//nomodule:d");
+ Artifact dObjectArtifact =
+ getBinArtifact("_objs/d/d.pic.o", getConfiguredTarget("//nomodule:d"));
CppCompileAction dObjectAction = (CppCompileAction) getGeneratingAction(dObjectArtifact);
assertThat(getNonSystemModuleMaps(dObjectAction.getInputs()))
.containsExactly(
getGenfilesArtifact("c.cppmap", "//nomodule:c"),
getGenfilesArtifact("d.cppmap", "//nomodule:d"));
assertThat(getHeaderModules(dObjectAction.getCcCompilationContext().getTransitiveModules(true)))
- .containsExactly(getBinArtifact("_objs/b/b.pic.pcm", "//module:b"));
+ .containsExactly(getBinArtifact("_objs/b/b.pic.pcm", getConfiguredTarget("//module:b")));
}
private void writeSimpleCcLibrary() throws Exception {
@@ -1236,7 +1244,7 @@
useConfiguration(flags);
scratch.overwriteFile("mode/BUILD", "cc_library(name = 'a', srcs = ['a.cc'])");
getConfiguredTarget("//mode:a");
- Artifact objectArtifact = getBinArtifact("_objs/a/a.pic.o", "//mode:a");
+ Artifact objectArtifact = getBinArtifact("_objs/a/a.pic.o", getConfiguredTarget("//mode:a"));
CppCompileAction action = (CppCompileAction) getGeneratingAction(objectArtifact);
return action.getCompilerOptions();
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java
index d7bf3ea..2342b6a 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java
@@ -24,9 +24,12 @@
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.bazel.rules.cpp.proto.BazelCcProtoAspect;
+import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.rules.cpp.CcCompilationContext;
import com.google.devtools.build.lib.rules.cpp.CcCompilationInfo;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.PathFragment;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -177,7 +180,14 @@
"--cpp_out=%s/external/bla",
getTargetConfiguration().getGenfilesFragment().toString()));
- Artifact headerFile = getGenfilesArtifactWithNoOwner("external/bla/foo/bar.pb.h");
+ Artifact headerFile =
+ getDerivedArtifact(
+ PathFragment.create("external/bla/foo/bar.pb.h"),
+ targetConfig.getGenfilesDirectory(),
+ getOwnerForAspect(
+ getConfiguredTarget("@bla//foo:bar_proto"),
+ ruleClassProvider.getNativeAspectClass(BazelCcProtoAspect.class.getSimpleName()),
+ AspectParameters.EMPTY));
CcCompilationContext ccCompilationContext =
target.get(CcCompilationInfo.PROVIDER).getCcCompilationContext();
assertThat(ccCompilationContext.getDeclaredIncludeSrcs()).containsExactly(headerFile);
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java
index 310129c..8983cca 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java
@@ -39,6 +39,7 @@
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType;
import com.google.devtools.build.lib.rules.apple.AppleToolchain;
@@ -62,6 +63,9 @@
*/
@RunWith(JUnit4.class)
public class BazelJ2ObjcLibraryTest extends J2ObjcLibraryTest {
+ protected NativeAspectClass getJ2ObjcAspect() {
+ return ruleClassProvider.getNativeAspectClass(J2ObjcAspect.NAME);
+ }
/**
* Gets the target with the given label, using the apple_binary multi-arch split transition with
@@ -256,11 +260,13 @@
ConfiguredTarget target = getJ2ObjCAspectConfiguredTarget(
"//java/com/google/dummy/test/proto:test");
- ConfiguredTarget proto =
- getConfiguredTarget(
- "//java/com/google/dummy/test/proto:test", getAppleCrosstoolConfiguration());
J2ObjcMappingFileProvider provider = target.getProvider(J2ObjcMappingFileProvider.class);
- Artifact classMappingFile = getGenfilesArtifact("test.clsmap.properties", proto);
+ Artifact classMappingFile =
+ getGenfilesArtifact(
+ "test.clsmap.properties",
+ getConfiguredTarget(
+ "//java/com/google/dummy/test/proto:test_proto", getAppleCrosstoolConfiguration()),
+ getJ2ObjcAspect());
assertThat(provider.getClassMappingFiles()).containsExactly(classMappingFile);
}
@@ -286,12 +292,13 @@
ConfiguredTarget target = getJ2ObjCAspectConfiguredTarget("//x:test");
ConfiguredTarget test = getConfiguredTarget("//x:test_proto", getAppleCrosstoolConfiguration());
J2ObjcMappingFileProvider provider = target.getProvider(J2ObjcMappingFileProvider.class);
- Artifact classMappingFile = getGenfilesArtifact("test.clsmap.properties", test);
+ Artifact classMappingFile =
+ getGenfilesArtifact("test.clsmap.properties", test, getJ2ObjcAspect());
assertThat(provider.getClassMappingFiles()).containsExactly(classMappingFile);
ObjcProvider objcProvider = target.get(ObjcProvider.SKYLARK_CONSTRUCTOR);
- Artifact headerFile = getGenfilesArtifact("test.j2objc.pb.h", test);
- Artifact sourceFile = getGenfilesArtifact("test.j2objc.pb.m", test);
+ Artifact headerFile = getGenfilesArtifact("test.j2objc.pb.h", test, getJ2ObjcAspect());
+ Artifact sourceFile = getGenfilesArtifact("test.j2objc.pb.m", test, getJ2ObjcAspect());
assertThat(objcProvider.get(ObjcProvider.HEADER)).contains(headerFile);
assertThat(objcProvider.get(ObjcProvider.SOURCE)).contains(sourceFile);
}
@@ -333,13 +340,15 @@
J2ObjcMappingFileProvider provider = target.getProvider(J2ObjcMappingFileProvider.class);
Artifact classMappingFile =
- getGenfilesArtifact("../external/bla/foo/test.clsmap.properties", test);
+ getGenfilesArtifact("../external/bla/foo/test.clsmap.properties", test, getJ2ObjcAspect());
assertThat(provider.getClassMappingFiles()).containsExactly(classMappingFile);
ObjcProvider objcProvider = target.get(ObjcProvider.SKYLARK_CONSTRUCTOR);
- Artifact headerFile = getGenfilesArtifact("../external/bla/foo/test.j2objc.pb.h", test);
- Artifact sourceFile = getGenfilesArtifact("../external/bla/foo/test.j2objc.pb.m", test);
+ Artifact headerFile =
+ getGenfilesArtifact("../external/bla/foo/test.j2objc.pb.h", test, getJ2ObjcAspect());
+ Artifact sourceFile =
+ getGenfilesArtifact("../external/bla/foo/test.j2objc.pb.m", test, getJ2ObjcAspect());
assertThat(objcProvider.get(ObjcProvider.HEADER)).contains(headerFile);
assertThat(objcProvider.get(ObjcProvider.SOURCE)).contains(sourceFile);
assertThat(objcProvider.get(ObjcProvider.INCLUDE))
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 7515365..f6a40e5 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
@@ -131,10 +131,10 @@
getConfiguredTarget("//foo:lib");
- Artifact a0 = getBinArtifact("_objs/lib/arc/0/a.o", "//foo:lib");
- Artifact a1 = getBinArtifact("_objs/lib/arc/1/a.o", "//foo:lib");
- Artifact a2 = getBinArtifact("_objs/lib/non_arc/a.o", "//foo:lib");
- Artifact b = getBinArtifact("_objs/lib/arc/b.o", "//foo:lib");
+ Artifact a0 = getBinArtifact("_objs/lib/arc/0/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact a1 = getBinArtifact("_objs/lib/arc/1/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact a2 = getBinArtifact("_objs/lib/non_arc/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact b = getBinArtifact("_objs/lib/arc/b.o", getConfiguredTarget("//foo:lib"));
assertThat(getGeneratingAction(a0)).isNotNull();
assertThat(getGeneratingAction(a1)).isNotNull();
@@ -157,10 +157,10 @@
getConfiguredTarget("//foo:lib");
- Artifact a0 = getBinArtifact("_objs/lib/foo/a.o", "//foo:lib");
- Artifact a1 = getBinArtifact("_objs/lib/foo/pkg1/a.o", "//foo:lib");
- Artifact a2 = getBinArtifact("_objs/lib/foo/pkg2/a.o", "//foo:lib");
- Artifact b = getBinArtifact("_objs/lib/foo/b.o", "//foo:lib");
+ Artifact a0 = getBinArtifact("_objs/lib/foo/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact a1 = getBinArtifact("_objs/lib/foo/pkg1/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact a2 = getBinArtifact("_objs/lib/foo/pkg2/a.o", getConfiguredTarget("//foo:lib"));
+ Artifact b = getBinArtifact("_objs/lib/foo/b.o", getConfiguredTarget("//foo:lib"));
assertThat(getGeneratingAction(a0)).isNotNull();
assertThat(getGeneratingAction(a1)).isNotNull();
@@ -738,7 +738,8 @@
"tools/osx/crosstool/iossim/libtool",
"-static",
"-filelist",
- getBinArtifact("lib-archive.objlist", "//objc:lib").getExecPathString(),
+ getBinArtifact("lib-archive.objlist", getConfiguredTarget("//objc:lib"))
+ .getExecPathString(),
"-arch_only",
"i386",
"-syslibroot",
@@ -768,7 +769,8 @@
"tools/osx/crosstool/ios/libtool",
"-static",
"-filelist",
- getBinArtifact("lib-archive.objlist", "//objc:lib").getExecPathString(),
+ getBinArtifact("lib-archive.objlist", getConfiguredTarget("//objc:lib"))
+ .getExecPathString(),
"-arch_only",
"armv7",
"-syslibroot",
@@ -809,8 +811,9 @@
AppleToolchain.sdkDir(),
"-o",
Iterables.getOnlyElement(linkAction.getOutputs()).getExecPathString(),
- getBinArtifact("liblib.a", "//objc2:lib").getExecPathString(),
- getBinArtifact("liblib_dep.a", "//objc:lib_dep").getExecPathString()));
+ getBinArtifact("liblib.a", getConfiguredTarget("//objc2:lib")).getExecPathString(),
+ getBinArtifact("liblib_dep.a", getConfiguredTarget("//objc:lib_dep"))
+ .getExecPathString()));
// TODO(hlopko): make containsExactly once crosstools are updated so
// link_dynamic_library.sh is not needed anymore
assertThat(baseArtifactNames(linkAction.getInputs())).containsAllOf(
@@ -847,8 +850,9 @@
AppleToolchain.sdkDir(),
"-o",
Iterables.getOnlyElement(linkAction.getOutputs()).getExecPathString(),
- getBinArtifact("liblib.a", "//objc2:lib").getExecPathString(),
- getBinArtifact("liblib_dep.a", "//objc:lib_dep").getExecPathString()));
+ getBinArtifact("liblib.a", getConfiguredTarget("//objc2:lib")).getExecPathString(),
+ getBinArtifact("liblib_dep.a", getConfiguredTarget("//objc:lib_dep"))
+ .getExecPathString()));
// TODO(hlopko): make containsExactly once crosstools are updated so
// link_dynamic_library.sh is not needed anymore
assertThat(baseArtifactNames(linkAction.getInputs())).containsAllOf(
@@ -1474,7 +1478,8 @@
.add("-static")
.add("-filelist")
.add(
- getBinArtifact("lib-archive.objlist", "//depender_lib:lib").getExecPathString())
+ getBinArtifact("lib-archive.objlist", getConfiguredTarget("//depender_lib:lib"))
+ .getExecPathString())
.add("-arch_only", "x86_64")
.add("-syslibroot")
.add(AppleToolchain.sdkDir())
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
index 51f61ff..68e3b0a 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
@@ -934,7 +934,8 @@
Artifact actoolZipOut = getBinArtifact("x" + artifactName(".actool.zip"),
getConfiguredTarget("//x:x"));
Artifact actoolPartialInfoplist =
- getBinArtifact("x" + artifactName(".actool-PartialInfo.plist"), "//x:x");
+ getBinArtifact(
+ "x" + artifactName(".actool-PartialInfo.plist"), getConfiguredTarget("//x:x"));
SpawnAction actoolZipAction = (SpawnAction) getGeneratingAction(actoolZipOut);
assertThat(actoolZipAction.getArguments())
.containsExactly(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
index ef30031..4da1ada 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -89,7 +89,7 @@
MemoizingEvaluator evaluator =
new InMemoryMemoizingEvaluator(
ImmutableMap.<SkyFunctionName, SkyFunction>builder()
- .put(SkyFunctions.ARTIFACT, new DummyArtifactFunction(artifactValueMap))
+ .put(Artifact.ARTIFACT, new DummyArtifactFunction(artifactValueMap))
.put(
SkyFunctions.ACTION_TEMPLATE_EXPANSION,
new ActionTemplateExpansionFunction(
@@ -264,9 +264,7 @@
}
@Override
public SkyValue compute(SkyKey skyKey, Environment env) {
- ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
- Artifact artifact = artifactSkyKey.getArtifact();
- return Preconditions.checkNotNull(artifactValueMap.get(artifact));
+ return Preconditions.checkNotNull(artifactValueMap.get(skyKey));
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 2b137ed..8a22813 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -31,6 +31,7 @@
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -150,10 +151,7 @@
actions.add(action);
file(input2.getPath(), "contents");
file(input1.getPath(), "source contents");
- evaluate(
- Iterables.toArray(
- ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2, tree)),
- SkyKey.class));
+ evaluate(Iterables.toArray(ImmutableSet.of(input2, input1, input2, tree), SkyKey.class));
SkyValue value = evaluateArtifactValue(output);
assertThat(((AggregatingArtifactValue) value).getInputs())
.containsExactly(
@@ -190,12 +188,14 @@
@Test
public void testActionTreeArtifactOutput() throws Throwable {
SpecialArtifact artifact = createDerivedTreeArtifactWithAction("treeArtifact");
- TreeFileArtifact treeFileArtifact1 = createFakeTreeFileArtifact(artifact, "child1", "hello1");
- TreeFileArtifact treeFileArtifact2 = createFakeTreeFileArtifact(artifact, "child2", "hello2");
+ TreeFileArtifact treeFileArtifact1 =
+ createFakeTreeFileArtifact(artifact, ALL_OWNER, "child1", "hello1");
+ TreeFileArtifact treeFileArtifact2 =
+ createFakeTreeFileArtifact(artifact, ALL_OWNER, "child2", "hello2");
TreeArtifactValue value = (TreeArtifactValue) evaluateArtifactValue(artifact);
- assertThat(value.getChildValues().get(treeFileArtifact1)).isNotNull();
- assertThat(value.getChildValues().get(treeFileArtifact2)).isNotNull();
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact1);
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact2);
assertThat(value.getChildValues().get(treeFileArtifact1).getDigest()).isNotNull();
assertThat(value.getChildValues().get(treeFileArtifact2).getDigest()).isNotNull();
}
@@ -209,15 +209,27 @@
// artifact2 is a tree artifact generated by action template.
SpecialArtifact artifact2 = createDerivedTreeArtifactOnly("treeArtifact2");
- TreeFileArtifact treeFileArtifact1 = createFakeTreeFileArtifact(artifact2, "child1", "hello1");
- TreeFileArtifact treeFileArtifact2 = createFakeTreeFileArtifact(artifact2, "child2", "hello2");
+ TreeFileArtifact treeFileArtifact1 =
+ createFakeTreeFileArtifact(
+ artifact2,
+ ActionTemplateExpansionValue.ActionTemplateExpansionKey.of(
+ (ActionLookupValue.ActionLookupKey) artifact2.getArtifactOwner(), 1),
+ "child1",
+ "hello1");
+ TreeFileArtifact treeFileArtifact2 =
+ createFakeTreeFileArtifact(
+ artifact2,
+ ActionTemplateExpansionValue.ActionTemplateExpansionKey.of(
+ (ActionLookupValue.ActionLookupKey) artifact2.getArtifactOwner(), 1),
+ "child2",
+ "hello2");
actions.add(
ActionsTestUtil.createDummySpawnActionTemplate(artifact1, artifact2));
TreeArtifactValue value = (TreeArtifactValue) evaluateArtifactValue(artifact2);
- assertThat(value.getChildValues().get(treeFileArtifact1)).isNotNull();
- assertThat(value.getChildValues().get(treeFileArtifact2)).isNotNull();
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact1);
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact2);
assertThat(value.getChildValues().get(treeFileArtifact1).getDigest()).isNotNull();
assertThat(value.getChildValues().get(treeFileArtifact2).getDigest()).isNotNull();
}
@@ -238,14 +250,26 @@
// artifact3 is a tree artifact generated by action template.
SpecialArtifact artifact3 = createDerivedTreeArtifactOnly("treeArtifact3");
- TreeFileArtifact treeFileArtifact1 = createFakeTreeFileArtifact(artifact3, "child1", "hello1");
- TreeFileArtifact treeFileArtifact2 = createFakeTreeFileArtifact(artifact3, "child2", "hello2");
+ TreeFileArtifact treeFileArtifact1 =
+ createFakeTreeFileArtifact(
+ artifact3,
+ ActionTemplateExpansionValue.ActionTemplateExpansionKey.of(
+ (ActionLookupValue.ActionLookupKey) artifact2.getArtifactOwner(), 2),
+ "child1",
+ "hello1");
+ TreeFileArtifact treeFileArtifact2 =
+ createFakeTreeFileArtifact(
+ artifact3,
+ ActionTemplateExpansionValue.ActionTemplateExpansionKey.of(
+ (ActionLookupValue.ActionLookupKey) artifact2.getArtifactOwner(), 2),
+ "child2",
+ "hello2");
actions.add(
ActionsTestUtil.createDummySpawnActionTemplate(artifact2, artifact3));
TreeArtifactValue value = (TreeArtifactValue) evaluateArtifactValue(artifact3);
- assertThat(value.getChildValues().get(treeFileArtifact1)).isNotNull();
- assertThat(value.getChildValues().get(treeFileArtifact2)).isNotNull();
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact1);
+ assertThat(value.getChildValues()).containsKey(treeFileArtifact2);
assertThat(value.getChildValues().get(treeFileArtifact1).getDigest()).isNotNull();
assertThat(value.getChildValues().get(treeFileArtifact2).getDigest()).isNotNull();
}
@@ -256,7 +280,10 @@
}
private Artifact createSourceArtifact(String path) {
- return new Artifact(PathFragment.create(path), ArtifactRoot.asSourceRoot(Root.fromPath(root)));
+ return new Artifact.SourceArtifact(
+ ArtifactRoot.asSourceRoot(Root.fromPath(root)),
+ PathFragment.create(path),
+ ArtifactOwner.NullArtifactOwner.INSTANCE);
}
private Artifact createDerivedArtifact(String path) {
@@ -293,8 +320,23 @@
private TreeFileArtifact createFakeTreeFileArtifact(
SpecialArtifact treeArtifact, String parentRelativePath, String content) throws Exception {
- TreeFileArtifact treeFileArtifact = ActionInputHelper.treeFileArtifact(
- treeArtifact, PathFragment.create(parentRelativePath));
+ return createFakeTreeFileArtifact(
+ treeArtifact,
+ ActionTemplateExpansionValue.ActionTemplateExpansionKey.of(
+ (ActionLookupValue.ActionLookupKey) treeArtifact.getArtifactOwner(), 0),
+ parentRelativePath,
+ content);
+ }
+
+ private TreeFileArtifact createFakeTreeFileArtifact(
+ SpecialArtifact treeArtifact,
+ ArtifactOwner artifactOwner,
+ String parentRelativePath,
+ String content)
+ throws Exception {
+ TreeFileArtifact treeFileArtifact =
+ ActionInputHelper.treeFileArtifact(
+ treeArtifact, PathFragment.create(parentRelativePath), artifactOwner);
Path path = treeFileArtifact.getPath();
FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
writeFile(path, content);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
index aa73683..e0ef017 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -45,8 +46,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
-import java.util.HashSet;
-import java.util.Set;
+import java.util.LinkedHashSet;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.Before;
@@ -54,7 +54,7 @@
abstract class ArtifactFunctionTestCase {
static final ActionLookupKey ALL_OWNER = new SingletonActionLookupKey();
- protected Set<ActionAnalysisMetadata> actions;
+ protected LinkedHashSet<ActionAnalysisMetadata> actions;
protected boolean fastDigest = false;
protected RecordingDifferencer differencer = new SequencedRecordingDifferencer();
protected SequentialBuildDriver driver;
@@ -97,7 +97,7 @@
new FileStateFunction(
new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper))
.put(FileValue.FILE, new FileFunction(pkgLocator))
- .put(SkyFunctions.ARTIFACT, new ArtifactFunction())
+ .put(Artifact.ARTIFACT, new ArtifactFunction())
.put(SkyFunctions.ACTION_EXECUTION, new SimpleActionExecutionFunction())
.put(
SkyFunctions.PACKAGE,
@@ -129,7 +129,7 @@
driver = new SequentialBuildDriver(evaluator);
PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
- actions = new HashSet<>();
+ actions = new LinkedHashSet<>();
}
protected void setupRoot(CustomInMemoryFs fs) throws IOException {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 6ccaa19..ecce7ea 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -153,9 +153,7 @@
new FileSymlinkInfiniteExpansionUniquenessFunction());
skyFunctions.put(
SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction());
- skyFunctions.put(
- SkyFunctions.ARTIFACT,
- artifactFakeFunction);
+ skyFunctions.put(Artifact.ARTIFACT, artifactFakeFunction);
progressReceiver = new RecordingEvaluationProgressReceiver();
differencer = new SequencedRecordingDifferencer();
@@ -896,10 +894,8 @@
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws SkyFunctionException, InterruptedException {
- ArtifactSkyKey artifactKey = (ArtifactSkyKey) skyKey.argument();
- Artifact artifact = artifactKey.getArtifact();
try {
- return FileArtifactValue.create(artifact.getPath());
+ return FileArtifactValue.create(((Artifact) skyKey).getPath());
} catch (IOException e) {
throw new SkyFunctionException(e, Transience.PERSISTENT){};
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index 52fc550..f93c53a 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -39,6 +39,7 @@
import com.google.devtools.build.lib.actions.ActionResult;
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
import com.google.devtools.build.lib.actions.BuildFailedException;
@@ -207,7 +208,7 @@
ImmutableMap.<SkyFunctionName, SkyFunction>builder()
.put(FileStateValue.FILE_STATE, new FileStateFunction(tsgmRef, externalFilesHelper))
.put(FileValue.FILE, new FileFunction(pkgLocator))
- .put(SkyFunctions.ARTIFACT, new ArtifactFunction())
+ .put(Artifact.ARTIFACT, new ArtifactFunction())
.put(
SkyFunctions.ACTION_EXECUTION,
new ActionExecutionFunction(skyframeActionExecutor, tsgmRef))
@@ -343,9 +344,12 @@
return createSourceArtifact(scratch.getFileSystem(), name);
}
- Artifact createSourceArtifact(FileSystem fs, String name) {
+ private static Artifact createSourceArtifact(FileSystem fs, String name) {
Path root = fs.getPath(TestUtils.tmpDir());
- return new Artifact(PathFragment.create(name), ArtifactRoot.asSourceRoot(Root.fromPath(root)));
+ return new Artifact.SourceArtifact(
+ ArtifactRoot.asSourceRoot(Root.fromPath(root)),
+ PathFragment.create(name),
+ ArtifactOwner.NullArtifactOwner.INSTANCE);
}
protected Artifact createDerivedArtifact(String name) {
