Create source artifacts in the ninja_build rule instead of requiring them to be explicitly listed in the ninja_build.srcs attribute.
Pros:
* Makes it unnecessary to duplicate information in BUILD and .ninja files
* Absolves Bazel from tracking gazillions of unnecessary files
Cons:
* Can be problematic if multiple --package_path entries exist (e.g. one can have file a/b/c in one root while there is a/b/BUILD in another)
* Invalidates the invariant that "bazel query 'deps(<target>)'" returns all files that can possibly be needed to build that target
That's why Ninja support is still in experimental mode.
RELNOTES: None.
PiperOrigin-RevId: 298863427
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 1c1cf48..675dcb0 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
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.SkyFunction;
/**
@@ -96,6 +97,23 @@
SpecialArtifact getSymlinkArtifact(PathFragment rootRelativePath, ArtifactRoot root);
/**
+ * Creates a source artifact.
+ *
+ * <p>Do <b>NOT</b> use this unless you have a very good reason to do so and have consulted with
+ * someone knowledgeable. Source artifacts should be only created through {@link
+ * com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget} by {@link
+ * ConfiguredTargetFactory}.
+ *
+ * <p>This method should only be used when that's not an option. Currently, the only known use
+ * case is the {@code ninja_build} rule, which, if it couldn't create source artifacts, would have
+ * to have every source artifact that Ninja actions use enumerated in its {@code srcs} attribute.
+ *
+ * <p>If you use this erroneously, inconsistencies can occur, for example, creating a source
+ * artifact with the wrong package path entry or in the wrong package.
+ */
+ Artifact getSourceArtifactForNinjaBuild(PathFragment execpath, Root root);
+
+ /**
* Returns the artifact for the derived file {@code rootRelativePath}, creating it if necessary,
* and setting the root of that artifact to {@code root}. The artifact will represent the output
* directory of a {@code Fileset}.
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 91d537b..80648ac 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
@@ -38,6 +38,7 @@
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.SkyFunction;
import java.io.PrintWriter;
import java.io.StringWriter;
@@ -312,6 +313,11 @@
}
@Override
+ public Artifact getSourceArtifactForNinjaBuild(PathFragment execPath, Root root) {
+ return artifactFactory.getSourceArtifact(execPath, root, owner);
+ }
+
+ @Override
public Artifact.DerivedArtifact getFilesetArtifact(
PathFragment rootRelativePath, ArtifactRoot root) {
Preconditions.checkState(enabled);
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuild.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuild.java
index 89b84b9..fe7ddea 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuild.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuild.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.bazel.rules.ninja.actions;
import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Maps;
@@ -69,7 +68,6 @@
ruleContext,
graphProvider.getOutputRoot(),
graphProvider.getWorkingDirectory(),
- createSrcsMap(ruleContext),
depsMapBuilder.build(),
symlinksMapBuilder.build(),
graphProvider.getOutputRootSymlinks());
@@ -162,14 +160,6 @@
return nestedSetBuilder.build();
}
- private static ImmutableSortedMap<PathFragment, Artifact> createSrcsMap(RuleContext ruleContext) {
- ImmutableList<Artifact> srcs = ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list();
- ImmutableSortedMap.Builder<PathFragment, Artifact> inputsMapBuilder =
- ImmutableSortedMap.naturalOrder();
- srcs.forEach(a -> inputsMapBuilder.put(a.getRootRelativePath(), a));
- return inputsMapBuilder.build();
- }
-
private static void createDepsMap(
RuleContext ruleContext,
PathFragment workingDirectory,
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuildRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuildRule.java
index 76e727b..805e14f 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuildRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaBuildRule.java
@@ -16,7 +16,6 @@
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
-import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
@@ -40,6 +39,10 @@
* Bazel will determine that those are duplicates and only execute each action once. Future
* improvements are planned to avoid creation of duplicate actions, probably with the help of some
* coordinating registry structure.
+ *
+ * <p>Currently all input files of the Ninja graph must be in a subdirectory of the package the
+ * {@code ninja_build} rule is in. It's currently okay if they are in a subpackage, although that
+ * may change later. For best results, put this rule in the top-level BUILD file.
*/
public class NinjaBuildRule implements RuleDefinition {
@Override
@@ -51,10 +54,6 @@
.allowedRuleClasses("ninja_graph")
.setDoc("ninja_graph that parses all Ninja files that compose a graph of actions."))
.add(
- attr("srcs", LABEL_LIST)
- .allowedFileTypes(FileTypeSet.ANY_FILE)
- .setDoc("Source files requested by Ninja graph actions."))
- .add(
attr("deps_mapping", BuildType.LABEL_DICT_UNARY)
.allowedFileTypes(FileTypeSet.ANY_FILE)
.setDoc(
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraph.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraph.java
index 7c75e51..6dfb5c3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraph.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraph.java
@@ -102,7 +102,6 @@
workingDirectory,
ImmutableSortedMap.of(),
ImmutableSortedMap.of(),
- ImmutableSortedMap.of(),
ImmutableSortedSet.of());
if (ruleContext.hasErrors()) {
return null;
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphArtifactsHelper.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphArtifactsHelper.java
index 3cd4e3d..377e1f2 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphArtifactsHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphArtifactsHelper.java
@@ -44,7 +44,6 @@
private final ImmutableSortedMap<PathFragment, Artifact> depsNameToArtifact;
private final ImmutableSortedMap<PathFragment, Artifact> symlinkPathToArtifact;
private final ImmutableSortedSet<PathFragment> outputRootSymlinks;
- private final ImmutableSortedMap<PathFragment, Artifact> srcsMap;
/**
* Constructor
@@ -53,8 +52,6 @@
* @param outputRootPath name of output directory for Ninja actions under execroot
* @param workingDirectory relative path under execroot, the root for interpreting all paths in
* Ninja file
- * @param srcsMap mapping between the path fragment and artifact for the files passed in 'srcs'
- * attribute
* @param depsNameToArtifact mapping between the path fragment in the Ninja file and prebuilt
* @param symlinkPathToArtifact mapping of paths to artifacts for input symlinks under output_root
* @param outputRootSymlinks list of output paths for which symlink artifacts should be created,
@@ -64,14 +61,12 @@
RuleContext ruleContext,
PathFragment outputRootPath,
PathFragment workingDirectory,
- ImmutableSortedMap<PathFragment, Artifact> srcsMap,
ImmutableSortedMap<PathFragment, Artifact> depsNameToArtifact,
ImmutableSortedMap<PathFragment, Artifact> symlinkPathToArtifact,
ImmutableSortedSet<PathFragment> outputRootSymlinks) {
this.ruleContext = ruleContext;
this.outputRootPath = outputRootPath;
this.workingDirectory = workingDirectory;
- this.srcsMap = srcsMap;
this.depsNameToArtifact = depsNameToArtifact;
this.symlinkPathToArtifact = symlinkPathToArtifact;
this.outputRootSymlinks = outputRootSymlinks;
@@ -84,9 +79,9 @@
DerivedArtifact createOutputArtifact(PathFragment pathRelativeToWorkingDirectory)
throws GenericParsingException {
- PathFragment pathRelativeToWorkspaceRoot =
- workingDirectory.getRelative(pathRelativeToWorkingDirectory);
- if (!pathRelativeToWorkspaceRoot.startsWith(outputRootPath)) {
+ PathFragment execPath = workingDirectory.getRelative(pathRelativeToWorkingDirectory);
+
+ if (!execPath.startsWith(outputRootPath)) {
throw new GenericParsingException(
String.format(
"Ninja actions are allowed to create outputs only under output_root,"
@@ -94,41 +89,47 @@
pathRelativeToWorkingDirectory));
}
// If the path was declared as output symlink, create a symlink artifact.
- if (outputRootSymlinks.contains(pathRelativeToWorkspaceRoot.relativeTo(outputRootPath))) {
+ if (outputRootSymlinks.contains(execPath.relativeTo(outputRootPath))) {
return ruleContext
.getAnalysisEnvironment()
- .getSymlinkArtifact(
- pathRelativeToWorkspaceRoot.relativeTo(outputRootPath), derivedOutputRoot);
+ .getSymlinkArtifact(execPath.relativeTo(outputRootPath), derivedOutputRoot);
}
- return ruleContext.getDerivedArtifact(
- pathRelativeToWorkspaceRoot.relativeTo(outputRootPath), derivedOutputRoot);
+ return ruleContext.getDerivedArtifact(execPath.relativeTo(outputRootPath), derivedOutputRoot);
}
- Artifact getInputArtifact(PathFragment pathRelativeToWorkingDirectory)
- throws GenericParsingException {
- Preconditions.checkNotNull(srcsMap);
- PathFragment pathRelativeToWorkspaceRoot =
- workingDirectory.getRelative(pathRelativeToWorkingDirectory);
- Artifact asInput = srcsMap.get(pathRelativeToWorkspaceRoot);
- Artifact depsMappingArtifact = depsNameToArtifact.get(pathRelativeToWorkingDirectory);
- Artifact symlinkMappingArtifact = symlinkPathToArtifact.get(pathRelativeToWorkingDirectory);
- // Symlinked artifact is by definition outside of sources, in the output directory.
- if (asInput != null && depsMappingArtifact != null) {
+ Artifact getInputArtifact(PathFragment workingDirectoryPath) throws GenericParsingException {
+ if (depsNameToArtifact.containsKey(workingDirectoryPath)) {
+ return depsNameToArtifact.get(workingDirectoryPath);
+ }
+
+ if (symlinkPathToArtifact.containsKey(workingDirectoryPath)) {
+ return symlinkPathToArtifact.get(workingDirectoryPath);
+ }
+
+ PathFragment execPath = workingDirectory.getRelative(workingDirectoryPath);
+ if (execPath.startsWith(outputRootPath)) {
+ // In the output directory, so it must be a derived artifact.
+ return ruleContext.getDerivedArtifact(execPath.relativeTo(outputRootPath), derivedOutputRoot);
+ }
+
+ if (!execPath.startsWith(ruleContext.getPackageDirectory())) {
throw new GenericParsingException(
String.format(
- "Source file '%s' is passed both in 'srcs' " + "and 'deps_mapping' attributes.",
- pathRelativeToWorkingDirectory));
+ "Source artifact '%s' is not under the package of the ninja_build rule", execPath));
}
- if (asInput != null) {
- return asInput;
- }
- if (depsMappingArtifact != null) {
- return depsMappingArtifact;
- }
- if (symlinkMappingArtifact != null) {
- return symlinkMappingArtifact;
- }
- return createOutputArtifact(pathRelativeToWorkingDirectory);
+
+ // Not a derived artifact. Create a corresponding source artifact. This isn't really great
+ // because we have no guarantee that the artifact is not under a different package which
+ // invalidates the guarantee of "bazel query" that the dependencies reported for a target are a
+ // superset of all possible targets that are needed to build it, worse yet, there isn't even a
+ // guarantee that there isn't a package on a different package path in between.
+ //
+ // TODO(lberki): Check whether there is a package in between from another package path entry.
+ // We probably can't prohibit packages in between, though. Schade.
+ return ruleContext
+ .getAnalysisEnvironment()
+ .getSourceArtifactForNinjaBuild(
+ execPath, ruleContext.getRule().getPackage().getSourceRoot());
}
public Artifact getDepsMappingArtifact(PathFragment fragment) {
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 da62d1f..3172d4a 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
@@ -53,6 +53,7 @@
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionName;
import java.io.IOException;
@@ -147,6 +148,11 @@
}
@Override
+ public Artifact getSourceArtifactForNinjaBuild(PathFragment execPath, Root root) {
+ return original.getSourceArtifactForNinjaBuild(execPath, root);
+ }
+
+ @Override
public Artifact getFilesetArtifact(PathFragment rootRelativePath, ArtifactRoot root) {
return original.getFilesetArtifact(rootRelativePath, root);
}
@@ -345,6 +351,11 @@
}
@Override
+ public Artifact getSourceArtifactForNinjaBuild(PathFragment execPath, Root root) {
+ return null;
+ }
+
+ @Override
public ExtendedEventHandler getEventHandler() {
return null;
}
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 12e34e6..e14b09f 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
@@ -1996,6 +1996,11 @@
}
@Override
+ public Artifact getSourceArtifactForNinjaBuild(PathFragment execPath, Root root) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public ExtendedEventHandler getEventHandler() {
return reporter;
}
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaBuildTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaBuildTest.java
index 5fa0b1f..787b6f5 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/ninja/NinjaBuildTest.java
@@ -37,7 +37,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Test for {@link NinjaBuild} configured target factory. */
+/** Test for the {@code NinjaBuild} configured target factory. */
@RunWith(JUnit4.class)
public class NinjaBuildTest extends BuildViewTestCase {
@@ -56,6 +56,23 @@
}
@Test
+ public void testSourceFileNotInSubtree() throws Exception {
+ rewriteWorkspace("dont_symlink_directories_in_execroot(paths=['out'])");
+
+ scratch.file("a/n.ninja", "rule cp", " command = cp $in $out", "build out/o: cp subdir/i");
+
+ scratch.file(
+ "a/BUILD",
+ "ninja_graph(name='graph', output_root='out', main='n.ninja')",
+ "ninja_build(name='build', ninja_graph=':graph', output_groups={'o': ['out/o']})");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//a:build");
+ assertContainsEvent(
+ "Source artifact 'subdir/i' is not under the package of the ninja_build rule");
+ }
+
+ @Test
public void testNinjaBuildRule() throws Exception {
rewriteWorkspace(
"workspace(name = 'test')",
@@ -351,7 +368,6 @@
" working_directory = 'build_config',",
" main = 'build_config/build.ninja')",
"ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
- " srcs = ['input'],",
" output_groups= {'main': ['out_file']})");
assertThat(configuredTarget).isInstanceOf(RuleConfiguredTarget.class);
RuleConfiguredTarget ninjaConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
diff --git a/src/test/java/com/google/devtools/build/lib/blackbox/tests/NinjaBlackBoxTest.java b/src/test/java/com/google/devtools/build/lib/blackbox/tests/NinjaBlackBoxTest.java
index 6162276..a8a415b 100644
--- a/src/test/java/com/google/devtools/build/lib/blackbox/tests/NinjaBlackBoxTest.java
+++ b/src/test/java/com/google/devtools/build/lib/blackbox/tests/NinjaBlackBoxTest.java
@@ -133,33 +133,6 @@
}
@Test
- public void testSourceFileIsMissing() throws Exception {
- context().write("input.txt", "World");
- context()
- .write(
- "build_dir/build.ninja",
- "rule echo",
- " command = echo \"Hello $$(cat ${in})!\" > ${out}",
- "build hello.txt: echo ../input.txt");
- context()
- .write(
- "BUILD",
- "ninja_graph(name = 'graph', output_root = 'build_dir',",
- " working_directory = 'build_dir',",
- " main = 'build_dir/build.ninja')",
- "ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
- " output_groups = {'group': ['hello.txt']})");
-
- BuilderRunner bazel = context().bazel().withFlags("--experimental_ninja_actions");
- ProcessResult result = bazel.shouldFail().build("//:ninja_target");
- assertThat(result.errString())
- .contains(
- "in ninja_build rule //:ninja_target: Ninja actions are allowed to create outputs only "
- + "under output_root, path '../input.txt' is not allowed.");
- assertThat(result.errString()).contains("FAILED: Build did NOT complete successfully");
- }
-
- @Test
public void testSourceFileIsMissingUnderOutputRoot() throws Exception {
context().write("input.txt", "World");
context()
@@ -625,7 +598,6 @@
" working_directory = 'build_dir',",
" main = 'build_dir/build.ninja')",
"ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
- " srcs = [\":input.txt\"],",
" output_groups= {'main': ['first.txt']})");
BuilderRunner bazel = context().bazel().withFlags("--experimental_ninja_actions");
