🌱 GN: Look at all target outputs when detecting duplicates

tools/gn/ninja_build_writer_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <sstream>
+
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "tools/gn/ninja_build_writer.h"
+#include "tools/gn/scheduler.h"
+#include "tools/gn/target.h"
+#include "tools/gn/test_with_scope.h"
+
+namespace {
+
+
+}  // namespace
+
+TEST(NinjaBuildWriter, TwoTargets) {
+  Scheduler scheduler;
+  TestWithScope setup;
+  Err err;
+
+  Target target_foo(setup.settings(), Label(SourceDir("//foo/"), "bar"));
+  target_foo.set_output_type(Target::ACTION);
+  target_foo.action_values().set_script(SourceFile("//foo/script.py"));
+  target_foo.action_values().outputs() = SubstitutionList::MakeForTest(
+      "//out/Debug/out1.out", "//out/Debug/out2.out");
+  target_foo.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(target_foo.OnResolved(&err));
+
+  Target target_bar(setup.settings(), Label(SourceDir("//bar/"), "bar"));
+  target_bar.set_output_type(Target::ACTION);
+  target_bar.action_values().set_script(SourceFile("//bar/script.py"));
+  target_bar.action_values().outputs() = SubstitutionList::MakeForTest(
+      "//out/Debug/out3.out", "//out/Debug/out4.out");
+  target_bar.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(target_bar.OnResolved(&err));
+
+  std::ostringstream ninja_out;
+  std::ostringstream depfile_out;
+  std::vector<const Settings*> all_settings = {setup.settings()};
+  std::vector<const Target*> targets = {&target_foo, &target_bar};
+  NinjaBuildWriter writer(setup.build_settings(), all_settings,
+                          setup.toolchain(), targets, ninja_out, depfile_out);
+  ASSERT_TRUE(writer.Run(&err));
+
+  const char expected_rule_gn[] = "rule gn\n";
+  const char expected_build_ninja[] =
+      "build build.ninja: gn\n"
+      "  generator = 1\n"
+      "  depfile = build.ninja.d\n"
+      "\n";
+  const char expected_link_pool[] =
+      "pool link_pool\n"
+      "  depth = 0\n"
+      "\n";
+  const char expected_toolchain[] =
+      "subninja toolchain.ninja\n"
+      "\n";
+  const char expected_targets[] =
+      "build foo$:bar: phony obj/foo/bar.stamp\n"
+      "build bar$:bar: phony obj/bar/bar.stamp\n"
+      "build bar: phony obj/bar/bar.stamp\n"
+      "\n";
+  const char expected_root_target[] =
+      "build all: phony obj/foo/bar.stamp $\n"
+      "    obj/bar/bar.stamp\n"
+      "default all\n";
+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_rule_gn));

[brettw, 2016/03/25 22:43:10]

I'm not a fan of gmock and haven't used it in the GN unit tests so far. I think
this would be better written as

std::string out_str = ninja_out.str();
EXPECT_NE(out_str.end(), out_str.find(expected_rule_gn));
...

<begin rant to the universe>

Your code creates a PolymorphicMatches<HasSubstrMatcher> which is a functor
factory thing that creates a MonomorphicMatcher<std::string> which forwards to a
HasSubstrMatcher<std::string> which executes find != end. This is all sort of
readable until it goes wrong and then you're totally screwed with 1000 lines of
template errors and are unable to step into anything in a debugger if it fails.

Also, while it's easy to read this gmock line like English, even expert C++
programmers that don't happen to know GMock well couldn't even describe what
this test is actually doing. My suggestion is slightly less semantically less
readable but is 100% clear to any C++ programmer and easy to debug if necessary.

[agrieve, 2016/03/29 15:59:33]

Thanks! Helpful rant actually. I agree!

+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_build_ninja));
+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_link_pool));
+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_toolchain));
+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_targets));
+  EXPECT_THAT(ninja_out.str(), testing::HasSubstr(expected_root_target));
+}
+
+TEST(NinjaBuildWriter, DuplicateOutputs) {
+  Scheduler scheduler;
+  TestWithScope setup;
+  Err err;
+
+  //setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
+
+  Target target_foo(setup.settings(), Label(SourceDir("//foo/"), "bar"));
+  target_foo.set_output_type(Target::ACTION);
+  target_foo.action_values().set_script(SourceFile("//foo/script.py"));
+  target_foo.action_values().outputs() = SubstitutionList::MakeForTest(
+      "//out/Debug/out1.out", "//out/Debug/out2.out");
+  target_foo.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(target_foo.OnResolved(&err));
+
+  Target target_bar(setup.settings(), Label(SourceDir("//bar/"), "bar"));
+  target_bar.set_output_type(Target::ACTION);
+  target_bar.action_values().set_script(SourceFile("//bar/script.py"));
+  target_bar.action_values().outputs() = SubstitutionList::MakeForTest(
+      "//out/Debug/out3.out", "//out/Debug/out2.out");
+  target_bar.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(target_bar.OnResolved(&err));
+
+  std::ostringstream ninja_out;
+  std::ostringstream depfile_out;
+  std::vector<const Settings*> all_settings = { setup.settings() };
+  std::vector<const Target*> targets = { &target_foo, &target_bar };
+  NinjaBuildWriter writer(setup.build_settings(), all_settings,
+                          setup.toolchain(), targets, ninja_out, depfile_out);
+  ASSERT_FALSE(writer.Run(&err));
+
+  const char expected_help_test[] =
+      "Two or more targets generate the same output:\n"
+      "  out2.out\n"
+      "\n"
+      "This is can often be fixed by changing one of the target names, or by \n"
+      "setting an output_name on one of them.\n"
+      "\n"
+      "Collisions:\n"
+      "  //foo:bar\n"
+      "  //bar:bar\n";
+
+  EXPECT_EQ(expected_help_test, err.help_text());
+}