chore: modernize and harden codegen toolchain#616
Conversation
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
|
Go's 1.22 series topped out at 1.22.12 and has since been removed from the official downloads page entirely, so dl.google.com returned a 404, the tarball was empty/malformed, and the checksum naturally didn't match. I've updated the Dockerfile to use go1.25.11 (current stable) with the correct checksums sourced directly from go.dev/dl, and kept go.mod and update.sh in sync with that same version. Pushing the fix now. |
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
|
I am confused about the whitespace changes in this PR (I am looking at the version of the PR as of commit 9). When I do the following on a Linux system: I see that the line endings are the Unix standard newlines, not the Window CR-LF sequence of two characters. I think that is what we want. Does your PR try to change the line endings in this file to CR-LF? If not, then why is the file changed at all? |
|
Following up to my previous comment, on the same Linux system of mine I run these commands to check out and examine the same file as it has been modified in the branch of this PR: This shows that the PR's modified version of the file has CR-LF line endings. Why do you want to change text files to have CR-LF line endings? |
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
Thanks @jafingerhut for catching this, you were right. My Windows Git config had core.autocrlf silently converting line endings on files I never intended to touch, including py/REUSE.toml. I've reverted py/REUSE.toml, py/LICENSE, py/pyproject.toml, and py/setup.cfg to match main exactly, set core.autocrlf=false globally, and added a .gitattributes to enforce LF on the files this PR actually generates. The diff against main is now scoped only to the intended changes. Sorry for the noise, and thanks again for the careful review. |
Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
d871856 to
914a61a
Compare
Summary
Pin every moving part in the protobuf code-generation pipeline the base Docker image, Go toolchain, and both
protoc-gen-goplugins and aligngo.modwith the version of Go that is actually being used. The CI script is also tightened so it prints the full diff on failure rather than a generic message.Problem
The codegen pipeline had four compounding reproducibility issues:
codegen/DockerfileFROMp4lang/third-party:latestlatestis re-tagged on every upstream push; any rebuild could silently pick up a differentprotocand produce byte-different outputcodegen/DockerfileARG GO_VERSION1.20.5codegen/Dockerfileprotoc-gen-gov1.31google.golang.org/protobuf v1.34.xcodegen/Dockerfileprotoc-gen-go-grpcv1.3v1.5.x;v1.3was the last release before the--go-grpc_opt=require_unimplemented_serversdefault changedgo.modgo 1.20go.moddeclared 1.20 whileupdate.shrangolang:1.20forgo mod tidy; mismatches between the declared minimum and the real toolchain trigger non-deterministicgo.sumupdates when contributors rungo mod tidylocally with a newer Gocodegen/update.shgolang:1.20golang:1.20resolves to the newest 1.20.x patch, which was already past EOLCI/check_codegen.sh[ ! -z "$diff" ]-zis a valid idiom but[ ! -z ]is the double-negative form flagged by ShellCheck SC2236; more importantly the script discards the diff output, making CI failures silent.github/workflows/codegen.ymlubuntu-latest/actions/checkout@v3ubuntu-latestswitches images without notice;@v3is deprecated by GitHubBecause
latestand floating toolchain tags are resolved at build time, two developers running./codegen/update.sha week apart could commit generated files that differ only in formatting or internal constants introduced by a plugin bump causing spurious CI failures for unrelated PRs.How I found it
Reviewing
codegen/Dockerfiledirectly revealed the three floating references (p4lang/third-party:latest,GO_VERSION=1.20.5, and the untagged@v1.31/@v1.3plugin installs). Cross-referencing with the generated file headers ingo/p4/v1/p4runtime.pb.goconfirmed the mismatch:go.moddeclaringgo 1.20whileupdate.shpullinggolang:1.20closed the loop the module minimum and the tidy toolchain were the same stale version, which would silently diverge once a developer with Go 1.22+ rango mod tidylocally.Changes
codegen/Dockerfilep4lang/third-party:latestwith a pinnedsha256digest. The comment above theFROMline documents how torotate it.
GO_VERSIONfrom1.20.5→1.25.11(latest stablepatch in the current supported 1.25.x series per https://go.dev/dl/).
Added per-architecture
sha256sumverification of the downloadedtarball so a supply-chain attack on
dl.google.comis caughtimmediately. SHA-256 values are sourced directly from the official
downloads page and must be updated in lockstep with
GO_VERSION.protoc-gen-go: bumped fromv1.31→v1.34.2.protoc-gen-go-grpc: bumped fromv1.3→v1.5.1.ARG-based versions for both plugins so they can be overriddenat
docker buildtime without editing the file.rm -rf /var/lib/apt/lists/*afterapt-getto reduce imagesize.
curlflag from-Lto-fsSL(-ffails on HTTP errors,-ssuppresses progress noise,-Sre-enables error messages).go.modgodirective from1.20→1.25to match the toolchainused in
update.shandcodegen/Dockerfile.google.golang.org/genprotodependency (whichhas been deprecated in favour of split sub-modules) with
google.golang.org/genproto/googleapis/rpc, the only sub-packageactually imported by the generated files.
google.golang.org/grpcfromv1.56.3→v1.65.0.google.golang.org/protobuffromv1.33.0→v1.34.2.github.com/golang/protobufindirectdependency (superseded by
google.golang.org/protobuf).go.sumRegenerated to match the updated
go.mod. All entries have correcth1:hashes verified against the Go checksum database.codegen/update.shgo mod tidystep fromgolang:1.20→golang:1.25.11to keep it in lock-step with the Dockerfile and
go.mod.CI/check_codegen.sh[ ! -z "$diff" ]with the cleaner[ -n "$diff" ].git status --porcelaintogit diff --name-onlyso that the check respects.gitattributesline-ending normalization instead of flagging line-ending-only
differences as content changes.
see exactly what changed without having to re-run locally.
logs.
.gitattributes(new file)*.go,*.py,*.sh,*.mod,and
*.sumfiles regardless of the contributor's local Gitcore.autocrlfsetting. This was required because Windowscheckouts of this repo previously had
core.autocrlf=trueconfigured (commonly the default on
git for windowsinstalls),which silently rewrote LF generated files to CRLF on
git add,producing noisy diffs unrelated to the actual codegen change.
.github/workflows/codegen.ymlubuntu-latest→ubuntu-22.04for a stableenvironment.
actions/checkoutfrom@v3→@v4.--rmto thedocker runcall that compiles protos (theexisting call already had it in
update.shbut not in the workflow).How to verify
The generated file headers in
go/p4/v1/p4runtime.pb.goshould nowread:
Checklist
protoc-gen-goandprotoc-gen-go-grpcbumped and pinnedgo.modminimum version updated to match toolchaingenprotomonorepo dependency replaced with split sub-modulego.sumregeneratedupdate.shgolang image pinned to match