Revert "Fix toolchain dependencies not triggering rebuilds in Ninja" This reverts commit 9ece3f5254c273cb46606a6571963f931c3b012d. Reason for revert: This doesn't pass chromium CQ. https://chromium-review.git.corp.google.com/c/chromium/src/+/7810380 Original change's description: > 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> # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 40643484 Change-Id: I8f38fc48dba7a214a6ef354d2550f91a842ca8c6 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/22520 Commit-Queue: Takuto Ikuta <tikuta@google.com> Reviewed-by: Junji Watanabe <jwata@google.com>
diff --git a/src/gn/ninja_action_target_writer.cc b/src/gn/ninja_action_target_writer.cc index 0d01d07..3aa5686 100644 --- a/src/gn/ninja_action_target_writer.cc +++ b/src/gn/ninja_action_target_writer.cc
@@ -73,11 +73,8 @@ // 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(); - NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> input_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 d6c2e56..4ba1733 100644 --- a/src/gn/ninja_bundle_data_target_writer.cc +++ b/src/gn/ninja_bundle_data_target_writer.cc
@@ -21,14 +21,11 @@ OutputFile(settings_->build_settings(), source_file)); } - 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> input_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<const Target*>(), /*num_output_uses=*/1); + output_files.insert(output_files.end(), input_deps.begin(), input_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 fdc02b1..5346e06 100644 --- a/src/gn/ninja_c_binary_target_writer.cc +++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -104,11 +104,8 @@ // 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. - NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> order_only_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 16fca06..0952c6f 100644 --- a/src/gn/ninja_copy_target_writer.cc +++ b/src/gn/ninja_copy_target_writer.cc
@@ -72,14 +72,13 @@ GetNinjaRulePrefixForToolchain(settings_) + GeneralTool::kGeneralToolCopy; size_t num_output_uses = target_->sources().size(); - NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> input_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()) - order_only_deps.push_back(data_dep->dependency_output()); + data_outs.push_back(data_dep->dependency_output()); } // Note that we don't write implicit deps for copy steps. "copy" only @@ -120,13 +119,10 @@ out_ << ": " << tool_name << " "; path_output_.WriteFile(out_, input_file); - if (!implicit_deps.empty()) { - out_ << " |"; - path_output_.WriteFiles(out_, implicit_deps); - } - if (!order_only_deps.empty()) { + if (!input_deps.empty() || !data_outs.empty()) { out_ << " ||"; - path_output_.WriteFiles(out_, order_only_deps); + path_output_.WriteFiles(out_, input_deps); + path_output_.WriteFiles(out_, data_outs); } 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 6fc5e43..da7da59 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 ca305e6..b3a15ba 100644 --- a/src/gn/ninja_create_bundle_target_writer.cc +++ b/src/gn/ninja_create_bundle_target_writer.cc
@@ -81,18 +81,16 @@ // Stamp users are CopyBundleData, CompileAssetsCatalog, PostProcessing and // StampForTarget. size_t num_stamp_uses = 4; - NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> order_only_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(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); + WriteCopyBundleDataSteps(order_only_deps, &output_files); + WriteCompileAssetsCatalogStep(order_only_deps, &output_files); + WritePostProcessingStep(post_processing_rule_name, order_only_deps, + &output_files); for (const Target* data_dep : resolved().GetDataDeps(target_)) { if (data_dep->has_dependency_output()) @@ -181,17 +179,14 @@ } 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, implicit_deps, order_only_deps, - output_files); + WriteCopyBundleFileRuleSteps(file_rule, 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" @@ -214,10 +209,6 @@ << 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); @@ -228,7 +219,6 @@ } void NinjaCreateBundleTargetWriter::WriteCompileAssetsCatalogStep( - const std::vector<OutputFile>& implicit_deps, const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files) { if (!TargetRequireAssetCatalogCompilation(target_)) @@ -261,10 +251,6 @@ 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); @@ -300,7 +286,6 @@ out_ << " | "; path_output_.WriteFile(out_, input_dep); - path_output_.WriteFiles(out_, implicit_deps); if (!order_only_deps.empty()) { out_ << " ||"; @@ -377,15 +362,13 @@ 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(implicit_deps, order_only_deps, - output_files); + WritePostProcessingInputDepsStampOrPhony(order_only_deps, output_files); DCHECK(!post_processing_input_stamp_file.value().empty()); out_ << "build"; @@ -408,7 +391,6 @@ 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; @@ -424,8 +406,7 @@ } DCHECK(!post_processing_input_files.empty()); - if (post_processing_input_files.size() == 1 && implicit_deps.empty() && - order_only_deps.empty()) + if (post_processing_input_files.size() == 1 && order_only_deps.empty()) return OutputFile(settings_->build_settings(), post_processing_input_files[0]); @@ -457,10 +438,6 @@ 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 7d603a0..6b5d6cc 100644 --- a/src/gn/ninja_create_bundle_target_writer.h +++ b/src/gn/ninja_create_bundle_target_writer.h
@@ -30,8 +30,7 @@ // 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>& implicit_deps, - const std::vector<OutputFile>& order_only_deps, + void WriteCopyBundleDataSteps(const std::vector<OutputFile>& order_only_deps, std::vector<OutputFile>* output_files); // Writes the step to copy files BundleFileRule into the bundle. @@ -39,7 +38,6 @@ // 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); @@ -47,7 +45,6 @@ // // 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); @@ -62,14 +59,12 @@ // 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 7aa1313..3dfd6a3 100644 --- a/src/gn/ninja_create_bundle_target_writer_unittest.cc +++ b/src/gn/ninja_create_bundle_target_writer_unittest.cc
@@ -75,15 +75,16 @@ 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\n" + "bar.bundle/Contents/Resources/input2.txt" + " || phony/baz/bar.inputdeps\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -123,15 +124,16 @@ 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\n" + "gen/bar.bundle/Contents/Resources/input2.txt || " + "phony/baz/bar.inputdeps\n" "build gen/bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -227,14 +229,16 @@ 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\n" + "bar.bundle/Contents/Resources/Assets.car || " + "phony/baz/bar.inputdeps\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -389,22 +393,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" @@ -413,7 +417,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\n" + "baz/bar/bar_partial_info.plist || phony/baz/bar.inputdeps\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -471,7 +475,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" @@ -479,21 +483,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\n" + "bar.bundle/_CodeSignature/CodeResources || phony/baz/bar.inputdeps\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -554,7 +558,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" @@ -564,22 +568,23 @@ "\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\n" + "bar.bundle/_CodeSignature/CodeResources || " + "toolchain/phony/baz/bar.inputdeps\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 b77157a..e9b44e8 100644 --- a/src/gn/ninja_rust_binary_target_writer.cc +++ b/src/gn/ninja_rust_binary_target_writer.cc
@@ -125,9 +125,8 @@ // 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. - NinjaTargetWriter::InputDeps stamp_deps = WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> order_only_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)); @@ -136,7 +135,6 @@ // -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 affc7d6..9b45628 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -453,8 +453,7 @@ } } -NinjaTargetWriter::InputDeps -NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( +std::vector<OutputFile> NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( const std::vector<const Target*>& additional_hard_deps, size_t num_output_uses) const { CHECK(target_->toolchain()) << "Toolchain not set on target " @@ -520,62 +519,55 @@ // 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. - toolchain_deps_targets.push_back(toolchain_dep.ptr); + input_deps_targets.push_back(toolchain_dep.ptr); } // --------- // Write the outputs. - if (input_deps_sources.empty() && input_deps_targets.empty() && - toolchain_deps_targets.empty()) - return InputDeps{}; // No input dependencies. + if (input_deps_sources.size() + input_deps_targets.size() == 0) + return std::vector<OutputFile>(); // No input dependencies. - InputDeps deps; + // 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; // File input deps. for (const SourceFile* source : input_deps_sources) - deps.order_only.push_back(OutputFile(settings_->build_settings(), *source)); + outs.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). - 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); + 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()); + } - // 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 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; - // 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(); + // If outs is empty, we don't need stamp or phony target for this. + if (outs.empty()) { + return outs; } OutputFile input_stamp_or_phony; @@ -604,16 +596,9 @@ out_ << "build "; path_output_.WriteFile(out_, input_stamp_or_phony); out_ << ": " << tool; - path_output_.WriteFiles(out_, deps.implicit); - if (!deps.order_only.empty()) { - out_ << " ||"; - path_output_.WriteFiles(out_, deps.order_only); - } + path_output_.WriteFiles(out_, outs); out_ << "\n"; - - InputDeps result; - result.implicit.push_back(input_stamp_or_phony); - return result; + return std::vector<OutputFile>{input_stamp_or_phony}; } void NinjaTargetWriter::WriteStampOrPhonyForTarget(
diff --git a/src/gn/ninja_target_writer.h b/src/gn/ninja_target_writer.h index 2da7e76..0936033 100644 --- a/src/gn/ninja_target_writer.h +++ b/src/gn/ninja_target_writer.h
@@ -82,19 +82,14 @@ 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 - // dependencies for the current target. + // order-only 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 struct. - InputDeps WriteInputDepsStampOrPhonyAndGetDep( + // are passed in, this returns an empty vector. + std::vector<OutputFile> 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 cd4a5e4..317ce2a 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. - NinjaTargetWriter::InputDeps WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> WriteInputDepsStampOrPhonyAndGetDep( const std::vector<const Target*>& additional_hard_deps, size_t num_stamp_uses) { return NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep( @@ -119,14 +119,13 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&base_target, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> 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(0u, dep.implicit.size()); - ASSERT_EQ(1u, dep.order_only.size()); - EXPECT_EQ("../../foo/script.py", dep.order_only[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("../../foo/script.py", dep[0].value()); EXPECT_EQ("", stream.str()); } @@ -134,14 +133,13 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> 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(0u, dep.implicit.size()); - ASSERT_EQ(1u, dep.order_only.size()); - EXPECT_EQ("phony/foo/base", dep.order_only[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("phony/foo/base", dep[0].value()); } { @@ -166,12 +164,11 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&action, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); - ASSERT_EQ(1u, dep.implicit.size()); - ASSERT_EQ(0u, dep.order_only.size()); - EXPECT_EQ("phony/foo/action.inputdeps", dep.implicit[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("phony/foo/action.inputdeps", dep[0].value()); EXPECT_EQ( "build phony/foo/action.inputdeps: phony ../../foo/script.py " "../../foo/action_source.txt ./target\n", @@ -219,14 +216,13 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&base_target, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> 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(0u, dep.implicit.size()); - ASSERT_EQ(1u, dep.order_only.size()); - EXPECT_EQ("../../foo/script.py", dep.order_only[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("../../foo/script.py", dep[0].value()); EXPECT_EQ("", stream.str()); } @@ -234,14 +230,13 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> 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(0u, dep.implicit.size()); - ASSERT_EQ(1u, dep.order_only.size()); - EXPECT_EQ("obj/foo/base.stamp", dep.order_only[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("obj/foo/base.stamp", dep[0].value()); } { @@ -266,12 +261,11 @@ { std::ostringstream stream; TestingNinjaTargetWriter writer(&action, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> dep = writer.WriteInputDepsStampOrPhonyAndGetDep( std::vector<const Target*>(), 10u); - ASSERT_EQ(1u, dep.implicit.size()); - ASSERT_EQ(0u, dep.order_only.size()); - EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep.implicit[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep[0].value()); EXPECT_EQ( "build obj/foo/action.inputdeps.stamp: stamp ../../foo/script.py " "../../foo/action_source.txt ./target\n", @@ -303,60 +297,16 @@ std::ostringstream stream; TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream); - auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<OutputFile> 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.implicit.size()); - ASSERT_EQ(0u, dep.order_only.size()); - EXPECT_EQ("phony/foo/setup", dep.implicit[0].value()); + ASSERT_EQ(1u, dep.size()); + EXPECT_EQ("phony/foo/setup", dep[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) {