[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)