Fix toolchain dependencies not triggering rebuilds in Ninja GN previously emitted order-only dependencies on toolchain dependencies (via the .inputdeps.stamp file). This meant that if the toolchain changed, targets compiled by it were not rebuilt. This change modifies WriteInputDepsStampOrPhonyAndGetDep (WIDSOPAGD) to return implicit dependencies separately from order-only dependencies. Toolchain dependencies are now returned as implicit dependencies, which ensures that Ninja triggers a rebuild when the toolchain changes. All target writers have been updated to write the result of WIDSOPAGD as either implicit or order-only dependencies. The action target writer is a special case, because before this change it would write the result of WIDSOPAGD as an implicit dependency, unlike all other target writers, which would write it as an order-only dependency. This means that before this change, all dependencies returned by WIDSOPAGD were implicit dependencies for action targets. Because scripts may depend on this behavior, any phony targets created by WIDSOPAGD for action targets have only implicit dependencies. Bug: 40643484 Change-Id: Iae5dbf87cd67ecd5c9d0f6138f17085233c844b3 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/22000 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Peter Collingbourne <pcc@google.com>
diff --git a/src/gn/ninja_action_target_writer.cc b/src/gn/ninja_action_target_writer.cc index 3aa5686..0d01d07 100644 --- a/src/gn/ninja_action_target_writer.cc +++ b/src/gn/ninja_action_target_writer.cc
@@ -73,8 +73,11 @@ // and the action will just depend on all the input deps directly. size_t num_output_uses = target_->output_type() == Target::ACTION ? 1u : target_->sources().size(); - std::vector<OutputFile> input_deps = WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( additional_hard_deps, num_output_uses); + std::vector<OutputFile> input_deps = stamp_deps.implicit; + input_deps.insert(input_deps.end(), stamp_deps.order_only.begin(), + stamp_deps.order_only.end()); out_ << std::endl; // Collects all output files for writing below.
diff --git a/src/gn/ninja_bundle_data_target_writer.cc b/src/gn/ninja_bundle_data_target_writer.cc index 4ba1733..d6c2e56 100644 --- a/src/gn/ninja_bundle_data_target_writer.cc +++ b/src/gn/ninja_bundle_data_target_writer.cc
@@ -21,11 +21,14 @@ OutputFile(settings_->build_settings(), source_file)); } - std::vector<OutputFile> input_deps = WriteInputDepsStampOrPhonyAndGetDep( - std::vector<const Target*>(), /*num_output_uses=*/1); - output_files.insert(output_files.end(), input_deps.begin(), input_deps.end()); + NinjaTargetWriter::InputDeps stamp_deps = + WriteInputDepsStampOrPhonyAndGetDep(std::vector<const Target*>(), + /*num_output_uses=*/1); + std::vector<OutputFile> implicit_deps = stamp_deps.implicit; + std::vector<OutputFile> order_only_deps = stamp_deps.order_only; + output_files.insert(output_files.end(), implicit_deps.begin(), + implicit_deps.end()); - std::vector<OutputFile> order_only_deps; for (const Target* data_dep : resolved().GetDataDeps(target_)) { if (data_dep->has_dependency_output()) order_only_deps.push_back(data_dep->dependency_output());
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc index 5346e06..fdc02b1 100644 --- a/src/gn/ninja_c_binary_target_writer.cc +++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -104,8 +104,11 @@ // that also use PCH files won't have a phony target even though having // one would make output ninja file size a bit lower. That's ok, binary // targets with a single source are rare. - std::vector<OutputFile> order_only_deps = WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), num_output_uses); + input_deps.insert(input_deps.end(), stamp_deps.implicit.begin(), + stamp_deps.implicit.end()); + std::vector<OutputFile> order_only_deps = stamp_deps.order_only; // For GCC builds, the .gch files are not object files, but still need to be // added as explicit dependencies below. The .gch output files are placed in
diff --git a/src/gn/ninja_copy_target_writer.cc b/src/gn/ninja_copy_target_writer.cc index 0952c6f..16fca06 100644 --- a/src/gn/ninja_copy_target_writer.cc +++ b/src/gn/ninja_copy_target_writer.cc
@@ -72,13 +72,14 @@ GetNinjaRulePrefixForToolchain(settings_) + GeneralTool::kGeneralToolCopy; size_t num_output_uses = target_->sources().size(); - std::vector<OutputFile> input_deps = WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), num_output_uses); + std::vector<OutputFile> implicit_deps = stamp_deps.implicit; + std::vector<OutputFile> order_only_deps = stamp_deps.order_only; - std::vector<OutputFile> data_outs; for (const Target* data_dep : resolved().GetDataDeps(target_)) { if (data_dep->has_dependency_output()) - data_outs.push_back(data_dep->dependency_output()); + order_only_deps.push_back(data_dep->dependency_output()); } // Note that we don't write implicit deps for copy steps. "copy" only @@ -119,10 +120,13 @@ out_ << ": " << tool_name << " "; path_output_.WriteFile(out_, input_file); - if (!input_deps.empty() || !data_outs.empty()) { + if (!implicit_deps.empty()) { + out_ << " |"; + path_output_.WriteFiles(out_, implicit_deps); + } + if (!order_only_deps.empty()) { out_ << " ||"; - path_output_.WriteFiles(out_, input_deps); - path_output_.WriteFiles(out_, data_outs); + path_output_.WriteFiles(out_, order_only_deps); } WriteValidations(); out_ << std::endl;
diff --git a/src/gn/ninja_copy_target_writer_unittest.cc b/src/gn/ninja_copy_target_writer_unittest.cc index da7da59..6fc5e43 100644 --- a/src/gn/ninja_copy_target_writer_unittest.cc +++ b/src/gn/ninja_copy_target_writer_unittest.cc
@@ -208,10 +208,10 @@ writer.Run(); const char expected_linux[] = - "build phony/foo/bar.inputdeps: phony phony/foo/action1 " + "build phony/foo/bar.inputdeps: phony || phony/foo/action1 " "phony/foo/action2\n" - "build action1.copy: copy action1.out || phony/foo/bar.inputdeps\n" - "build action2.copy: copy action2.out || phony/foo/bar.inputdeps\n" + "build action1.copy: copy action1.out | phony/foo/bar.inputdeps\n" + "build action2.copy: copy action2.out | phony/foo/bar.inputdeps\n" "\n" "build phony/foo/bar: phony action1.copy action2.copy\n"; std::string out_str = out.str();
diff --git a/src/gn/ninja_create_bundle_target_writer.cc b/src/gn/ninja_create_bundle_target_writer.cc index b3a15ba..ca305e6 100644 --- a/src/gn/ninja_create_bundle_target_writer.cc +++ b/src/gn/ninja_create_bundle_target_writer.cc
@@ -81,16 +81,18 @@ // Stamp users are CopyBundleData, CompileAssetsCatalog, PostProcessing and // StampForTarget. size_t num_stamp_uses = 4; - std::vector<OutputFile> order_only_deps = WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), num_stamp_uses); + std::vector<OutputFile> implicit_deps = stamp_deps.implicit; + std::vector<OutputFile> order_only_deps = stamp_deps.order_only; std::string post_processing_rule_name = WritePostProcessingRuleDefinition(); std::vector<OutputFile> output_files; - WriteCopyBundleDataSteps(order_only_deps, &output_files); - WriteCompileAssetsCatalogStep(order_only_deps, &output_files); - WritePostProcessingStep(post_processing_rule_name, order_only_deps, - &output_files); + WriteCopyBundleDataSteps(implicit_deps, order_only_deps, &output_files); + WriteCompileAssetsCatalogStep(implicit_deps, order_only_deps, &output_files); + WritePostProcessingStep(post_processing_rule_name, implicit_deps, + order_only_deps, &output_files); for (const Target* data_dep : resolved().GetDataDeps(target_)) { if (data_dep->has_dependency_output()) @@ -179,14 +181,17 @@ } void NinjaCreateBundleTargetWriter::WriteCopyBundleDataSteps( + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { for (const BundleFileRule& file_rule : target_->bundle_data().file_rules()) - WriteCopyBundleFileRuleSteps(file_rule, order_only_deps, output_files); + WriteCopyBundleFileRuleSteps(file_rule, implicit_deps, order_only_deps, + output_files); } void NinjaCreateBundleTargetWriter::WriteCopyBundleFileRuleSteps( const BundleFileRule& file_rule, + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { // Note that we don't write implicit deps for copy steps. "copy_bundle_data" @@ -209,6 +214,10 @@ << GeneralTool::kGeneralToolCopyBundleData << " "; path_output_.WriteFile(out_, source_file); + if (!implicit_deps.empty()) { + out_ << " |"; + path_output_.WriteFiles(out_, implicit_deps); + } if (!order_only_deps.empty()) { out_ << " ||"; path_output_.WriteFiles(out_, order_only_deps); @@ -219,6 +228,7 @@ } void NinjaCreateBundleTargetWriter::WriteCompileAssetsCatalogStep( + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { if (!TargetRequireAssetCatalogCompilation(target_)) @@ -251,6 +261,10 @@ WriteOutput(partial_info_plist); out_ << ": " << GetNinjaRulePrefixForToolchain(settings_) << GeneralTool::kGeneralToolStamp; + if (!implicit_deps.empty()) { + out_ << " |"; + path_output_.WriteFiles(out_, implicit_deps); + } if (!order_only_deps.empty()) { out_ << " ||"; path_output_.WriteFiles(out_, order_only_deps); @@ -286,6 +300,7 @@ out_ << " | "; path_output_.WriteFile(out_, input_dep); + path_output_.WriteFiles(out_, implicit_deps); if (!order_only_deps.empty()) { out_ << " ||"; @@ -362,13 +377,15 @@ void NinjaCreateBundleTargetWriter::WritePostProcessingStep( const std::string& post_processing_rule_name, + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { if (post_processing_rule_name.empty()) return; OutputFile post_processing_input_stamp_file = - WritePostProcessingInputDepsStampOrPhony(order_only_deps, output_files); + WritePostProcessingInputDepsStampOrPhony(implicit_deps, order_only_deps, + output_files); DCHECK(!post_processing_input_stamp_file.value().empty()); out_ << "build"; @@ -391,6 +408,7 @@ OutputFile NinjaCreateBundleTargetWriter::WritePostProcessingInputDepsStampOrPhony( + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { std::vector<SourceFile> post_processing_input_files; @@ -406,7 +424,8 @@ } DCHECK(!post_processing_input_files.empty()); - if (post_processing_input_files.size() == 1 && order_only_deps.empty()) + if (post_processing_input_files.size() == 1 && implicit_deps.empty() && + order_only_deps.empty()) return OutputFile(settings_->build_settings(), post_processing_input_files[0]); @@ -438,6 +457,10 @@ out_ << " "; path_output_.WriteFile(out_, source); } + if (!implicit_deps.empty()) { + out_ << " |"; + path_output_.WriteFiles(out_, implicit_deps); + } if (!order_only_deps.empty()) { out_ << " ||"; path_output_.WriteFiles(out_, order_only_deps);
diff --git a/src/gn/ninja_create_bundle_target_writer.h b/src/gn/ninja_create_bundle_target_writer.h index 6b5d6cc..7d603a0 100644 --- a/src/gn/ninja_create_bundle_target_writer.h +++ b/src/gn/ninja_create_bundle_target_writer.h
@@ -30,7 +30,8 @@ // Writes the steps to copy files into the bundle. // // The list of newly created files will be added to |output_files|. - void WriteCopyBundleDataSteps(const std::vector<OutputFile>& order_only_deps, + void WriteCopyBundleDataSteps(const std::vector<OutputFile>& implicit_deps, + const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files); // Writes the step to copy files BundleFileRule into the bundle. @@ -38,6 +39,7 @@ // The list of newly created files will be added to |output_files|. void WriteCopyBundleFileRuleSteps( const BundleFileRule& file_rule, + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files); @@ -45,6 +47,7 @@ // // The list of newly created files will be added to |output_files|. void WriteCompileAssetsCatalogStep( + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files); @@ -59,12 +62,14 @@ // post-processing may depends on the full bundle structure, this step will // depends on all files generated via other rules. void WritePostProcessingStep(const std::string& post_processing_rule_name, + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files); // Writes the stamp file or phony target for the post-processing input // dependencies. OutputFile WritePostProcessingInputDepsStampOrPhony( + const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files);
diff --git a/src/gn/ninja_create_bundle_target_writer_unittest.cc b/src/gn/ninja_create_bundle_target_writer_unittest.cc index 3dfd6a3..7aa1313 100644 --- a/src/gn/ninja_create_bundle_target_writer_unittest.cc +++ b/src/gn/ninja_create_bundle_target_writer_unittest.cc
@@ -75,16 +75,15 @@ writer.Run(); const char expected[] = - "build phony/baz/bar.inputdeps: phony phony/foo/bar " + "build phony/baz/bar.inputdeps: phony || phony/foo/bar " "phony/foo/data\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || phony/baz/bar.inputdeps\n" + "../../foo/input1.txt | phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || phony/baz/bar.inputdeps\n" + "../../foo/input2.txt | phony/baz/bar.inputdeps\n" "build phony/baz/bar: phony " "bar.bundle/Contents/Resources/input1.txt " - "bar.bundle/Contents/Resources/input2.txt" - " || phony/baz/bar.inputdeps\n" + "bar.bundle/Contents/Resources/input2.txt\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -124,16 +123,15 @@ writer.Run(); const char expected[] = - "build phony/baz/bar.inputdeps: phony phony/foo/bar " + "build phony/baz/bar.inputdeps: phony || phony/foo/bar " "phony/foo/data\n" "build gen/bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || phony/baz/bar.inputdeps\n" + "../../foo/input1.txt | phony/baz/bar.inputdeps\n" "build gen/bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || phony/baz/bar.inputdeps\n" + "../../foo/input2.txt | phony/baz/bar.inputdeps\n" "build phony/baz/bar: phony " "gen/bar.bundle/Contents/Resources/input1.txt " - "gen/bar.bundle/Contents/Resources/input2.txt || " - "phony/baz/bar.inputdeps\n" + "gen/bar.bundle/Contents/Resources/input2.txt\n" "build gen/bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -229,16 +227,14 @@ writer.Run(); const char expected[] = - "build phony/baz/bar.inputdeps: phony phony/foo/bar " + "build phony/baz/bar.inputdeps: phony || phony/foo/bar " "phony/foo/data\n" "build bar.bundle/Contents/Resources/Assets.car: compile_xcassets " - "../../foo/Foo.xcassets | phony/foo/data || " - "phony/baz/bar.inputdeps\n" + "../../foo/Foo.xcassets | phony/foo/data phony/baz/bar.inputdeps\n" " product_type = com.apple.product-type\n" " xcasset_compiler_flags = --app-icon foo\n" "build phony/baz/bar: phony " - "bar.bundle/Contents/Resources/Assets.car || " - "phony/baz/bar.inputdeps\n" + "bar.bundle/Contents/Resources/Assets.car\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -393,22 +389,22 @@ writer.Run(); const char expected[] = - "build phony/baz/bar.inputdeps: phony phony/biz/assets " + "build phony/baz/bar.inputdeps: phony || phony/biz/assets " "phony/foo/assets phony/foo/bar phony/foo/data " "phony/qux/info_plist phony/quz/assets\n" "build bar.bundle/Contents/Info.plist: copy_bundle_data " - "../../qux/qux-Info.plist || phony/baz/bar.inputdeps\n" + "../../qux/qux-Info.plist | phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || phony/baz/bar.inputdeps\n" + "../../foo/input1.txt | phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || phony/baz/bar.inputdeps\n" + "../../foo/input2.txt | phony/baz/bar.inputdeps\n" "build phony/baz/bar.xcassets.inputdeps: phony " "phony/foo/assets " "phony/quz/assets phony/biz/assets\n" "build bar.bundle/Contents/Resources/Assets.car | " "baz/bar/bar_partial_info.plist: compile_xcassets " "../../foo/Foo.xcassets ../../quz/Quz.xcassets " - "../../biz/Biz.xcassets | phony/baz/bar.xcassets.inputdeps || " + "../../biz/Biz.xcassets | phony/baz/bar.xcassets.inputdeps " "phony/baz/bar.inputdeps\n" " product_type = com.apple.product-type\n" " partial_info_plist = baz/bar/bar_partial_info.plist\n" @@ -417,7 +413,7 @@ "bar.bundle/Contents/Resources/input1.txt " "bar.bundle/Contents/Resources/input2.txt " "bar.bundle/Contents/Resources/Assets.car " - "baz/bar/bar_partial_info.plist || phony/baz/bar.inputdeps\n" + "baz/bar/bar_partial_info.plist\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -475,7 +471,7 @@ writer.Run(); const char expected[] = - "build phony/baz/bar.inputdeps: phony ./quz phony/foo/bar " + "build phony/baz/bar.inputdeps: phony || ./quz phony/foo/bar " "phony/foo/data\n" "rule __baz_bar___toolchain_default__post_processing_rule\n" " command = ../../build/codesign.py -b=quz bar.bundle\n" @@ -483,21 +479,21 @@ " restat = 1\n" "\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || phony/baz/bar.inputdeps\n" + "../../foo/input1.txt | phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || phony/baz/bar.inputdeps\n" + "../../foo/input2.txt | phony/baz/bar.inputdeps\n" "build phony/baz/bar.postprocessing.inputdeps: phony " "../../build/codesign.py " "quz " "bar.bundle/Contents/Resources/input1.txt " - "bar.bundle/Contents/Resources/input2.txt || " + "bar.bundle/Contents/Resources/input2.txt | " "phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/quz bar.bundle/_CodeSignature/CodeResources: " "__baz_bar___toolchain_default__post_processing_rule " "| phony/baz/bar.postprocessing.inputdeps\n" "build phony/baz/bar: phony " "bar.bundle/Contents/quz " - "bar.bundle/_CodeSignature/CodeResources || phony/baz/bar.inputdeps\n" + "bar.bundle/_CodeSignature/CodeResources\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -558,7 +554,7 @@ writer.Run(); const char expected[] = - "build toolchain/phony/baz/bar.inputdeps: phony ./quz " + "build toolchain/phony/baz/bar.inputdeps: phony || ./quz " "toolchain/phony/foo/bar " "toolchain/phony/foo/data\n" "rule __baz_bar___toolchain_default__post_processing_rule\n" @@ -568,23 +564,22 @@ "\n" "build bar.bundle/Contents/Resources/input1.txt: " "toolchain_copy_bundle_data " - "../../foo/input1.txt || toolchain/phony/baz/bar.inputdeps\n" + "../../foo/input1.txt | toolchain/phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/Resources/input2.txt: " "toolchain_copy_bundle_data " - "../../foo/input2.txt || toolchain/phony/baz/bar.inputdeps\n" + "../../foo/input2.txt | toolchain/phony/baz/bar.inputdeps\n" "build toolchain/phony/baz/bar.postprocessing.inputdeps: phony " "../../build/codesign.py " "quz " "bar.bundle/Contents/Resources/input1.txt " - "bar.bundle/Contents/Resources/input2.txt || " + "bar.bundle/Contents/Resources/input2.txt | " "toolchain/phony/baz/bar.inputdeps\n" "build bar.bundle/Contents/quz bar.bundle/_CodeSignature/CodeResources: " "__baz_bar___toolchain_default__post_processing_rule " "| toolchain/phony/baz/bar.postprocessing.inputdeps\n" "build toolchain/phony/baz/bar: phony " "bar.bundle/Contents/quz " - "bar.bundle/_CodeSignature/CodeResources || " - "toolchain/phony/baz/bar.inputdeps\n" + "bar.bundle/_CodeSignature/CodeResources\n" "build bar.bundle: phony toolchain/phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str);
diff --git a/src/gn/ninja_rust_binary_target_writer.cc b/src/gn/ninja_rust_binary_target_writer.cc index e9b44e8..b77157a 100644 --- a/src/gn/ninja_rust_binary_target_writer.cc +++ b/src/gn/ninja_rust_binary_target_writer.cc
@@ -125,8 +125,9 @@ // Ninja to make sure the inputs are up to date before compiling this source, // but changes in the inputs deps won't cause the file to be recompiled. See // the comment on NinjaCBinaryTargetWriter::Run for more detailed explanation. - std::vector<OutputFile> order_only_deps = WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), num_output_uses); + std::vector<OutputFile> order_only_deps = stamp_deps.order_only; std::copy(input_deps.begin(), input_deps.end(), std::back_inserter(order_only_deps)); @@ -135,6 +136,7 @@ // -Ldependency. Also assemble a list of extra (i.e. implicit) deps // for ninja dependency tracking. UniqueVector<OutputFile> implicit_deps; + implicit_deps.Append(stamp_deps.implicit.begin(), stamp_deps.implicit.end()); AppendSourcesAndInputsToImplicitDeps(&implicit_deps); implicit_deps.Append(classified_deps.extra_object_files.begin(), classified_deps.extra_object_files.end());
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc index 9b45628..affc7d6 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -453,7 +453,8 @@ } } -std::vector<OutputFile> NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( +NinjaTargetWriter::InputDeps +NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( const std::vector<const Target*>& additional_hard_deps, size_t num_output_uses) const { CHECK(target_->toolchain()) << "Toolchain not set on target " @@ -519,55 +520,62 @@ // toolchains often have more than one dependency, we could consider writing // a toolchain-specific phony target and only include the phony here. // Note that these are usually empty/small. + std::vector<const Target*> toolchain_deps_targets; const LabelTargetVector& toolchain_deps = target_->toolchain()->deps(); for (const auto& toolchain_dep : toolchain_deps) { // This could theoretically duplicate dependencies already in the list, // but it shouldn't happen in practice, is inconvenient to check for, // and only results in harmless redundant dependencies listed. - input_deps_targets.push_back(toolchain_dep.ptr); + toolchain_deps_targets.push_back(toolchain_dep.ptr); } // --------- // Write the outputs. - if (input_deps_sources.size() + input_deps_targets.size() == 0) - return std::vector<OutputFile>(); // No input dependencies. + if (input_deps_sources.empty() && input_deps_targets.empty() && + toolchain_deps_targets.empty()) + return InputDeps{}; // No input dependencies. - // If we're only generating one input dependency, return it directly instead - // of writing a phony target for it. - if (input_deps_sources.size() == 1 && input_deps_targets.size() == 0) - return std::vector<OutputFile>{ - OutputFile(settings_->build_settings(), *input_deps_sources[0])}; - if (input_deps_sources.size() == 0 && input_deps_targets.size() == 1) { - const auto& dep = *input_deps_targets[0]; - if (!dep.has_dependency_output()) - return std::vector<OutputFile>(); - return std::vector<OutputFile>{dep.dependency_output()}; - } - - std::vector<OutputFile> outs; + InputDeps deps; // File input deps. for (const SourceFile* source : input_deps_sources) - outs.push_back(OutputFile(settings_->build_settings(), *source)); + deps.order_only.push_back(OutputFile(settings_->build_settings(), *source)); // Target input deps. Sort by label so the output is deterministic (otherwise // some of the targets will have gone through std::sets which will have // sorted them by pointer). - std::sort( - input_deps_targets.begin(), input_deps_targets.end(), - [](const Target* a, const Target* b) { return a->label() < b->label(); }); - for (auto* dep : input_deps_targets) { - if (dep->has_dependency_output()) - outs.push_back(dep->dependency_output()); - } + auto add_target_deps = [](std::vector<OutputFile>& deps, + std::vector<const Target*>& targets) { + std::sort(targets.begin(), targets.end(), + [](const Target* a, const Target* b) { + return a->label() < b->label(); + }); + for (auto* dep : targets) { + if (dep->has_dependency_output()) + deps.push_back(dep->dependency_output()); + } + }; + add_target_deps(deps.order_only, input_deps_targets); + add_target_deps(deps.implicit, toolchain_deps_targets); - // If there are multiple inputs, but the phony target would be referenced only - // once, don't write it but depend on the inputs directly. - if (num_output_uses == 1u) - return outs; + // If we're only generating one input dependency, or if there are no + // dependencies, return it directly instead of writing a phony target for it. + // Also, if there are multiple inputs, but the phony target would be + // referenced only once, don't write it but depend on the inputs directly. + if (deps.implicit.size() + deps.order_only.size() <= 1 || + num_output_uses == 1u) + return deps; - // If outs is empty, we don't need stamp or phony target for this. - if (outs.empty()) { - return outs; + // Action targets are special because all of their dependencies are implicit + // dependencies. This is because prior to + // https://gn-review.googlesource.com/c/gn/+/22000 action targets had an + // implicit dependency on inputdeps whereas other target types had an + // order-only dependency on inputdeps (which at the time only had implicit + // dependencies), and scripts may be depending on that. + if (target_->output_type() == Target::ACTION || + target_->output_type() == Target::ACTION_FOREACH) { + deps.implicit.insert(deps.implicit.end(), deps.order_only.begin(), + deps.order_only.end()); + deps.order_only.clear(); } OutputFile input_stamp_or_phony; @@ -596,9 +604,16 @@ out_ << "build "; path_output_.WriteFile(out_, input_stamp_or_phony); out_ << ": " << tool; - path_output_.WriteFiles(out_, outs); + path_output_.WriteFiles(out_, deps.implicit); + if (!deps.order_only.empty()) { + out_ << " ||"; + path_output_.WriteFiles(out_, deps.order_only); + } out_ << "\n"; - return std::vector<OutputFile>{input_stamp_or_phony}; + + InputDeps result; + result.implicit.push_back(input_stamp_or_phony); + return result; } void NinjaTargetWriter::WriteStampOrPhonyForTarget(
diff --git a/src/gn/ninja_target_writer.h b/src/gn/ninja_target_writer.h index 0936033..2da7e76 100644 --- a/src/gn/ninja_target_writer.h +++ b/src/gn/ninja_target_writer.h
@@ -82,14 +82,19 @@ bool indent, bool always_write); + struct InputDeps { + std::vector<OutputFile> implicit; + std::vector<OutputFile> order_only; + }; + // Writes to the output stream a phony rule for input dependencies, and // returns the file to be appended to source rules that encodes the - // order-only dependencies for the current target. + // dependencies for the current target. // If num_output_uses is small, this might return all input dependencies // directly, without writing a phony rule. // If there are no implicit dependencies and no additional target dependencies - // are passed in, this returns an empty vector. - std::vector<OutputFile> WriteInputDepsStampOrPhonyAndGetDep( + // are passed in, this returns an empty struct. + InputDeps WriteInputDepsStampOrPhonyAndGetDep( const std::vector<const Target*>& additional_hard_deps, size_t num_output_uses) const;
diff --git a/src/gn/ninja_target_writer_unittest.cc b/src/gn/ninja_target_writer_unittest.cc index 317ce2a..cd4a5e4 100644 --- a/src/gn/ninja_target_writer_unittest.cc +++ b/src/gn/ninja_target_writer_unittest.cc
@@ -22,7 +22,7 @@ void Run() override {} // Make this public so the test can call it. - std::vector<OutputFile> WriteInputDepsStampOrPhonyAndGetDep( + NinjaTargetWriter::InputDeps WriteInputDepsStampOrPhonyAndGetDep( const std::vector<const Target*>& additional_hard_deps, size_t num_stamp_uses) { return NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( @@ -119,13 +119,14 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&base_target, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); // Since there is only one dependency, it should just be returned and // nothing written to the stream. - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("../../foo/script.py", dep[0].value()); + ASSERT_EQ(0u, dep.implicit.size()); + ASSERT_EQ(1u, dep.order_only.size()); + EXPECT_EQ("../../foo/script.py", dep.order_only[0].value()); EXPECT_EQ("", stream.str()); } @@ -133,13 +134,14 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); // Since there is only one dependency, a stamp file will be returned // directly without writing any additional rules. - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("phony/foo/base", dep[0].value()); + ASSERT_EQ(0u, dep.implicit.size()); + ASSERT_EQ(1u, dep.order_only.size()); + EXPECT_EQ("phony/foo/base", dep.order_only[0].value()); } { @@ -164,11 +166,12 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&action, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("phony/foo/action.inputdeps", dep[0].value()); + ASSERT_EQ(1u, dep.implicit.size()); + ASSERT_EQ(0u, dep.order_only.size()); + EXPECT_EQ("phony/foo/action.inputdeps", dep.implicit[0].value()); EXPECT_EQ( "build phony/foo/action.inputdeps: phony ../../foo/script.py " "../../foo/action_source.txt ./target\n", @@ -216,13 +219,14 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&base_target, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); // Since there is only one dependency, it should just be returned and // nothing written to the stream. - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("../../foo/script.py", dep[0].value()); + ASSERT_EQ(0u, dep.implicit.size()); + ASSERT_EQ(1u, dep.order_only.size()); + EXPECT_EQ("../../foo/script.py", dep.order_only[0].value()); EXPECT_EQ("", stream.str()); } @@ -230,13 +234,14 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); // Since there is only one dependency, a stamp file will be returned // directly without writing any additional rules. - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("obj/foo/base.stamp", dep[0].value()); + ASSERT_EQ(0u, dep.implicit.size()); + ASSERT_EQ(1u, dep.order_only.size()); + EXPECT_EQ("obj/foo/base.stamp", dep.order_only[0].value()); } { @@ -261,11 +266,12 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&action, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep[0].value()); + ASSERT_EQ(1u, dep.implicit.size()); + ASSERT_EQ(0u, dep.order_only.size()); + EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep.implicit[0].value()); EXPECT_EQ( "build obj/foo/action.inputdeps.stamp: stamp ../../foo/script.py " "../../foo/action_source.txt ./target\n", @@ -297,16 +303,60 @@ std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); // Since there is more than one dependency, a stamp file will be returned // and the rule for the stamp file will be written to the stream. - ASSERT_EQ(1u, dep.size()); - EXPECT_EQ("phony/foo/setup", dep[0].value()); + ASSERT_EQ(1u, dep.implicit.size()); + ASSERT_EQ(0u, dep.order_only.size()); + EXPECT_EQ("phony/foo/setup", dep.implicit[0].value()); EXPECT_EQ("", stream.str()); } +// Tests WriteInputDepsStampOrPhonyAndGetDep when a toolchain dependency and +// another dependency are present. +TEST(NinjaTargetWriter, + WriteInputDepsStampOrPhonyAndGetDepWithToolchainAndAnotherDep) { + TestWithScope setup; + Err err; + + Target toolchain_dep_target(setup.settings(), + Label(SourceDir("//foo/"), "setup")); + toolchain_dep_target.set_output_type(Target::ACTION); + toolchain_dep_target.SetToolchain(setup.toolchain()); + ASSERT_TRUE(toolchain_dep_target.OnResolved(&err)); + setup.toolchain()->deps().push_back(LabelTargetPair(&toolchain_dep_target)); + + Target base_target(setup.settings(), Label(SourceDir("//foo/"), "base")); + base_target.set_output_type(Target::ACTION); + base_target.visibility().SetPublic(); + base_target.SetToolchain(setup.toolchain()); + base_target.action_values().set_script(SourceFile("//foo/script.py")); + ASSERT_TRUE(base_target.OnResolved(&err)); + + Target target(setup.settings(), Label(SourceDir("//foo/"), "target")); + target.set_output_type(Target::EXECUTABLE); + target.SetToolchain(setup.toolchain()); + target.public_deps().push_back(LabelTargetPair(&base_target)); + ASSERT_TRUE(target.OnResolved(&err)); + + std::ostringstream stream; + TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<const Target*>(), 10u); + + // Since there is a toolchain dependency, it should be returned as an implicit + // dependency. + ASSERT_EQ(1u, dep.implicit.size()); + EXPECT_EQ("phony/foo/target.inputdeps", dep.implicit[0].value()); + ASSERT_EQ(0u, dep.order_only.size()); + EXPECT_EQ( + "build phony/foo/target.inputdeps: phony phony/foo/setup || " + "phony/foo/base\n", + stream.str()); +} + // Tests that validation dependencies are written to the generated Ninja file // with the "|@" syntax for a generic target. TEST(NinjaTargetWriter, WriteValidations) {