Fix various issues with `gn analyze`.

I had the name of one of the input fields wrong, and I had
an error case not returning output correctly.

R=brettw@chromium.org
BUG=555273

Review-Url: https://codereview.chromium.org/2341683006
Cr-Commit-Position: refs/heads/master@{#419075}

tools/gn/analyzer.cc

@@ -157,7 +157,7 @@ Err JSONToInputs(const Label& default_toolchain,
     inputs->source_vec.push_back(SourceFile(s));
   }
 
-  strings = GetStringVector(*dict, "compile_targets", &err);
+  strings = GetStringVector(*dict, "additional_compile_targets", &err);
   if (err.has_error())
     return err;
 
@@ -193,7 +193,7 @@ Err JSONToInputs(const Label& default_toolchain,
 }
 
 std::string OutputsToJSON(const Outputs& outputs,
-                          const Label& default_toolchain) {
+                          const Label& default_toolchain, Err *err) {
   std::string output;
   auto value = base::MakeUnique<base::DictionaryValue>();
 
@@ -208,7 +208,8 @@ std::string OutputsToJSON(const Outputs& outputs,
     WriteLabels(default_toolchain, *value, "test_targets", outputs.test_labels);
   }
 
-  base::JSONWriter::Write(*value.get(), &output);
+  if (!base::JSONWriter::Write(*value.get(), &output))
+    *err = Err(Location(), "Failed to marshal JSON value for output");
   return output;
 }
 
@@ -237,9 +238,7 @@ std::string Analyzer::Analyze(const std::string& input, Err* err) const {
   Err local_err = JSONToInputs(default_toolchain_, input, &inputs);
   if (local_err.has_error()) {
     outputs.error = local_err.message();
-    if (err)
-      *err = Err();
-    return "";
+    return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   LabelSet invalid_labels;
@@ -250,17 +249,13 @@ std::string Analyzer::Analyze(const std::string& input, Err* err) const {
   if (!invalid_labels.empty()) {
     outputs.error = "Invalid targets";
     outputs.invalid_labels = invalid_labels;
-    if (err)
-      *err = Err();
-    return OutputsToJSON(outputs, default_toolchain_);
+    return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   TargetSet affected_targets = AllAffectedTargets(inputs.source_files);
   if (affected_targets.empty()) {
     outputs.status = "No dependency";
-    if (err)
-      *err = Err();
-    return OutputsToJSON(outputs, default_toolchain_);
+    return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   // TODO: We can do smarter things when we detect changes to build files.
@@ -272,9 +267,7 @@ std::string Analyzer::Analyze(const std::string& input, Err* err) const {
     outputs.status = "Found dependency (all)";
     outputs.compile_labels = inputs.compile_labels;
     outputs.test_labels = inputs.test_labels;
-    if (err)
-      *err = Err();
-    return OutputsToJSON(outputs, default_toolchain_);
+    return OutputsToJSON(outputs, default_toolchain_, err);
   }
 
   TargetSet compile_targets = TargetsFor(inputs.compile_labels);
@@ -293,8 +286,7 @@ std::string Analyzer::Analyze(const std::string& input, Err* err) const {
     outputs.status = "No dependency";
   else
     outputs.status = "Found dependency";
-  *err = Err();
-  return OutputsToJSON(outputs, default_toolchain_);
+  return OutputsToJSON(outputs, default_toolchain_, err);
 }
 
 TargetSet Analyzer::AllAffectedTargets(

tools/gn/analyzer_unittest.cc

@@ -120,7 +120,7 @@ TEST_F(AnalyzerTest, AllWasPruned) {
   RunBasicTest(
       "{"
       "  \"files\": [ \"//d/b.cc\" ],"
-      "  \"compile_targets\": [ \"all\" ],"
+      "  \"additional_compile_targets\": [ \"all\" ],"
       "  \"test_targets\": [ ]"
       "}",
       "{"
@@ -134,7 +134,7 @@ TEST_F(AnalyzerTest, NoDependency) {
   RunBasicTest(
       "{"
       "  \"files\":[ \"//missing.cc\" ],"
-      "  \"compile_targets\": [ \"all\" ],"
+      "  \"additional_compile_targets\": [ \"all\" ],"
       "  \"test_targets\": [ \"//:a\" ]"
       "}",
       "{"
@@ -148,7 +148,7 @@ TEST_F(AnalyzerTest, NoFilesNoTargets) {
   RunBasicTest(
       "{"
       "  \"files\": [],"
-      "  \"compile_targets\": [],"
+      "  \"additional_compile_targets\": [],"
       "  \"test_targets\": []"
       "}",
       "{"
@@ -162,7 +162,7 @@ TEST_F(AnalyzerTest, OneTestTargetModified) {
   RunBasicTest(
       "{"
       "  \"files\": [ \"//a.cc\" ],"
-      "  \"compile_targets\": [],"
+      "  \"additional_compile_targets\": [],"
       "  \"test_targets\": [ \"//:a\" ]"
       "}",
       "{"
@@ -171,3 +171,32 @@ TEST_F(AnalyzerTest, OneTestTargetModified) {
       "\"test_targets\":[\"//:a\"]"
       "}");
 }
+
+TEST_F(AnalyzerTest, FilesArentSourceAbsolute) {
+  RunBasicTest(
+      "{"
+      "  \"files\": [ \"a.cc\" ],"
+      "  \"additional_compile_targets\": [],"
+      "  \"test_targets\": [ \"//:a\" ]"
+      "}",
+      "{"
+      "\"error\":"
+      "\"\\\"a.cc\\\" is not a source-absolute or absolute path.\","
+      "\"invalid_targets\":[]"
+      "}");
+}
+
+TEST_F(AnalyzerTest, WrongInputFields) {
+  RunBasicTest(
+      "{"
+      "  \"files\": [ \"//a.cc\" ],"
+      "  \"compile_targets\": [],"
+      "  \"test_targets\": [ \"//:a\" ]"
+      "}",
+      "{"
+      "\"error\":"
+      "\"Input does not have a key named "
+      "\\\"additional_compile_targets\\\" with a list value.\","
+      "\"invalid_targets\":[]"
+      "}");
+}

tools/gn/command_analyze.cc

@@ -34,27 +34,26 @@ const char kAnalyze_Help[] =
     "\n"
     "   - \"files\": A list of the filenames to check.\n"
     "\n"
-    "   - \"compile_targets\": A list of the labels targets that we wish to\n"
-    "     rebuild, but aren't necessarily needed for testing. The\n"
-    "     important difference between this field and \"test_targets\"\n"
-    "     is that if an item in the compile_targets list is a group, then\n"
+    "   - \"test_targets\": A list of the labels for targets that\n"
+    "     are needed to run the tests we wish to run.\n"
+    "\n"
+    "   - \"additional_compile_targets\": A list of the labels for\n"
+    "     targets that we wish to rebuild, but aren't necessarily needed\n"
+    "     for testing. The important difference between this field and\n"
+    "     \"test_targets\" is that if an item in the\n"
+    "     additional_compile_targets list refers to a group, then\n"
     "     any dependencies of that group will be returned if they are out\n"
     "     of date, but the group itself does not need to be. If the\n"
     "     dependencies themselves are groups, the same filtering is\n"
     "     repeated. This filtering can be used to avoid rebuilding\n"
     "     dependencies of a group that are unaffected by the input files.\n"
-    "     The list may contain the string \"all\" to refer to a\n"
-    "     pseudo-group that contains every root target in the build graph.\n"
+    "     The list may also contain the string \"all\" to refer to a\n"
+    "     pseudo-group that contains every root target in the build\n"
+    "     graph.\n"
     "\n"
     "     This filtering behavior is also known as \"pruning\" the list\n"
     "     of compile targets.\n"
     "\n"
-    "   - \"test_targets\": A list of the labels for targets that\n"
-    "     are needed to run the tests we wish to run. Unlike\n"
-    "     \"compile_targets\", this list may not contain the string \"all\",\n"
-    "     because having a test be dependent on everything in the build\n"
-    "     would be silly.\n"
-    "\n"
     "  output_path is a path indicating where the results of the command\n"
     "  are to be written. The results will be a file containing a JSON\n"
     "  object with one or more of following fields:\n"

tools/gn/docs/reference.md

@@ -259,27 +259,26 @@
 
    - "files": A list of the filenames to check.
 
-   - "compile_targets": A list of the labels targets that we wish to
-     rebuild, but aren't necessarily needed for testing. The
-     important difference between this field and "test_targets"
-     is that if an item in the compile_targets list is a group, then
+   - "test_targets": A list of the labels for targets that
+     are needed to run the tests we wish to run.
+
+   - "additional_compile_targets": A list of the labels for
+     targets that we wish to rebuild, but aren't necessarily needed
+     for testing. The important difference between this field and
+     "test_targets" is that if an item in the
+     additional_compile_targets list refers to a group, then
      any dependencies of that group will be returned if they are out
      of date, but the group itself does not need to be. If the
      dependencies themselves are groups, the same filtering is
      repeated. This filtering can be used to avoid rebuilding
      dependencies of a group that are unaffected by the input files.
-     The list may contain the string "all" to refer to a
-     pseudo-group that contains every root target in the build graph.
+     The list may also contain the string "all" to refer to a
+     pseudo-group that contains every root target in the build
+     graph.
 
      This filtering behavior is also known as "pruning" the list
      of compile targets.
 
-   - "test_targets": A list of the labels for targets that
-     are needed to run the tests we wish to run. Unlike
-     "compile_targets", this list may not contain the string "all",
-     because having a test be dependent on everything in the build
-     would be silly.
-
   output_path is a path indicating where the results of the command
   are to be written. The results will be a file containing a JSON
   object with one or more of following fields: