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/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index 95714cd..f18e14f 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
@@ -228,7 +228,7 @@
throw new IllegalArgumentException(
"it is illegal to create an artifact with an empty execPath");
}
- this.hashCode = execPath.hashCode();
+ this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
this.root = root;
this.execPath = execPath;
this.rootRelativePath = rootRelativePath;
@@ -641,6 +641,9 @@
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;
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) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
index 101cb1b..90f6707 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
@@ -22,14 +22,35 @@
*/
public final class OutputFile extends FileTarget {
+ /**
+ * A kind of output file.
+ *
+ * The FILESET kind is only supported for a non-open-sourced {@code fileset} rule.
+ */
+ public enum Kind {
+ FILE,
+ FILESET
+ }
+
private final Rule generatingRule;
+ private final Kind kind;
/**
* Constructs an output file with the given label, which must be in the given
* package.
*/
OutputFile(Package pkg, Label label, Rule generatingRule) {
+ this(pkg, label, Kind.FILE, generatingRule);
+ }
+
+ /**
+ * Constructs an output file with the given label, which must be in the given
+ * package.
+ */
+ OutputFile(Package pkg, Label label,
+ Kind kind, Rule generatingRule) {
super(pkg, label);
+ this.kind = kind;
this.generatingRule = generatingRule;
}
@@ -50,6 +71,13 @@
return generatingRule;
}
+ /**
+ * Returns the kind of this output file.
+ */
+ public Kind getKind() {
+ return kind;
+ }
+
@Override
public String getTargetKind() {
return targetKind();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index c7cca63..37c8767 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -607,7 +607,7 @@
reportWarning("target '" + getName() + "' is both a rule and a file; please choose "
+ "another name for the rule", eventHandler);
}
- OutputFile outputFile = new OutputFile(pkg, label, this);
+ OutputFile outputFile = new OutputFile(pkg, label, ruleClass.getOutputFileKind(), this);
outputFilesBuilder.add(outputFile);
return outputFile;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 0abecd2..d832468 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -615,6 +615,7 @@
private final Map<String, Attribute> attributes = new LinkedHashMap<>();
private final Set<Label> requiredToolchains = new HashSet<>();
private boolean supportsPlatforms = true;
+ private OutputFile.Kind outputFileKind = OutputFile.Kind.FILE;
/**
* Constructs a new {@code RuleClassBuilder} using all attributes from all
@@ -734,6 +735,7 @@
supportsConstraintChecking,
requiredToolchains,
supportsPlatforms,
+ outputFileKind,
attributes.values());
}
@@ -1080,6 +1082,18 @@
}
/**
+ * Sets the kind of output files this rule creates.
+ * DO NOT USE! This only exists to support the non-open-sourced {@code fileset} rule.
+ * {@see OutputFile.Kind}.
+ */
+ public Builder setOutputFileKind(OutputFile.Kind outputFileKind) {
+ this.outputFileKind = outputFileKind;
+ return this;
+ }
+
+
+
+ /**
* Declares that instances of this rule are compatible with the specified environments,
* in addition to the defaults declared by their environment groups. This can be overridden
* by rule-specific declarations. See
@@ -1264,6 +1278,7 @@
@Nullable private final Label ruleDefinitionEnvironmentLabel;
@Nullable private final String ruleDefinitionEnvironmentHashCode;
+ private final OutputFile.Kind outputFileKind;
/**
* The set of configuration fragments which are legal for this rule's implementation to access.
@@ -1328,6 +1343,7 @@
boolean supportsConstraintChecking,
Set<Label> requiredToolchains,
boolean supportsPlatforms,
+ OutputFile.Kind outputFileKind,
Collection<Attribute> attributes) {
this.name = name;
this.key = key;
@@ -1350,6 +1366,7 @@
this.optionReferenceFunction = optionReferenceFunction;
this.ruleDefinitionEnvironmentLabel = ruleDefinitionEnvironmentLabel;
this.ruleDefinitionEnvironmentHashCode = ruleDefinitionEnvironmentHashCode;
+ this.outputFileKind = outputFileKind;
validateNoClashInPublicNames(attributes);
this.attributes = ImmutableList.copyOf(attributes);
this.workspaceOnly = workspaceOnly;
@@ -2174,6 +2191,11 @@
return supportsPlatforms;
}
+ @Nullable
+ public OutputFile.Kind getOutputFileKind() {
+ return outputFileKind;
+ }
+
public static boolean isThirdPartyPackage(PackageIdentifier packageIdentifier) {
if (!packageIdentifier.getRepository().isMain()) {
return false;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index fad7c84..fa0eefb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -146,7 +146,7 @@
}
ConfiguredAspect configuredAspect = builder.build();
- SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext);
+ SkylarkProviderValidationUtil.validateArtifacts(ruleContext);
return configuredAspect;
}
