From 4c785b037023a7efc0e3f6d0c6dd1753561d55f7 Mon Sep 17 00:00:00 2001 From: Daniel McIlvaney Date: Wed, 3 Jun 2026 13:50:15 -0700 Subject: [PATCH] ci: add python linters to cover future python code --- .devcontainer/Dockerfile.AZL-3.0 | 6 +- .devcontainer/README.md | 4 + .devcontainer/devcontainer.json | 7 +- .github/workflows/python.yml | 35 +++++ .vscode/extensions.json | 7 + .vscode/settings.json | 22 ++- docs/developer/how-to/get-started.md | 10 ++ docs/developer/reference/coding-standards.md | 11 ++ .../app/azldev/core/sources/render_process.py | 39 +++-- magefiles/magecheckfix/checkfix.go | 137 ++++++++++++++++-- pyrightconfig.json | 18 +++ ruff.toml | 26 ++++ 12 files changed, 293 insertions(+), 29 deletions(-) create mode 100644 .github/workflows/python.yml create mode 100644 .vscode/extensions.json mode change 100644 => 100755 internal/app/azldev/core/sources/render_process.py create mode 100644 pyrightconfig.json create mode 100644 ruff.toml diff --git a/.devcontainer/Dockerfile.AZL-3.0 b/.devcontainer/Dockerfile.AZL-3.0 index 19013af7..3ae8daf4 100644 --- a/.devcontainer/Dockerfile.AZL-3.0 +++ b/.devcontainer/Dockerfile.AZL-3.0 @@ -2,9 +2,13 @@ FROM mcr.microsoft.com/azurelinux/base/core:3.0 # Install required packages RUN tdnf -y update && \ - tdnf -y install ca-certificates dnf dnf-utils which gh git golang gawk tar shadow-utils sudo tree bash-completion moby-engine moby-cli build-essential && \ + tdnf -y install ca-certificates dnf dnf-utils which gh git golang gawk tar shadow-utils sudo tree bash-completion moby-engine moby-cli build-essential python3-pip nodejs && \ tdnf clean all +# Install ruff and pyright for Python linting/formatting/type-checking +# (used by 'mage check python' / 'mage fix python'). pyright requires node (installed above). +RUN pip3 install --no-cache-dir ruff pyright + # Define the 'll' alias for all users RUN echo 'alias ll="ls -lah"' >> /etc/profile.d/aliases.sh && \ chmod +x /etc/profile.d/aliases.sh && \ diff --git a/.devcontainer/README.md b/.devcontainer/README.md index bfe9caaa..6b155d9e 100644 --- a/.devcontainer/README.md +++ b/.devcontainer/README.md @@ -9,6 +9,7 @@ The dev container uses the `azl_dev_3.0` image defined in `Dockerfile.AZL-3.0`. - Go development tools, - Mage build system, - golangci-lint, +- ruff and pyright (plus Node.js, required by pyright) for Python linting/formatting/type-checking, - required development dependencies for VS Code. ## Getting Started @@ -47,6 +48,9 @@ The dev container automatically installs these VS Code extensions: - JSON Language Features - JSON editing support - markdownlint - Markdown linting - YAML Language Support - YAML editing support +- Ruff (charliermarsh.ruff) - Python linting/formatting +- Pylance (ms-python.vscode-pylance) - Python type checking +- EditorConfig (editorconfig.editorconfig) - EditorConfig support ## Customization diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 6b3a6814..4ef10ff9 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -4,18 +4,23 @@ "dockerfile": "Dockerfile.AZL-3.0", "context": "." }, - "runArgs": ["--privileged"], + "runArgs": [ + "--privileged" + ], // Features to add to the dev container. More info: https://containers.dev/features. "features": {}, // Configure tool-specific properties. "customizations": { "vscode": { "extensions": [ + "charliermarsh.ruff", "davidanson.vscode-markdownlint", + "editorconfig.editorconfig", "github.copilot-chat", "github.copilot", "github.vscode-pull-request-github", "golang.go", + "ms-python.vscode-pylance", "ms-vscode.vscode-json", "redhat.vscode-yaml" ], diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml new file mode 100644 index 00000000..20824921 --- /dev/null +++ b/.github/workflows/python.yml @@ -0,0 +1,35 @@ +name: "Python lint and type-check" +on: + push: + branches: [main] + pull_request: + branches: [main] + workflow_dispatch: + schedule: + # Run every night at 3:15am PST (11:15am UTC) + - cron: "15 11 * * *" + +# Cancel in-progress runs of this workflow if a new run is triggered. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +permissions: {} + +jobs: + python: + name: "Lint and type-check" + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + - name: Get go tools + uses: ./.github/getgotools + - name: Install ruff and pyright + # ubuntu-latest ships Python and Node.js; pyright requires Node. + run: pip install ruff pyright + - name: Run mage check python + run: mage -v check python diff --git a/.vscode/extensions.json b/.vscode/extensions.json new file mode 100644 index 00000000..68f655ac --- /dev/null +++ b/.vscode/extensions.json @@ -0,0 +1,7 @@ +{ + "recommendations": [ + "charliermarsh.ruff", + "editorconfig.editorconfig", + "ms-python.vscode-pylance" + ] +} diff --git a/.vscode/settings.json b/.vscode/settings.json index 7d6937f6..cdb7526d 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -2,7 +2,26 @@ "go.buildTags": "scenario", "go.lintTool": "golangci-lint-v2", "go.testTimeout": "5m", - "go.testFlags": ["-v"], + "go.testFlags": [ + "-v" + ], + // Lint rules for python + "[python]": { + "editor.defaultFormatter": "charliermarsh.ruff", + "editor.formatOnSave": true, + "editor.codeActionsOnSave": { + "source.fixAll.ruff": "explicit", + "source.organizeImports.ruff": "explicit", + "source.fixAll": "explicit", + "source.fixAll.pylance": "explicit" + } + }, + "python.analysis.diagnosticsSource": "Pylance", + "python.missingPackage.severity": "Error", + "python.testing.unittestEnabled": false, + "python.testing.pytestEnabled": true, + "ruff.configuration": "${workspaceFolder}/ruff.toml", + "ruff.lint.enable": true, "cSpell.words": [ "acarl", "acobaugh", @@ -132,4 +151,3 @@ "wrapcheck" ] } - diff --git a/docs/developer/how-to/get-started.md b/docs/developer/how-to/get-started.md index d09888fd..721257dc 100644 --- a/docs/developer/how-to/get-started.md +++ b/docs/developer/how-to/get-started.md @@ -12,6 +12,16 @@ go install github.com/magefile/mage@latest ``` +- **ruff** and **pyright** *(only if you touch Python code)* - Python linter/formatter + and type checker, used by `mage check python` / `mage fix python`. pyright requires + Node.js. The dev container installs all of these for you. + + ```bash + # On Azure Linux + tdnf install -y python3-pip nodejs + pip3 install ruff pyright + ``` + ## Initial Setup 1. Fork the repository (strongly recommended for all contributors) diff --git a/docs/developer/reference/coding-standards.md b/docs/developer/reference/coding-standards.md index f3b2fc6b..cce8e365 100644 --- a/docs/developer/reference/coding-standards.md +++ b/docs/developer/reference/coding-standards.md @@ -50,6 +50,17 @@ - See [azldev command guidelines](../reference/command-guidelines.md) for details on command structure and naming conventions. +## Python Code Style + +Some functionality is implemented in Python scripts (e.g. scripts embedded in the +Go binary). + +- **Linting & formatting**: All Python must pass `ruff` (config: [`ruff.toml`](../../../ruff.toml)). + Run `mage check python` to lint and `mage fix python` to auto-format and apply fixes. +- **Type checking**: All Python must pass `pyright` (config: [`pyrightconfig.json`](../../../pyrightconfig.json)). + This is also run by `mage check python`. +- Both checks are part of `mage check all`. + ## Logging Guidelines - Use structured logging with the `slog` package diff --git a/internal/app/azldev/core/sources/render_process.py b/internal/app/azldev/core/sources/render_process.py old mode 100644 new mode 100755 index c9978c4c..36d28f41 --- a/internal/app/azldev/core/sources/render_process.py +++ b/internal/app/azldev/core/sources/render_process.py @@ -2,8 +2,9 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -"""Process RPM specs inside a mock chroot: run rpmautospec and spectool for each -component, writing per-component results to a JSON file in the staging directory. +r"""Run rpmautospec and spectool for each component inside a mock chroot. + +Writes per-component results to a JSON file in the staging directory. This script is embedded in the azldev Go binary and executed inside a mock chroot during ``azldev component render``. It avoids the need for complex inline shell @@ -20,35 +21,41 @@ Results are written to ``/results.json``:: [ - {"name": "curl", "specFiles": "Source0: curl-8.5.0.tar.xz\\nPatch0: fix.patch", "error": null}, + {"name": "curl", "specFiles": "Source0: curl-8.5.0.tar.xz\nPatch0: fix.patch", "error": null}, {"name": "broken", "specFiles": "", "error": "rpmautospec failed: ..."} ] Progress is reported to stderr as ``PROGRESS / ``. """ +from __future__ import annotations + import json -import os import subprocess import sys from concurrent.futures import ThreadPoolExecutor, as_completed +from pathlib import Path + +# Number of positional arguments expected on the command line (script, staging_dir, max_workers). +EXPECTED_ARG_COUNT = 3 -def process_component(staging_dir: str, name: str, spec_filename: str) -> dict: +def process_component(staging_dir: str, name: str, spec_filename: str) -> dict[str, str | None]: """Run rpmautospec + spectool for a single component, returning a result dict. Trust boundary: name and spec_filename are validated by BatchProcess in mockprocessor.go (validateComponentInput rejects path separators, empty values, and non-basename spec filenames) before this script is invoked. """ - comp_dir = os.path.join(staging_dir, name) - spec_path = os.path.join(comp_dir, spec_filename) + comp_dir = Path(staging_dir) / name + spec_path = comp_dir / spec_filename # rpmautospec: expand %autorelease / %autochangelog in-place. rpa_result = subprocess.run( - ["rpmautospec", "process-distgit", spec_path, spec_path], + ["rpmautospec", "process-distgit", str(spec_path), str(spec_path)], capture_output=True, text=True, + check=False, ) if rpa_result.returncode != 0: @@ -66,10 +73,11 @@ def process_component(staging_dir: str, name: str, spec_filename: str) -> dict: f"_sourcedir {comp_dir}", "-l", "-a", - spec_path, + str(spec_path), ], capture_output=True, text=True, + check=False, ) if st_result.returncode != 0: @@ -83,15 +91,16 @@ def process_component(staging_dir: str, name: str, spec_filename: str) -> dict: def main() -> int: - if len(sys.argv) != 3: + """Read inputs.json, process every component in parallel, and write results.json.""" + if len(sys.argv) != EXPECTED_ARG_COUNT: print(f"usage: {sys.argv[0]} ", file=sys.stderr) return 1 staging_dir = sys.argv[1] max_workers = int(sys.argv[2]) - inputs_path = os.path.join(staging_dir, "inputs.json") + inputs_path = Path(staging_dir) / "inputs.json" - with open(inputs_path) as f: + with inputs_path.open() as f: inputs = json.load(f) # Mark all paths as git-safe (ownership mismatch between host and chroot). @@ -123,7 +132,7 @@ def main() -> int: name = futures[future] try: completed_results[name] = future.result() - except Exception as exc: + except Exception as exc: # noqa: BLE001 - record any worker failure as a per-component error completed_results[name] = { "name": name, "specFiles": "", @@ -138,9 +147,9 @@ def main() -> int: # Write results to a file in the staging directory rather than stdout. # This avoids bufio.Scanner token size limits in the Go caller, which # would truncate large JSON payloads (e.g., 7k components ≈ 560KB). - results_path = os.path.join(staging_dir, "results.json") + results_path = Path(staging_dir) / "results.json" - with open(results_path, "w") as results_file: + with results_path.open("w") as results_file: json.dump(results, results_file) return 0 diff --git a/magefiles/magecheckfix/checkfix.go b/magefiles/magecheckfix/checkfix.go index 7a982baf..fff1bef8 100644 --- a/magefiles/magecheckfix/checkfix.go +++ b/magefiles/magecheckfix/checkfix.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "os/exec" "path" "strings" @@ -24,16 +25,20 @@ const ( TargetLint = "lint" TargetStatic = "static" TargetLicenses = "licenses" + TargetPython = "python" ) var ( - ErrCheck = errors.New("check failed") - ErrFix = errors.New("fix failed") - ErrLint = errors.New("lint failed") - ErrToolPath = errors.New("path not set for tool") + ErrCheck = errors.New("check failed") + ErrFix = errors.New("fix failed") + ErrLint = errors.New("lint failed") + ErrToolPath = errors.New("path not set for tool") + ErrRuffNotFound = errors.New("ruff not found on PATH") + ErrPyrightNotFnd = errors.New("pyright not found on PATH") + ErrTypeCheck = errors.New("type check failed") ) -// Check one of: [all, mod, lint, static, licenses]. +// Check one of: [all, mod, lint, static, licenses, python]. // BASH-COMPLETION: This is scanned by the bash completion script, keep it in sync with the script. func Check(target string) error { mg.SerialDeps(magesrc.Generate) @@ -42,7 +47,7 @@ func Check(target string) error { switch target { case TargetAll: - mg.SerialDeps(modCheck, lintCheck, static, licenseCheck) + mg.SerialDeps(modCheck, lintCheck, static, licenseCheck, pythonCheck) case TargetMod: mg.SerialDeps(modCheck) case TargetLint: @@ -51,9 +56,11 @@ func Check(target string) error { mg.SerialDeps(static) case TargetLicenses: mg.SerialDeps(licenseCheck) + case TargetPython: + mg.SerialDeps(pythonCheck) default: return fmt.Errorf("%w: unknown check target '%s'. Available targets: %v", - ErrCheck, target, []string{TargetAll, TargetMod, TargetLint, TargetStatic, TargetLicenses}) + ErrCheck, target, []string{TargetAll, TargetMod, TargetLint, TargetStatic, TargetLicenses, TargetPython}) } mageutil.MagePrintln(mageutil.MsgSuccess, "Done checking.") @@ -61,7 +68,7 @@ func Check(target string) error { return nil } -// Fix one of: [all, mod, lint]. +// Fix one of: [all, mod, lint, python]. // BASH-COMPLETION: This is scanned by the bash completion script, keep it in sync with the script. func Fix(target string) error { mg.SerialDeps(magesrc.Generate) @@ -70,14 +77,16 @@ func Fix(target string) error { switch target { case TargetAll: - mg.SerialDeps(modFix, lintFix) + mg.SerialDeps(modFix, lintFix, pythonFix) case TargetMod: mg.SerialDeps(modFix) case TargetLint: mg.SerialDeps(lintFix) + case TargetPython: + mg.SerialDeps(pythonFix) default: return fmt.Errorf("%w: unknown fix target '%s'. Available targets: %v", - ErrFix, target, []string{TargetAll, TargetMod, TargetLint}) + ErrFix, target, []string{TargetAll, TargetMod, TargetLint, TargetPython}) } mageutil.MagePrintln(mageutil.MsgSuccess, "Done fixing.") @@ -239,6 +248,114 @@ func editorconfigCheck() error { return nil } +// findRuff locates the ruff executable on the PATH. ruff is a Python tool and is not managed via the Go +// tools modules, so developers must install it themselves (the VS Code ruff extension bundles a copy that +// is not exposed on the PATH). +func findRuff() (string, error) { + ruffPath, err := exec.LookPath("ruff") + if err != nil { + return "", fmt.Errorf("%w: install it with 'pip install ruff' or see "+ + "https://docs.astral.sh/ruff/installation/: %w", ErrRuffNotFound, err) + } + + return ruffPath, nil +} + +// findPyright locates the pyright executable on the PATH. Like ruff, pyright is a Python tool that is not +// managed via the Go tools modules, so developers must install it themselves. +func findPyright() (string, error) { + pyrightPath, err := exec.LookPath("pyright") + if err != nil { + return "", fmt.Errorf("%w: install it with 'pip install pyright' or see "+ + "https://microsoft.github.io/pyright/#/installation: %w", ErrPyrightNotFnd, err) + } + + return pyrightPath, nil +} + +func pythonCheck() error { + mg.SerialDeps(ruffCheck, pyrightCheck) + + return nil +} + +func ruffCheck() error { + mageutil.MagePrintln(mageutil.MsgStart, "Linting Python with ruff...") + + ruffPath, err := findRuff() + if err != nil { + return mageutil.PrintAndReturnError("Failed to find ruff.", ErrCheck, err) + } + + // Lint check. -q suppresses the "All checks passed!" banner on success; violations are still + // printed on failure. + if err := sh.RunV(ruffPath, "check", "-q", "."); err != nil { + return mageutil.PrintAndReturnError( + "Python lint issues found. Please run 'mage fix python' to auto-fix.", ErrCheck, ErrLint) + } + + // Format check. -q suppresses the "N files already formatted" banner on success. + if err := sh.RunV(ruffPath, "format", "--check", "-q", "."); err != nil { + return mageutil.PrintAndReturnError( + "Python files are not formatted. Please run 'mage fix python' to format them.", ErrCheck, ErrLint) + } + + mageutil.MagePrintln(mageutil.MsgSuccess, "Done linting Python.") + + return nil +} + +func pyrightCheck() error { + mageutil.MagePrintln(mageutil.MsgStart, "Type-checking Python with pyright...") + + pyrightPath, err := findPyright() + if err != nil { + return mageutil.PrintAndReturnError("Failed to find pyright.", ErrCheck, err) + } + + // pyright always prints a summary line even on success, so capture its output and only surface + // it when there's a problem (mirrors golangciLintCheck above to keep checks quiet on success). + output, err := sh.Output(pyrightPath) + if err != nil { + if output != "" { + for _, line := range strings.Split(output, "\n") { + mageutil.MagePrintln(mageutil.MsgInfo, line) + } + } + + return mageutil.PrintAndReturnError( + "Python type errors found; please review the errors displayed above.", ErrCheck, ErrTypeCheck) + } + + mageutil.MagePrintln(mageutil.MsgSuccess, "Done type-checking Python.") + + return nil +} + +func pythonFix() error { + mageutil.MagePrintln(mageutil.MsgStart, "Fixing Python formatting...") + + ruffPath, err := findRuff() + if err != nil { + return mageutil.PrintAndReturnError("Failed to find ruff.", ErrFix, err) + } + + // Auto-fix lint issues. + if err := sh.RunV(ruffPath, "check", "--fix", "."); err != nil { + return mageutil.PrintAndReturnError( + "Failed to auto-fix all Python lint issues; please review any errors displayed above.", ErrFix, err) + } + + // Format. + if err := sh.RunV(ruffPath, "format", "."); err != nil { + return mageutil.PrintAndReturnError("Failed to format Python files.", ErrFix, err) + } + + mageutil.MagePrintln(mageutil.MsgSuccess, "Done fixing Python.") + + return nil +} + // licenseCheck checks the licenses of the dependencies and writes the results to a CSV file. func licenseCheck() error { mg.SerialDeps(mageutil.CreateLicenseDir) diff --git a/pyrightconfig.json b/pyrightconfig.json new file mode 100644 index 00000000..6fe8889c --- /dev/null +++ b/pyrightconfig.json @@ -0,0 +1,18 @@ +{ + "exclude": [ + "**/node_modules", + "**/__pycache__", + "**/.venv", + "**/venv", + "out", + "**/.*" + ], + "typeCheckingMode": "standard", + "pythonVersion": "3.12", + "pythonPlatform": "Linux", + "reportUntypedFunctionDecorator": "error", + "reportMissingTypeStubs": "warning", + "reportMissingParameterType": "error", + "reportMissingTypeArgument": "error", + "reportUnknownParameterType": "error" +} diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 00000000..a5af63c0 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,26 @@ +extend-exclude = ["out"] + +line-length = 120 +target-version = "py312" + +[lint] +select = ["ALL"] # Everything +ignore = [ + "COM812", # Conflicts with formatter. + "S603", # Check for validation of input to subprocess calls, almost always a false positive + "S607", # Requiring abspath for all binaries used is very restrictive + "T201", # These scripts are fine to use `print` for output +] + +# We want D401 still, even though google style omits it. +extend-select = ["D401"] + +fixable = ["ALL"] + +[lint.isort] +# Just force this, its better than dealing with a mishmash of modules that have annotations +# and those that don't +required-imports = ["from __future__ import annotations"] + +[lint.pydocstyle] +convention = "google"