Remove LLVM gtest dep and add pre-commit regression check. (#1040)

Co-authored-by: Geoff Romer <gromer@google.com>

.github/workflows/pre-commit.yaml

@@ -14,6 +14,8 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
+        with:
+          submodules: true
       - uses: actions/setup-python@v2
       - uses: pre-commit/action@v2.0.2
         env:

.pre-commit-config.yaml

@@ -67,6 +67,14 @@ repos:
     hooks:
       - id: prettier
   # Run linters last, as formatters and other checks may fix issues.
+  - repo: local
+    hooks:
+      - id: forbid-llvm-googletest
+        name: Checks for deps on LLVM's version of GoogleTest
+        entry: scripts/forbid_llvm_googletest.py
+        language: python
+        files: ^.*/BUILD$
+        pass_filenames: false
   - repo: https://github.com/PyCQA/flake8
     rev: cbeb4c9c4137cff1568659fcc48e8b85cddd0c8d # frozen: 4.0.1
     hooks:

common/BUILD

@@ -61,6 +61,5 @@ cc_test(
         ":string_helpers",
         "@com_google_googletest//:gtest_main",
         "@llvm-project//llvm:Support",
-        "@llvm-project//llvm:TestingSupport",
     ],
 )

common/string_helpers_test.cpp

@@ -10,10 +10,8 @@
 #include <string>
 
 #include "llvm/Support/Error.h"
-#include "llvm/Testing/Support/Error.h"
 
-using ::llvm::FailedWithMessage;
-using ::llvm::HasValue;
+using ::llvm::toString;
 using ::testing::Eq;
 using ::testing::Optional;
 
@@ -58,89 +56,91 @@ TEST(UnescapeStringLiteral, Nul) {
 }
 
 TEST(ParseBlockStringLiteral, FailTooFewLines) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(""),
-                       FailedWithMessage("Too few lines"));
+  EXPECT_THAT(toString(ParseBlockStringLiteral("").takeError()),
+              Eq("Too few lines"));
 }
 
 TEST(ParseBlockStringLiteral, FailNoLeadingTripleQuotes) {
-  EXPECT_THAT_EXPECTED(
-      ParseBlockStringLiteral("'a'\n"),
-      FailedWithMessage("Should start with triple quotes: 'a'"));
+  EXPECT_THAT(toString(ParseBlockStringLiteral("'a'\n").takeError()),
+              Eq("Should start with triple quotes: 'a'"));
 }
 
 TEST(ParseBlockStringLiteral, FailInvalideFiletypeIndicator) {
-  EXPECT_THAT_EXPECTED(
-      ParseBlockStringLiteral("\"\"\"carbon file\n"),
-      FailedWithMessage(
-          "Invalid characters in file type indicator: carbon file"));
+  EXPECT_THAT(
+      toString(ParseBlockStringLiteral("\"\"\"carbon file\n").takeError()),
+      Eq("Invalid characters in file type indicator: carbon file"));
 }
 
 TEST(ParseBlockStringLiteral, FailEndingTripleQuotes) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral("\"\"\"\n"),
-                       FailedWithMessage("Should end with triple quotes: "));
+  EXPECT_THAT(toString(ParseBlockStringLiteral("\"\"\"\n").takeError()),
+              Eq("Should end with triple quotes: "));
 }
 
 TEST(ParseBlockStringLiteral, FailWrongIndent) {
-  EXPECT_THAT_EXPECTED(
-      ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal
     with wrong indent
-     """)"),
-      FailedWithMessage(
-          "Wrong indent for line:     with wrong indent, expected 5"));
+     """)";
+  EXPECT_THAT(toString(ParseBlockStringLiteral(Input).takeError()),
+              Eq("Wrong indent for line:     with wrong indent, expected 5"));
 }
 
 TEST(ParseBlockStringLiteral, FailInvalidEscaping) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      \q
-     """)"),
-                       FailedWithMessage("Invalid escaping in \\q"));
+     """)";
+  EXPECT_THAT(toString(ParseBlockStringLiteral(Input).takeError()),
+              Eq("Invalid escaping in \\q"));
 }
 
 TEST(ParseBlockStringLiteral, OkEmptyString) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
-""")"),
-                       HasValue(""));
+  constexpr char Input[] = R"("""
+""")";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(""));
 }
 
 TEST(ParseBlockStringLiteral, OkOneLineString) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal
-     """)"),
-                       HasValue(R"(A block string literal
-)"));
+     """)";
+  constexpr char Expected[] = R"(A block string literal
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkTwoLineString) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal
        with indent.
-     """)"),
-                       HasValue(R"(A block string literal
+     """)";
+  constexpr char Expected[] = R"(A block string literal
   with indent.
-)"));
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWithFileTypeIndicator) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""carbon
+  constexpr char Input[] = R"("""carbon
      A block string literal
        with file type indicator.
-     """)"),
-                       HasValue(R"(A block string literal
+     """)";
+  constexpr char Expected[] = R"(A block string literal
   with file type indicator.
-)"));
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWhitespaceAfterOpeningQuotes) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal
-     """)"),
-                       HasValue(R"(A block string literal
-)"));
+     """)";
+  constexpr char Expected[] = R"(A block string literal
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWithEmptyLines) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal
 
        with
@@ -148,47 +148,52 @@ TEST(ParseBlockStringLiteral, OkWithEmptyLines) {
        empty
 
        lines.
-     """)"),
-                       HasValue(R"(A block string literal
+     """)";
+  constexpr char Expected[] = R"(A block string literal
 
   with
 
   empty
 
   lines.
-)"));
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWithSlashNewlineEscape) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal\
-     """)"),
-                       HasValue("A block string literal"));
+     """)";
+  constexpr char Expected[] = "A block string literal";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWithDoubleSlashNewline) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal\\
-     """)"),
-                       HasValue(R"(A block string literal\
-)"));
+     """)";
+  constexpr char Expected[] = R"(A block string literal\
+)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkWithTripleSlashNewline) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal\\\
-     """)"),
-                       HasValue(R"(A block string literal\)"));
+     """)";
+  constexpr char Expected[] = R"(A block string literal\)";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 TEST(ParseBlockStringLiteral, OkMultipleSlashes) {
-  EXPECT_THAT_EXPECTED(ParseBlockStringLiteral(R"("""
+  constexpr char Input[] = R"("""
      A block string literal\
      \
      \
      \
-     """)"),
-                       HasValue("A block string literal"));
+     """)";
+  constexpr char Expected[] = "A block string literal";
+  EXPECT_THAT(ParseBlockStringLiteral(Input).get(), Eq(Expected));
 }
 
 }  // namespace

scripts/forbid_llvm_googletest.py

@@ -0,0 +1,77 @@
+#!/usr/bin/env python3
+
+"""Detects and prevents dependencies on LLVM's googletest.
+
+Carbon uses googletest directly, and it's a significantly more recent version
+than is provided by LLVM. Using both versions in the same binary leads to
+problems, so this detects dependencies.
+
+We also have some dependency checking at //bazel/check_deps. This is a separate
+script because check_deps relies on being able to validate specific binaries
+which change infrequently, whereas this effectively monitors all cc_test rules,
+the set of which is expected to be altered more often.
+"""
+
+__copyright__ = """
+Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+Exceptions. See /LICENSE for license information.
+SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+"""
+
+import os
+import shutil
+import subprocess
+from pathlib import Path
+
+_MESSAGE = """\
+Dependencies on @llvm-project//llvm:gtest are forbidden, but a dependency path
+was detected:
+
+%s
+Carbon uses GoogleTest through @com_google_googletest, which is a different
+version than LLVM uses at @llvm-project//llvm:gtest. As a consequence,
+dependencies on @llvm-project//llvm:gtest must be avoided.
+"""
+
+
+def locate_bazel() -> str:
+    """Returns the bazel command.
+
+    We use the `BAZEL` environment variable if present. If not, then we try to
+    use `bazelisk` and then `bazel`.
+    """
+    bazel = os.environ.get("BAZEL")
+    if bazel:
+        return bazel
+
+    if shutil.which("bazelisk"):
+        return "bazelisk"
+
+    if shutil.which("bazel"):
+        return "bazel"
+
+    exit("Unable to run Bazel")
+
+
+def main() -> None:
+    # Change the working directory to the repository root so that the remaining
+    # operations reliably operate relative to that root.
+    os.chdir(Path(__file__).parent.parent)
+    args = [
+        locate_bazel(),
+        "query",
+        "somepath(//..., @llvm-project//llvm:gtest)",
+    ]
+    p = subprocess.run(
+        args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
+    )
+    if p.returncode != 0:
+        print(p.stderr)
+        exit(f"bazel query returned {p.returncode}")
+    if p.stdout:
+        exit(_MESSAGE % p.stdout)
+    print("Done!")
+
+
+if __name__ == "__main__":
+    main()