From 6fe84111221ed2649e69e1387096c94be35daa38 Mon Sep 17 00:00:00 2001 From: Jon Meow <46229924+jonmeow@users.noreply.github.com> Date: Tue, 22 Feb 2022 10:10:41 -0800 Subject: [PATCH] Refactor common script functionality and reimplement the buildifier pre-commit (#1080) Moves common script logic into utils.py (not a great name, but couldn't come up with better). This is in particular to make the buildifier.py script really trivial, allowing that pre-commit to be easily added. However, scripts have also been diverging on how we find bazel, so I'm trying to unify that. The advantage of reimplementing buildifier's pre-commit is that (a) we can now run buildifier server-side, and (b) we can stop advising installing it manually. Then the only Linux-specific package manager is Cargo, which is only used for watchman, which is optional -- so stop highlighting Linux-specific package managers in the tool instructions. --- .github/workflows/pre-commit.yaml | 3 - .pre-commit-config.yaml | 13 ++- docs/project/contribution_tools.md | 60 +---------- scripts/BUILD | 39 +++++++ scripts/create_compdb.py | 16 +-- scripts/fix_cc_deps.py | 114 ++------------------- scripts/forbid_llvm_googletest.py | 30 +----- scripts/run_buildifier.py | 26 +++++ scripts/scripts_utils.py | 157 +++++++++++++++++++++++++++++ 9 files changed, 252 insertions(+), 206 deletions(-) create mode 100755 scripts/run_buildifier.py create mode 100644 scripts/scripts_utils.py diff --git a/.github/workflows/pre-commit.yaml b/.github/workflows/pre-commit.yaml index c866dc32eac2..9aac6048d69d 100644 --- a/.github/workflows/pre-commit.yaml +++ b/.github/workflows/pre-commit.yaml @@ -16,6 +16,3 @@ jobs: - uses: actions/checkout@v2 - uses: actions/setup-python@v2 - uses: pre-commit/action@v2.0.2 - env: - # bazel-buildifier relies on a locally installed copy. - SKIP: bazel-buildifier diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 421d3e59cd94..bfa83d1158db 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -51,15 +51,12 @@ repos: language: python files: ^.*/(BUILD|[^/]+\.(h|cpp))$ pass_filenames: false + # Formatters should be run late so that they can re-format any prior changes. - repo: https://github.com/psf/black rev: fc0be6eb1e2a96091e6f64009ee5e9081bf8b6c6 # frozen: 22.1.0 hooks: - id: black - - repo: https://github.com/jlebar/pre-commit-hooks.git - rev: fb5e481f35f5a9b32a06ccfc28b8d6dda4525f75 - hooks: - - id: bazel-buildifier - repo: https://github.com/pre-commit/mirrors-clang-format rev: 1fc50313b6e8c2580c4736af57575e0b7de1501c # frozen: v13.0.0 hooks: @@ -68,6 +65,14 @@ repos: rev: ea782651a7e32f40a3d13b76c79d5a2474ee8723 # frozen: v2.5.1 hooks: - id: prettier + - repo: local + hooks: + - id: buildifier + name: Bazel buildifier + entry: scripts/run_buildifier.py + language: python + files: '^(.*/)?(BUILD\.bazel|BUILD|WORKSPACE)$|\.BUILD$|\.bzl$' + # Run linters last, as formatters and other checks may fix issues. - repo: local hooks: diff --git a/docs/project/contribution_tools.md b/docs/project/contribution_tools.md index a7d536ff25a5..1335f444a9dc 100644 --- a/docs/project/contribution_tools.md +++ b/docs/project/contribution_tools.md @@ -18,12 +18,8 @@ contributions. - [Linux and MacOS](#linux-and-macos) - [Homebrew](#homebrew) - [`python3` and `pip3`](#python3-and-pip3) - - [Linux only](#linux-only) - - [`go get`](#go-get) - - [Cargo (optional)](#cargo-optional) - [Main tools](#main-tools) - [Bazel and Bazelisk](#bazel-and-bazelisk) - - [buildifier](#buildifier) - [Clang and LLVM](#clang-and-llvm) - [Manual installations (not recommended)](#manual-installations-not-recommended) - [pre-commit](#pre-commit) @@ -111,38 +107,6 @@ periodically run `pip3 list --outdated`, then `pip3 install -U ` to upgrade desired packages. Keep in mind when upgrading that version dependencies may mean packages _should_ be outdated, and not be upgraded. -### Linux only - -Linux-specific package managers are typically used for packages which work -through [brew](#homebrew) on MacOS, but not on Linux. - -Installation instructions assume Debian- or Ubuntu-based Linux distributions -with [apt]() available. - -#### `go get` - -[go get](https://golang.org/pkg/cmd/go/internal/get/) is Go's package manager. - -Our recommended way of installing is: - -```bash -apt install golang -``` - -To get the latest version of `go` packages, it will be necessary to periodically -re-run the original `go get ...` command used to install the package. - -#### Cargo (optional) - -Rust's [Cargo](https://doc.rust-lang.org/cargo/) package manager is used to -install a couple tools on Linux. - -Our recommended way of installing is to run -[the canonical install command](https://rustup.rs/). - -To get the latest version of `cargo` packages, it will be necessary to -periodically re-run the original `cargo install ...` command used. - ## Main tools These tools are key for contributions, primarily focused on validating @@ -160,26 +124,6 @@ Our recommended way of installing is: brew install bazelisk ``` -### buildifier - -[Buildifier](https://github.com/bazelbuild/buildtools/tree/master/buildifier) is -a tool for formatting Bazel BUILD files, and is distributing separately from -Bazel. - -Our recommended way of installing is: - -- Linux: - - ```bash - go get github.com/bazelbuild/buildtools/buildifier - ``` - -- MacOS: - - ```bash - brew install buildifier - ``` - ### Clang and LLVM [Clang](https://clang.llvm.org/) and [LLVM](https://llvm.org/) are used to @@ -339,6 +283,10 @@ Our recommended way of installing is: - Linux: + > If you don't have Rust's [Cargo](https://doc.rust-lang.org/cargo/) package + > manager, install it first with + > [the official install command](https://rustup.rs/). + ```bash brew install watchman cargo install --git https://github.com/jgavris/rs-git-fsmonitor.git diff --git a/scripts/BUILD b/scripts/BUILD index 5f6fbd96f6e7..a8c271611f76 100644 --- a/scripts/BUILD +++ b/scripts/BUILD @@ -4,11 +4,18 @@ load("@mypy_integration//:mypy.bzl", "mypy_test") +py_library( + name = "scripts_utils", + srcs = ["scripts_utils.py"], +) + # Note that this Python script is intended to be run directly, and not through # Bazel. We have a rule for it so we can type check it. py_binary( name = "create_compdb", + testonly = 1, srcs = ["create_compdb.py"], + deps = [":scripts_utils"], ) mypy_test( @@ -21,7 +28,9 @@ mypy_test( # Bazel. We have a rule for it so we can type check it. py_binary( name = "fix_cc_deps", + testonly = 1, srcs = ["fix_cc_deps.py"], + deps = [":scripts_utils"], ) mypy_test( @@ -29,3 +38,33 @@ mypy_test( include_imports = True, deps = [":fix_cc_deps"], ) + +# Note that this Python script is intended to be run directly, and not through +# Bazel. We have a rule for it so we can type check it. +py_binary( + name = "forbid_llvm_googletest", + testonly = 1, + srcs = ["forbid_llvm_googletest.py"], + deps = [":scripts_utils"], +) + +mypy_test( + name = "forbid_llvm_googletest_mypy_test", + include_imports = True, + deps = [":forbid_llvm_googletest"], +) + +# Note that this Python script is intended to be run directly, and not through +# Bazel. We have a rule for it so we can type check it. +py_binary( + name = "run_buildifier", + testonly = 1, + srcs = ["run_buildifier.py"], + deps = [":scripts_utils"], +) + +mypy_test( + name = "run_buildifier_mypy_test", + include_imports = True, + deps = [":run_buildifier"], +) diff --git a/scripts/create_compdb.py b/scripts/create_compdb.py index dfc40103a633..5990928e2e7e 100755 --- a/scripts/create_compdb.py +++ b/scripts/create_compdb.py @@ -24,27 +24,19 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception """ import json -import os import re -import shutil import subprocess import sys from pathlib import Path -# 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) +import scripts_utils # type: ignore + +scripts_utils.chdir_repo_root() directory = Path.cwd() # 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 not bazel: - bazel = "bazelisk" - if not shutil.which(bazel): - bazel = "bazel" - if not shutil.which(bazel): - sys.exit("Unable to run Bazel") +bazel = scripts_utils.locate_bazel() # Load compiler flags. We do this first in order to fail fast if not run from # the workspace root. diff --git a/scripts/fix_cc_deps.py b/scripts/fix_cc_deps.py index e8058e513105..e1c61a8e5f88 100755 --- a/scripts/fix_cc_deps.py +++ b/scripts/fix_cc_deps.py @@ -14,17 +14,13 @@ Exceptions. See /LICENSE for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception """ -import hashlib -import os -from pathlib import Path -import platform import re -import shutil import subprocess from typing import Callable, Dict, List, NamedTuple, Set, Tuple -import urllib.request from xml.etree import ElementTree +import scripts_utils # type: ignore + # Maps external repository names to a method translating bazel labels to file # paths for that repository. @@ -32,21 +28,6 @@ EXTERNAL_REPOS: Dict[str, Callable[[str], str]] = { "@llvm-project": lambda x: re.sub("^(.*:(lib|include))/", "", x) } -URL = "https://github.com/bazelbuild/buildtools/releases/download/4.2.5/" - -# Checksums gathered with: -# for v in darwin-amd64 darwin-arm64 linux-amd64 linux-arm64 windows-amd64.exe -# do -# echo \"$v\": \"$(wget -q -O - https://github.com/bazelbuild/buildtools/releases/download/4.2.5/buildozer-$v | sha256sum | cut -d ' ' -f1)\", \# noqa: E501 -# done -VERSIONS = { - "darwin-amd64": "3fe671620e6cb7d2386f9da09c1de8de88b02b9dd9275cdecd8b9e417f74df1b", # noqa: E501 - "darwin-arm64": "ff4d297023fe3e0fd14113c78f04cef55289ca5bfe5e45a916be738b948dc743", # noqa: E501 - "linux-amd64": "e8e39b71c52318a9030dd9fcb9bbfd968d0e03e59268c60b489e6e6fc1595d7b", # noqa: E501 - "linux-arm64": "96227142969540def1d23a9e8225524173390d23f3d7fd56ce9c4436953f02fc", # noqa: E501 - "windows-amd64.exe": "2a9a7176cbd3b2f0ef989502128efbafd3b156ddabae93b9c979cd4017ffa300", # noqa: E501 -} - class Rule(NamedTuple): # For cc_* rules: @@ -62,82 +43,6 @@ class Rule(NamedTuple): outs: Set[str] -def get_hash(file: Path) -> str: - """Returns the sha256 of a file.""" - digest = hashlib.sha256() - with file.open("rb") as f: - while True: - chunk = f.read(1024 * 64) - if not chunk: - break - digest.update(chunk) - return digest.hexdigest() - - -def install_buildozer() -> str: - """Install buildozer to a cache.""" - cache_dir = Path.home().joinpath(".cache", "carbon-lang-pre-commit") - cache_dir.mkdir(parents=True, exist_ok=True) - - # Translate platform information into Bazel's release form. - machine = platform.machine() - if machine == "x86_64": - machine = "amd64" - version = f"{platform.system().lower()}-{machine}" - - # Get ready to add .exe for Windows. - ext = "" - if platform.system() == "Windows": - ext = ".exe" - - # Ensure the platform is supported, and grab its hash. - if version not in VERSIONS: - # If this because a platform support issue, we may need to print errors. - exit(f"No buildozer available for platform: {version}") - want_hash = VERSIONS[version] - - # Check if there's a cached file that can be used. - local_path = cache_dir.joinpath(f"buildozer{ext}") - if local_path.is_file() and want_hash == get_hash(local_path): - return str(local_path) - - # Download buildozer. - url = f"{URL}/buildozer-{version}{ext}" - with urllib.request.urlopen(url) as response: - with local_path.open("wb") as f: - shutil.copyfileobj(response, f) - local_path.chmod(0o755) - - # Verify the downloaded hash. - found_hash = get_hash(local_path) - if want_hash != found_hash: - exit( - f"Downloaded buildozer-{version} but found sha256 {found_hash}, " - f"wanted {want_hash}" - ) - - return str(local_path) - - -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 remap_file(label: str) -> str: """Remaps a bazel label to a file.""" repo, _, path = label.partition("//") @@ -164,7 +69,7 @@ def get_bazel_list(list_child: ElementTree.Element, is_file: bool) -> Set[str]: return results -def get_rules(targets: str, keep_going: bool) -> Dict[str, Rule]: +def get_rules(bazel: str, targets: str, keep_going: bool) -> Dict[str, Rule]: """Queries the specified targets, returning the found rules. keep_going will be set to true for external repositories, where sometimes we @@ -173,7 +78,7 @@ def get_rules(targets: str, keep_going: bool) -> Dict[str, Rule]: The return maps rule names to rule data. """ args = [ - "bazel", + bazel, "query", "--output=xml", f"kind('(cc_binary|cc_library|cc_test|genrule)', set({targets}))", @@ -271,15 +176,14 @@ def get_missing_deps( 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) + scripts_utils.chdir_repo_root() + bazel = scripts_utils.locate_bazel() print("Querying bazel for Carbon targets...") - carbon_rules = get_rules("//...", False) + carbon_rules = get_rules(bazel, "//...", False) print("Querying bazel for external targets...") external_repo_query = " ".join([f"{repo}//..." for repo in EXTERNAL_REPOS]) - external_rules = get_rules(external_repo_query, True) + external_rules = get_rules(bazel, external_repo_query, True) print("Building header map...") header_to_rule_map: Dict[str, Set[str]] = {} @@ -307,7 +211,7 @@ def main() -> None: if all_missing_deps: print("Checking buildozer availability...") - buildozer = install_buildozer() + buildozer = scripts_utils.get_release(scripts_utils.Release.BUILDOZER) print("Fixing dependencies...") SEPARATOR = "\n- " diff --git a/scripts/forbid_llvm_googletest.py b/scripts/forbid_llvm_googletest.py index 00cfeb39a033..14c27c76fc55 100755 --- a/scripts/forbid_llvm_googletest.py +++ b/scripts/forbid_llvm_googletest.py @@ -18,10 +18,9 @@ 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 + +import scripts_utils # type: ignore _MESSAGE = """\ Dependencies on @llvm-project//llvm:gtest are forbidden, but a dependency path @@ -34,31 +33,10 @@ 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) + scripts_utils.chdir_repo_root() args = [ - locate_bazel(), + scripts_utils.locate_bazel(), "query", "somepath(//..., @llvm-project//llvm:gtest)", ] diff --git a/scripts/run_buildifier.py b/scripts/run_buildifier.py new file mode 100755 index 000000000000..37969e67f21c --- /dev/null +++ b/scripts/run_buildifier.py @@ -0,0 +1,26 @@ +#!/usr/bin/env python3 + +"""Runs buildifier on passed-in BUILD files, mainly for pre-commit.""" + +__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 subprocess +import sys + +import scripts_utils # type: ignore + + +def main() -> None: + files = sys.argv[1:] + if not files: + return + buildifier = scripts_utils.get_release(scripts_utils.Release.BUILDIFIER) + subprocess.check_call([buildifier] + files) + + +if __name__ == "__main__": + main() diff --git a/scripts/scripts_utils.py b/scripts/scripts_utils.py new file mode 100644 index 000000000000..2ac6b24cb759 --- /dev/null +++ b/scripts/scripts_utils.py @@ -0,0 +1,157 @@ +"""Utilities for scripts.""" + +__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 +""" + +from enum import Enum +import hashlib +import os +from pathlib import Path +import platform +import shutil +import time +from typing import Optional +import urllib.request + +_URL = "https://github.com/bazelbuild/buildtools/releases/download/4.2.5/" + +"""Version SHAs. + +Gather shas with: + for f in buildozer buildifier; do + echo \"$f\": { + for v in darwin-amd64 darwin-arm64 linux-amd64 linux-arm64 \ + windows-amd64.exe + do + echo "\"$v\": \"$(wget -q -O - https://github.com/bazelbuild/buildtools/releases/download/4.2.5/$f-$v | sha256sum | cut -d ' ' -f1)\", # noqa: E501" + done + echo }, + done +""" +_VERSION_SHAS = { + "buildozer": { + "darwin-amd64": "3fe671620e6cb7d2386f9da09c1de8de88b02b9dd9275cdecd8b9e417f74df1b", # noqa: E501 + "darwin-arm64": "ff4d297023fe3e0fd14113c78f04cef55289ca5bfe5e45a916be738b948dc743", # noqa: E501 + "linux-amd64": "e8e39b71c52318a9030dd9fcb9bbfd968d0e03e59268c60b489e6e6fc1595d7b", # noqa: E501 + "linux-arm64": "96227142969540def1d23a9e8225524173390d23f3d7fd56ce9c4436953f02fc", # noqa: E501 + "windows-amd64.exe": "2a9a7176cbd3b2f0ef989502128efbafd3b156ddabae93b9c979cd4017ffa300", # noqa: E501 + }, + "buildifier": { + "darwin-amd64": "757f246040aceb2c9550d02ef5d1f22d3ef1ff53405fe76ef4c6239ef1ea2cc1", # noqa: E501 + "darwin-arm64": "4cf02e051f6cda18765935cb6e77cc938cf8b405064589a50fe9582f82c7edaf", # noqa: E501 + "linux-amd64": "f94e71b22925aff76ce01a49e1c6c6d31f521bbbccff047b81f2ea01fd01a945", # noqa: E501 + "linux-arm64": "2113d79e45efb51e2b3013c8737cb66cadae3fd89bd7e820438cb06201e50874", # noqa: E501 + "windows-amd64.exe": "4185a40d3154cacbe8b79f570b94e2c6f74fc9e317362b7d028c2e6c94edf9ba", # noqa: E501 + }, +} + + +class Release(Enum): + BUILDOZER = "buildozer" + BUILDIFIER = "buildifier" + + +def chdir_repo_root() -> None: + """Change the working directory to the repository root. + + This is done so that scripts run from a consistent directory. + """ + os.chdir(Path(__file__).parent.parent) + + +def _get_hash(file: Path) -> str: + """Returns the sha256 of a file.""" + digest = hashlib.sha256() + with file.open("rb") as f: + while True: + chunk = f.read(1024 * 64) + if not chunk: + break + digest.update(chunk) + return digest.hexdigest() + + +def _download(url: str, local_path: Path) -> Optional[int]: + """Downloads the URL to the path. Returns an HTTP error code on failure.""" + with urllib.request.urlopen(url) as response: + if response.code != 200: + return int(response.code) + with local_path.open("wb") as f: + shutil.copyfileobj(response, f) + return None + + +def get_release(release: Release) -> str: + """Install a file to carbon-lang's cache. + + release: The release to cache. + """ + cache_dir = Path.home().joinpath(".cache", "carbon-lang-scripts") + cache_dir.mkdir(parents=True, exist_ok=True) + + # Translate platform information into Bazel's release form. + machine = platform.machine() + if machine == "x86_64": + machine = "amd64" + version = f"{platform.system().lower()}-{machine}" + + # Get ready to add .exe for Windows. + ext = "" + if platform.system() == "Windows": + ext = ".exe" + + # Ensure the platform is supported, and grab its hash. + if version not in _VERSION_SHAS[release.value]: + # If this because a platform support issue, we may need to print errors. + exit(f"No {release.value} release available for platform: {version}") + want_hash = _VERSION_SHAS[release.value][version] + + # Check if there's a cached file that can be used. + local_path = cache_dir.joinpath(f"{release.value}{ext}") + if local_path.is_file() and want_hash == _get_hash(local_path): + return str(local_path) + + # Download the file. + url = f"{_URL}/{release.value}-{version}{ext}" + retries = 5 + while True: + err = _download(url, local_path) + if err is None: + break + retries -= 1 + if retries == 0: + exit(f"Failed to download {release.value}-{version}: HTTP {err}.") + time.sleep(1) + local_path.chmod(0o755) + + # Verify the downloaded hash. + found_hash = _get_hash(local_path) + if want_hash != found_hash: + exit( + f"Downloaded {release.value}-{version} but found sha256 " + f"{found_hash}, wanted {want_hash}" + ) + + return str(local_path) + + +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")