Make data_deps in copy() targets work.
Looks like copy targets were never updated for data_deps.
Without this change, having a copy() target have data_deps on
another target and building the copy() target didn't build the
data_dep. For all other target types, it does build the dep,
so this looks like it's unintentional.
See https://gn-review.googlesource.com/c/gn/+/8861/ for a
larger, standalone repro.
Change-Id: I866bbb162619c57636907af13f34213949a54882
Bug: none
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/8860
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
diff --git a/src/gn/ninja_copy_target_writer.cc b/src/gn/ninja_copy_target_writer.cc
index f350086..9299223 100644
--- a/src/gn/ninja_copy_target_writer.cc
+++ b/src/gn/ninja_copy_target_writer.cc
@@ -73,13 +73,17 @@
std::vector<OutputFile> input_deps = WriteInputDepsStampAndGetDep(
std::vector<const Target*>(), num_stamp_uses);
+ std::vector<OutputFile> data_outs;
+ for (const auto& dep : target_->data_deps())
+ data_outs.push_back(dep.ptr->dependency_output_file());
+
// Note that we don't write implicit deps for copy steps. "copy" only
// depends on the output files themselves, rather than having includes
// (the possibility of generated #includes is the main reason for implicit
// dependencies).
//
// It would seem that specifying implicit dependencies on the deps of the
- // copy command would still be harmeless. But Chrome implements copy tools
+ // copy command would still be harmless. But Chrome implements copy tools
// as hard links (much faster) which don't change the timestamp. If the
// ninja rule looks like this:
// output: copy input | foo.stamp
@@ -97,8 +101,9 @@
//
// Note that there is the need in some cases for order-only dependencies
// where a command might need to make sure something else runs before it runs
- // to avoid conflicts. Such cases should be avoided where possible, but
- // sometimes that's not possible.
+ // to avoid conflicts. This is also needed for data_deps on a copy target.
+ // Such cases should be avoided where possible, but sometimes that's not
+ // possible.
for (const auto& input_file : target_->sources()) {
OutputFile output_file =
SubstitutionWriter::ApplyPatternToSourceAsOutputFile(
@@ -109,9 +114,10 @@
path_output_.WriteFile(out_, output_file);
out_ << ": " << tool_name << " ";
path_output_.WriteFile(out_, input_file);
- if (!input_deps.empty()) {
+ if (!input_deps.empty() || !data_outs.empty()) {
out_ << " ||";
path_output_.WriteFiles(out_, input_deps);
+ path_output_.WriteFiles(out_, data_outs);
}
out_ << std::endl;
}
diff --git a/src/gn/ninja_copy_target_writer_unittest.cc b/src/gn/ninja_copy_target_writer_unittest.cc
index 0b4caea..f641ffa 100644
--- a/src/gn/ninja_copy_target_writer_unittest.cc
+++ b/src/gn/ninja_copy_target_writer_unittest.cc
@@ -92,3 +92,35 @@
std::string out_str = out.str();
EXPECT_EQ(expected_linux, out_str);
}
+
+TEST(NinjaCopyTargetWriter, DataDeps) {
+ Err err;
+ TestWithScope setup;
+
+ Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
+ target.set_output_type(Target::COPY_FILES);
+ target.sources().push_back(SourceFile("//foo/input1.txt"));
+ target.action_values().outputs() =
+ SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.out");
+
+ Target data_dep(setup.settings(), Label(SourceDir("//foo/"), "datadep"));
+ data_dep.set_output_type(Target::ACTION);
+ data_dep.visibility().SetPublic();
+ data_dep.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(data_dep.OnResolved(&err));
+
+ target.data_deps().push_back(LabelTargetPair(&data_dep));
+ target.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(target.OnResolved(&err));
+
+ std::ostringstream out;
+ NinjaCopyTargetWriter writer(&target, out);
+ writer.Run();
+
+ const char expected_linux[] =
+ "build input1.out: copy ../../foo/input1.txt || obj/foo/datadep.stamp\n"
+ "\n"
+ "build obj/foo/bar.stamp: stamp input1.out\n";
+ std::string out_str = out.str();
+ EXPECT_EQ(expected_linux, out_str);
+}
