Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(31)

Unified Diff: tools/gn/ninja_copy_target_writer.cc

Issue 418573002: Don't add implicit deps to copy commands in GN. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Created 6 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « tools/gn/ninja_binary_target_writer_unittest.cc ('k') | tools/gn/ninja_copy_target_writer_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: tools/gn/ninja_copy_target_writer.cc
diff --git a/tools/gn/ninja_copy_target_writer.cc b/tools/gn/ninja_copy_target_writer.cc
index 3facd62fe9ab990098a6006d1e4ed46fb158b92b..40eeb49cf3e5ee2642c807d66818fb49c14f1146 100644
--- a/tools/gn/ninja_copy_target_writer.cc
+++ b/tools/gn/ninja_copy_target_writer.cc
@@ -25,9 +25,27 @@ void NinjaCopyTargetWriter::Run() {
std::string rule_prefix = helper_.GetRulePrefix(target_->settings());
- std::string implicit_deps =
- WriteInputDepsStampAndGetDep(std::vector<const Target*>());
-
+ // 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
+ // as hard links (much faster) which don't change the timestamp. If the
+ // ninja rule looks like this:
+ // output: copy input | foo.stamp
+ // The copy will not make a new timestamp on the output file, but the
+ // foo.stamp file generated from a previous step will have a new timestamp.
+ // The copy rule will therefore look out-of-date to Ninja and the rule will
+ // get rebuilt.
+ //
+ // If this copy is copying a generated file, not listing the implicit
+ // dependency will be fine as long as the input to the copy is properly
+ // listed as the output from the step that generated it.
+ //
+ // Moreover, doing this assumes that the copy step is always a simple
+ // locally run command, so there is no need for a toolchain dependency.
for (size_t i = 0; i < target_->sources().size(); i++) {
const SourceFile& input_file = target_->sources()[i];
@@ -47,7 +65,7 @@ void NinjaCopyTargetWriter::Run() {
path_output_.WriteFile(out_, output_file);
out_ << ": " << rule_prefix << "copy ";
path_output_.WriteFile(out_, input_file);
- out_ << implicit_deps << std::endl;
+ out_ << std::endl;
}
// Write out the rule for the target to copy all of them.
« no previous file with comments | « tools/gn/ninja_binary_target_writer_unittest.cc ('k') | tools/gn/ninja_copy_target_writer_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698