Repository navigation
ci: make the Lychee pre-commit hook portable on Windows - #3135
Conversation
… 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>
|
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. |
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>
|
/ok to test |
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
|
|
Two non-blocking suggestions. 1. The smoke check could verify a Lychee report instead of matching pre-commit's output
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. 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 Caveats: I haven't run this. I haven't confirmed that Lychee 0.24.2 writes the JSON report with 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 Also, Suggestion: add a small consistency check. There is precedent in |
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Thanks, implemented in 6178349. The smoke check now generates a JSON report and validates it with the existing 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.
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 |
|
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:
To not run the full CI again, I will now:
|
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.mdasks 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
lychee-bin==0.24.2wheel installed in its isolated environment. Preserve the existing Lychee version, arguments, and file selection.lychee/lychee.exedirectly, pass through arguments, and return its exit status. This keeps executable selection tied to the pinned dependency.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:
pwsh), cmdThis 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