Do not add .inputdeps paths to --ninja-outputs-file
This fixes a nasty bug where the content of --ninja-outputs-file
would mistakenly include the sources of copy() targets, as well as
intermediate `.inputdeps` Ninja targets created by the NinjaCopyTargetWriter.
This only happened when there were more than one generated source
listed in the copy() target's 'sources' argument.
The CL fixes the issue and adds a unit-test to ensure this does not
regress in the future.
Bug: 448860851
Change-Id: I195c2cc4b8b0ec5ef3902eaf6d831b220fd3c52c
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19960
Reviewed-by: Takuto Ikuta <tikuta@google.com>
Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/ninja_copy_target_writer_unittest.cc b/src/gn/ninja_copy_target_writer_unittest.cc
index 1b5077a..e094f80 100644
--- a/src/gn/ninja_copy_target_writer_unittest.cc
+++ b/src/gn/ninja_copy_target_writer_unittest.cc
@@ -6,6 +6,7 @@
#include <sstream>
#include "gn/ninja_copy_target_writer.h"
+#include "gn/substitution_list.h"
#include "gn/target.h"
#include "gn/test_with_scope.h"
#include "util/test/test.h"
@@ -124,3 +125,101 @@
std::string out_str = out.str();
EXPECT_EQ(expected_linux, out_str);
}
+
+TEST(NinjaCopyTargetWriter, NoSourcesInOutputs) {
+ Err err;
+ TestWithScope setup;
+ setup.build_settings()->set_no_stamp_files(true);
+
+ // First with a single action / output / copy
+ {
+ Target action1(setup.settings(), Label(SourceDir("//foo/"), "action1"));
+ action1.set_output_type(Target::ACTION);
+ action1.visibility().SetPublic();
+ action1.SetToolchain(setup.toolchain());
+ action1.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/action1.out");
+ ASSERT_TRUE(action1.OnResolved(&err));
+
+ Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
+ target.set_output_type(Target::COPY_FILES);
+ target.sources().push_back(
+ action1.computed_outputs()[0].AsSourceFile(setup.build_settings()));
+ target.SetToolchain(setup.toolchain());
+ target.private_deps().push_back(LabelTargetPair(&action1));
+ target.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.copy");
+ ASSERT_TRUE(target.OnResolved(&err));
+
+ std::ostringstream out;
+ std::vector<OutputFile> ninja_outputs;
+ NinjaCopyTargetWriter writer(&target, out);
+ writer.SetNinjaOutputs(&ninja_outputs);
+ writer.Run();
+
+ const char expected_linux[] =
+ "build action1.copy: copy action1.out || phony/foo/action1\n"
+ "\n"
+ "build phony/foo/bar: phony action1.copy\n";
+ std::string out_str = out.str();
+ EXPECT_EQ(expected_linux, out_str);
+
+ EXPECT_EQ(2u, ninja_outputs.size());
+ EXPECT_EQ(ninja_outputs[0].value(), "action1.copy");
+ EXPECT_EQ(ninja_outputs[1].value(), "phony/foo/bar");
+ }
+
+ // Second, with two actions / outputs / copies, which is what trigerred
+ // the bug in https://gn.issues.chromium.org/448860851
+ {
+ Target action1(setup.settings(), Label(SourceDir("//foo/"), "action1"));
+ action1.set_output_type(Target::ACTION);
+ action1.visibility().SetPublic();
+ action1.SetToolchain(setup.toolchain());
+ action1.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/action1.out");
+ ASSERT_TRUE(action1.OnResolved(&err));
+
+ Target action2(setup.settings(), Label(SourceDir("//foo/"), "action2"));
+ action2.set_output_type(Target::ACTION);
+ action2.visibility().SetPublic();
+ action2.SetToolchain(setup.toolchain());
+ action2.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/action2.out");
+ ASSERT_TRUE(action2.OnResolved(&err));
+
+ Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
+ target.set_output_type(Target::COPY_FILES);
+ target.sources().push_back(
+ action1.computed_outputs()[0].AsSourceFile(setup.build_settings()));
+ target.sources().push_back(
+ action2.computed_outputs()[0].AsSourceFile(setup.build_settings()));
+ target.SetToolchain(setup.toolchain());
+ target.private_deps().push_back(LabelTargetPair(&action1));
+ target.private_deps().push_back(LabelTargetPair(&action2));
+ target.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.copy");
+ ASSERT_TRUE(target.OnResolved(&err));
+
+ std::ostringstream out;
+ std::vector<OutputFile> ninja_outputs;
+ NinjaCopyTargetWriter writer(&target, out);
+ writer.SetNinjaOutputs(&ninja_outputs);
+ writer.Run();
+
+ const char expected_linux[] =
+ "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"
+ "\n"
+ "build phony/foo/bar: phony action1.copy action2.copy\n";
+ std::string out_str = out.str();
+ EXPECT_EQ(expected_linux, out_str);
+
+ EXPECT_EQ(3u, ninja_outputs.size());
+ EXPECT_EQ(ninja_outputs[0].value(), "action1.copy");
+ EXPECT_EQ(ninja_outputs[1].value(), "action2.copy");
+ EXPECT_EQ(ninja_outputs[2].value(), "phony/foo/bar");
+ }
+}
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc
index 1bbe31e..e5248a1 100644
--- a/src/gn/ninja_target_writer.cc
+++ b/src/gn/ninja_target_writer.cc
@@ -571,10 +571,12 @@
GeneralTool::kGeneralToolStamp;
}
+ // These are not real outputs, so do not use WriteOutput() here.
+ // See https://gn.issues.chromium.org/448860851.
out_ << "build ";
- WriteOutput(input_stamp_or_phony);
+ path_output_.WriteFile(out_, input_stamp_or_phony);
out_ << ": " << tool;
- WriteOutputs(outs);
+ path_output_.WriteFiles(out_, outs);
out_ << "\n";
return std::vector<OutputFile>{input_stamp_or_phony};
}
