bitflicker64 commented on code in PR #497:
URL: https://github.com/apache/hugegraph-doc/pull/497#discussion_r4075345155
##########
scripts/versioning.py:
##########
@@ -4068,30 +4070,10 @@ def build(args: argparse.Namespace) -> None:
json.dumps(override, ensure_ascii=False), encoding="utf-8"
)
hugo = os.environ.get("HUGO_BIN", "hugo")
- go = os.environ.get("GO_BIN", "go")
- go_executable = shutil.which(go)
+ go_executable = shutil.which(os.environ.get("GO_BIN", "go"))
if go_executable is None:
- fail(f"Go executable is unavailable: {go}")
- module_result = subprocess.run(
- [
- go_executable,
- "mod",
- "download",
- "-json",
- "github.com/pgsty/[email protected]",
- ],
- cwd=assembly,
- check=True,
- stdout=subprocess.PIPE,
- text=True,
- )
- module = json.loads(module_result.stdout)
- if (
- module.get("Path") != "github.com/pgsty/oink"
- or module.get("Version") != "v1.0.0"
- or module.get("Sum") !=
"h1:E+WHFP9zSRT+5RKoIkWNp+ASRGS1BKG+rDEi9by/BjE="
- ):
- fail(f"unexpected OINK module metadata: {module!r}")
+ fail("Go executable is unavailable")
+ module = download_locked(assembly)
Review Comment:
🧹 Lock-file mismatches now abort with a traceback instead of a clean
`fail()`.
The removed code ended in `fail(f"unexpected OINK module metadata:
{module!r}")`, and `fail` raises `SystemExit(message)` (line 396) - one line,
no traceback, and it printed the offending metadata. `download_locked` instead
raises bare `ValueError`s (`scripts/oink_module.py:30,33,48,53,56,59`) that
carry no values, and `command` lets `subprocess.CalledProcessError` escape.
`main()` (line 4513) has no handler, so both surface as tracebacks.
Reproduced by substituting `h1:WRONG=` for the archive hash in a scratch
copy of `go.sum`:
```
subprocess.CalledProcessError: Command '['go', 'mod', 'download', '-json',
'github.com/pgsty/[email protected]']' returned non-zero exit status 1
```
and the `go mod download` JSON that would name the mismatch is discarded.
Requested change: put the actual and expected values into each `ValueError`
message, and wrap this call so `ValueError` and `CalledProcessError` are
reported through `fail()` like every other build error in this file.
##########
.github/workflows/hugo.yml:
##########
@@ -231,11 +231,7 @@ jobs:
restore-keys: ${{ runner.os }}-hugo-${{ env.HUGO_VERSION }}-${{
matrix.version.id }}-
- name: Verify pinned OINK module
run: |
- go mod verify
- test "$(go list -m -f '{{ .Path }}')" =
"github.com/apache/hugegraph-doc"
- test "$(go list -m all | wc -l)" -eq 2
- test "$(go list -m -f '{{ .Path }}@{{ .Version }}'
github.com/pgsty/oink)" = "github.com/pgsty/[email protected]"
- test -z "$(go list -m -f '{{ with .Replace }}{{ .Path }}@{{ .Version
}}{{ end }}' github.com/pgsty/oink)"
+ python3 scripts/oink_module.py
Review Comment:
🧹 On the dispatch path this step no longer asserts anything master controls.
`prepare` requires `test "$GITHUB_REF" = "refs/heads/master"` (line 124), so
this YAML is always master's, but `source_sha` is the resolved candidate branch
head (line 159) and the build job checks that out (line 204). The five inline
assertions this replaces were therefore master-authoritative against the
candidate's `go.mod`; `scripts/oink_module.py` comes out of the candidate
checkout, so the step is now self-attested. The following step already runs
candidate `versioning.py`, so this is not a new execution boundary - what is
lost is the one cheap master-side check that would catch accidental drift in a
candidate's `go.mod` before a staging build.
Requested change: keep a version-independent assertion inline alongside the
helper call, so master still bounds the graph while the version floats:
```bash
test "$(go list -m -f '{{ .Path }}')" = "github.com/apache/hugegraph-doc"
test "$(go list -m all | wc -l)" -eq 2
test -z "$(go list -m -f '{{ with .Replace }}{{ .Path }}{{ end }}'
github.com/pgsty/oink)"
python3 scripts/oink_module.py
```
##########
scripts/versioning.py:
##########
@@ -4068,30 +4070,10 @@ def build(args: argparse.Namespace) -> None:
json.dumps(override, ensure_ascii=False), encoding="utf-8"
)
hugo = os.environ.get("HUGO_BIN", "hugo")
- go = os.environ.get("GO_BIN", "go")
- go_executable = shutil.which(go)
+ go_executable = shutil.which(os.environ.get("GO_BIN", "go"))
Review Comment:
🧹 `GO_BIN` is now resolved twice, and the failure message lost the value.
This line keeps guarding line 4135, where `go_executable` sets the `PATH`
Hugo inherits, so the check must stay. But the old code passed `go_executable`
straight into the `go mod download` subprocess, which made it the single
resolution point; `oink_module.command` now re-reads `os.environ.get("GO_BIN",
"go")` raw, so the binary that validates the lock files and the binary
prepended to Hugo's `PATH` are resolved independently and can differ. The
message on line 4075 also dropped the value - it was `fail(f"Go executable is
unavailable: {go}")` before, which told the operator what `GO_BIN` had been set
to.
Requested change: pass the resolved `go_executable` into
`download_locked`/`command` so one path is used end to end, and restore the
value in the failure message.
##########
scripts/oink_module.py:
##########
@@ -0,0 +1,65 @@
+#!/usr/bin/env python3
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0.
+
+"""Resolve the sole theme dependency from the checked-in Go lock files."""
+import json
+import os
+import re
+import subprocess
+from pathlib import Path
+
+MODULE = "github.com/pgsty/oink"
Review Comment:
🧹 The contributor docs still state the hard pin this PR replaces.
`README.md:35` ("The module graph must resolve
`github.com/pgsty/[email protected]`"), its Chinese mirror `README.md:119`, and
`contribution.md:33` ("must contain exactly the pinned
`github.com/pgsty/[email protected]` dependency for this site") are prescriptive
statements of the verification contract. After this diff the enforced contract
is "whatever `go.mod` pins, verified against `go.sum`" - `locked_version()`
accepts any release version. The version they name happens to match master
today, but the rule they describe is the one being removed here, and they are
the instructions a contributor follows before editing.
Requested change: reword those three lines to describe the lock-file
contract rather than a literal version, so they do not need editing again on
the 1.1 bump.
Separately, for #496 rather than this PR: `NOTICE:9` ("This product includes
OINK v1.0.0") is still accurate on master but is now the only place the version
is written by hand with nothing checking it. An assertion against
`locked_version()`'s return would keep it honest across the bump.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
