Check that most output artifacts are under a directory determined by the repository and package of the rule being analyzed. Currently this directory is PACKAGE for rules in the main repository and external/REPOSITORY_NAME/PACKAGE for rules in other repositories.
This is a plan to fix #293. Ideally, we would simply make it impossible to create artifacts not under that location, but in practice, we cannot do that because some rules do want to do this, mostly those that are already problematic due to shared actions. So the battle plan is to eliminate as many calls to AnalysisEnvironment.getDerivedArtifact() as I possibly can and audit the rest.
--
MOS_MIGRATED_REVID=99351151
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 0bccc72..07f7a02 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
@@ -45,6 +45,11 @@
* Returns the artifact for the derived file {@code rootRelativePath}.
*
* <p>Creates the artifact if necessary and sets the root of that artifact to {@code root}.
+ *
+ * <p>This method can create artifacts anywhere in the output tree, thus making it possible for
+ * artifacts generated by two different rules to clash. To avoid this, use the methods
+ * {@code getUniqueDirectoryArtifact} and {@code getPackageRelativeArtifact} on
+ * {@link RuleContext}.
*/
Artifact getDerivedArtifact(PathFragment rootRelativePath, Root root);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
index 3ded47d..9abcb25 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -125,7 +125,7 @@
* <p>For example "//pkg:target" -> "pkg/<fragment>/target.
*/
public static PathFragment getUniqueDirectory(Label label, PathFragment fragment) {
- return label.getPackageFragment().getRelative(fragment)
+ return label.getPackageIdentifier().getPathFragment().getRelative(fragment)
.getRelative(label.getName());
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PseudoAction.java b/src/main/java/com/google/devtools/build/lib/analysis/PseudoAction.java
index 2ac3bcc..d71998b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/PseudoAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/PseudoAction.java
@@ -88,9 +88,8 @@
}
public static Artifact getDummyOutput(RuleContext ruleContext) {
- return ruleContext.getAnalysisEnvironment().getDerivedArtifact(
- ruleContext.getLabel().toPathFragment().replaceName(
- ruleContext.getLabel().getName() + ".extra_action_dummy"),
+ return ruleContext.getPackageRelativeArtifact(
+ ruleContext.getLabel().getName() + ".extra_action_dummy",
ruleContext.getConfiguration().getGenfilesDirectory());
}
}
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 8f5351f..875e89f 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
@@ -358,10 +358,10 @@
@Override
public void attributeError(String attrName, String message) {
reportError(rule.getAttributeLocation(attrName),
- prefixAttributeMessage(Attribute.isImplicit(attrName)
- ? "(an implicit dependency)"
- : attrName,
- message));
+ prefixAttributeMessage(Attribute.isImplicit(attrName)
+ ? "(an implicit dependency)"
+ : attrName,
+ message));
}
/**
@@ -373,10 +373,10 @@
@Override
public void attributeWarning(String attrName, String message) {
reportWarning(rule.getAttributeLocation(attrName),
- prefixAttributeMessage(Attribute.isImplicit(attrName)
- ? "(an implicit dependency)"
- : attrName,
- message));
+ prefixAttributeMessage(Attribute.isImplicit(attrName)
+ ? "(an implicit dependency)"
+ : attrName,
+ message));
}
private String prefixAttributeMessage(String attrName, String message) {
@@ -424,8 +424,12 @@
* signature.
*/
private Artifact internalCreateOutputArtifact(Target target) {
+ 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());
Root root = getBinOrGenfilesDirectory();
- return getAnalysisEnvironment().getDerivedArtifact(Util.getWorkspaceRelativePath(target), root);
+ return getPackageRelativeArtifact(target.getName(), root);
}
/**
@@ -440,6 +444,71 @@
: getConfiguration().getGenfilesDirectory();
}
+ /**
+ * 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 getPackageRelativeArtifact(String relative, Root root) {
+ return getPackageRelativeArtifact(new PathFragment(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 getPackageRelativeArtifact(PathFragment relative, Root root) {
+ return getDerivedArtifact(getPackageDirectory().getRelative(relative), root);
+ }
+
+ /**
+ * Returns the root-relative path fragment under which output artifacts of this rule should go.
+ *
+ * <p>Note that:
+ * <ul>
+ * <li>This doesn't guarantee that there are no clashes with rules in the same package.
+ * <li>If possible, {@link #getPackageRelativeArtifact(PathFragment, Root)} should be used
+ * instead of this method.
+ * </ul>
+ *
+ * Ideally, user-visible artifacts should all have corresponding output file targets, all others
+ * should go into a rule-specific directory.
+ * {@link #getUniqueDirectoryArtifact(String, PathFragment, Root)}) ensures that this is the case.
+ */
+ public PathFragment getPackageDirectory() {
+ return getLabel().getPackageIdentifier().getPathFragment();
+ }
+
+ /**
+ * Creates an artifact under a given root with the given root-relative path.
+ *
+ * <p>Verifies that it is in the root-relative directory corresponding to the package of the rule,
+ * thus ensuring that it doesn't clash with other artifacts generated by other rules using this
+ * method.
+ */
+ public Artifact getDerivedArtifact(PathFragment rootRelativePath, Root root) {
+ Preconditions.checkState(rootRelativePath.startsWith(getPackageDirectory()),
+ "Output artifact '%s' not under package directory '%s' for target '%s'",
+ rootRelativePath, getPackageDirectory(), getLabel());
+ return getAnalysisEnvironment().getDerivedArtifact(rootRelativePath, root);
+ }
+ /**
+ * Creates an artifact in a directory that is unique to the rule, thus guaranteeing that it never
+ * clashes with artifacts created by other rules.
+ */
+ public Artifact getUniqueDirectoryArtifact(
+ String uniqueDirectory, String relative, Root root) {
+ return getUniqueDirectoryArtifact(uniqueDirectory, new PathFragment(relative), root);
+ }
+
+ public Artifact getUniqueDirectoryArtifact(
+ String uniqueDirectory, PathFragment relative, Root root) {
+ return getDerivedArtifact(getUniqueDirectory(uniqueDirectory).getRelative(relative), root);
+ }
+
+ public PathFragment getArtifactPackagePrefix() {
+ return getLabel().getPackageIdentifier().getPathFragment();
+ }
+
private Attribute getAttribute(String attributeName) {
// TODO(bazel-team): We should check original rule for such attribute first, because aspect
// can't contain empty attribute. Consider changing type of prerequisiteMap from
@@ -982,9 +1051,7 @@
* Only use from Skylark. Returns the implicit output artifact for a given output path.
*/
public Artifact getImplicitOutputArtifact(String path) {
- Root root = getBinOrGenfilesDirectory();
- PathFragment packageFragment = getLabel().getPackageFragment();
- return getAnalysisEnvironment().getDerivedArtifact(packageFragment.getRelative(path), root);
+ return getPackageRelativeArtifact(path, getBinOrGenfilesDirectory());
}
/**
@@ -1061,7 +1128,7 @@
*/
public final Artifact getRelatedArtifact(PathFragment pathFragment, String extension) {
PathFragment file = FileSystemUtils.replaceExtension(pathFragment, extension);
- return getAnalysisEnvironment().getDerivedArtifact(file, getConfiguration().getBinDirectory());
+ return getDerivedArtifact(file, getConfiguration().getBinDirectory());
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index dd91cfb..fcfce07 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -88,7 +88,7 @@
*/
private RunfilesSupport(RuleContext ruleContext, Artifact executable, Runfiles runfiles,
List<String> appendingArgs, boolean createSymlinks) {
- owningExecutable = executable;
+ owningExecutable = Preconditions.checkNotNull(executable);
this.createSymlinks = createSymlinks;
// Adding run_under target to the runfiles manifest so it would become part
@@ -123,19 +123,6 @@
.build();
}
- private RunfilesSupport(Runfiles runfiles, Artifact runfilesInputManifest,
- Artifact runfilesManifest, Artifact runfilesMiddleman, Artifact sourcesManifest,
- Artifact owningExecutable, boolean createSymlinks, ImmutableList<String> args) {
- this.runfiles = runfiles;
- this.runfilesInputManifest = runfilesInputManifest;
- this.runfilesManifest = runfilesManifest;
- this.runfilesMiddleman = runfilesMiddleman;
- this.sourcesManifest = sourcesManifest;
- this.owningExecutable = owningExecutable;
- this.createSymlinks = createSymlinks;
- this.args = args;
- }
-
/**
* Returns the executable owning this RunfilesSupport. Only use from Skylark.
*/
@@ -149,10 +136,6 @@
* returns null.
*/
public PathFragment getRunfilesDirectoryExecPath() {
- if (owningExecutable == null) {
- return null;
- }
-
PathFragment executablePath = owningExecutable.getExecPath();
return executablePath.getParentDirectory().getChild(
executablePath.getBaseName() + RUNFILES_DIR_EXT);
@@ -181,14 +164,14 @@
return runfilesInputManifest;
}
- private Artifact createRunfilesInputManifestArtifact(ActionConstructionContext context) {
+ private Artifact createRunfilesInputManifestArtifact(RuleContext context) {
// The executable may be null for emptyRunfiles
PathFragment relativePath = (owningExecutable != null)
? owningExecutable.getRootRelativePath()
- : Util.getWorkspaceRelativePath(context.getRule());
+ : context.getPackageDirectory().getRelative(context.getLabel().getName());
String basename = relativePath.getBaseName();
PathFragment inputManifestPath = relativePath.replaceName(basename + ".runfiles_manifest");
- return context.getAnalysisEnvironment().getDerivedArtifact(inputManifestPath,
+ return context.getDerivedArtifact(inputManifestPath,
context.getConfiguration().getBinDirectory());
}
@@ -259,7 +242,6 @@
/**
* Returns the Sources manifest.
- * This may be null if the owningRule has no executable.
*/
public Artifact getSourceManifest() {
return sourcesManifest;
@@ -302,7 +284,7 @@
PathFragment outputManifestPath = runfilesDir.getRelative("MANIFEST");
BuildConfiguration config = context.getConfiguration();
- Artifact outputManifest = context.getAnalysisEnvironment().getDerivedArtifact(
+ Artifact outputManifest = context.getDerivedArtifact(
outputManifestPath, config.getBinDirectory());
context.getAnalysisEnvironment().registerAction(new SymlinkTreeAction(
context.getActionOwner(), inputManifest, outputManifest, /*filesetTree=*/false));
@@ -318,18 +300,14 @@
*/
private Artifact createSourceManifest(ActionConstructionContext context, Runfiles runfiles) {
// Put the sources only manifest next to the MANIFEST file but call it SOURCES.
- PathFragment runfilesDir = getRunfilesDirectoryExecPath();
- if (runfilesDir != null) {
- PathFragment sourcesManifestPath = runfilesDir.getRelative("SOURCES");
- Artifact sourceOnlyManifest = context.getAnalysisEnvironment().getDerivedArtifact(
- sourcesManifestPath, context.getConfiguration().getBinDirectory());
- context.getAnalysisEnvironment().registerAction(
- SourceManifestAction.forRunfiles(
- ManifestType.SOURCES_ONLY, context.getActionOwner(), sourceOnlyManifest, runfiles));
- return sourceOnlyManifest;
- } else {
- return null;
- }
+ PathFragment executablePath = owningExecutable.getRootRelativePath();
+ PathFragment sourcesManifestPath = executablePath.getParentDirectory().getChild(
+ executablePath.getBaseName() + ".runfiles.SOURCES");
+ Artifact sourceOnlyManifest = context.getDerivedArtifact(
+ sourcesManifestPath, context.getConfiguration().getBinDirectory());
+ context.getAnalysisEnvironment().registerAction(SourceManifestAction.forRunfiles(
+ ManifestType.SOURCES_ONLY, context.getActionOwner(), sourceOnlyManifest, runfiles));
+ return sourceOnlyManifest;
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ActionConstructionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ActionConstructionContext.java
index b7461e5..71261a7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ActionConstructionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ActionConstructionContext.java
@@ -14,9 +14,12 @@
package com.google.devtools.build.lib.analysis.actions;
import com.google.devtools.build.lib.actions.ActionOwner;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.vfs.PathFragment;
/**
* A temporary interface to allow migration from RuleConfiguredTarget to RuleContext. It bundles
@@ -34,4 +37,13 @@
/** The current analysis environment. */
AnalysisEnvironment getAnalysisEnvironment();
+
+ /**
+ * Creates an artifact under a given root with the given root-relative path.
+ *
+ * <p>Verifies that it is in the root-relative directory corresponding to the package of the rule,
+ * thus ensuring that it doesn't clash with other artifacts generated by other rules using this
+ * method.
+ */
+ Artifact getDerivedArtifact(PathFragment rootRelativePath, Root root);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java
index 1128617..4ffaa92 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java
@@ -135,9 +135,8 @@
*/
public static Artifact createFile(RuleContext ruleContext,
String fileName, CharSequence contents, boolean executable) {
- Artifact scriptFileArtifact = ruleContext.getAnalysisEnvironment().getDerivedArtifact(
- ruleContext.getTarget().getLabel().getPackageFragment().getRelative(fileName),
- ruleContext.getConfiguration().getGenfilesDirectory());
+ Artifact scriptFileArtifact = ruleContext.getPackageRelativeArtifact(
+ fileName, ruleContext.getConfiguration().getGenfilesDirectory());
ruleContext.registerAction(new FileWriteAction(
ruleContext.getActionOwner(), scriptFileArtifact, contents, executable));
return scriptFileArtifact;
