diff options
Diffstat (limited to 'src')
16 files changed, 231 insertions, 26 deletions
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 95714cdc8a..f18e14f0d0 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 @@ public class Artifact 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 @@ public class Artifact 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 e417853ce1..ee6063ec6f 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 @@ public interface AnalysisEnvironment extends ActionRegistry { */ 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 d41125096f..8a3faea0a3 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 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment { @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 2d3478ab3b..ca706fc105 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 @@ public final class RuleContext extends TargetContext * 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 @@ public final class RuleContext extends TargetContext * @see #createOutputArtifact() */ public Artifact createOutputArtifact(OutputFile out) { - return internalCreateOutputArtifact(out); + return internalCreateOutputArtifact(out, out.getKind()); } /** @@ -572,13 +572,22 @@ public final class RuleContext extends TargetContext * {@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 @@ public final class RuleContext extends TargetContext * 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 95e48a390a..43de2518e0 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 d118a4ac5e..a13b2818f0 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 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi { @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 2c9b62fe6b..1a687c16c1 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 @@ public final class SkylarkRuleConfiguredTargetUtil { 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 101cb1b394..90f6707d9a 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 @@ import com.google.devtools.build.lib.events.Location; */ 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 @@ public final class OutputFile extends FileTarget { 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 c7cca63e85..37c8767a1e 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 @@ public final class Rule implements Target, DependencyFilter.AttributeInfoProvide 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 0abecd2fdc..d83246871d 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 @@ public class RuleClass { 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 @@ public class RuleClass { supportsConstraintChecking, requiredToolchains, supportsPlatforms, + outputFileKind, attributes.values()); } @@ -1080,6 +1082,18 @@ public class RuleClass { } /** + * 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 @@ public class RuleClass { @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 @@ public class RuleClass { boolean supportsConstraintChecking, Set<Label> requiredToolchains, boolean supportsPlatforms, + OutputFile.Kind outputFileKind, Collection<Attribute> attributes) { this.name = name; this.key = key; @@ -1350,6 +1366,7 @@ public class RuleClass { 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 @@ public class RuleClass { 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 fad7c84150..fa0eefb361 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 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory { } ConfiguredAspect configuredAspect = builder.build(); - SkylarkProviderValidationUtil.checkOrphanArtifacts(ruleContext); + SkylarkProviderValidationUtil.validateArtifacts(ruleContext); return configuredAspect; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java index a86066d1a5..51c0fcf6e3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java @@ -194,6 +194,11 @@ public final class AnalysisTestUtil { } @Override + public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() { + return original.getTreeArtifactsConflictingWithFiles(); + } + + @Override public ActionKeyContext getActionKeyContext() { return original.getActionKeyContext(); } @@ -398,7 +403,12 @@ public final class AnalysisTestUtil { @Override public ImmutableSet<Artifact> getOrphanArtifacts() { - return ImmutableSet.<Artifact>of(); + return ImmutableSet.of(); + } + + @Override + public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() { + return ImmutableSet.of(); } @Override 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 e687bc260f..360717f5fa 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 @@ -1142,6 +1142,32 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } /** + * Gets a tree artifact, creating it if necessary. {@code ArtifactOwner} should be a genuine + * {@link ConfiguredTargetKey} corresponding to a {@link ConfiguredTarget}. If called from a test + * that does not exercise the analysis phase, the convenience methods {@link + * #getBinArtifactWithNoOwner} or {@link #getGenfilesArtifactWithNoOwner} should be used instead. + */ + protected Artifact getTreeArtifact( + PathFragment rootRelativePath, ArtifactRoot root, ArtifactOwner owner) { + return view.getArtifactFactory().getTreeArtifact(rootRelativePath, root, owner); + } + + /** + * Gets a Tree 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 final Artifact getTreeArtifact(String packageRelativePath, ConfiguredTarget owner) { + return getPackageRelativeTreeArtifact( + packageRelativePath, + getConfiguration(owner).getBinDirectory(RepositoryName.MAIN), + ConfiguredTargetKey.of( + owner, skyframeExecutor.getConfiguration(reporter, owner.getConfigurationKey()))); + } + + + /** * Gets a derived Artifact for testing with path of the form * root/owner.getPackageFragment()/packageRelativePath. * @@ -1155,6 +1181,20 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } /** + * Gets a tree Artifact for testing with path of the form + * root/owner.getPackageFragment()/packageRelativePath. + * + * @see #getDerivedArtifact(PathFragment, ArtifactRoot, ArtifactOwner) + */ + private Artifact getPackageRelativeTreeArtifact( + String packageRelativePath, ArtifactRoot root, ArtifactOwner owner) { + return getTreeArtifact( + owner.getLabel().getPackageFragment().getRelative(packageRelativePath), + root, owner); + } + + + /** * Gets a derived Artifact for testing in the {@link BuildConfiguration#getBinDirectory}. This * method should only be used for tests that do no analysis, and so there is no ConfiguredTarget * to own this artifact. If the test runs the analysis phase, {@link #getBinArtifact(String, @@ -1922,6 +1962,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase { } @Override + public ImmutableSet<Artifact> getTreeArtifactsConflictingWithFiles() { + throw new UnsupportedOperationException(); + } + + @Override public ActionKeyContext getActionKeyContext() { return actionKeyContext; } diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index c82b2aeb25..b9b2f4c33c 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java @@ -894,7 +894,7 @@ public class RuleClassTest extends PackageLoadingTestCase { supportsConstraintChecking, /*requiredToolchains=*/ ImmutableSet.<Label>of(), /*supportsPlatforms=*/ true, - ImmutableList.copyOf(attributes)); + OutputFile.Kind.FILE, ImmutableList.copyOf(attributes)); } private static RuleClass createParentRuleClass() { diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java index 7d41301892..f894e4d32b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java @@ -705,7 +705,7 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { assertNoEvents(); SpawnAction splitAction = - getGeneratingSpawnAction(getBinArtifact("dexsplits/top", topTarget)); + getGeneratingSpawnAction(getTreeArtifact("dexsplits/top", topTarget)); assertThat(splitAction.getArguments()).contains("--main-dex-list"); assertThat(splitAction.getArguments()).contains("--minimal-main-dex"); assertThat(ActionsTestUtil.baseArtifactNames(getNonToolInputs(splitAction))) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 51fa9d33f0..877b1a3f24 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -1658,6 +1658,30 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { + " rule 'r' should be created by the same rule."); } + @Test + public void testFileAndDirectory() throws Exception { + scratch.file( + "ext.bzl", + "def _extrule(ctx):", + " dir = ctx.actions.declare_directory('foo/bar/baz')", + " ctx.actions.run_shell(", + " outputs = [dir],", + " command = 'mkdir -p ' + dir.path + ' && echo wtf > ' + dir.path + '/wtf.txt')", + "", + "extrule = rule(", + " _extrule,", + " outputs = {", + " 'out': 'foo/bar/baz',", + " },", + ")"); + scratch.file("BUILD", "load(':ext.bzl', 'extrule')", "", "extrule(", " name = 'test'", ")"); + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//:test"); + assertContainsEvent("ERROR /workspace/BUILD:3:1: in extrule rule //:test:"); + assertContainsEvent("he following directories were also declared as files:"); + assertContainsEvent("foo/bar/baz"); + } + /** * Skylark integration test that forces inlining. */ |