Don't register a NinjaAction if the output of that NinjaAction is already
created by a symlink action created from that output being specified in the
output_root_inputs attribute of the ninja_graph rule.
RELNOTES: None.
PiperOrigin-RevId: 300134451
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaActionsHelper.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaActionsHelper.java
index c6b33b2..6b33ebf 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaActionsHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaActionsHelper.java
@@ -15,8 +15,10 @@
package com.google.devtools.build.lib.bazel.rules.ninja.actions;
import com.google.common.base.Ascii;
+import com.google.common.base.Joiner;
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.Sets;
import com.google.devtools.build.lib.actions.Artifact;
@@ -38,6 +40,7 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayDeque;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -61,6 +64,7 @@
private final ImmutableSortedMap<String, String> executionInfo;
private final PhonyTargetArtifacts phonyTargetArtifacts;
private final List<PathFragment> pathsToBuild;
+ private final ImmutableSet<PathFragment> outputRootInputsSymlinks;
/**
* Constructor
@@ -79,7 +83,8 @@
ImmutableSortedMap<PathFragment, NinjaTarget> allUsualTargets,
ImmutableSortedMap<PathFragment, PhonyTarget> phonyTargets,
PhonyTargetArtifacts phonyTargetArtifacts,
- List<PathFragment> pathsToBuild) {
+ List<PathFragment> pathsToBuild,
+ ImmutableSet<PathFragment> outputRootInputsSymlinks) {
this.ruleContext = ruleContext;
this.artifactsHelper = artifactsHelper;
this.allUsualTargets = allUsualTargets;
@@ -88,6 +93,7 @@
this.executionInfo = createExecutionInfo(ruleContext);
this.phonyTargetArtifacts = phonyTargetArtifacts;
this.pathsToBuild = pathsToBuild;
+ this.outputRootInputsSymlinks = outputRootInputsSymlinks;
}
void createNinjaActions() throws GenericParsingException {
@@ -109,10 +115,39 @@
PathFragment fragment = queue.remove();
NinjaTarget target = allUsualTargets.get(fragment);
if (target != null) {
- if (visitedTargets.add(target)) {
- createNinjaAction(target);
+ // If the output is already created by a symlink action created from specifying that
+ // file in output_root_inputs attribute of the ninja_graph rule, do not create other
+ // actions that output that same file, since that will result in an action conflict.
+ if (!outputRootInputsSymlinks.contains(fragment)) {
+ if (visitedTargets.add(target)) {
+ createNinjaAction(target);
+ }
+ enqueuer.accept(target.getAllInputs());
+ } else {
+ // Sanity check that the Ninja action we're skipping (because its outputs are already
+ // being symlinked using output_root_inputs) has only symlink outputs specified in
+ // output_root_inputs. Otherwise we might skip some other outputs.
+ List<PathFragment> outputsInOutputRootInputsSymlinks = new ArrayList<>();
+ List<PathFragment> outputsNotInOutputRootInputsSymlinks = new ArrayList<>();
+ for (PathFragment output : target.getAllOutputs()) {
+ if (outputRootInputsSymlinks.contains(output)) {
+ outputsInOutputRootInputsSymlinks.add(output);
+ } else {
+ outputsNotInOutputRootInputsSymlinks.add(output);
+ }
+ }
+ if (!outputsNotInOutputRootInputsSymlinks.isEmpty()) {
+ throw new GenericParsingException(
+ "Ninja target "
+ + target.getRuleName()
+ + " has "
+ + "outputs in output_root_inputs and other outputs not in output_root_inputs:\n"
+ + "Outputs in output_root_inputs:\n "
+ + Joiner.on(" \n").join(outputsInOutputRootInputsSymlinks)
+ + "\nOutputs not in output_root_inputs:\n "
+ + Joiner.on(" \n").join(outputsNotInOutputRootInputsSymlinks));
+ }
}
- enqueuer.accept(target.getAllInputs());
} else {
PhonyTarget phonyTarget = phonyTargets.get(fragment);
// Phony target can be null, if the path in neither usual or phony target,
@@ -126,7 +161,6 @@
private void createNinjaAction(NinjaTarget target) throws GenericParsingException {
NinjaRule rule = getNinjaRule(target);
-
NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
ImmutableList.Builder<Artifact> outputsBuilder = ImmutableList.builder();
boolean isAlwaysDirty = fillArtifacts(target, inputsBuilder, outputsBuilder);
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 fe7ddea..11cb1db 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
@@ -84,7 +84,8 @@
graphProvider.getUsualTargets(),
graphProvider.getPhonyTargetsMap(),
phonyTargetArtifacts,
- pathsToBuild)
+ pathsToBuild,
+ graphProvider.getOutputRootInputsSymlinks())
.createNinjaActions();
if (!checkOrphanArtifacts(ruleContext)) {
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 6dfb5c3..bea9f17 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
@@ -14,10 +14,12 @@
package com.google.devtools.build.lib.bazel.rules.ninja.actions;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
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.ImmutableSortedSet;
import com.google.common.collect.Iterables;
@@ -125,28 +127,35 @@
.pipeline(mainArtifact.getPath());
targetsPreparer.process(ninjaTargets);
+ NestedSet<Artifact> outputRootInputsSymlinks =
+ createSymlinkActions(ruleContext, sourceRoot, outputRootInputs, artifactsHelper);
+ if (ruleContext.hasErrors()) {
+ return null;
+ }
+
+ ImmutableSortedSet<PathFragment> outputRootSymlinksPathFragments =
+ outputRootSymlinks.stream()
+ .map(PathFragment::create)
+ .collect(toImmutableSortedSet(Comparator.comparing(PathFragment::getPathString)));
+
+ ImmutableSet<PathFragment> outputRootInputsSymlinksPathFragments =
+ outputRootInputsSymlinks.toList().stream()
+ .map(Artifact::getExecPath)
+ .collect(toImmutableSet());
+
NinjaGraphProvider ninjaGraphProvider =
new NinjaGraphProvider(
outputRoot,
workingDirectory,
targetsPreparer.getUsualTargets(),
targetsPreparer.getPhonyTargetsMap(),
- outputRootSymlinks.stream()
- .map(PathFragment::create)
- .collect(
- toImmutableSortedSet(Comparator.comparing(PathFragment::getPathString))));
-
- NestedSet<Artifact> filesToBuild =
- createSymlinkActions(
- ruleContext, sourceRoot, outputRoot, outputRootInputs, artifactsHelper);
- if (ruleContext.hasErrors()) {
- return null;
- }
+ outputRootSymlinksPathFragments,
+ outputRootInputsSymlinksPathFragments);
return new RuleConfiguredTargetBuilder(ruleContext)
.addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
.addProvider(NinjaGraphProvider.class, ninjaGraphProvider)
- .setFilesToBuild(filesToBuild)
+ .setFilesToBuild(outputRootInputsSymlinks)
.build();
} catch (GenericParsingException | IOException e) {
// IOException is possible with reading Ninja file, describing the action graph.
@@ -158,16 +167,19 @@
private NestedSet<Artifact> createSymlinkActions(
RuleContext ruleContext,
Root sourceRoot,
- PathFragment outputRootPath,
List<String> outputRootInputs,
NinjaGraphArtifactsHelper artifactsHelper)
throws GenericParsingException {
+
if (outputRootInputs.isEmpty()) {
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
}
- NestedSetBuilder<Artifact> filesToBuild = NestedSetBuilder.stableOrder();
+
+ NestedSetBuilder<Artifact> symlinks = NestedSetBuilder.stableOrder();
Path outputRootInSources =
- Preconditions.checkNotNull(sourceRoot.asPath()).getRelative(outputRootPath);
+ Preconditions.checkNotNull(sourceRoot.asPath())
+ .getRelative(artifactsHelper.getOutputRootPath());
+
for (String input : outputRootInputs) {
// output_root_inputs are relative to the output_root directory, and we should
// pass inside createOutputArtifact() paths, relative to working directory.
@@ -177,7 +189,7 @@
.getOutputRootPath()
.getRelative(input)
.relativeTo(artifactsHelper.getWorkingDirectory()));
- filesToBuild.add(derivedArtifact);
+ symlinks.add(derivedArtifact);
// This method already expects the path relative to output_root.
PathFragment absolutePath =
outputRootInSources.getRelative(PathFragment.create(input)).asFragment();
@@ -190,7 +202,7 @@
"Symlinking %s under <execroot>/%s", input, artifactsHelper.getOutputRootPath()));
ruleContext.registerAction(symlinkAction);
}
- return filesToBuild.build();
+ return symlinks.build();
}
private static class TargetsPreparer {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphProvider.java
index 7bb50bb..deff5a5 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaGraphProvider.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.bazel.rules.ninja.actions;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
@@ -32,18 +33,22 @@
private final ImmutableSortedMap<PathFragment, NinjaTarget> usualTargets;
private final ImmutableSortedMap<PathFragment, PhonyTarget> phonyTargetsMap;
private final ImmutableSortedSet<PathFragment> outputRootSymlinks;
+ private final ImmutableSet<PathFragment> outputRootInputsSymlinks;
public NinjaGraphProvider(
PathFragment outputRoot,
PathFragment workingDirectory,
ImmutableSortedMap<PathFragment, NinjaTarget> usualTargets,
ImmutableSortedMap<PathFragment, PhonyTarget> phonyTargetsMap,
- ImmutableSortedSet<PathFragment> outputRootSymlinks) {
+ ImmutableSortedSet<PathFragment> outputRootSymlinks,
+ ImmutableSet<PathFragment> outputRootInputsSymlinks) {
+
this.outputRoot = outputRoot;
this.workingDirectory = workingDirectory;
this.usualTargets = usualTargets;
this.phonyTargetsMap = phonyTargetsMap;
this.outputRootSymlinks = outputRootSymlinks;
+ this.outputRootInputsSymlinks = outputRootInputsSymlinks;
}
public PathFragment getOutputRoot() {
@@ -66,4 +71,8 @@
public ImmutableSortedSet<PathFragment> getOutputRootSymlinks() {
return outputRootSymlinks;
}
+
+ public ImmutableSet<PathFragment> getOutputRootInputsSymlinks() {
+ return outputRootInputsSymlinks;
+ }
}
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 ef63388..4d7e5d3 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
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.bazel.rules.ninja;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
@@ -463,4 +464,88 @@
assertThat(commandLines.get(0).commandLine.toString())
.endsWith("cd build_config && ln -s fictive-file dangling_symlink");
}
+
+ @Test
+ public void testOutputRootInputsWithConflictingNinjaActionOutput() throws Exception {
+ rewriteWorkspace(
+ "workspace(name = 'test')",
+ "dont_symlink_directories_in_execroot(paths = ['build_config'])");
+
+ scratch.file("build_config/hello.txt", "hello");
+ scratch.file(
+ "build_config/build.ninja",
+ "rule hello_world",
+ " command = echo \"Hello World!\" > ${out}",
+ "build build_config/hello.txt: hello_world",
+ "rule echo",
+ " command = echo \"Hello $$(cat ${in})!\" > ${out}",
+ "build build_config/out.txt: echo build_config/hello.txt");
+
+ // Working directory is workspace root.
+ ConfiguredTarget configuredTarget =
+ scratchConfiguredTarget(
+ "",
+ "ninja_target",
+ "ninja_graph(name = 'graph', output_root = 'build_config',",
+ " main = 'build_config/build.ninja',",
+ // hello.txt will be symlinked using a symlink action, but will also be an output of
+ // the NinjaAction created from the ninja rule above.
+ " output_root_inputs = ['hello.txt'])",
+ "ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
+ " output_groups= {'main': ['build_config/out.txt']})");
+
+ RuleConfiguredTarget ninjaConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+ ImmutableList<ActionAnalysisMetadata> actions = ninjaConfiguredTarget.getActions();
+ // The build.ninja file has rules for two actions, but only one of them should have been
+ // registered. Normally this would produce an ActionsConflictException, but we skip the action.
+ assertThat(actions).hasSize(1);
+ assertThat(actions.get(0).getOutputs().asList().get(0).getExecPathString())
+ .isEqualTo("build_config/out.txt");
+ }
+
+ @Test
+ public void testOutputRootInputsWithConflictingNinjaActionOutputThrowsErrorOnNonSymlinkOutput()
+ throws Exception {
+
+ rewriteWorkspace(
+ "workspace(name = 'test')",
+ "dont_symlink_directories_in_execroot(paths = ['build_config'])");
+
+ scratch.file("build_config/hello.txt", "hello");
+ scratch.file(
+ "build_config/build.ninja",
+ "rule hello_world",
+ " command = echo \"Hello World!\" > ${out}",
+ "build build_config/hello.txt build_config/not_an_input_symlink.txt: hello_world",
+ "rule echo",
+ " command = echo \"Hello $$(cat ${in})!\" > ${out}",
+ "build build_config/out.txt: echo build_config/hello.txt");
+
+ String message =
+ "in ninja_build rule //:ninja_target: Ninja target hello_world has outputs in "
+ + "output_root_inputs and other outputs not in output_root_inputs:\n"
+ + "Outputs in output_root_inputs:\n"
+ + " build_config/hello.txt\n"
+ + "Outputs not in output_root_inputs:\n"
+ + " build_config/not_an_input_symlink.txt";
+
+ // A GenericParsingException is what's actually created, however the rule code and the
+ // testing framework only relays the exception's messasge wrapped in an AssertionError.
+ Throwable throwable =
+ assertThrows(
+ AssertionError.class,
+ () ->
+ scratchConfiguredTarget(
+ "",
+ "ninja_target",
+ "ninja_graph(name = 'graph', output_root = 'build_config',",
+ " main = 'build_config/build.ninja',",
+ // hello.txt will be symlinked using a symlink action, but will also be an
+ // output of
+ // the NinjaAction created from the ninja rule above.
+ " output_root_inputs = ['hello.txt'])",
+ "ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
+ " output_groups= {'main': ['build_config/out.txt']})"));
+ assertThat(throwable).hasMessageThat().contains(message);
+ }
}
