From c3a40b8cf3de25bb4a893a76d704fd4a8b7f8e89 Mon Sep 17 00:00:00 2001 From: sdarwin Date: Mon, 5 Oct 2026 14:16:53 -0600 Subject: [PATCH 1/3] Enforce commit and file policy in CI rather than only at push time [GH #164] --- .git-hooks/pre-push.py | 240 ++++--------- .github/CODEOWNERS | 32 ++ .github/rulesets/README.md | 122 +++++++ .../rulesets/default-branch-protection.json | 39 +++ .github/workflows/policy-check.yml | 73 ++++ CONTRIBUTING.md | 27 ++ HACKING.md | 20 ++ dev_tools/bin/edg-check-policy | 261 ++++++++++++++ dev_tools/pylibs/edgpolicy/__init__.py | 323 ++++++++++++++++++ dev_tools/pylibs/edgpolicy/py.typed | 0 dev_tools/typecheck-python.sh | 4 + 11 files changed, 964 insertions(+), 177 deletions(-) create mode 100644 .github/CODEOWNERS create mode 100644 .github/rulesets/README.md create mode 100644 .github/rulesets/default-branch-protection.json create mode 100644 .github/workflows/policy-check.yml create mode 100755 dev_tools/bin/edg-check-policy create mode 100644 dev_tools/pylibs/edgpolicy/__init__.py create mode 100644 dev_tools/pylibs/edgpolicy/py.typed diff --git a/.git-hooks/pre-push.py b/.git-hooks/pre-push.py index aba11c4a77..88b6c912e6 100755 --- a/.git-hooks/pre-push.py +++ b/.git-hooks/pre-push.py @@ -32,16 +32,17 @@ # pushing anything. Information about why the push is rejected may be sent to # the user by writing to standard error. # +# The policy this hook applies is defined in the edgpolicy library, which the +# policy CI workflow also uses. This hook exists to give fast local feedback; +# CI is what actually enforces the policy. +# -import json -import os import platform -import re -import shutil import subprocess import sys import tempfile +import edgpolicy import edgutil from pathlib import Path @@ -71,49 +72,15 @@ # On Unix-like systems we can regain access to terminal input using /dev/tty. sys.stdin = open('/dev/tty') - # -# File paths that begin with the prefixes listed here are excluded from file -# level checking. +# How each kind of finding reported by edgpolicy is introduced to the user. # -EXCLUDES_PATH_PREFIXES = [ - # Don't check tests as they're not necessarily "EDG-style". - 'tests/', - # Don't check the generated builtin files as they're very large and there's - # really nothing the user can do but accept them. - 'dev_tools/builtins/builtins_', - # Don't check GitHub workflow yml files; long lines are all but required. - '.github/workflows/', - # Don't check the error_msg.txt file. - 'src/error_msg.txt', - # Don't check AGENT files. - 'AGENTS.md', - '.agents/' -] - -def _is_excluded_file_path(path_str: str) -> bool: - return any( - path_str.startswith(prefix) for prefix in EXCLUDES_PATH_PREFIXES - ) - -COMMIT_SUBJECT_POLICY_REGEX = re.compile( - r'^.*\[(GH #[0-9]+|(EDG[cfjp]+fe/([0-9]+))|,)*\].*$' -) - -TOOL_PATH_GIT = shutil.which('git') - -def run_git(args, *, check = True, text = True): - '''Run a git command and return the completed process.''' - return subprocess.run( - [TOOL_PATH_GIT, *args], - check = check, - capture_output = True, - text = text - ) - -def git_output(args): - '''Run a git command and return stripped stdout.''' - return run_git(args).stdout.strip() +FINDING_KIND_HEADINGS = { + 'misspelling': 'New misspelled words:', + 'typo': 'New typos:', + 'coding-error': 'Coding errors:', + 'overlong-line': 'Lines too long:' +} def print_indented(text, prefix): '''Print each line of text with the given prefix.''' @@ -123,20 +90,12 @@ def print_indented(text, prefix): for line in text.splitlines(): print(f"{prefix}{line}") -def git_show_blob(revision_path: str) -> bytes: - '''Return the blob contents for revision:path, or b'' if missing.''' - completed_process = run_git( - ['show', revision_path], check = False, text = False - ) - if completed_process.returncode != 0: - return b'' - return completed_process.stdout - class PrePushChecker: '''Runs EDG pre-push policy checks against new commits and changed files.''' def __init__(self): self.errors = 0 + self.repository = edgpolicy.GitRepository(edgutil.find_mono_repo_or_exit()) def confirm_okay(self): '''Ask the user whether a policy warning is acceptable. @@ -176,14 +135,16 @@ def check_commits(self): As commits are walked, check to make sure they match formatting guidance. Returns the oldest new commit hash, or None if there are no new commits. - Exits immediately if a commit subject fails the PR-number policy. + A commit subject that fails the policy aborts the push outright when + edgpolicy says the rule is binding, and is otherwise offered for + confirmation like any other finding. ''' first_new_commit = None - commits = git_output(['log', '--pretty=%H']).splitlines() + commits = self.repository.lines(['log', '--pretty=%H']) for commit in commits: # Break if the remote already has this commit. - contains = run_git( + contains = self.repository.run( ['branch', '-r', '--contains', commit], check = False ).stdout.strip() if contains: @@ -191,150 +152,75 @@ def check_commits(self): print('New commit:') print('') - show_output = git_output(['show', '--no-patch', commit]) + show_output = self.repository.output(['show', '--no-patch', commit]) print_indented(show_output, ' ') print('') - subject = git_output(['show', '--no-patch', '--format=%s', commit]) - if COMMIT_SUBJECT_POLICY_REGEX.match(subject) is None: - print( - " [POLICY] Commit message doesn't contain valid PR number (or [])." - ) - sys.exit(1) + subject_error = edgpolicy.commit_subject_error( + self.repository.subject_of(commit) + ) + if subject_error is not None: + print(f" [POLICY] {subject_error}") + print_indented(edgpolicy.SUBJECT_POLICY_DESCRIPTION, ' ') + if edgpolicy.ENFORCE_COMMIT_SUBJECT_POLICY: + sys.exit(1) + self.confirm_okay() first_new_commit = commit return first_new_commit + def report_findings(self, findings): + '''Print findings grouped by kind, confirming once per group.''' + for kind, heading in FINDING_KIND_HEADINGS.items(): + messages = [ + message for finding_kind, message in findings + if finding_kind == kind + ] + if not messages: + continue + + print(f" {heading}") + for message in messages: + print(f" - {message}") + self.confirm_okay() + def check_changed_files(self, first_new_commit): '''Run per-file policy checks for files changed since first_new_commit~1.''' - remote_head = f"{first_new_commit}~1" + base_rev = f"{first_new_commit}~1" print('Checking changed files:') - changed_files = git_output( - ['diff', '--no-commit-id', '--name-only', '-r', f"{remote_head}..HEAD"] - ).splitlines() - - mono_repo_dir = edgutil.find_mono_repo_or_exit() - containing_tmp_dir = mono_repo_dir / '.tmp' - - with tempfile.TemporaryDirectory(dir = containing_tmp_dir) as tmp_dir_str: - input_spec = {} - for file_path_str in changed_files: - if _is_excluded_file_path(file_path_str): - continue - - file_path = Path(file_path_str) - named_tmp_args = { - 'prefix': file_path.stem, - 'suffix': file_path.suffix, - 'dir': tmp_dir_str, - 'delete': False - } - with tempfile.NamedTemporaryFile(**named_tmp_args) as file_handle: - file_handle.write(git_show_blob(f"HEAD:{file_path_str}")) - new_file = ( - Path(file_handle.name).relative_to(mono_repo_dir).as_posix() - ) - with tempfile.NamedTemporaryFile(**named_tmp_args) as file_handle: - file_handle.write(git_show_blob(f"{remote_head}:{file_path_str}")) - old_file = ( - Path(file_handle.name).relative_to(mono_repo_dir).as_posix() - ) - - input_spec[file_path_str] = [old_file, new_file] + containing_tmp_dir = self.repository.repo_dir / '.tmp' + containing_tmp_dir.mkdir(exist_ok = True) + with tempfile.TemporaryDirectory( + dir = str(containing_tmp_dir) + ) as tmp_dir_str: tmp_dir = Path(tmp_dir_str) - input_file = tmp_dir / 'input.json' - output_file = tmp_dir / 'output.json' - with open(input_file, 'w') as file_handle: - json.dump(input_spec, file_handle) - - use_docker = ( - shutil.which('docker') is not None and - edgutil.is_docker_preferred(mono_repo_dir) + + change_spec = edgpolicy.collect_change_spec( + self.repository, base_rev, 'HEAD', tmp_dir ) - if use_docker: - file_change_check_proc = subprocess.run( - [ - shutil.which('edg-exec'), - '--', - 'edg-check-file-changes', - str(input_file.relative_to(mono_repo_dir).as_posix()), - str(output_file.relative_to(mono_repo_dir).as_posix()) - ], - cwd = mono_repo_dir, - stdin = subprocess.DEVNULL - ) - else: - file_change_check_proc = subprocess.run( - [ - shutil.which('edg-check-file-changes'), - str(input_file), - str(output_file) - ], - cwd = mono_repo_dir, - stdin = subprocess.DEVNULL - ) - - if file_change_check_proc.returncode != 0: - sys.exit(1) - with open(output_file, 'r') as file_handle: - results = json.load(file_handle) + use_docker = edgpolicy.should_use_docker(self.repository.repo_dir) + results = edgpolicy.run_file_checks( + self.repository, change_spec, tmp_dir, use_docker = use_docker + ) + if results is None: + sys.exit(1) - for file_path_str, file_results in results.items(): + for file_path_str in sorted(results): print(f" Checking file: {file_path_str}") - misspellings = file_results['misspellings'] - if misspellings is None: + findings = edgpolicy.summarize_file_result(results[file_path_str]) + if any(kind == 'spelling-unavailable' for kind, _ in findings): print(' Spell check failed:') if not use_docker: print(' docker not used.') print(' aspell not found or missing dictionary.') - elif len(misspellings) != 0: - print(' New misspelled words:') - for misspelling in misspellings: - print(f" - {misspelling}") - self.confirm_okay() - - typos = file_results['typos'] - if len(typos) != 0: - print(' New typos:') - for typo in typos: - print(f" - {typo}") - self.confirm_okay() - - coding_errors = file_results['coding_errors'] - if len(coding_errors) != 0: - endif_hits = coding_errors['missing_endifs'] - if len(endif_hits) != 0: - print(f" Missing comment on #endif:") - print_indented('\n'.join(endif_hits), ' ') - self.confirm_okay() - - else_hits = coding_errors['missing_elses'] - if len(else_hits) != 0: - print(f" Missing comment on #else:") - print_indented('\n'.join(else_hits), ' ') - self.confirm_okay() - - closing_comments = coding_errors['closing_comments'] - if len(closing_comments) != 0: - print_indented('\n'.join(closing_comments), ' ') - self.confirm_okay() - - overlong_lines = file_results['overlong_lines'] - if len(overlong_lines) != 0: - for line_no, (line_start, lines) in overlong_lines.items(): - print(f" Line {line_no} too long:") - line_end = line_start + (len(lines) - 1) - print(f" {line_start} -> {line_end} ".center(79, '=')) - for line in lines: - print(line, end = '') - print('=' * 79) - self.confirm_okay() + + self.report_findings(findings) def run(self) -> bool: '''Execute the full pre-push check sequence.''' diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000000..01e7d538ba --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,32 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +# Code owners are requested for review automatically. They are only +# *required* to approve when the branch ruleset enables "Require review from +# Code Owners"; see .github/rulesets/README.md. +# +# Teams must have write access to the repository to be assignable here. +# Until the owning teams exist, every entry below is commented out so that +# GitHub does not report an invalid CODEOWNERS file. + +# Fallback owner for anything not matched below. +# * @edgcpp/maintainers + +# Front end sources and the headers they publish. +# /src/ @edgcpp/front-end-maintainers +# /include_c++/ @edgcpp/front-end-maintainers +# /include_c99/ @edgcpp/front-end-maintainers +# /lib_src/ @edgcpp/front-end-maintainers + +# Developer tooling, the test harness, and the build system. +# /dev_tools/ @edgcpp/tooling-maintainers +# /cmake/ @edgcpp/tooling-maintainers +# /CMakeLists.txt @edgcpp/tooling-maintainers +# /CMakePresets.json @edgcpp/tooling-maintainers + +# Project policy and automation. Changes here alter what CI enforces, so +# these paths warrant the closest review. +# /.github/ @edgcpp/maintainers +# /.git-hooks/ @edgcpp/maintainers diff --git a/.github/rulesets/README.md b/.github/rulesets/README.md new file mode 100644 index 0000000000..39137e2008 --- /dev/null +++ b/.github/rulesets/README.md @@ -0,0 +1,122 @@ +# Branch rulesets + +GitHub does not read rulesets out of a repository, so the JSON here is not +applied automatically. It is the reviewed, version-controlled statement of +what the branch protection *should* be; applying it is a deliberate act by +someone with admin rights, either from the command line below or through +**Settings -> Rules -> Rulesets** in the web UI. + +Keeping the definition in the repository means a change to the project's +enforcement is proposed, reviewed, and recorded the same way a code change +is, instead of appearing in the settings UI with no history and no +discussion. + +## What enforces what + +It is worth being precise about where each kind of rule actually lives, +because the three categories behave very differently. + +- `.git-hooks/pre-push.py` lives in the repository and runs on the + contributor's own machine, so it is advisory: it can be skipped with + `git push --no-verify` and is absent unless `dev-init.py` was run. +- `.github/workflows/*.yml` lives in the repository and runs on GitHub, so + it cannot be skipped, but a failing run blocks nothing on its own. +- Rulesets live in GitHub's settings, not in the repository, and are + enforced by GitHub on push and on merge. Only the actors listed in + `bypass_actors` can get around them. +- `.github/CODEOWNERS` lives in the repository but has no effect until a + ruleset turns on code owner review. + +The middle two are the overlap worth understanding. A workflow file is +version controlled and runs on every pull request, but a failing run blocks +nothing until its job name is listed as a required status check in a +ruleset. `CODEOWNERS` is the mirror image: the file is version controlled, +but it only gates anything when `require_code_owner_review` is turned on +here. + +None of this needs GitHub Enterprise. Rulesets, required status checks, +required reviews, and `CODEOWNERS` are all available on free public +repositories. True server-side `pre-receive` hooks are Enterprise Server +only, but a required status check covers the same ground: it runs code the +project controls, on GitHub's infrastructure, and a contributor cannot skip +it. + +The one capability genuinely out of reach is **push rulesets** (restricting +file paths, file extensions, or file sizes at push time), which require an +organization on GitHub Team or Enterprise Cloud. + +## Applying the ruleset + +With the `gh` CLI, authenticated as a repository admin: + +```sh +# First time: create it. +gh api --method POST repos/edgcpp/compiler/rulesets \ + --input .github/rulesets/default-branch-protection.json + +# Afterwards: update the existing ruleset in place. +gh api --method PUT repos/edgcpp/compiler/rulesets/RULESET_ID \ + --input .github/rulesets/default-branch-protection.json +``` + +Find `RULESET_ID` with: + +```sh +gh api repos/edgcpp/compiler/rulesets --jq '.[] | "\(.id)\t\(.name)"' +``` + +To confirm what is live, and to check it still matches this file: + +```sh +gh api repos/edgcpp/compiler/rulesets/RULESET_ID \ + --jq '{name, enforcement, rules: [.rules[].type]}' +``` + +## What this ruleset does + +Beyond the `deletion` and `non_fast_forward` rules that were already in +place, it adds: + +- **A pull request is required to change the default branch.** Direct + pushes to `main` stop being possible, which is the gap that lets work + reach `main` today without any check running at all. +- **One approving review**, with stale approvals dismissed when new commits + are pushed, and the last push requiring approval from someone other than + the person who pushed it. +- **No merge commits.** `allowed_merge_methods` lists only `squash` and + `rebase`, so pull requests cannot be merged with a merge commit and the + history on `main` stays linear. Note this governs the merge button only; + to stop merge commits arriving any other way, also untick + **Settings -> General -> Pull Requests -> Allow merge commits**, or add a + `required_linear_history` rule. +- **Required status checks**: the policy check and both Python typecheck + legs must pass, and a branch must be up to date with `main` before it can + merge. + +`bypass_actors` is deliberately empty, so the rules apply to administrators +as well. If the project decides maintainers need an escape hatch, add them +here rather than disabling the rule, so the exception is visible. + +## Checks deliberately not required yet + +`Build & Test CI` and `Strict Build Check` run on every pull request but are +not listed as required. The test legs have a 360 minute timeout and the +Windows build depends on an external SoftFloat checkout, so making them +blocking is a scheduling and flakiness decision for the maintainers rather +than a policy one. They can be added to `required_status_checks` later +using the job names exactly as they appear in the checks list, for example +`Test edg_x86_64` or `Linux GCC debug`. + +## Settings that are not expressible here + +Two things still have to be set by hand in the web UI, because they are +repository options rather than rules: + +- **Settings -> General -> Pull Requests -> Allow auto-merge**, if the + project wants pull requests to merge themselves once checks pass. +- **Settings -> Actions -> General -> Fork pull request workflows**, which + governs whether workflows on pull requests from forks run without + approval. Leaving this at the default ("Require approval for first-time + contributors") is recommended; note that it means the policy check will + not report on a new contributor's pull request until a maintainer + approves the run. diff --git a/.github/rulesets/default-branch-protection.json b/.github/rulesets/default-branch-protection.json new file mode 100644 index 0000000000..158adc3fcd --- /dev/null +++ b/.github/rulesets/default-branch-protection.json @@ -0,0 +1,39 @@ +{ + "name": "Default Branch Protection", + "target": "branch", + "enforcement": "active", + "conditions": { + "ref_name": { + "include": ["~DEFAULT_BRANCH"], + "exclude": [] + } + }, + "bypass_actors": [], + "rules": [ + { "type": "deletion" }, + { "type": "non_fast_forward" }, + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": 1, + "dismiss_stale_reviews_on_push": true, + "require_code_owner_review": false, + "require_last_push_approval": true, + "required_review_thread_resolution": false, + "allowed_merge_methods": ["squash", "rebase"] + } + }, + { + "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": true, + "do_not_enforce_on_create": false, + "required_status_checks": [ + { "context": "Commit and file policy" }, + { "context": "typecheck-python (3.6)" }, + { "context": "typecheck-python (3.14)" } + ] + } + } + ] +} diff --git a/.github/workflows/policy-check.yml b/.github/workflows/policy-check.yml new file mode 100644 index 0000000000..b8b40d6969 --- /dev/null +++ b/.github/workflows/policy-check.yml @@ -0,0 +1,73 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +# Enforce the commit and file change policies that the pre-push hook checks +# locally. The hook is advisory by nature: it is only installed by +# dev-init.py, it can be skipped with "git push --no-verify", and it never +# runs for work merged through the GitHub web UI. This workflow re-checks +# the same rules, from the same edgpolicy library, where they cannot be +# skipped. +# +# Marking this workflow's job as a required status check is what actually +# makes the policy binding; see .github/rulesets/README.md. +# +# This runs on a stock runner rather than in edgcpp/ci-runner because the +# file checks need aspell, which the CI image does not carry. + +name: Policy Check + +# Pull requests only: the policy is defined over the range of commits a +# pull request proposes, which a push event does not describe. +on: + pull_request: + +permissions: + contents: read + +concurrency: + group: policy-check-${{ github.ref }} + cancel-in-progress: true + +jobs: + policy: + name: Commit and file policy + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - name: Check out the pull request head + uses: actions/checkout@v7 + with: + # Check out the branch itself rather than the synthetic merge + # commit, so the commits reported are the ones the author wrote. + ref: ${{ github.event.pull_request.head.sha }} + # The policy runs over a revision range, so the base commit and + # the merge base between it and the head must both be present. + fetch-depth: 0 + + - name: Install aspell + run: | + sudo apt-get update + sudo apt-get install --no-install-recommends -y aspell aspell-en + + # get-comments and check-closing-comments are C++ helpers that + # edg-check-file-changes shells out to. They install into + # dev_tools/bin alongside the Python tooling. + - name: Build the dev_tools helpers + run: | + cmake -S dev_tools -B dev_tools/build -DCMAKE_BUILD_TYPE=Release + cmake --build dev_tools/build --target install --parallel + + - name: Check commit and file policy + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + export PATH="${GITHUB_WORKSPACE}/dev_tools/bin:${PATH}" + export PYTHONPATH="${GITHUB_WORKSPACE}/dev_tools/pylibs" + edg-check-policy \ + --base-rev "${BASE_SHA}" \ + --head-rev "${HEAD_SHA}" \ + --no-docker diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 91d0753e5f..d3db2b9c9c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,6 +81,33 @@ existing issues and discussion (it's possible someone is already working on the item or that there has been significant discussion about the desired implementation characeristics). +### What CI Enforces + +Some of the items below are checked automatically on every pull request by +the `Policy Check` workflow, which runs the same rules as the `pre-push` +hook installed by `dev-init.py`. + +These **fail** the check: + +- Changed files must stay within 79 columns, must comment their `#endif` + and `#else` directives, and must not contain repeated-word typos. + +These are **reported but do not fail** the check: + +- Commit subjects should carry a bracketed tag naming at least one GitHub + issue or legacy PR, such as `[GH #213]` or `[EDGcpfe/29032]`. Whether + commits that belong to no issue must still carry an empty `[]` is an + open question, so this is advisory for now. +- Spelling, because there is no project dictionary and a compiler code + base trips `aspell` constantly. Please still read the spelling + annotations; they catch real mistakes. + +You can run the same check locally before opening a pull request: + +```sh +edg-check-policy --base-rev origin/main +``` + ### Checklist for Success > [!NOTE] diff --git a/HACKING.md b/HACKING.md index 0cb7261fdd..d2658d7acf 100644 --- a/HACKING.md +++ b/HACKING.md @@ -12,6 +12,26 @@ As a result it is *highly recommended* to use the Docker based development process as this will typically provide the best experience (even if directly running a Linux-based operation system). +## Project Policy Checks + +`dev-init.py` installs a `pre-push` hook that checks new commits against the +project's commit message and coding style policies. The hook exists to tell +you about problems before you push; it is not what enforces them. It is only +present if you ran `dev-init.py`, it is skipped by `git push --no-verify`, and +it never runs for work done through the GitHub web interface. + +Enforcement happens in the `Policy Check` workflow, which applies the same +rules from the same `edgpolicy` library when a pull request is opened. To +run those checks yourself at any point: + +```sh +edg-check-policy --base-rev origin/main +``` + +Use `--strict` to also fail on the spelling findings that the workflow only +reports as warnings. See `.github/rulesets/README.md` for how the checks are +tied to branch protection. + ## Prerequisites The first step (regardless of Docker use) is to make EDG development tools diff --git a/dev_tools/bin/edg-check-policy b/dev_tools/bin/edg-check-policy new file mode 100755 index 0000000000..b4be3c6134 --- /dev/null +++ b/dev_tools/bin/edg-check-policy @@ -0,0 +1,261 @@ +#!/usr/bin/env python3 + +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Check a revision range against the project's commit and file policies. + +This is the non-interactive counterpart to the pre-push hook. Both read +their rules from edgpolicy, but where the hook asks the developer whether +a finding is acceptable, this reports every finding and fails, which is +what a required status check needs to do. +''' + +import argparse +import os +import sys +import tempfile + +from pathlib import Path +from typing import FrozenSet, List, Optional + +import edgpolicy +import edgutil + +COMMIT_SUBJECT_FINDING_KIND = 'commit-subject' + +# +# Findings of these kinds are reported without failing the run. +# +# aspell has no project dictionary to work from, so on a compiler code base +# it reports identifiers, standards vocabulary, and abbreviations as new +# misspellings on essentially every change; measured against recent history +# it fires between 1 and 52 times per commit while the mechanical checks +# fire between 0 and 3 times. Spelling is therefore advisory, and the +# mechanical checks are enforced. Use --strict to fail on everything. +# +ADVISORY_FINDING_KINDS = frozenset([ + 'misspelling', + 'spelling-unavailable' +]) + +def default_warning_kinds() -> FrozenSet[str]: + '''Return the finding kinds that should be reported but not enforced. + + The commit subject rule joins them unless edgpolicy says it is binding. + ''' + if edgpolicy.ENFORCE_COMMIT_SUBJECT_POLICY: + return ADVISORY_FINDING_KINDS + return ADVISORY_FINDING_KINDS | {COMMIT_SUBJECT_FINDING_KIND} + +class PolicyReporter: + '''Collects findings and prints them for humans and for GitHub.''' + + def __init__(self, warning_kinds: FrozenSet[str]) -> None: + self.violations = 0 + self.warning_kinds = warning_kinds + self.in_github_actions = 'GITHUB_ACTIONS' in os.environ + + def report(self, kind: str, message: str, path: Optional[str]) -> None: + '''Record a finding at the severity configured for its kind.''' + if kind in self.warning_kinds: + self.warning(message, path) + else: + self.violation(message, path) + + def _annotate(self, level: str, message: str, path: Optional[str]) -> None: + if not self.in_github_actions: + return + location = f" file={path}" if path else '' + # Annotations must be one line to be parsed by the Actions runner, and + # these messages can quote a line of the diff, so flatten every line + # break rather than just the newline. + flattened = message.replace('\r', ' ').replace('\n', ' ') + print(f"::{level}{location}::{flattened}") + + def violation(self, message: str, path: Optional[str] = None) -> None: + self.violations += 1 + print(f" [POLICY] {message}") + self._annotate('error', message, path) + + def warning(self, message: str, path: Optional[str] = None) -> None: + print(f" [WARNING] {message}") + self._annotate('warning', message, path) + +def check_commit_subjects( + repository: edgpolicy.GitRepository, + reporter: PolicyReporter, + base_rev: str, + head_rev: str +) -> None: + '''Check every commit subject in base_rev..head_rev.''' + commits = repository.lines( + ['log', '--no-merges', '--pretty=%H', f"{base_rev}..{head_rev}"] + ) + + if not commits: + print('No new commits to check.') + return + + print(f"Checking {len(commits)} commit subject(s):") + rejected = 0 + for commit in commits: + subject = repository.subject_of(commit) + short_hash = commit[:12] + print(f" {short_hash} {subject}") + + error = edgpolicy.commit_subject_error(subject) + if error is not None: + rejected += 1 + reporter.report( + COMMIT_SUBJECT_FINDING_KIND, f"{short_hash}: {error}", None + ) + + if rejected: + print('') + print(edgpolicy.SUBJECT_POLICY_DESCRIPTION) + +def check_changed_files( + repository: edgpolicy.GitRepository, + reporter: PolicyReporter, + base_rev: str, + head_rev: str, + allow_docker: bool = True +) -> None: + '''Run the file level checks over base_rev..head_rev.''' + containing_tmp_dir = repository.repo_dir / '.tmp' + containing_tmp_dir.mkdir(exist_ok = True) + + with tempfile.TemporaryDirectory( + dir = str(containing_tmp_dir) + ) as tmp_dir_str: + tmp_dir = Path(tmp_dir_str) + + change_spec = edgpolicy.collect_change_spec( + repository, base_rev, head_rev, tmp_dir + ) + if not change_spec: + print('No changed files to check.') + return + + print(f"Checking {len(change_spec)} changed file(s):") + results = edgpolicy.run_file_checks( + repository, + change_spec, + tmp_dir, + use_docker = ( + allow_docker and edgpolicy.should_use_docker(repository.repo_dir) + ) + ) + + if results is None: + edgutil.eprint('error: edg-check-file-changes failed to run') + reporter.violation('File change checks could not be run.') + return + + # The checker drops a file from its results if checking it raised, so + # a missing entry means the file went unchecked rather than passed. + for path_str in sorted(set(change_spec) - set(results)): + reporter.warning( + f"{path_str}: the file checker returned no result for this file", + path_str + ) + + for path_str in sorted(results): + findings = edgpolicy.summarize_file_result(results[path_str]) + if not findings: + continue + + print(f" {path_str}:") + for kind, message in findings: + reporter.report(kind, f"{path_str}: {message}", path_str) + +def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: + parser = argparse.ArgumentParser( + description = ( + 'Check a revision range against the project commit and file policies.' + ) + ) + parser.add_argument( + '--base-rev', + default = 'origin/main', + help = 'the revision the range starts after (default: origin/main)' + ) + parser.add_argument( + '--head-rev', + default = 'HEAD', + help = 'the revision the range ends at (default: HEAD)' + ) + parser.add_argument( + '--skip-commits', + action = 'store_true', + help = 'do not check commit subjects' + ) + parser.add_argument( + '--skip-files', + action = 'store_true', + help = 'do not check changed files' + ) + parser.add_argument( + '--strict', + action = 'store_true', + help = 'fail on advisory findings (spelling, commit subjects) as well' + ) + parser.add_argument( + '--no-docker', + action = 'store_true', + help = ( + 'run the file checks directly rather than in the dev container, for ' + 'hosts that have docker but no EDG images (such as CI runners)' + ) + ) + return parser.parse_args(argv) + +def main() -> None: + args = parse_args() + + repo_dir = edgutil.find_mono_repo_or_exit() + repository = edgpolicy.GitRepository(repo_dir) + reporter = PolicyReporter( + frozenset() if args.strict else default_warning_kinds() + ) + + # Compare against the point the branch left the base so that commits + # already on the base branch are not re-checked. + merge_base = repository.run( + ['merge-base', args.base_rev, args.head_rev], check = False + ) + if merge_base.returncode != 0: + edgutil.eprint( + f"error: no merge base for {args.base_rev} and {args.head_rev}" + ) + sys.exit(1) + base_rev = str(merge_base.stdout).strip() + + print(f"Base: {base_rev}") + print(f"Head: {repository.output(['rev-parse', args.head_rev])}") + print('') + + if not args.skip_commits: + check_commit_subjects(repository, reporter, base_rev, args.head_rev) + print('') + + if not args.skip_files: + check_changed_files( + repository, + reporter, + base_rev, + args.head_rev, + allow_docker = not args.no_docker + ) + print('') + + if reporter.violations: + print(f"Failed: {reporter.violations} policy violation(s).") + sys.exit(1) + + print('Passed: no policy violations.') + +main() diff --git a/dev_tools/pylibs/edgpolicy/__init__.py b/dev_tools/pylibs/edgpolicy/__init__.py new file mode 100644 index 0000000000..eec590dd85 --- /dev/null +++ b/dev_tools/pylibs/edgpolicy/__init__.py @@ -0,0 +1,323 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Shared definitions for the project's commit and file change policies. + +The pre-push hook (fast local feedback) and the policy CI workflow +(enforcement) both read their rules from here so the two cannot drift +apart. Nothing in this module prompts, prints, or exits; callers decide +how to report what they are told. +''' + +import json +import re +import shutil +import subprocess +import tempfile + +from pathlib import Path +from typing import Any, Dict, List, Optional, Sequence, Tuple + +import edgutil + +# +# File paths that begin with the prefixes listed here are excluded from file +# level checking. +# +EXCLUDED_PATH_PREFIXES = ( + # Don't check tests as they're not necessarily "EDG-style". + 'tests/', + # Don't check the generated builtin files as they're very large and there's + # really nothing the user can do but accept them. + 'dev_tools/builtins/builtins_', + # Don't check GitHub workflow yml files; long lines are all but required. + '.github/workflows/', + # Don't check the error_msg.txt file. + 'src/error_msg.txt', + # Don't check AGENT files. + 'AGENTS.md', + '.agents/' +) + +# +# The widest a line may be, measured in columns after tab expansion. +# +MAX_LINE_COLUMNS = 79 + +# +# A commit subject must carry a bracketed tag naming the work it belongs to. +# A tag holds one or more comma separated references, each of which is either +# a GitHub issue ("GH #123") or a legacy internal PR ("EDGcpfe/29032"). +# +SUBJECT_REFERENCE_REGEX = re.compile(r'GH #[0-9]+|EDG[cfjp]+fe/[0-9]+') + +BRACKETED_TAG_REGEX = re.compile(r'\[([^\[\]]*)\]') + +SUBJECT_POLICY_DESCRIPTION = ( + 'Commit subjects must end with a bracketed tag naming at least one ' + 'GitHub issue or legacy PR, for example "[GH #213]", ' + '"[EDGcpfe/29032]", or "[GH #213,GH #214]".' +) + +# +# Whether a commit subject that names no issue is a hard failure. +# +# Commits that belong to no issue have historically carried an empty "[]" +# to satisfy the rule. Whether to keep requiring that is an open policy +# question for the maintainers, so the check currently reports without +# failing. Setting this to True makes it binding again; the pre-push hook +# and the policy CI job both read this one flag, so there is nothing else +# to change. +# +ENFORCE_COMMIT_SUBJECT_POLICY = False + +def is_excluded_path(path_str: str) -> bool: + '''Return True when a repository path is exempt from file level checks.''' + return any( + path_str.startswith(prefix) for prefix in EXCLUDED_PATH_PREFIXES + ) + +def is_text_blob(data: bytes) -> bool: + '''Return True when a blob can be checked as UTF-8 text.''' + if b'\x00' in data: + return False + + try: + data.decode('utf-8') + except UnicodeDecodeError: + return False + + return True + +def _is_reference_list(tag_body: str) -> bool: + '''Return True when a tag's contents are a comma separated reference + list holding at least one reference.''' + if not tag_body.strip(): + return False + + return all( + SUBJECT_REFERENCE_REGEX.fullmatch(reference.strip()) is not None + for reference in tag_body.split(',') + ) + +def commit_subject_error(subject: str) -> Optional[str]: + '''Return a description of why a commit subject is rejected, or None. + + An empty tag ("[]") is not a reference list and so does not satisfy the + policy, even though earlier revisions of the pre-push hook accepted it. + ''' + tags = BRACKETED_TAG_REGEX.findall(subject) + if not tags: + return "Commit subject has no bracketed tag." + + if not any(_is_reference_list(tag) for tag in tags): + if any(not tag.strip() for tag in tags): + return "Commit subject has an empty tag ('[]')." + return "Commit subject's tag is not a valid reference list." + + return None + +class GitRepository: + '''Minimal read-only git helper scoped to one checkout.''' + + def __init__(self, repo_dir: Path) -> None: + self.repo_dir = repo_dir + self._tool_path = edgutil.RequiredToolPath('git') + + def run( + self, + args: Sequence[str], + check: bool = True, + text: bool = True + ) -> 'subprocess.CompletedProcess[Any]': + '''Run a git command in the repository and return the result. + + Capture is spelled out rather than using capture_output, and decoding + with universal_newlines rather than text, because both shorthands + postdate the Python 3.6 floor this tooling still supports. + ''' + return subprocess.run( + [str(self._tool_path), *args], + cwd = str(self.repo_dir), + check = check, + stdout = subprocess.PIPE, + stderr = subprocess.PIPE, + universal_newlines = text + ) + + def output(self, args: Sequence[str]) -> str: + '''Run a git command and return its stripped stdout.''' + return str(self.run(args).stdout).strip() + + def lines(self, args: Sequence[str]) -> List[str]: + '''Run a git command and return its stdout split into lines.''' + return self.output(args).splitlines() + + def show_blob(self, revision_path: str) -> bytes: + '''Return the blob contents for revision:path, or b'' if missing.''' + completed_process = self.run( + ['show', revision_path], check = False, text = False + ) + if completed_process.returncode != 0: + return b'' + return bytes(completed_process.stdout) + + def subject_of(self, commit: str) -> str: + '''Return the subject line of a commit.''' + return self.output(['show', '--no-patch', '--format=%s', commit]) + + def changed_files(self, base_rev: str, head_rev: str) -> List[str]: + '''Return paths modified between two revisions, ignoring deletions.''' + return self.lines([ + 'diff', + '--name-only', + '--diff-filter=d', + f"{base_rev}..{head_rev}" + ]) + +def collect_change_spec( + repository: GitRepository, + base_rev: str, + head_rev: str, + tmp_dir: Path +) -> Dict[str, List[str]]: + '''Materialize both sides of every changed file under tmp_dir. + + Returns the mapping that edg-check-file-changes consumes: repository path + to a [old_file, new_file] pair, each given relative to the repository so + the checker can run inside or outside a container. + + Binary files are left out. The checks behind this all read their input + as UTF-8 text, and handing them an image makes the checker raise part way + through and drop the file from its own results, which reads as a silent + pass. Excluding them here keeps that from happening. + ''' + change_spec = {} + + for path_str in repository.changed_files(base_rev, head_rev): + if is_excluded_path(path_str): + continue + + blobs = [ + repository.show_blob(f"{revision}:{path_str}") + for revision in (base_rev, head_rev) + ] + if not all(is_text_blob(blob) for blob in blobs): + continue + + file_path = Path(path_str) + + def materialize(data: bytes) -> str: + '''Write one side of the change to tmp_dir, returning its path.''' + with tempfile.NamedTemporaryFile( + prefix = file_path.stem, + suffix = file_path.suffix, + dir = str(tmp_dir), + delete = False + ) as file_handle: + file_handle.write(data) + return ( + Path(file_handle.name).relative_to(repository.repo_dir).as_posix() + ) + + change_spec[path_str] = [materialize(blob) for blob in blobs] + + return change_spec + +def run_file_checks( + repository: GitRepository, + change_spec: Dict[str, List[str]], + tmp_dir: Path, + use_docker: bool = False +) -> Optional[Dict[str, Any]]: + '''Run edg-check-file-changes over a change spec. + + Returns the checker's results, or None if the checker itself failed. + ''' + input_file = tmp_dir / 'input.json' + output_file = tmp_dir / 'output.json' + + with open(input_file, 'w') as file_handle: + json.dump(change_spec, file_handle) + + def repo_relative(path: Path) -> str: + return path.relative_to(repository.repo_dir).as_posix() + + if use_docker: + command = [ + str(edgutil.RequiredToolPath('edg-exec')), + '--', + 'edg-check-file-changes', + repo_relative(input_file), + repo_relative(output_file) + ] + else: + command = [ + str(edgutil.RequiredToolPath('edg-check-file-changes')), + str(input_file), + str(output_file) + ] + + completed_process = subprocess.run( + command, + cwd = str(repository.repo_dir), + stdin = subprocess.DEVNULL + ) + if completed_process.returncode != 0: + return None + + with open(output_file, 'r') as file_handle: + results = json.load(file_handle) + + return dict(results) + +def should_use_docker(repo_dir: Path) -> bool: + '''Return True when file checks should run inside the dev container.''' + return ( + shutil.which('docker') is not None and + edgutil.is_docker_preferred(repo_dir) + ) + +def summarize_file_result( + file_result: Dict[str, Any] +) -> List[Tuple[str, str]]: + '''Flatten one file's checker output into (kind, message) findings. + + A None misspelling list means spell checking was unavailable, which is + reported as its own kind so callers can treat it as a warning rather + than a violation. + ''' + findings = [] + + misspellings = file_result['misspellings'] + if misspellings is None: + findings.append( + ('spelling-unavailable', 'aspell not found or missing dictionary') + ) + else: + for misspelling in misspellings: + findings.append(('misspelling', f"new misspelled word: {misspelling}")) + + for typo in file_result['typos']: + findings.append(('typo', f"repeated-word typo: {typo.strip()}")) + + coding_errors = file_result['coding_errors'] + if coding_errors: + for hit in coding_errors['missing_endifs']: + findings.append(('coding-error', f"#endif without a comment: {hit}")) + for hit in coding_errors['missing_elses']: + findings.append(('coding-error', f"#else without a comment: {hit}")) + for hit in coding_errors['closing_comments']: + findings.append(('coding-error', f"closing comment: {hit.strip()}")) + + for line_no in sorted(file_result['overlong_lines'], key = int): + findings.append( + ( + 'overlong-line', + f"line {line_no} exceeds {MAX_LINE_COLUMNS} columns" + ) + ) + + return findings diff --git a/dev_tools/pylibs/edgpolicy/py.typed b/dev_tools/pylibs/edgpolicy/py.typed new file mode 100644 index 0000000000..e69de29bb2 diff --git a/dev_tools/typecheck-python.sh b/dev_tools/typecheck-python.sh index 4772b2f9d0..d137811fc6 100755 --- a/dev_tools/typecheck-python.sh +++ b/dev_tools/typecheck-python.sh @@ -56,6 +56,7 @@ pushd bin/ > /dev/null check_types_strict edg-bench-review check_types_strict edg-bench-run-delta check_types_strict edg-bench-run-save + check_types_strict edg-check-policy check_types edg-cmakedef check_types_strict edg-docker-bench check_types_strict edg-docker-test @@ -112,6 +113,9 @@ pushd pylibs/ > /dev/null pushd edgpack/ > /dev/null check_types __init__.py popd > /dev/null + pushd edgpolicy/ > /dev/null + check_types_strict __init__.py + popd > /dev/null pushd edgshell/ > /dev/null check_types __init__.py popd > /dev/null From ce68be20098a41403bcbff3dc5a1a6b9e7ecdbd8 Mon Sep 17 00:00:00 2001 From: sdarwin Date: Tue, 6 Oct 2026 11:35:26 -0600 Subject: [PATCH 2/3] Restructure policy checks into a single tool [GH #164] Co-authored-by: Cursor --- .git-hooks/pre-push.py | 191 +----- CONTRIBUTING.md | 31 +- HACKING.md | 20 - dev_tools/bin/edg-check-policy | 813 ++++++++++++++++++++----- dev_tools/pylibs/edgpolicy/__init__.py | 323 ---------- dev_tools/pylibs/edgpolicy/py.typed | 0 dev_tools/typecheck-python.sh | 3 - 7 files changed, 702 insertions(+), 679 deletions(-) delete mode 100644 dev_tools/pylibs/edgpolicy/__init__.py delete mode 100644 dev_tools/pylibs/edgpolicy/py.typed diff --git a/.git-hooks/pre-push.py b/.git-hooks/pre-push.py index 88b6c912e6..5301a66a7e 100755 --- a/.git-hooks/pre-push.py +++ b/.git-hooks/pre-push.py @@ -32,20 +32,15 @@ # pushing anything. Information about why the push is rejected may be sent to # the user by writing to standard error. # -# The policy this hook applies is defined in the edgpolicy library, which the -# policy CI workflow also uses. This hook exists to give fast local feedback; -# CI is what actually enforces the policy. +# All of the policy itself lives in edg-check-policy, which CI and users at a +# shell run too. This hook only arranges for a usable terminal and then asks +# that tool to review everything that has not been pushed yet. # import platform +import shutil import subprocess import sys -import tempfile - -import edgpolicy -import edgutil - -from pathlib import Path if platform.system() == 'Windows': # On Windows we need to use the kernel API to detect if this script is @@ -68,172 +63,24 @@ creationflags = subprocess.CREATE_NEW_CONSOLE ) sys.exit(delegate_proc.returncode) -else: - # On Unix-like systems we can regain access to terminal input using /dev/tty. - sys.stdin = open('/dev/tty') - -# -# How each kind of finding reported by edgpolicy is introduced to the user. -# -FINDING_KIND_HEADINGS = { - 'misspelling': 'New misspelled words:', - 'typo': 'New typos:', - 'coding-error': 'Coding errors:', - 'overlong-line': 'Lines too long:' -} - -def print_indented(text, prefix): - '''Print each line of text with the given prefix.''' - if not text: - return - - for line in text.splitlines(): - print(f"{prefix}{line}") - -class PrePushChecker: - '''Runs EDG pre-push policy checks against new commits and changed files.''' - - def __init__(self): - self.errors = 0 - self.repository = edgpolicy.GitRepository(edgutil.find_mono_repo_or_exit()) - - def confirm_okay(self): - '''Ask the user whether a policy warning is acceptable. - - When stdout is a terminal, prompt on /dev/tty for y/n/q. Otherwise record - an error without prompting (e.g. non-interactive pushes). - ''' - if not sys.stdout.isatty(): - self.errors = 1 - return - - tries = 0 - while True: - response = input(' Okay? [ynq] ').lower() - - if response == 'y': - return - if response == 'n': - self.errors = 1 - return - if response == 'q': - self.errors = 1 - sys.exit(1) - - print( - "Please answer 'y' (to accept), 'n' (to continue check) or " - "'q' (to quit)." - ) - tries += 1 - if tries >= 10: - print('Too many attempts.') - sys.exit(1) - - def check_commits(self): - '''Walk commits until one already on a remote branch is found. - - As commits are walked, check to make sure they match formatting guidance. - - Returns the oldest new commit hash, or None if there are no new commits. - A commit subject that fails the policy aborts the push outright when - edgpolicy says the rule is binding, and is otherwise offered for - confirmation like any other finding. - ''' - first_new_commit = None - - commits = self.repository.lines(['log', '--pretty=%H']) - for commit in commits: - # Break if the remote already has this commit. - contains = self.repository.run( - ['branch', '-r', '--contains', commit], check = False - ).stdout.strip() - if contains: - break - print('New commit:') - print('') - show_output = self.repository.output(['show', '--no-patch', commit]) - print_indented(show_output, ' ') - print('') +TOOL_PATH_CHECK_POLICY = shutil.which('edg-check-policy') - subject_error = edgpolicy.commit_subject_error( - self.repository.subject_of(commit) - ) - if subject_error is not None: - print(f" [POLICY] {subject_error}") - print_indented(edgpolicy.SUBJECT_POLICY_DESCRIPTION, ' ') - if edgpolicy.ENFORCE_COMMIT_SUBJECT_POLICY: - sys.exit(1) - self.confirm_okay() - - first_new_commit = commit - - return first_new_commit - - def report_findings(self, findings): - '''Print findings grouped by kind, confirming once per group.''' - for kind, heading in FINDING_KIND_HEADINGS.items(): - messages = [ - message for finding_kind, message in findings - if finding_kind == kind - ] - if not messages: - continue - - print(f" {heading}") - for message in messages: - print(f" - {message}") - self.confirm_okay() - - def check_changed_files(self, first_new_commit): - '''Run per-file policy checks for files changed since - first_new_commit~1.''' - base_rev = f"{first_new_commit}~1" - print('Checking changed files:') - - containing_tmp_dir = self.repository.repo_dir / '.tmp' - containing_tmp_dir.mkdir(exist_ok = True) - - with tempfile.TemporaryDirectory( - dir = str(containing_tmp_dir) - ) as tmp_dir_str: - tmp_dir = Path(tmp_dir_str) - - change_spec = edgpolicy.collect_change_spec( - self.repository, base_rev, 'HEAD', tmp_dir - ) - - use_docker = edgpolicy.should_use_docker(self.repository.repo_dir) - results = edgpolicy.run_file_checks( - self.repository, change_spec, tmp_dir, use_docker = use_docker - ) - if results is None: - sys.exit(1) - - for file_path_str in sorted(results): - print(f" Checking file: {file_path_str}") - - findings = edgpolicy.summarize_file_result(results[file_path_str]) - if any(kind == 'spelling-unavailable' for kind, _ in findings): - print(' Spell check failed:') - if not use_docker: - print(' docker not used.') - print(' aspell not found or missing dictionary.') - - self.report_findings(findings) - - def run(self) -> bool: - '''Execute the full pre-push check sequence.''' - first_new_commit = self.check_commits() - - # Skip per file checks if we didn't actually detect a new commit. - if first_new_commit is not None: - self.check_changed_files(first_new_commit) +def main(): + if TOOL_PATH_CHECK_POLICY is None: + print( + 'edg-check-policy tool is required, but was not found.\n' + 'Has dev-init.py been run, and is dev_tools/bin on PATH?', + file = sys.stderr + ) + sys.exit(1) - return self.errors == 0 + completed_process = subprocess.run([ + TOOL_PATH_CHECK_POLICY, + '--unpushed', + '--interactive' + ]) -def main(): - checker = PrePushChecker() - sys.exit(0 if checker.run() else 1) + sys.exit(completed_process.returncode) main() diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d3db2b9c9c..4cfe353a51 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,33 +81,22 @@ existing issues and discussion (it's possible someone is already working on the item or that there has been significant discussion about the desired implementation characeristics). -### What CI Enforces +### Automated Checks -Some of the items below are checked automatically on every pull request by -the `Policy Check` workflow, which runs the same rules as the `pre-push` -hook installed by `dev-init.py`. - -These **fail** the check: - -- Changed files must stay within 79 columns, must comment their `#endif` - and `#else` directives, and must not contain repeated-word typos. - -These are **reported but do not fail** the check: - -- Commit subjects should carry a bracketed tag naming at least one GitHub - issue or legacy PR, such as `[GH #213]` or `[EDGcpfe/29032]`. Whether - commits that belong to no issue must still carry an empty `[]` is an - open question, so this is advisory for now. -- Spelling, because there is no project dictionary and a compiler code - base trips `aspell` constantly. Please still read the spelling - annotations; they catch real mistakes. - -You can run the same check locally before opening a pull request: +Some of the checklist below is checked automatically when you open a pull +request. `edg-check-policy` is the tool that does it, and you can run it +yourself at any point to see what a pull request would report: ```sh edg-check-policy --base-rev origin/main ``` +Commit subjects must name the work they belong to, and changed files must +follow the project's coding style; both will fail the check. Spelling is +reported but does not fail, because there is no project dictionary and a +compiler code base trips `aspell` constantly. Please still read what it +says; it catches real mistakes. + ### Checklist for Success > [!NOTE] diff --git a/HACKING.md b/HACKING.md index d2658d7acf..0cb7261fdd 100644 --- a/HACKING.md +++ b/HACKING.md @@ -12,26 +12,6 @@ As a result it is *highly recommended* to use the Docker based development process as this will typically provide the best experience (even if directly running a Linux-based operation system). -## Project Policy Checks - -`dev-init.py` installs a `pre-push` hook that checks new commits against the -project's commit message and coding style policies. The hook exists to tell -you about problems before you push; it is not what enforces them. It is only -present if you ran `dev-init.py`, it is skipped by `git push --no-verify`, and -it never runs for work done through the GitHub web interface. - -Enforcement happens in the `Policy Check` workflow, which applies the same -rules from the same `edgpolicy` library when a pull request is opened. To -run those checks yourself at any point: - -```sh -edg-check-policy --base-rev origin/main -``` - -Use `--strict` to also fail on the spelling findings that the workflow only -reports as warnings. See `.github/rulesets/README.md` for how the checks are -tied to branch protection. - ## Prerequisites The first step (regardless of Docker use) is to make EDG development tools diff --git a/dev_tools/bin/edg-check-policy b/dev_tools/bin/edg-check-policy index b4be3c6134..8386edb144 100755 --- a/dev_tools/bin/edg-check-policy +++ b/dev_tools/bin/edg-check-policy @@ -5,26 +5,70 @@ # See https://edgcpp.org/LICENSE.txt for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -'''Check a revision range against the project's commit and file policies. +'''Check a range of commits against the project's commit and file policies. -This is the non-interactive counterpart to the pre-push hook. Both read -their rules from edgpolicy, but where the hook asks the developer whether -a finding is acceptable, this reports every finding and fails, which is -what a required status check needs to do. +This is the high level policy tool. It decides what to check and how to +report it, and delegates the per-file work to edg-check-file-changes. The +pre-push hook, CI, and anyone at a shell all run this same tool; the only +differences are the revision range and whether findings are offered for +interactive review. ''' import argparse +import json import os +import platform +import re +import shutil +import subprocess import sys import tempfile from pathlib import Path -from typing import FrozenSet, List, Optional +from typing import Any, Dict, List, NamedTuple, Optional, Sequence -import edgpolicy import edgutil -COMMIT_SUBJECT_FINDING_KIND = 'commit-subject' +# +# File paths that begin with the prefixes listed here are excluded from file +# level checking. +# +EXCLUDED_PATH_PREFIXES = ( + # Don't check tests as they're not necessarily "EDG-style". + 'tests/', + # Don't check the generated builtin files as they're very large and there's + # really nothing the user can do but accept them. + 'dev_tools/builtins/builtins_', + # Don't check GitHub workflow yml files; long lines are all but required. + '.github/workflows/', + # Don't check the error_msg.txt file. + 'src/error_msg.txt', + # Don't check AGENT files. + 'AGENTS.md', + '.agents/' +) + +# +# The widest a line may be, measured in columns after tab expansion. This +# mirrors the limit edg-check-file-changes applies; it is used here only to +# describe a finding. +# +MAX_LINE_COLUMNS = 79 + +# +# A commit subject must carry a bracketed tag naming the work it belongs to. +# A tag holds one or more comma separated references, each of which is either +# a GitHub issue ("GH #123") or a legacy internal PR ("EDGcpfe/29032"). +# +SUBJECT_REFERENCE_REGEX = re.compile(r'GH #[0-9]+|EDG[cfjp]+fe/[0-9]+') + +BRACKETED_TAG_REGEX = re.compile(r'\[([^\[\]]*)\]') + +SUBJECT_POLICY_DESCRIPTION = ( + 'Commit subjects must end with a bracketed tag naming at least one ' + 'GitHub issue or legacy PR, for example "[GH #213]", ' + '"[EDGcpfe/29032]", or "[GH #213,GH #214]".' +) # # Findings of these kinds are reported without failing the run. @@ -33,149 +77,595 @@ COMMIT_SUBJECT_FINDING_KIND = 'commit-subject' # it reports identifiers, standards vocabulary, and abbreviations as new # misspellings on essentially every change; measured against recent history # it fires between 1 and 52 times per commit while the mechanical checks -# fire between 0 and 3 times. Spelling is therefore advisory, and the -# mechanical checks are enforced. Use --strict to fail on everything. +# fire between 0 and 3 times. Spelling is therefore advisory and the +# mechanical checks are enforced. --strict enforces everything. # ADVISORY_FINDING_KINDS = frozenset([ 'misspelling', - 'spelling-unavailable' + 'spelling-unavailable', + 'checker-no-result' ]) -def default_warning_kinds() -> FrozenSet[str]: - '''Return the finding kinds that should be reported but not enforced. +# +# git's empty tree, used as the base when the range starts at a root commit +# and so has no parent to compare against. +# +EMPTY_TREE_OBJECT = '4b825dc642cb6eb9a060e54bf8d69288fbee4904' + +class Finding(NamedTuple): + '''One policy finding, for severity accounting and CI annotations. + + The human readable form is printed separately by whichever check produced + it, so that each kind of finding keeps its own presentation. + ''' + kind: str + message: str + path: Optional[str] = None + line: Optional[int] = None + +def is_excluded_path(path_str: str) -> bool: + '''Return True when a repository path is exempt from file level checks.''' + return any( + path_str.startswith(prefix) for prefix in EXCLUDED_PATH_PREFIXES + ) + +def is_text_blob(data: bytes) -> bool: + '''Return True when a blob can be checked as UTF-8 text.''' + if b'\x00' in data: + return False + + try: + data.decode('utf-8') + except UnicodeDecodeError: + return False + + return True + +def _is_reference_list(tag_body: str) -> bool: + '''Return True when a tag's contents are a comma separated reference + list holding at least one reference.''' + if not tag_body.strip(): + return False + + return all( + SUBJECT_REFERENCE_REGEX.fullmatch(reference.strip()) is not None + for reference in tag_body.split(',') + ) + +def check_for_subject_errors(subject: str) -> List[str]: + '''Return the reasons a commit subject fails the policy, if any. - The commit subject rule joins them unless edgpolicy says it is binding. + An empty tag ("[]") does not name any work and so does not satisfy the + policy, even though the regex this replaced accepted it. + # + # TO RELAX THIS RULE: to go back to accepting commits that reference no + # issue, return an empty list for the two "no tag" cases below and keep + # only the malformed reference list case. Nothing else needs to change. + # ''' - if edgpolicy.ENFORCE_COMMIT_SUBJECT_POLICY: - return ADVISORY_FINDING_KINDS - return ADVISORY_FINDING_KINDS | {COMMIT_SUBJECT_FINDING_KIND} + errors = [] + + tags = BRACKETED_TAG_REGEX.findall(subject) + if not tags: + errors.append('Commit subject has no bracketed tag.') + elif not any(_is_reference_list(tag) for tag in tags): + if any(not tag.strip() for tag in tags): + errors.append("Commit subject has an empty tag ('[]').") + else: + errors.append("Commit subject's tag is not a valid reference list.") + + return errors + +def print_indented(text: str, prefix: str) -> None: + '''Print each line of text with the given prefix.''' + if not text: + return + + for line in text.splitlines(): + print(f"{prefix}{line}") + +def _leading_line_number(hit: str) -> Optional[int]: + '''Return the line number from a ":" checker hit, if present.''' + line_no, _, _ = hit.partition(':') + if line_no.isdigit(): + return int(line_no) + return None + +class GitRepository: + '''Minimal read-only git helper scoped to one checkout.''' + + def __init__(self, repo_dir: Path) -> None: + self.repo_dir = repo_dir + self._tool_path = edgutil.RequiredToolPath('git') + + def run( + self, + args: Sequence[str], + check: bool = True, + text: bool = True + ) -> 'subprocess.CompletedProcess[Any]': + '''Run a git command in the repository and return the result. + + Capture is spelled out rather than using capture_output, and decoding + with universal_newlines rather than text, because both shorthands + postdate the Python 3.6 floor this tooling still supports. + ''' + return subprocess.run( + [str(self._tool_path), *args], + cwd = str(self.repo_dir), + check = check, + stdout = subprocess.PIPE, + stderr = subprocess.PIPE, + universal_newlines = text + ) + + def output(self, args: Sequence[str]) -> str: + '''Run a git command and return its stripped stdout.''' + return str(self.run(args).stdout).strip() + + def lines(self, args: Sequence[str]) -> List[str]: + '''Run a git command and return its stdout split into lines.''' + return self.output(args).splitlines() + + def show_blob(self, revision_path: str) -> bytes: + '''Return the blob contents for revision:path, or b'' if missing.''' + completed_process = self.run( + ['show', revision_path], check = False, text = False + ) + if completed_process.returncode != 0: + return b'' + return bytes(completed_process.stdout) + + def subject_of(self, commit: str) -> str: + '''Return the subject line of a commit.''' + return self.output(['show', '--no-patch', '--format=%s', commit]) + + def is_on_a_remote(self, commit: str) -> bool: + '''Return True when some remote branch already contains a commit.''' + completed_process = self.run( + ['branch', '-r', '--contains', commit], check = False + ) + return bool(str(completed_process.stdout).strip()) + + def parent_or_empty_tree(self, commit: str) -> str: + '''Return a commit's first parent, or the empty tree for a root commit.''' + completed_process = self.run( + ['rev-parse', '--verify', f"{commit}^"], check = False + ) + if completed_process.returncode != 0: + return EMPTY_TREE_OBJECT + return str(completed_process.stdout).strip() class PolicyReporter: - '''Collects findings and prints them for humans and for GitHub.''' + '''Accounts for findings and reports them to the user and to GitHub. - def __init__(self, warning_kinds: FrozenSet[str]) -> None: - self.violations = 0 - self.warning_kinds = warning_kinds + Each check prints its own findings in whatever form reads best, then + hands them here. In interactive mode a group of findings is offered for + confirmation, which is how the pre-push hook has always behaved. + Otherwise each finding is counted and annotated for GitHub. + ''' + + def __init__(self, interactive: bool = False, strict: bool = False) -> None: + self.interactive = interactive + self.strict = strict + self.errors = 0 self.in_github_actions = 'GITHUB_ACTIONS' in os.environ - def report(self, kind: str, message: str, path: Optional[str]) -> None: - '''Record a finding at the severity configured for its kind.''' - if kind in self.warning_kinds: - self.warning(message, path) - else: - self.violation(message, path) + def _is_advisory(self, kind: str) -> bool: + return not self.strict and kind in ADVISORY_FINDING_KINDS - def _annotate(self, level: str, message: str, path: Optional[str]) -> None: + def _annotate(self, level: str, finding: Finding) -> None: if not self.in_github_actions: return - location = f" file={path}" if path else '' + + location = '' + if finding.path is not None: + location = f" file={finding.path}" + if finding.line is not None: + location += f",line={finding.line}" + # Annotations must be one line to be parsed by the Actions runner, and # these messages can quote a line of the diff, so flatten every line # break rather than just the newline. - flattened = message.replace('\r', ' ').replace('\n', ' ') + flattened = finding.message.replace('\r', ' ').replace('\n', ' ') print(f"::{level}{location}::{flattened}") - def violation(self, message: str, path: Optional[str] = None) -> None: - self.violations += 1 - print(f" [POLICY] {message}") - self._annotate('error', message, path) - - def warning(self, message: str, path: Optional[str] = None) -> None: - print(f" [WARNING] {message}") - self._annotate('warning', message, path) - -def check_commit_subjects( - repository: edgpolicy.GitRepository, - reporter: PolicyReporter, - base_rev: str, - head_rev: str -) -> None: - '''Check every commit subject in base_rev..head_rev.''' - commits = repository.lines( - ['log', '--no-merges', '--pretty=%H', f"{base_rev}..{head_rev}"] - ) + def confirm_okay(self) -> None: + '''Ask the user whether a policy warning is acceptable. - if not commits: - print('No new commits to check.') - return + When stdout is a terminal, prompt on stdin for y/n/q. Otherwise record + an error without prompting (e.g. non-interactive pushes). + ''' + if not sys.stdout.isatty(): + self.errors += 1 + return - print(f"Checking {len(commits)} commit subject(s):") - rejected = 0 - for commit in commits: - subject = repository.subject_of(commit) - short_hash = commit[:12] - print(f" {short_hash} {subject}") - - error = edgpolicy.commit_subject_error(subject) - if error is not None: - rejected += 1 - reporter.report( - COMMIT_SUBJECT_FINDING_KIND, f"{short_hash}: {error}", None + tries = 0 + while True: + response = input(' Okay? [ynq] ').lower() + + if response == 'y': + return + if response == 'n': + self.errors += 1 + return + if response == 'q': + self.errors += 1 + sys.exit(1) + + print( + "Please answer 'y' (to accept), 'n' (to continue check) or " + "'q' (to quit)." ) + tries += 1 + if tries >= 10: + print('Too many attempts.') + sys.exit(1) + + def report(self, findings: List[Finding]) -> None: + '''Take a group of already printed findings.''' + if not findings: + return - if rejected: - print('') - print(edgpolicy.SUBJECT_POLICY_DESCRIPTION) - -def check_changed_files( - repository: edgpolicy.GitRepository, - reporter: PolicyReporter, - base_rev: str, - head_rev: str, - allow_docker: bool = True -) -> None: - '''Run the file level checks over base_rev..head_rev.''' - containing_tmp_dir = repository.repo_dir / '.tmp' - containing_tmp_dir.mkdir(exist_ok = True) - - with tempfile.TemporaryDirectory( - dir = str(containing_tmp_dir) - ) as tmp_dir_str: - tmp_dir = Path(tmp_dir_str) - - change_spec = edgpolicy.collect_change_spec( - repository, base_rev, head_rev, tmp_dir - ) - if not change_spec: - print('No changed files to check.') + if self.interactive: + self.confirm_okay() return - print(f"Checking {len(change_spec)} changed file(s):") - results = edgpolicy.run_file_checks( - repository, - change_spec, - tmp_dir, - use_docker = ( - allow_docker and edgpolicy.should_use_docker(repository.repo_dir) - ) + for finding in findings: + if self._is_advisory(finding.kind): + self._annotate('warning', finding) + else: + self.errors += 1 + self._annotate('error', finding) + + def report_fatal(self, findings: List[Finding]) -> None: + '''Take findings that end the run, whether or not review was offered. + + Used for the commit subject rule, which is enforced rather than + advisory: there is no point reviewing the rest of a change when the + first thing to fix is the history. + ''' + for finding in findings: + self.errors += 1 + self._annotate('error', finding) + +class PolicyChecker: + '''Runs the project's policy checks over a range of commits.''' + + def __init__( + self, + repository: GitRepository, + reporter: PolicyReporter, + use_docker: bool = False + ) -> None: + self.repository = repository + self.reporter = reporter + self.use_docker = use_docker + + def unpushed_commits(self) -> List[str]: + '''Return the commits on HEAD that no remote branch contains yet. + + This is the range the pre-push hook cares about: everything the user is + about to publish, newest first. + ''' + commits = [] + for commit in self.repository.lines(['log', '--pretty=%H']): + if self.repository.is_on_a_remote(commit): + break + commits.append(commit) + return commits + + def range_commits(self, base_rev: str, head_rev: str) -> List[str]: + '''Return the non-merge commits in base_rev..head_rev, newest first.''' + return self.repository.lines( + ['log', '--no-merges', '--pretty=%H', f"{base_rev}..{head_rev}"] + ) + + def check_commit_subjects(self, commits: List[str], verbose: bool) -> None: + '''Check every commit subject, aborting on the first failure. + + The subject rule is enforced rather than advisory, so a bad subject + stops the run instead of being offered for confirmation. There is no + point checking the rest when the first thing to fix is the history. + ''' + for commit in commits: + subject = self.repository.subject_of(commit) + + if verbose: + print('New commit:') + print('') + print_indented( + self.repository.output(['show', '--no-patch', commit]), ' ' + ) + print('') + else: + print(f" {commit[:12]} {subject}") + + subject_errors = check_for_subject_errors(subject) + if subject_errors: + for subject_error in subject_errors: + print(f" [POLICY] {subject_error}") + print_indented(SUBJECT_POLICY_DESCRIPTION, ' ') + self.reporter.report_fatal([ + Finding('commit-subject', f"{commit[:12]}: {subject_error}") + for subject_error in subject_errors + ]) + sys.exit(1) + + def collect_change_spec( + self, + base_rev: str, + head_rev: str, + tmp_dir: Path + ) -> Dict[str, List[str]]: + '''Materialize both sides of every changed file under tmp_dir. + + Returns the mapping edg-check-file-changes consumes: repository path to + an [old_file, new_file] pair, each relative to the repository so the + checker can run inside or outside a container. + + Binary files are left out. Everything downstream reads its input as + UTF-8 text, and handing it an image makes the checker raise part way + through and drop the file from its own results, which reads as a silent + pass. + ''' + change_spec = {} + + changed_files = self.repository.lines([ + 'diff', '--name-only', '--diff-filter=d', f"{base_rev}..{head_rev}" + ]) + + for path_str in changed_files: + if is_excluded_path(path_str): + continue + + blobs = [ + self.repository.show_blob(f"{revision}:{path_str}") + for revision in (base_rev, head_rev) + ] + if not all(is_text_blob(blob) for blob in blobs): + continue + + file_path = Path(path_str) + + def materialize(data: bytes) -> str: + '''Write one side of the change out, returning its relative path.''' + with tempfile.NamedTemporaryFile( + prefix = file_path.stem, + suffix = file_path.suffix, + dir = str(tmp_dir), + delete = False + ) as file_handle: + file_handle.write(data) + return Path(file_handle.name).relative_to( + self.repository.repo_dir + ).as_posix() + + change_spec[path_str] = [materialize(blob) for blob in blobs] + + return change_spec + + def run_file_checks( + self, + change_spec: Dict[str, List[str]], + tmp_dir: Path + ) -> Optional[Dict[str, Any]]: + '''Run edg-check-file-changes over a change spec. + + Returns the checker's results, or None if the checker itself failed. + ''' + input_file = tmp_dir / 'input.json' + output_file = tmp_dir / 'output.json' + + with open(input_file, 'w') as file_handle: + json.dump(change_spec, file_handle) + + def repo_relative(path: Path) -> str: + return path.relative_to(self.repository.repo_dir).as_posix() + + if self.use_docker: + command = [ + str(edgutil.RequiredToolPath('edg-exec')), + '--', + 'edg-check-file-changes', + repo_relative(input_file), + repo_relative(output_file) + ] + else: + command = [ + str(edgutil.RequiredToolPath('edg-check-file-changes')), + str(input_file), + str(output_file) + ] + + completed_process = subprocess.run( + command, + cwd = str(self.repository.repo_dir), + stdin = subprocess.DEVNULL ) + if completed_process.returncode != 0: + return None + + with open(output_file, 'r') as file_handle: + results = json.load(file_handle) + + return dict(results) + + def _report_spelling( + self, + path_str: str, + file_results: Dict[str, Any] + ) -> None: + misspellings = file_results['misspellings'] + + if misspellings is None: + print(' Spell check failed:') + if not self.use_docker: + print(' docker not used.') + print(' aspell not found or missing dictionary.') + self.reporter.report([ + Finding( + 'spelling-unavailable', + f"{path_str}: aspell not found or missing dictionary", + path_str + ) + ]) + return - if results is None: - edgutil.eprint('error: edg-check-file-changes failed to run') - reporter.violation('File change checks could not be run.') + if len(misspellings) != 0: + print(' New misspelled words:') + for misspelling in misspellings: + print(f" - {misspelling}") + self.reporter.report([ + Finding( + 'misspelling', + f"{path_str}: new misspelled word: {misspelling}", + path_str + ) + for misspelling in misspellings + ]) + + def _report_typos( + self, + path_str: str, + file_results: Dict[str, Any] + ) -> None: + typos = file_results['typos'] + if len(typos) == 0: return - # The checker drops a file from its results if checking it raised, so - # a missing entry means the file went unchecked rather than passed. - for path_str in sorted(set(change_spec) - set(results)): - reporter.warning( - f"{path_str}: the file checker returned no result for this file", - path_str + print(' New typos:') + for typo in typos: + print(f" - {typo}") + self.reporter.report([ + Finding( + 'typo', f"{path_str}: repeated-word typo: {typo.strip()}", path_str ) + for typo in typos + ]) + + def _report_coding_errors( + self, + path_str: str, + file_results: Dict[str, Any] + ) -> None: + coding_errors = file_results['coding_errors'] + if len(coding_errors) == 0: + return - for path_str in sorted(results): - findings = edgpolicy.summarize_file_result(results[path_str]) - if not findings: + for key, heading, description in ( + ('missing_endifs', 'Missing comment on #endif:', '#endif'), + ('missing_elses', 'Missing comment on #else:', '#else') + ): + hits = coding_errors[key] + if len(hits) == 0: continue - print(f" {path_str}:") - for kind, message in findings: - reporter.report(kind, f"{path_str}: {message}", path_str) + print(f" {heading}") + print_indented('\n'.join(hits), ' ') + self.reporter.report([ + Finding( + 'coding-error', + f"{path_str}: {description} without a comment", + path_str, + _leading_line_number(hit) + ) + for hit in hits + ]) + + closing_comments = coding_errors['closing_comments'] + if len(closing_comments) != 0: + print_indented('\n'.join(closing_comments), ' ') + self.reporter.report([ + Finding( + 'coding-error', + f"{path_str}: closing comment: {hit.strip()}", + path_str + ) + for hit in closing_comments + ]) + + def _report_overlong_lines( + self, + path_str: str, + file_results: Dict[str, Any] + ) -> None: + '''Print each overlong line with its surrounding context. + + The window is delimited with '=' so that it is obvious at a glance + which part of the file is too wide. + ''' + overlong_lines = file_results['overlong_lines'] + if len(overlong_lines) == 0: + return + + for line_no, (line_start, lines) in overlong_lines.items(): + print(f" Line {line_no} too long:") + line_end = line_start + (len(lines) - 1) + print(f" {line_start} -> {line_end} ".center(MAX_LINE_COLUMNS, '=')) + for line in lines: + print(line, end = '') + print('=' * MAX_LINE_COLUMNS) + + self.reporter.report([ + Finding( + 'overlong-line', + f"{path_str}: line {line_no} exceeds {MAX_LINE_COLUMNS} columns", + path_str, + int(line_no) + ) + for line_no in sorted(overlong_lines, key = int) + ]) + + def check_changed_files(self, base_rev: str, head_rev: str) -> None: + '''Run the file level checks over base_rev..head_rev.''' + print('Checking changed files:') + + containing_tmp_dir = self.repository.repo_dir / '.tmp' + containing_tmp_dir.mkdir(exist_ok = True) + + with tempfile.TemporaryDirectory( + dir = str(containing_tmp_dir) + ) as tmp_dir_str: + tmp_dir = Path(tmp_dir_str) + + change_spec = self.collect_change_spec(base_rev, head_rev, tmp_dir) + if not change_spec: + print(' No changed files to check.') + return + + results = self.run_file_checks(change_spec, tmp_dir) + if results is None: + edgutil.eprint('error: edg-check-file-changes failed to run') + sys.exit(1) + + # The checker drops a file from its results if checking it raised, so + # a missing entry means the file went unchecked rather than passed. + for path_str in sorted(set(change_spec) - set(results)): + print(f" Checking file: {path_str}") + print(' No result returned by edg-check-file-changes.') + self.reporter.report([ + Finding( + 'checker-no-result', + f"{path_str}: the file checker returned no result", + path_str + ) + ]) + + for path_str in sorted(results): + print(f" Checking file: {path_str}") + file_results = results[path_str] + + self._report_spelling(path_str, file_results) + self._report_typos(path_str, file_results) + self._report_coding_errors(path_str, file_results) + self._report_overlong_lines(path_str, file_results) def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: parser = argparse.ArgumentParser( description = ( - 'Check a revision range against the project commit and file policies.' + 'Check a range of commits against the project commit and file ' + 'policies.' ) ) parser.add_argument( @@ -188,6 +678,22 @@ def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: default = 'HEAD', help = 'the revision the range ends at (default: HEAD)' ) + parser.add_argument( + '--unpushed', + action = 'store_true', + help = ( + 'check every commit on HEAD that no remote branch contains yet, ' + 'instead of an explicit range; this is what the pre-push hook wants' + ) + ) + parser.add_argument( + '--interactive', + action = 'store_true', + help = ( + 'offer each group of findings for review instead of failing on it, ' + 'reading the answer from the terminal' + ) + ) parser.add_argument( '--skip-commits', action = 'store_true', @@ -201,7 +707,7 @@ def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: parser.add_argument( '--strict', action = 'store_true', - help = 'fail on advisory findings (spelling, commit subjects) as well' + help = 'fail on advisory findings (spelling) as well' ) parser.add_argument( '--no-docker', @@ -213,47 +719,74 @@ def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: ) return parser.parse_args(argv) +def _reopen_terminal_input() -> None: + '''Point stdin at the terminal so findings can be reviewed. + + A hook is handed the list of refs being pushed on stdin, so interactive + review has to read the user's answers from the terminal instead. On + Windows the wrapper has already arranged for a console, whose stdin is + usable as it is. + ''' + if platform.system() != 'Windows': + sys.stdin = open('/dev/tty') + def main() -> None: args = parse_args() + if args.interactive: + _reopen_terminal_input() + repo_dir = edgutil.find_mono_repo_or_exit() - repository = edgpolicy.GitRepository(repo_dir) + repository = GitRepository(repo_dir) reporter = PolicyReporter( - frozenset() if args.strict else default_warning_kinds() + interactive = args.interactive, strict = args.strict ) - - # Compare against the point the branch left the base so that commits - # already on the base branch are not re-checked. - merge_base = repository.run( - ['merge-base', args.base_rev, args.head_rev], check = False + use_docker = ( + not args.no_docker and + shutil.which('docker') is not None and + edgutil.is_docker_preferred(repo_dir) ) - if merge_base.returncode != 0: - edgutil.eprint( - f"error: no merge base for {args.base_rev} and {args.head_rev}" + checker = PolicyChecker(repository, reporter, use_docker = use_docker) + + if args.unpushed: + commits = checker.unpushed_commits() + base_rev = ( + repository.parent_or_empty_tree(commits[-1]) if commits else None ) - sys.exit(1) - base_rev = str(merge_base.stdout).strip() + head_rev = 'HEAD' + else: + merge_base = repository.run( + ['merge-base', args.base_rev, args.head_rev], check = False + ) + if merge_base.returncode != 0: + edgutil.eprint( + f"error: no merge base for {args.base_rev} and {args.head_rev}" + ) + sys.exit(1) + base_rev = str(merge_base.stdout).strip() + head_rev = args.head_rev + commits = checker.range_commits(base_rev, head_rev) + + print(f"Base: {base_rev}") + print(f"Head: {repository.output(['rev-parse', head_rev])}") + print('') - print(f"Base: {base_rev}") - print(f"Head: {repository.output(['rev-parse', args.head_rev])}") - print('') + if not commits: + print('No new commits to check.') + return if not args.skip_commits: - check_commit_subjects(repository, reporter, base_rev, args.head_rev) + if not args.unpushed: + print(f"Checking {len(commits)} commit subject(s):") + checker.check_commit_subjects(commits, verbose = args.unpushed) print('') - if not args.skip_files: - check_changed_files( - repository, - reporter, - base_rev, - args.head_rev, - allow_docker = not args.no_docker - ) + if not args.skip_files and base_rev is not None: + checker.check_changed_files(base_rev, head_rev) print('') - if reporter.violations: - print(f"Failed: {reporter.violations} policy violation(s).") + if reporter.errors: + print(f"Failed: {reporter.errors} policy finding(s) not accepted.") sys.exit(1) print('Passed: no policy violations.') diff --git a/dev_tools/pylibs/edgpolicy/__init__.py b/dev_tools/pylibs/edgpolicy/__init__.py deleted file mode 100644 index eec590dd85..0000000000 --- a/dev_tools/pylibs/edgpolicy/__init__.py +++ /dev/null @@ -1,323 +0,0 @@ -# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM -# Exceptions. -# See https://edgcpp.org/LICENSE.txt for license information. -# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -'''Shared definitions for the project's commit and file change policies. - -The pre-push hook (fast local feedback) and the policy CI workflow -(enforcement) both read their rules from here so the two cannot drift -apart. Nothing in this module prompts, prints, or exits; callers decide -how to report what they are told. -''' - -import json -import re -import shutil -import subprocess -import tempfile - -from pathlib import Path -from typing import Any, Dict, List, Optional, Sequence, Tuple - -import edgutil - -# -# File paths that begin with the prefixes listed here are excluded from file -# level checking. -# -EXCLUDED_PATH_PREFIXES = ( - # Don't check tests as they're not necessarily "EDG-style". - 'tests/', - # Don't check the generated builtin files as they're very large and there's - # really nothing the user can do but accept them. - 'dev_tools/builtins/builtins_', - # Don't check GitHub workflow yml files; long lines are all but required. - '.github/workflows/', - # Don't check the error_msg.txt file. - 'src/error_msg.txt', - # Don't check AGENT files. - 'AGENTS.md', - '.agents/' -) - -# -# The widest a line may be, measured in columns after tab expansion. -# -MAX_LINE_COLUMNS = 79 - -# -# A commit subject must carry a bracketed tag naming the work it belongs to. -# A tag holds one or more comma separated references, each of which is either -# a GitHub issue ("GH #123") or a legacy internal PR ("EDGcpfe/29032"). -# -SUBJECT_REFERENCE_REGEX = re.compile(r'GH #[0-9]+|EDG[cfjp]+fe/[0-9]+') - -BRACKETED_TAG_REGEX = re.compile(r'\[([^\[\]]*)\]') - -SUBJECT_POLICY_DESCRIPTION = ( - 'Commit subjects must end with a bracketed tag naming at least one ' - 'GitHub issue or legacy PR, for example "[GH #213]", ' - '"[EDGcpfe/29032]", or "[GH #213,GH #214]".' -) - -# -# Whether a commit subject that names no issue is a hard failure. -# -# Commits that belong to no issue have historically carried an empty "[]" -# to satisfy the rule. Whether to keep requiring that is an open policy -# question for the maintainers, so the check currently reports without -# failing. Setting this to True makes it binding again; the pre-push hook -# and the policy CI job both read this one flag, so there is nothing else -# to change. -# -ENFORCE_COMMIT_SUBJECT_POLICY = False - -def is_excluded_path(path_str: str) -> bool: - '''Return True when a repository path is exempt from file level checks.''' - return any( - path_str.startswith(prefix) for prefix in EXCLUDED_PATH_PREFIXES - ) - -def is_text_blob(data: bytes) -> bool: - '''Return True when a blob can be checked as UTF-8 text.''' - if b'\x00' in data: - return False - - try: - data.decode('utf-8') - except UnicodeDecodeError: - return False - - return True - -def _is_reference_list(tag_body: str) -> bool: - '''Return True when a tag's contents are a comma separated reference - list holding at least one reference.''' - if not tag_body.strip(): - return False - - return all( - SUBJECT_REFERENCE_REGEX.fullmatch(reference.strip()) is not None - for reference in tag_body.split(',') - ) - -def commit_subject_error(subject: str) -> Optional[str]: - '''Return a description of why a commit subject is rejected, or None. - - An empty tag ("[]") is not a reference list and so does not satisfy the - policy, even though earlier revisions of the pre-push hook accepted it. - ''' - tags = BRACKETED_TAG_REGEX.findall(subject) - if not tags: - return "Commit subject has no bracketed tag." - - if not any(_is_reference_list(tag) for tag in tags): - if any(not tag.strip() for tag in tags): - return "Commit subject has an empty tag ('[]')." - return "Commit subject's tag is not a valid reference list." - - return None - -class GitRepository: - '''Minimal read-only git helper scoped to one checkout.''' - - def __init__(self, repo_dir: Path) -> None: - self.repo_dir = repo_dir - self._tool_path = edgutil.RequiredToolPath('git') - - def run( - self, - args: Sequence[str], - check: bool = True, - text: bool = True - ) -> 'subprocess.CompletedProcess[Any]': - '''Run a git command in the repository and return the result. - - Capture is spelled out rather than using capture_output, and decoding - with universal_newlines rather than text, because both shorthands - postdate the Python 3.6 floor this tooling still supports. - ''' - return subprocess.run( - [str(self._tool_path), *args], - cwd = str(self.repo_dir), - check = check, - stdout = subprocess.PIPE, - stderr = subprocess.PIPE, - universal_newlines = text - ) - - def output(self, args: Sequence[str]) -> str: - '''Run a git command and return its stripped stdout.''' - return str(self.run(args).stdout).strip() - - def lines(self, args: Sequence[str]) -> List[str]: - '''Run a git command and return its stdout split into lines.''' - return self.output(args).splitlines() - - def show_blob(self, revision_path: str) -> bytes: - '''Return the blob contents for revision:path, or b'' if missing.''' - completed_process = self.run( - ['show', revision_path], check = False, text = False - ) - if completed_process.returncode != 0: - return b'' - return bytes(completed_process.stdout) - - def subject_of(self, commit: str) -> str: - '''Return the subject line of a commit.''' - return self.output(['show', '--no-patch', '--format=%s', commit]) - - def changed_files(self, base_rev: str, head_rev: str) -> List[str]: - '''Return paths modified between two revisions, ignoring deletions.''' - return self.lines([ - 'diff', - '--name-only', - '--diff-filter=d', - f"{base_rev}..{head_rev}" - ]) - -def collect_change_spec( - repository: GitRepository, - base_rev: str, - head_rev: str, - tmp_dir: Path -) -> Dict[str, List[str]]: - '''Materialize both sides of every changed file under tmp_dir. - - Returns the mapping that edg-check-file-changes consumes: repository path - to a [old_file, new_file] pair, each given relative to the repository so - the checker can run inside or outside a container. - - Binary files are left out. The checks behind this all read their input - as UTF-8 text, and handing them an image makes the checker raise part way - through and drop the file from its own results, which reads as a silent - pass. Excluding them here keeps that from happening. - ''' - change_spec = {} - - for path_str in repository.changed_files(base_rev, head_rev): - if is_excluded_path(path_str): - continue - - blobs = [ - repository.show_blob(f"{revision}:{path_str}") - for revision in (base_rev, head_rev) - ] - if not all(is_text_blob(blob) for blob in blobs): - continue - - file_path = Path(path_str) - - def materialize(data: bytes) -> str: - '''Write one side of the change to tmp_dir, returning its path.''' - with tempfile.NamedTemporaryFile( - prefix = file_path.stem, - suffix = file_path.suffix, - dir = str(tmp_dir), - delete = False - ) as file_handle: - file_handle.write(data) - return ( - Path(file_handle.name).relative_to(repository.repo_dir).as_posix() - ) - - change_spec[path_str] = [materialize(blob) for blob in blobs] - - return change_spec - -def run_file_checks( - repository: GitRepository, - change_spec: Dict[str, List[str]], - tmp_dir: Path, - use_docker: bool = False -) -> Optional[Dict[str, Any]]: - '''Run edg-check-file-changes over a change spec. - - Returns the checker's results, or None if the checker itself failed. - ''' - input_file = tmp_dir / 'input.json' - output_file = tmp_dir / 'output.json' - - with open(input_file, 'w') as file_handle: - json.dump(change_spec, file_handle) - - def repo_relative(path: Path) -> str: - return path.relative_to(repository.repo_dir).as_posix() - - if use_docker: - command = [ - str(edgutil.RequiredToolPath('edg-exec')), - '--', - 'edg-check-file-changes', - repo_relative(input_file), - repo_relative(output_file) - ] - else: - command = [ - str(edgutil.RequiredToolPath('edg-check-file-changes')), - str(input_file), - str(output_file) - ] - - completed_process = subprocess.run( - command, - cwd = str(repository.repo_dir), - stdin = subprocess.DEVNULL - ) - if completed_process.returncode != 0: - return None - - with open(output_file, 'r') as file_handle: - results = json.load(file_handle) - - return dict(results) - -def should_use_docker(repo_dir: Path) -> bool: - '''Return True when file checks should run inside the dev container.''' - return ( - shutil.which('docker') is not None and - edgutil.is_docker_preferred(repo_dir) - ) - -def summarize_file_result( - file_result: Dict[str, Any] -) -> List[Tuple[str, str]]: - '''Flatten one file's checker output into (kind, message) findings. - - A None misspelling list means spell checking was unavailable, which is - reported as its own kind so callers can treat it as a warning rather - than a violation. - ''' - findings = [] - - misspellings = file_result['misspellings'] - if misspellings is None: - findings.append( - ('spelling-unavailable', 'aspell not found or missing dictionary') - ) - else: - for misspelling in misspellings: - findings.append(('misspelling', f"new misspelled word: {misspelling}")) - - for typo in file_result['typos']: - findings.append(('typo', f"repeated-word typo: {typo.strip()}")) - - coding_errors = file_result['coding_errors'] - if coding_errors: - for hit in coding_errors['missing_endifs']: - findings.append(('coding-error', f"#endif without a comment: {hit}")) - for hit in coding_errors['missing_elses']: - findings.append(('coding-error', f"#else without a comment: {hit}")) - for hit in coding_errors['closing_comments']: - findings.append(('coding-error', f"closing comment: {hit.strip()}")) - - for line_no in sorted(file_result['overlong_lines'], key = int): - findings.append( - ( - 'overlong-line', - f"line {line_no} exceeds {MAX_LINE_COLUMNS} columns" - ) - ) - - return findings diff --git a/dev_tools/pylibs/edgpolicy/py.typed b/dev_tools/pylibs/edgpolicy/py.typed deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/dev_tools/typecheck-python.sh b/dev_tools/typecheck-python.sh index d137811fc6..948e9264e6 100755 --- a/dev_tools/typecheck-python.sh +++ b/dev_tools/typecheck-python.sh @@ -113,9 +113,6 @@ pushd pylibs/ > /dev/null pushd edgpack/ > /dev/null check_types __init__.py popd > /dev/null - pushd edgpolicy/ > /dev/null - check_types_strict __init__.py - popd > /dev/null pushd edgshell/ > /dev/null check_types __init__.py popd > /dev/null From a27b56a570dc69965320128d6eac858fe08913fa Mon Sep 17 00:00:00 2001 From: sdarwin Date: Tue, 6 Oct 2026 12:02:57 -0600 Subject: [PATCH 3/3] Hand the pre-push hook's terminal to edg-check-policy [GH #164] git supplies the refs being pushed on a pre-push hook's standard input, so the checker cannot inherit that stream and still read the user's answers to the interactive prompts. Reopening sys.stdin inside the checker rebound only the Python level object and left file descriptor 0 pointing at git's pipe, and it also required a general purpose tool to know how git invokes hooks. Open the terminal in the hook instead and pass it as the child's standard input, which fixes the descriptor for the whole process tree and keeps that knowledge in the one place that needs it. Pushing with no terminal attached previously raised an unhandled OSError from open('/dev/tty'), so such a push died with a traceback rather than reporting anything. Decide what it should do instead of leaving it to chance: when there is nowhere to ask a question or nowhere to read the answer, fall back to the accounting used when nobody is watching, which is what CI already does. Advisory findings are then reported without blocking and enforced ones still fail, so a push from a GUI client or a script reaches the same verdict as the pull request it turns into. This also settles the case of a push whose output is redirected, which counted an error without trying to prompt. Co-authored-by: Cursor --- .git-hooks/pre-push.py | 32 ++++++++++++++++++++++---- dev_tools/bin/edg-check-policy | 42 +++++++++++++++------------------- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/.git-hooks/pre-push.py b/.git-hooks/pre-push.py index 5301a66a7e..bb4a96fc2d 100755 --- a/.git-hooks/pre-push.py +++ b/.git-hooks/pre-push.py @@ -66,6 +66,25 @@ TOOL_PATH_CHECK_POLICY = shutil.which('edg-check-policy') +def terminal_input(): + '''Open the terminal to hand to the checker as its standard input. + + git supplies the refs being pushed on this hook's stdin, so the checker + cannot simply inherit it and still read the user's answers. On Windows + the relaunch above has already arranged a standalone console, whose stdin + is usable as it is, so that is inherited instead. + ''' + if platform.system() == 'Windows': + return None + + try: + return open('/dev/tty') + except OSError: + # Pushing with no terminal attached, so there is nobody to prompt. The + # checker reports its findings without asking rather than reading the + # refs above as if they were answers. + return subprocess.DEVNULL + def main(): if TOOL_PATH_CHECK_POLICY is None: print( @@ -75,11 +94,14 @@ def main(): ) sys.exit(1) - completed_process = subprocess.run([ - TOOL_PATH_CHECK_POLICY, - '--unpushed', - '--interactive' - ]) + completed_process = subprocess.run( + [ + TOOL_PATH_CHECK_POLICY, + '--unpushed', + '--interactive' + ], + stdin = terminal_input() + ) sys.exit(completed_process.returncode) diff --git a/dev_tools/bin/edg-check-policy b/dev_tools/bin/edg-check-policy index 8386edb144..21eadd4b69 100755 --- a/dev_tools/bin/edg-check-policy +++ b/dev_tools/bin/edg-check-policy @@ -17,7 +17,6 @@ interactive review. import argparse import json import os -import platform import re import shutil import subprocess @@ -246,7 +245,14 @@ class PolicyReporter: ''' def __init__(self, interactive: bool = False, strict: bool = False) -> None: - self.interactive = interactive + # Prompting needs both somewhere to ask and somewhere to be answered. + # When either is missing, as for a push from a GUI client that has no + # controlling terminal, fall back to the accounting used when nobody is + # watching. That reaches the same verdict the pull request will, rather + # than refusing the push over warnings nobody is there to accept. + self.interactive = ( + interactive and sys.stdin.isatty() and sys.stdout.isatty() + ) self.strict = strict self.errors = 0 self.in_github_actions = 'GITHUB_ACTIONS' in os.environ @@ -273,16 +279,20 @@ class PolicyReporter: def confirm_okay(self) -> None: '''Ask the user whether a policy warning is acceptable. - When stdout is a terminal, prompt on stdin for y/n/q. Otherwise record - an error without prompting (e.g. non-interactive pushes). + Prompts on stdin for y/n/q. Only reached when a terminal is available + both ways, which the caller arranges: git hands a hook the refs being + pushed on stdin, so the pre-push hook opens the terminal itself and + passes that to this tool instead. ''' - if not sys.stdout.isatty(): - self.errors += 1 - return - tries = 0 while True: - response = input(' Okay? [ynq] ').lower() + try: + response = input(' Okay? [ynq] ').lower() + except EOFError: + # The terminal went away mid-review, so let the finding stand. + print() + self.errors += 1 + return if response == 'y': return @@ -719,23 +729,9 @@ def parse_args(argv: Optional[List[str]] = None) -> argparse.Namespace: ) return parser.parse_args(argv) -def _reopen_terminal_input() -> None: - '''Point stdin at the terminal so findings can be reviewed. - - A hook is handed the list of refs being pushed on stdin, so interactive - review has to read the user's answers from the terminal instead. On - Windows the wrapper has already arranged for a console, whose stdin is - usable as it is. - ''' - if platform.system() != 'Windows': - sys.stdin = open('/dev/tty') - def main() -> None: args = parse_args() - if args.interactive: - _reopen_terminal_input() - repo_dir = edgutil.find_mono_repo_or_exit() repository = GitRepository(repo_dir) reporter = PolicyReporter(