[action][data deps] Make data-deps order-only deps of the action outputs
This change makes action() data_deps behave in the same manner as
executable() data_deps: They are order-only deps of the outputs of the
action() itself, and not order-only deps of the stampfile for the
action.
After this change, when build the output of an action:
ninja path/to/action/output
the data_deps will also be verified as up-to-date, and built if
necessary.
Before this change, the data_deps for the action() would not have been
verified as up-to-date.
Change-Id: I30e9aff8af60df233294ed40f119889af5ee0ad5
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/15720
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: Aaron Wood <aaronwood@google.com>
diff --git a/src/gn/ninja_action_target_writer.cc b/src/gn/ninja_action_target_writer.cc
index 336a9f7..d6d3aa8 100644
--- a/src/gn/ninja_action_target_writer.cc
+++ b/src/gn/ninja_action_target_writer.cc
@@ -35,17 +35,34 @@
// operating on the result of that previous step, so we need to be sure to
// serialize these.
std::vector<const Target*> additional_hard_deps;
- std::vector<OutputFile> data_outs;
+ std::vector<OutputFile> order_only_deps;
const auto& target_deps = resolved().GetTargetDeps(target_);
for (const Target* dep : target_deps.linked_deps()) {
if (dep->IsDataOnly()) {
- data_outs.push_back(dep->dependency_output_file());
+ order_only_deps.push_back(dep->dependency_output_file());
} else {
additional_hard_deps.push_back(dep);
}
}
+ // Add all data-deps to the order-only-deps for the action. The data_deps
+ // field is used to implement different use-cases, including:
+ //
+ // - Files needed at only runtime by the outputs of the action, and therefore
+ // need be built if ninja is building the action's outputs. But they do
+ // not "dirty" the action's outputs if the data_deps alone are "dirty".
+ // If ninja had the concept of "weak" dependencies, that would be used
+ // instead, but that isn't available, so order-only dependencies are used.
+ //
+ // - Files that _may_ need to be used to perform the action, and a depfile
+ // will be used to promote these order-only deps to implicit dependencies,
+ // and on an incremental build, if the now-implicit dependencies are
+ // 'dirty', this action will be considered 'dirty' as well.
+ //
+ for (const Target* data_dep : target_deps.data_deps())
+ order_only_deps.push_back(data_dep->dependency_output_file());
+
// For ACTIONs, the input deps appear only once in the generated ninja
// file, so WriteInputDepsStampAndGetDep() won't create a stamp file
// and the action will just depend on all the input deps directly.
@@ -60,7 +77,8 @@
if (target_->output_type() == Target::ACTION_FOREACH) {
// Write separate build lines for each input source file.
- WriteSourceRules(custom_rule_name, input_deps, &output_files);
+ WriteSourceRules(custom_rule_name, input_deps, order_only_deps,
+ &output_files);
} else {
DCHECK(target_->output_type() == Target::ACTION);
@@ -78,6 +96,13 @@
out_ << " |";
path_output_.WriteFiles(out_, input_deps);
}
+ if (!order_only_deps.empty()) {
+ // Write any order-only deps out for actions just like they are for
+ // binaries.
+ out_ << " ||";
+ path_output_.WriteFiles(out_, order_only_deps);
+ }
+
out_ << std::endl;
if (target_->action_values().has_depfile()) {
WriteDepfile(SourceFile());
@@ -94,14 +119,13 @@
}
out_ << std::endl;
- // Write the stamp, which also depends on all data deps. These are needed at
- // runtime and should be compiled when the action is, but don't need to be
- // done before we run the action.
+ // Write the stamp, which doesn't need to depend on the data deps because they
+ // have been added as order-only deps of the action output itself.
+ //
// TODO(thakis): If the action has just a single output, make things depend
// on that output directly without writing a stamp file.
- for (const Target* data_dep : target_deps.data_deps())
- data_outs.push_back(data_dep->dependency_output_file());
- WriteStampForTarget(output_files, data_outs);
+ std::vector<OutputFile> stamp_file_order_only_deps;
+ WriteStampForTarget(output_files, stamp_file_order_only_deps);
}
std::string NinjaActionTargetWriter::WriteRuleDefinition() {
@@ -178,6 +202,7 @@
void NinjaActionTargetWriter::WriteSourceRules(
const std::string& custom_rule_name,
const std::vector<OutputFile>& input_deps,
+ const std::vector<OutputFile>& order_only_deps,
std::vector<OutputFile>* output_files) {
EscapeOptions args_escape_options;
args_escape_options.mode = ESCAPE_NINJA_COMMAND;
@@ -200,6 +225,12 @@
out_ << " |";
path_output_.WriteFiles(out_, input_deps);
}
+ if (!order_only_deps.empty()) {
+ // Write any order-only deps out for actions just like they are written
+ // out for binaries.
+ out_ << " ||";
+ path_output_.WriteFiles(out_, order_only_deps);
+ }
out_ << std::endl;
// Response files require a unique name be defined.
diff --git a/src/gn/ninja_action_target_writer.h b/src/gn/ninja_action_target_writer.h
index 58b9489..eff087b 100644
--- a/src/gn/ninja_action_target_writer.h
+++ b/src/gn/ninja_action_target_writer.h
@@ -40,6 +40,7 @@
// input_deps are the dependencies common to all build steps.
void WriteSourceRules(const std::string& custom_rule_name,
const std::vector<OutputFile>& input_deps,
+ const std::vector<OutputFile>& order_only_deps,
std::vector<OutputFile>* output_files);
// Writes the output files generated by the output template for the given
diff --git a/src/gn/ninja_action_target_writer_unittest.cc b/src/gn/ninja_action_target_writer_unittest.cc
index d061e76..aafc779 100644
--- a/src/gn/ninja_action_target_writer_unittest.cc
+++ b/src/gn/ninja_action_target_writer_unittest.cc
@@ -209,13 +209,12 @@
" restat = 1\n"
"\n"
"build foo.out: __foo_bar___rule | ../../foo/script.py "
- "../../foo/included.txt ../../foo/source.txt obj/foo/dep.stamp\n"
+ "../../foo/included.txt ../../foo/source.txt obj/foo/dep.stamp || "
+ "obj/foo/datadep.stamp\n"
"\n"
- "build obj/foo/bar.stamp: stamp foo.out || "
- "obj/foo/datadep.stamp\n";
+ "build obj/foo/bar.stamp: stamp foo.out\n";
- std::string out_str = out.str();
- EXPECT_EQ(expected, out_str);
+ EXPECT_EQ(expected, out.str());
}
@@ -290,15 +289,16 @@
"../../foo/included.txt obj/foo/dep.stamp\n"
"\n"
"build input1.out: __foo_bar___rule ../../foo/input1.txt | "
- "obj/foo/bar.inputdeps.stamp\n"
+ "obj/foo/bar.inputdeps.stamp || obj/foo/bundle_data_dep.stamp "
+ "obj/foo/datadep.stamp\n"
" source_name_part = input1\n"
"build input2.out: __foo_bar___rule ../../foo/input2.txt | "
- "obj/foo/bar.inputdeps.stamp\n"
+ "obj/foo/bar.inputdeps.stamp || obj/foo/bundle_data_dep.stamp "
+ "obj/foo/datadep.stamp\n"
" source_name_part = input2\n"
"\n"
"build obj/foo/bar.stamp: "
- "stamp input1.out input2.out || obj/foo/bundle_data_dep.stamp "
- "obj/foo/datadep.stamp\n";
+ "stamp input1.out input2.out\n";
std::string out_str = out.str();
#if defined(OS_WIN)
