Fix `equals()` and `hashCode()` for artifacts: artifacts of different classes are not equal.
Also validate that there are no tree and file artifacts with the same exec path.
Fixes #4668.
Closes #5284.
Change-Id: Id97c0407a476a5bfc697b4ca7b858e3d0c0f8c75
PiperOrigin-RevId: 198540425
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
index e417853..ee6063e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java
@@ -154,5 +154,11 @@
*/
ImmutableSet<Artifact> getOrphanArtifacts();
+ /**
+ * Returns the set of tree artifacts that have the same exec path as some other artifacts. Should
+ * only be called after the ConfiguredTarget is created.
+ */
+ ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles();
+
ActionKeyContext getActionKeyContext();
}
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 d411250..8a3faea 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
@@ -164,11 +164,45 @@
@Override
public ImmutableSet<Artifact> getOrphanArtifacts() {
if (!allowRegisteringActions) {
- return ImmutableSet.<Artifact>of();
+ return ImmutableSet.of();
}
return ImmutableSet.copyOf(getOrphanArtifactMap().keySet());
}
+ @Override
+ public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() {
+ if (!allowRegisteringActions) {
+ return ImmutableSet.of();
+ }
+
+ boolean hasTreeArtifacts = false;
+ for (Artifact artifact : artifacts.keySet()) {
+ if (artifact.isTreeArtifact()) {
+ hasTreeArtifacts = true;
+ break;
+ }
+ }
+ if (!hasTreeArtifacts) {
+ return ImmutableSet.of();
+ }
+
+ HashSet<PathFragment> collect = new HashSet<>();
+ for (Artifact artifact : artifacts.keySet()) {
+ if (!artifact.isSourceArtifact() && !artifact.isTreeArtifact()) {
+ collect.add(artifact.getExecPath());
+ }
+ }
+
+ ImmutableSet.Builder<Artifact> sameExecPathTreeArtifacts = ImmutableSet.builder();
+ for (Artifact artifact : artifacts.keySet()) {
+ if (artifact.isTreeArtifact() && collect.contains(artifact.getExecPath())) {
+ sameExecPathTreeArtifacts.add(artifact);
+ }
+ }
+
+ return sameExecPathTreeArtifacts.build();
+ }
+
private Map<Artifact, String> getOrphanArtifactMap() {
// Construct this set to avoid poor performance under large --runs_per_test.
Set<Artifact> artifactsWithActions = new HashSet<>();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 2d3478a..ca706fc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -554,7 +554,7 @@
* which this target (which must be an OutputFile or a Rule) is associated.
*/
public Artifact createOutputArtifact() {
- return internalCreateOutputArtifact(getTarget());
+ return internalCreateOutputArtifact(getTarget(), OutputFile.Kind.FILE);
}
/**
@@ -563,7 +563,7 @@
* @see #createOutputArtifact()
*/
public Artifact createOutputArtifact(OutputFile out) {
- return internalCreateOutputArtifact(out);
+ return internalCreateOutputArtifact(out, out.getKind());
}
/**
@@ -572,13 +572,22 @@
* {@link #createOutputArtifact(OutputFile)} can have a more specific
* signature.
*/
- private Artifact internalCreateOutputArtifact(Target target) {
+ private Artifact internalCreateOutputArtifact(Target target, OutputFile.Kind outputFileKind) {
Preconditions.checkState(
target.getLabel().getPackageIdentifier().equals(getLabel().getPackageIdentifier()),
"Creating output artifact for target '%s' in different package than the rule '%s' "
+ "being analyzed", target.getLabel(), getLabel());
ArtifactRoot root = getBinOrGenfilesDirectory();
- return getPackageRelativeArtifact(target.getName(), root);
+ PathFragment packageRelativePath = getPackageDirectory()
+ .getRelative(PathFragment.create(target.getName()));
+ switch (outputFileKind) {
+ case FILE:
+ return getDerivedArtifact(packageRelativePath, root);
+ case FILESET:
+ return getAnalysisEnvironment().getFilesetArtifact(packageRelativePath, root);
+ default:
+ throw new IllegalStateException();
+ }
}
/**
@@ -604,6 +613,14 @@
* Creates an artifact in a directory that is unique to the package that contains the rule, thus
* guaranteeing that it never clashes with artifacts created by rules in other packages.
*/
+ public Artifact getPackageRelativeTreeArtifact(String relative, ArtifactRoot root) {
+ return getPackageRelativeTreeArtifact(PathFragment.create(relative), root);
+ }
+
+ /**
+ * Creates an artifact in a directory that is unique to the package that contains the rule, thus
+ * guaranteeing that it never clashes with artifacts created by rules in other packages.
+ */
public Artifact getBinArtifact(String relative) {
return getBinArtifact(PathFragment.create(relative));
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
index 95e48a3..43de251 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviderValidationUtil.java
@@ -13,30 +13,38 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis;
-import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.syntax.EvalException;
-
/**
* Utility class to validate results of executing Skylark rules and aspects.
*/
public class SkylarkProviderValidationUtil {
- public static void checkOrphanArtifacts(RuleContext ruleContext) throws EvalException {
+ public static void validateArtifacts(RuleContext ruleContext) throws EvalException {
+ ImmutableSet<Artifact> treeArtifactsConflictingWithFiles =
+ ruleContext.getAnalysisEnvironment().getTreeArtifactsConflictingWithFiles();
+ if (!treeArtifactsConflictingWithFiles.isEmpty()) {
+ throw new EvalException(
+ null,
+ "The following directories were also declared as files:\n"
+ + artifactsDescription(treeArtifactsConflictingWithFiles));
+ }
+
ImmutableSet<Artifact> orphanArtifacts =
ruleContext.getAnalysisEnvironment().getOrphanArtifacts();
if (!orphanArtifacts.isEmpty()) {
- throw new EvalException(null, "The following files have no generating action:\n"
- + Joiner.on("\n").join(Iterables.transform(orphanArtifacts,
- new Function<Artifact, String>() {
- @Override
- public String apply(Artifact artifact) {
- return artifact.getRootRelativePathString();
- }
- })));
+ throw new EvalException(
+ null,
+ "The following files have no generating action:\n"
+ + artifactsDescription(orphanArtifacts));
}
}
+
+ private static String artifactsDescription(ImmutableSet<Artifact> artifacts) {
+ return Joiner.on("\n")
+ .join(Iterables.transform(artifacts, Artifact::getRootRelativePathString));
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
index d118a4a..a13b281 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -105,14 +105,22 @@
@Override
public Artifact declareDirectory(String filename, Object sibling) throws EvalException {
context.checkMutable("actions.declare_directory");
+ Artifact result;
if (Runtime.NONE.equals(sibling)) {
- return ruleContext.getPackageRelativeTreeArtifact(
- PathFragment.create(filename), newFileRoot());
+ result =
+ ruleContext.getPackageRelativeTreeArtifact(PathFragment.create(filename), newFileRoot());
} else {
PathFragment original = ((Artifact) sibling).getRootRelativePath();
PathFragment fragment = original.replaceName(filename);
- return ruleContext.getTreeArtifact(fragment, newFileRoot());
+ result = ruleContext.getTreeArtifact(fragment, newFileRoot());
}
+ if (!result.isTreeArtifact()) {
+ throw new EvalException(
+ null,
+ String.format(
+ "'%s' has already been declared as a regular file, not directory.", filename));
+ }
+ return result;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index 2c9b62f..1a687c1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -122,7 +122,7 @@
return null;
}
ConfiguredTarget configuredTarget = createTarget(skylarkRuleContext, target);
- SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
+ SkylarkProviderValidationUtil.validateArtifacts(ruleContext);
checkDeclaredProviders(configuredTarget, advertisedProviders, location);
return configuredTarget;
} catch (EvalException e) {
