Skip to content

ci: make the Lychee pre-commit hook portable on Windows - #3135

Merged
rwgk merged 9 commits into
mainfrom
rwgk/lychee-portable-pre-commit
Oct 16, 2026
Merged

rwgk merged 9 commits into
mainfrom
rwgk/lychee-portable-pre-commit

Conversation

@rwgk

@rwgk rwgk commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The existing Lychee pre-commit hook uses a shell launcher, which can fail when contributors invoke pre-commit from PowerShell or cmd. The Windows workaround in CONTRIBUTING.md asks contributors to use Git Bash or skip Lychee.

This PR makes the configured hook work from PowerShell, cmd, and Git Bash, and adds a small permanent smoke check to catch hook setup and invocation problems early.

Solution

  • Configure Lychee as a Python pre-commit hook with the pinned native lychee-bin==0.24.2 wheel installed in its isolated environment. Preserve the existing Lychee version, arguments, and file selection.
  • Use a small Python launcher to locate that environment's native lychee / lychee.exe directly, pass through arguments, and return its exit status. This keeps executable selection tied to the pinned dependency.
  • Fold the Windows guidance into the normal pre-commit instructions: contributors use the same commands from PowerShell, cmd, or Git Bash.

The same hook also works on Linux.

Permanent offline smoke check

The smoke helper prepares a copy of the checked-in pre-commit configuration with offline checking and JSON report output enabled for Lychee. CI then invokes the hook directly from each shell against the repository's root README.md. This exercises the configured hook, its isolated environment, the Python launcher, and the native executable together.

The check follows README's local links without sending external link-check requests. It is a small permanent pre-flight check that helps distinguish hook problems from external link failures. The workflow comment asks that it stay before any network-dependent Lychee steps as CI evolves. Dependency installation can still download packages.

A broken local README link also fails this check, even when the hook itself works; this is an accepted tradeoff for using a real document. Each successful shell check must produce a fresh, nonempty Lychee JSON report accepted by the existing documentation link-check validator. Missing, malformed, or inconsistent reports fail. The helper consumes each report after verification, so a skipped hook cannot reuse an earlier shell's result.

Linux and Windows shell coverage

The pre-commit workflow runs the same offline smoke check in each shell:

pre-commit run lychee --config <offline-config> --files README.md --verbose
OS Shells
Linux Bash
Windows Git Bash, PowerShell 7 (pwsh), cmd

This exercises shell command lookup and quoting along with the configured hook. The Windows shells run as separate steps in the existing job, sharing the offline configuration and pre-commit hook environment. Temporary paths contain spaces and stay on the checkout's drive; each result is verified and temporary files are cleaned up afterward. After preparation succeeds, a failure in one shell does not prevent the remaining shell checks from running.

The full pre-commit suite runs once per OS with Lychee skipped; full documentation link checking uses the existing dedicated Lychee action in this PR.

Validation

Hosted pre-commit CI exercises the offline smoke check in all four OS/shell combinations and the full pre-commit suite on each OS.

The offline smoke check and full pre-commit pass locally on all three stack layers. Additional local checks confirmed failure propagation, rejection of skipped hooks and invalid reports, and report cleanup after verification.

Scope

This extracts the portability change for separate review. Checkout-link remapping remains in #3118; consolidating full link checking through pre-commit remains in #3130.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

… ci]

Install the pinned native Lychee wheel in the hook environment and resolve its executable from that environment directly. Document the same pre-commit invocation for PowerShell, cmd, and Git Bash, and extract focused launcher tests.

Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the CI/CD CI/CD infrastructure label Oct 16, 2026
@rwgk rwgk added the bug Something isn't working label Oct 16, 2026
@rwgk rwgk self-assigned this Oct 16, 2026
@rwgk rwgk added this to the cuda.bindings next milestone Oct 16, 2026
@rwgk
rwgk added this pull request to stack #3136 October 16, 2026 15:52
rwgk added 6 commits October 16, 2026 09:36
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@rwgk

rwgk commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test

@rwgk rwgk added enhancement Any code-related improvements and removed bug Something isn't working labels Oct 16, 2026
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@rwgk
rwgk marked this pull request as ready for review October 16, 2026 19:03
@rwgk
rwgk requested review from juenglin and mdboom October 16, 2026 19:04
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: db7335b4-68ce-43c8-ab67-a9c08ec1a26e
📥 Commits

Reviewing files that changed from the base of the PR and between 54e0c3e and 6178349.

📒 Files selected for processing: 2
  • .github/actions/check-doc-links/action.yml
  • ci/tools/smoke_test_lychee_precommit.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Developer Experience
    • Link-checking pre-commit checks now run from PowerShell, Command Prompt, and Git Bash on Windows, as well as Bash on other supported systems.
    • An offline check validates link-checking behavior before the full pre-commit run and reports failures.
  • Documentation
    • Updated pre-commit guidance to reflect the supported command-line environments and removed the Windows-specific workaround instructions.

The Lychee pre-commit hook now runs through a local Python wrapper. The workflow prepares an offline configuration and checks hook execution across Bash, PowerShell, and cmd. Contributor instructions list these shells as supported for the documented commands.

Changes: Lychee pre-commit

Layer / File(s) Summary
Configure the local Lychee hook
.pre-commit-config.yaml, ci/tools/run_lychee.py, .github/actions/check-doc-links/action.yml
The hook uses lychee-bin==0.24.2 and a local wrapper. The wrapper selects the platform-specific executable, forwards arguments, and returns Lychee’s exit status. The action comment identifies lychee-bin as the version reference.
Prepare and verify offline checks
ci/tools/smoke_test_lychee_precommit.py
The script prepares an offline configuration, verifies output and exit status, and restricts cleanup to matching temporary directories.
Run checks across shells
.github/workflows/pre-commit.yml, CONTRIBUTING.md
The workflow tests the hook from Bash on Linux and Windows, and from PowerShell and cmd on Windows. Contributor guidance lists these shells and removes the Windows workaround instructions.

Suggested reviewers: juenglin

Priority: ⬇️ Low

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 61783

The change aims to make Lychee pre-commit checks portable across the documented shells. The available evidence shows the smoke check validates its output and cleanup; no actionable merge-blocking issue is identified, so the change appears ready for normal checks.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
Doc Preview CI
Preview removed because the pull request was closed or merged.

@juenglin
juenglin requested a review from jpascucci-nv October 16, 2026 20:09
@juenglin

Copy link
Copy Markdown
Contributor

Two non-blocking suggestions.

1. The smoke check could verify a Lychee report instead of matching pre-commit's output

verify() in ci/tools/smoke_test_lychee_precommit.py decides whether the hook ran by looking for - hook id: lychee and Skipped in pre-commit's human-readable output, and only when the exit code is 0. That works today, but it depends on pre-commit's wording and on --verbose.

Sketch: have the hook write a Lychee JSON report and validate that. A skipped hook produces no report. A hook that ran but checked nothing, or whose report disagrees with its exit status, fails. ci/tools/retry_lychee.py already has read_report(path, exit_code), which rejects total == 0 and failures that disagree with the exit code, and both files are in ci/tools/.

from retry_lychee import read_report  # same schema checks as the real link-check retries

REPORT_NAME = "lychee.json"


def write_config(directory: Path) -> None:
    config = yaml.safe_load((ROOT / ".pre-commit-config.yaml").read_text(encoding="utf-8"))
    [hook] = [hook for repo in config["repos"] for hook in repo["hooks"] if hook["id"] == "lychee"]
    report = directory / REPORT_NAME
    # The absolute path with spaces is one list element, so it still exercises quoting.
    hook.setdefault("args", []).extend(["--offline", "--format", "json", "--output", str(report)])
    (directory / "pre-commit.yaml").write_text(yaml.safe_dump(config), encoding="utf-8")


def verify(log: Path, exit_code: int) -> int:
    print(log.read_text(encoding="utf-8"), end="")
    report = log.with_name(REPORT_NAME)  # the log and the report share the smoke directory
    try:
        if exit_code != 0:
            return exit_code
        if not report.is_file():
            raise SystemExit(f"The configured Lychee hook produced no report ({report}); was it skipped?")
        # Raises if no links were checked or the report disagrees with exit status 0.
        read_report(report, exit_code)
        return 0
    finally:
        # The config is shared by every shell, so consume the report to avoid a stale pass.
        report.unlink(missing_ok=True)

In main() the call becomes return verify(ROOT / log, int(exit_code)). pre-commit.yml needs no change, since the steps still pass the log and exit code and the report sits next to the log.

Caveats: I haven't run this. I haven't confirmed that Lychee 0.24.2 writes the JSON report with --offline, or that total is above 0 for README.md in that mode. It also ties the smoke check to the JSON schema read_report validates, which seems right, since a Lychee bump that changes the schema would then fail in both places at once.

2. The Lychee version is now pinned in two places, with only comments linking them

Before this PR, the hook was pinned through the upstream repo's rev: (a frozen commit SHA) plus the LYCHEE_VERSION=0.24.2 argument. Now the hook is repo: local and pins lychee-bin==0.24.2 in additional_dependencies. A repo: local hook has no rev:, so pre-commit autoupdate doesn't see it. The docs link check pins LYCHEE_VERSION: v0.24.2 in .github/actions/check-doc-links/action.yml. Nothing checks that the two agree, only the comment # Must match check-doc-links/action.yml. If one is bumped alone, local pre-commit and documentation CI would quietly run different Lychee versions. That could mean different link-checking behavior, or different JSON report schemas, which retry_lychee.py depends on.

Also, action.yml still says # Must match the pinned rev in .pre-commit-config.yaml. This PR removes that rev, so the comment is stale unless #3118 lands first and updates it.

Suggestion: add a small consistency check. There is precedent in ci/tools/check_pixi_cuda_version.py, which runs as a local pre-commit hook. It would parse both pins and fail if they differ. A single source of truth, such as the action reading the version from .pre-commit-config.yaml, would also work but is more invasive. I'd be fine with a follow-up.

@rwgk
rwgk marked this pull request as draft October 16, 2026 21:38
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@rwgk

rwgk commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

@juenglin

  1. The smoke check could verify a Lychee report instead of matching pre-commit's output

Thanks, implemented in 6178349. The smoke check now generates a JSON report and validates it with the existing read_report() helper. It preserves nonzero pre-commit results and consumes the report after every verification, so a later skipped hook cannot reuse an earlier shell’s successful report.

I confirmed that Lychee 0.24.2 produces a valid, nonempty report in offline mode. Hosted validation passes on Linux and in Git Bash, PowerShell, and cmd on Windows. Local checks also cover missing, empty, malformed, and inconsistent reports.

  1. The Lychee version is now pinned in two places, with only comments linking them

Agreed that having one source of truth would be preferable. The separate pre-commit and CI pins predate this PR; #3130 removes the dedicated Lychee action and makes development and CI use the same pinned pre-commit hook. I’d therefore leave the consistency machinery for that consolidation.

I did correct the stale “pinned rev” comment here to refer to the lychee-bin dependency pin. Thanks for catching that!

@rwgk

rwgk commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Commit 6178349 only changed a comment and the smoke check, which is exercised by a push to the PR, as verified with codex assistance:

I verified the actual logs:

  • Both Linux and Windows checked out 6178349.
  • Preparation, hook execution, JSON verification, and cleanup succeeded in Linux Bash and all three Windows shells.
  • The updated verifier requires a valid JSON report before returning success, so these results confirm the new report-verification code ran.
  • Full CI passed on the preceding commit. The subsequent changes are exactly the smoke helper and comment correction.

To not run the full CI again, I will now:

  • Temporarily add [no-ci] to the PR title.
  • Undraft.
  • Squash-merge, while being careful to remove [no-ci] from the suggested squash-commit message.
  • Remove [no-ci] from the PR title again to not be misleading for people looking at the title in passing in the future.

@rwgk rwgk changed the title ci: make the Lychee pre-commit hook portable on Windows [no-ci] ci: make the Lychee pre-commit hook portable on Windows Oct 16, 2026
@rwgk
rwgk marked this pull request as ready for review October 16, 2026 22:12
@rwgk
rwgk merged commit e8dacbc into main Oct 16, 2026
30 checks passed
@rwgk
rwgk deleted the rwgk/lychee-portable-pre-commit branch October 16, 2026 22:26
@rwgk rwgk changed the title [no-ci] ci: make the Lychee pre-commit hook portable on Windows ci: make the Lychee pre-commit hook portable on Windows Oct 16, 2026
github-actions Bot pushed a commit that referenced this pull request Oct 17, 2026
Removed preview folders for the following PRs:
- PR #3109
- PR #3114
- PR #3117
- PR #3124
- PR #3125
- PR #3132
- PR #3135
- PR #3138
- PR #3139
- PR #3140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD infrastructure enhancement Any code-related improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants