commit 3e35519a885a6793f7655d993e7e728a437f3709
author dpranke <dpranke@chromium.org> Mon Sep 19 19:52:56 2016 -0700
committer Commit bot <commit-bot@chromium.org> Tue Sep 20 02:55:30 2016 +0000
tree 1a65e33c5334d95978734da23a814760bf867b11
parent 0fc3da1228cae5a180d6eea3503287f97a6fe6c6

Fix a bug in `gn analyze` related to build-only changes.

`gn analyze` would incorrectly claim that no compile was necessary
if given a list of files that included build file changes but not
any source file changes.

R=brettw@chromium.org
BUG=648448

Review-Url: https://codereview.chromium.org/2355713002
Cr-Original-Commit-Position: refs/heads/master@{#419662}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 253b1190a67b4ccac5c8b2583e6b19c6a1c5e3f9

tools/gn/analyzer.cc

diff --git a/tools/gn/analyzer.cc b/tools/gn/analyzer.cc
index 8ee303f..f89c93d 100644
--- a/tools/gn/analyzer.cc
+++ b/tools/gn/analyzer.cc
@@ -252,17 +252,11 @@
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
-  TargetSet affected_targets = AllAffectedTargets(inputs.source_files);
-  if (affected_targets.empty()) {
-    outputs.status = "No dependency";
-    return OutputsToJSON(outputs, default_toolchain_, err);
-  }
-
-  // TODO: We can do smarter things when we detect changes to build files.
-  // For example, if all of the ninja files are unchanged, we know that we
-  // can ignore changes to these files. Also, for most .gn files, we can
-  // treat a change as simply affecting every target, config, or toolchain
-  // defined in that file.
+  // TODO(crbug.com/555273): We can do smarter things when we detect changes
+  // to build files. For example, if all of the ninja files are unchanged,
+  // we know that we can ignore changes to these files. Also, for most .gn
+  // files, we can treat a change as simply affecting every target, config,
+  // or toolchain defined in that file.
   if (AnyBuildFilesWereModified(inputs.source_files)) {
     outputs.status = "Found dependency (all)";
     outputs.compile_labels = inputs.compile_labels;
@@ -270,6 +264,12 @@
     return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
+  TargetSet affected_targets = AllAffectedTargets(inputs.source_files);
+  if (affected_targets.empty()) {
+    outputs.status = "No dependency";
+    return OutputsToJSON(outputs, default_toolchain_, err);
+  }
+
   TargetSet compile_targets = TargetsFor(inputs.compile_labels);
   if (inputs.compile_included_all) {
     for (auto& root : roots_)

tools/gn/analyzer_unittest.cc

diff --git a/tools/gn/analyzer_unittest.cc b/tools/gn/analyzer_unittest.cc
index dba0e77..4d5c8d4 100644
--- a/tools/gn/analyzer_unittest.cc
+++ b/tools/gn/analyzer_unittest.cc
@@ -200,3 +200,20 @@
       "\"invalid_targets\":[]"
       "}");
 }
+
+TEST_F(AnalyzerTest, BuildFilesWereModified) {
+  // This tests that if a build file is modified, we bail out early with
+  // "Found dependency (all)" error since we can't handle changes to
+  // build files yet (crbug.com/555273).
+  RunBasicTest(
+      "{"
+      "  \"files\": [ \"//a.cc\", \"//BUILD.gn\" ],"
+      "  \"additional_compile_targets\": [],"
+      "  \"test_targets\": [ \"//:a\" ]"
+      "}",
+      "{"
+      "\"compile_targets\":[],"
+      "\"status\":\"Found dependency (all)\","
+      "\"test_targets\":[\"//:a\"]"
+      "}");
+}