From 6a4901a995504018c6624c94e4a5ea12078782c3 Mon Sep 17 00:00:00 2001 From: Jon Meow <46229924+jonmeow@users.noreply.github.com> Date: Wed, 26 Jan 2022 09:28:45 -0800 Subject: [PATCH] Remove LLVM gtest dep and add pre-commit regression check. (#1040) Co-authored-by: Geoff Romer --- .github/workflows/pre-commit.yaml | 2 + .pre-commit-config.yaml | 8 ++ common/BUILD | 1 - common/string_helpers_test.cpp | 121 ++++++++++++++++-------------- scripts/forbid_llvm_googletest.py | 77 +++++++++++++++++++ 5 files changed, 150 insertions(+), 59 deletions(-) create mode 100755 scripts/forbid_llvm_googletest.py diff --git a/.github/workflows/pre-commit.yaml b/.github/workflows/pre-commit.yaml index def61b284430..3a06a20c5800 100644 --- a/.github/workflows/pre-commit.yaml +++ b/.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: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 09fc8fe925bd..c15c2349ef0e 100644 --- a/.pre-commit-config.yaml +++ b/.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: diff --git a/common/BUILD b/common/BUILD index 390a7f812e84..668ab957591d 100644 --- a/common/BUILD +++ b/common/BUILD @@ -61,6 +61,5 @@ cc_test( ":string_helpers", "@com_google_googletest//:gtest_main", "@llvm-project//llvm:Support", - "@llvm-project//llvm:TestingSupport", ], ) diff --git a/common/string_helpers_test.cpp b/common/string_helpers_test.cpp index 89c7a446f8c1..24b1bf7b7592 100644 --- a/common/string_helpers_test.cpp +++ b/common/string_helpers_test.cpp @@ -10,10 +10,8 @@ #include #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 diff --git a/scripts/forbid_llvm_googletest.py b/scripts/forbid_llvm_googletest.py new file mode 100755 index 000000000000..00cfeb39a033 --- /dev/null +++ b/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()