Do not re-run Ninja actions when only order-only inputs change.
RELNOTES: None.
PiperOrigin-RevId: 312347597
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java
index 644078d..19cb6a6 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
+import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaTarget;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -53,12 +54,15 @@
@Nullable private final Artifact depFile;
private final ImmutableMap<PathFragment, Artifact> allowedDerivedInputs;
private final ArtifactRoot derivedOutputRoot;
+ private final NestedSet<Artifact> originalInputs; // This does not include order-only inputs.
+ private final NestedSet<Artifact> orderOnlyInputs;
public NinjaAction(
ActionOwner owner,
Root sourceRoot,
NestedSet<Artifact> tools,
NestedSet<Artifact> inputs,
+ NestedSet<Artifact> orderOnlyInputs,
List<? extends Artifact> outputs,
CommandLines commandLines,
ActionEnvironment env,
@@ -88,6 +92,7 @@
/* resultConsumer= */ null);
this.sourceRoot = sourceRoot;
this.depFile = depFile;
+
ImmutableMap.Builder<PathFragment, Artifact> allowedDerivedInputsBuilder =
ImmutableMap.builder();
for (Artifact input : inputs.toList()) {
@@ -96,7 +101,10 @@
}
}
this.allowedDerivedInputs = allowedDerivedInputsBuilder.build();
+
this.derivedOutputRoot = derivedOutputRoot;
+ this.originalInputs = inputs;
+ this.orderOnlyInputs = orderOnlyInputs;
}
@Override
@@ -113,18 +121,31 @@
throws EnvironmentalExecException {
checkOutputsForDirectories(actionExecutionContext);
- if (depFile != null) {
+ if (depFile == null) {
+ // The inputs may have been modified during input discovery to include order-only inputs.
+ // Restore the original inputs to exclude those order-only inputs, so that order-only inputs
+ // do not cause the action to be rerun. This needs to be done when there is no dep file
+ // because updateInputsFromDepfile() will recalculate all the dependencies, and even if an
+ // input is order-only, it will cause a rebuild if it's listed in the depfile.
+ // Note also that this must check if the order-only inputs are empty, because if they are,
+ // discoversInputs() will return false, causing updateInputs() to throw an error.
+ if (!orderOnlyInputs.isEmpty()) {
+ updateInputs(originalInputs);
+ }
+ } else {
updateInputsFromDepfile(actionExecutionContext);
}
}
private void updateInputsFromDepfile(ActionExecutionContext actionExecutionContext)
throws EnvironmentalExecException {
+
boolean siblingRepositoryLayout =
actionExecutionContext
.getOptions()
.getOptions(StarlarkSemanticsOptions.class)
.experimentalSiblingRepositoryLayout;
+
CppIncludeExtractionContext scanningContext =
actionExecutionContext.getContext(CppIncludeExtractionContext.class);
ArtifactResolver artifactResolver = scanningContext.getArtifactResolver();
@@ -142,6 +163,7 @@
} else {
execRelativePath = inputPath.asFragment().relativeTo(execRoot.asFragment());
}
+
Artifact inputArtifact = null;
if (allowedDerivedInputs.containsKey(execRelativePath)) {
// Predeclared generated input.
@@ -169,6 +191,7 @@
+ "a source input, or a pre-declared generated input",
execRelativePath));
}
+
inputsBuilder.add(inputArtifact);
}
updateInputs(inputsBuilder.build());
@@ -180,7 +203,7 @@
@Override
public boolean discoversInputs() {
- return depFile != null;
+ return depFile != null || !orderOnlyInputs.isEmpty();
}
@Override
@@ -190,7 +213,12 @@
@Override
public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) {
- updateInputs(getInputs());
- return getInputs();
+ NestedSet<Artifact> inputs =
+ NestedSetBuilder.<Artifact>stableOrder()
+ .addTransitive(getInputs())
+ .addTransitive(orderOnlyInputs)
+ .build();
+ updateInputs(inputs);
+ return inputs;
}
}
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 4945775..f83a22d 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
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.bazel.rules.ninja.file.GenericParsingException;
import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaRuleVariable;
import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaTarget;
+import com.google.devtools.build.lib.bazel.rules.ninja.parser.NinjaTarget.InputKind;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.TargetUtils;
@@ -40,6 +41,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
import javax.annotation.Nullable;
@@ -157,8 +159,10 @@
private void createNinjaAction(NinjaTarget target) throws GenericParsingException {
NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
+ NestedSetBuilder<Artifact> orderOnlyInputsBuilder = NestedSetBuilder.stableOrder();
ImmutableList.Builder<Artifact> outputsBuilder = ImmutableList.builder();
- boolean isAlwaysDirty = fillArtifacts(target, inputsBuilder, outputsBuilder);
+ boolean isAlwaysDirty =
+ fillArtifacts(target, inputsBuilder, orderOnlyInputsBuilder, outputsBuilder);
ImmutableSortedMap<NinjaRuleVariable, String> resolvedMap = target.computeRuleVariables();
String command = resolvedMap.get(NinjaRuleVariable.COMMAND);
@@ -182,6 +186,7 @@
artifactsHelper.getSourceRoot(),
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
inputsBuilder.build(),
+ orderOnlyInputsBuilder.build(),
outputs,
commandLines,
Preconditions.checkNotNull(ruleContext.getConfiguration()).getActionEnvironment(),
@@ -228,17 +233,25 @@
private boolean fillArtifacts(
NinjaTarget target,
NestedSetBuilder<Artifact> inputsBuilder,
+ NestedSetBuilder<Artifact> orderOnlyInputsBuilder,
ImmutableList.Builder<Artifact> outputsBuilder)
throws GenericParsingException {
+
boolean isAlwaysDirty = false;
- for (PathFragment input : target.getAllInputs()) {
+ for (Map.Entry<InputKind, PathFragment> entry : target.getAllInputsAndKind()) {
+
+ InputKind kind = entry.getKey();
+ boolean isOrderOnly = (kind == InputKind.ORDER_ONLY);
+ NestedSetBuilder<Artifact> builder = isOrderOnly ? orderOnlyInputsBuilder : inputsBuilder;
+
+ PathFragment input = entry.getValue();
PhonyTarget phonyTarget = this.phonyTargets.get(input);
if (phonyTarget != null) {
- inputsBuilder.addTransitive(phonyTargetArtifacts.getPhonyTargetArtifacts(input));
- isAlwaysDirty |= phonyTarget.isAlwaysDirty();
+ builder.addTransitive(phonyTargetArtifacts.getPhonyTargetArtifacts(input));
+ isAlwaysDirty |= (phonyTarget.isAlwaysDirty() && !isOrderOnly);
} else {
Artifact artifact = artifactsHelper.getInputArtifact(input);
- inputsBuilder.add(artifact);
+ builder.add(artifact);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaTarget.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaTarget.java
index 33956e6..b8a3a27 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/parser/NinjaTarget.java
@@ -193,7 +193,7 @@
*/
private final ImmutableSortedMap<NinjaRuleVariable, NinjaVariableValue> ruleVariables;
- public NinjaTarget(
+ private NinjaTarget(
String ruleName,
ImmutableSortedKeyListMultimap<InputKind, PathFragment> inputs,
ImmutableSortedKeyListMultimap<OutputKind, PathFragment> outputs,
@@ -226,6 +226,10 @@
return inputs.values();
}
+ public Collection<Map.Entry<InputKind, PathFragment>> getAllInputsAndKind() {
+ return inputs.entries();
+ }
+
public Collection<PathFragment> getExplicitInputs() {
return inputs.get(InputKind.EXPLICIT);
}
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 c427461..10abca8 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
@@ -608,4 +608,97 @@
assertThat(Files.exists(rspFile)).isTrue();
assertThat(Files.readAllLines(rspFile)).containsExactly("../input.txt");
}
+
+ @Test
+ public void testOrderOnlyInputsDoNotCauseRebuild() throws Exception {
+ context().write("input.txt", "abc");
+ context().write("order-only-input.txt", "123");
+ context()
+ .write(
+ "build_dir/build.ninja",
+ "rule cat",
+ " command = cat ${in} ${order_only_input} > ${out}",
+ "build output.txt: cat ../input.txt || ../order-only-input.txt",
+ " order_only_input = ../order-only-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 = {'main': ['output.txt']})");
+
+ BuilderRunner bazel = context().bazel().withFlags("--experimental_ninja_actions");
+
+ assertConfigured(bazel.build("//:ninja_target"));
+ Path outputPath = context().resolveExecRootPath(bazel, "build_dir/output.txt");
+ assertThat(outputPath.toFile().exists()).isTrue();
+ assertThat(Files.readAllLines(outputPath)).containsExactly("abc", "123");
+
+ // Change the order-only input...
+ context().write("order-only-input.txt", "456");
+ // ...but nothing should rerun....
+ assertNothingConfigured(bazel.build("//:ninja_target"));
+ // ...and the output should remain the same.
+ assertThat(Files.readAllLines(outputPath)).containsExactly("abc", "123");
+
+ // Change the main input...
+ context().write("input.txt", "xyz");
+ bazel.build("//:ninja_target");
+ // ...and the output should have the new content from the order-only input.
+ assertThat(Files.readAllLines(outputPath)).containsExactly("xyz", "456");
+ }
+
+ @Test
+ public void testOrderOnlyInputsInDepFileCauseRebuild() throws Exception {
+ context().write("build_dir/in.txt", "abc");
+ context().write("build_dir/order-only-input1.txt", "123");
+ context().write("build_dir/order-only-input2.txt", "456");
+ context()
+ .write(
+ "build_dir/build.ninja",
+ "rule cat",
+ " command = cat ${in} ${order_only_input1} ${order_only_input2} > ${out} ; "
+ // TODO(b/156961649): Having to put "build_dir/" in front of ${order_only_input2}
+ // might be a bug
+ + "echo \"${out} : build_dir/${order_only_input2}\" > ${depfile}",
+ " depfile = df",
+ "build output.txt: cat in.txt || order-only-input1.txt order-only-input2.txt",
+ " order_only_input1 = order-only-input1.txt",
+ " order_only_input2 = order-only-input2.txt");
+
+ context()
+ .write(
+ "BUILD",
+ "ninja_graph(name = 'graph', output_root = 'build_dir',",
+ " working_directory = 'build_dir',",
+ " main = 'build_dir/build.ninja',",
+ " output_root_inputs = ['in.txt', 'order-only-input1.txt', 'order-only-input2.txt'],)",
+ "ninja_build(name = 'ninja_target', ninja_graph = 'graph',",
+ " output_groups = {'main': ['output.txt']})");
+
+ BuilderRunner bazel = context().bazel().withFlags("--experimental_ninja_actions");
+
+ ProcessResult pr = bazel.build("//:ninja_target");
+ System.out.println(pr.outString());
+ assertConfigured(pr);
+ Path outputPath = context().resolveExecRootPath(bazel, "build_dir/output.txt");
+ assertThat(outputPath.toFile().exists()).isTrue();
+ assertThat(Files.readAllLines(outputPath)).containsExactly("abc", "123", "456");
+
+ // Change the order-only input...
+ context().write("build_dir/order-only-input1.txt", "789");
+ // ...but nothing should rerun....
+ assertNothingConfigured(bazel.build("//:ninja_target"));
+ // ...and the output should remain the same.
+ assertThat(Files.readAllLines(outputPath)).containsExactly("abc", "123", "456");
+
+ // But change the order-only input that's listed in the depfile...
+ context().write("build_dir/order-only-input2.txt", "xyz");
+ bazel.build("//:ninja_target");
+ // ...and the output should have the new content from both order-only inputs.
+ assertThat(Files.readAllLines(outputPath)).containsExactly("abc", "789", "xyz");
+ }
}
