From 746064dbc963683595e7d4e7b727ace75018016d Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 25 Aug 2026 22:02:46 -0700 Subject: [PATCH 1/2] ci(tools): build, vet, and test the Go modules under tools/ Nothing did this. No workflow ran `go test` over any module under tools/, so the tests those modules carry had not executed since they were written and a pull request could break them and still go green. That is not hypothetical. tools/docs-version-sync is on main with three failing tests, all expecting bitnami-cassandra in the EA-CVE section where it is no longer classified. Either the data moved and the tests did not, or the classification changed and the expectation is stale. Neither is this change's to decide, so its tests are excluded with a reference to NVIDIA/nvcf#1223 and the module is still built and vetted. Everything else is covered now rather than after that is resolved. Modules are discovered rather than listed, so a new tool is covered the moment it has a go.mod. A list would need editing by exactly the person least likely to think of it. Three details the first version got wrong, each found by running it: tools/go-toolchain has no source at all, deliberately, because rules_go's from_file requires a file named exactly go.mod. Building it errors with "matched no packages", which is not a failure, so a module with nothing to build is now recognised by having no packages rather than by name. The nested-test probe used a ** glob, and bash needs globstar for that and has it off by default, so a module whose tests sit one directory down looked like it had none. tools/byoo is exactly that shape. `go build ./...` writes each main package's executable into the working directory, which under -C is the module itself. Running the check left binaries behind and rewrote tools/changelog-site/changelog-site, which is committed. Output now goes to a scratch directory. Verified to fail, not merely to pass: a broken test in a covered module, a build break in the module whose tests are excluded, and a vet-only problem are each reported and exit non-zero, and a clean tree exits zero leaving nothing behind. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/build-test.yml | 18 +++++ tools/ci/check-go-tools | 122 +++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100755 tools/ci/check-go-tools diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 4c1623f3a..11cd62a89 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -118,6 +118,24 @@ jobs: exit 1 fi + go-tools: + name: Go tools + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + + - uses: actions/setup-go@v5 + with: + go-version-file: tools/go-toolchain/go.mod + + # The repo tooling modules carry tests that no workflow ran, so a pull + # request could break them and still go green. tools/docs-version-sync + # reached main with three failing tests that way. + - name: Build, vet, and test the Go modules under tools/ + run: tools/ci/check-go-tools + github-release-helper: name: GitHub release helper runs-on: ubuntu-latest diff --git a/tools/ci/check-go-tools b/tools/ci/check-go-tools new file mode 100755 index 000000000..94867c2c9 --- /dev/null +++ b/tools/ci/check-go-tools @@ -0,0 +1,122 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Build, vet, and test every Go module under tools/. +# +# Nothing did this before. The repo tooling modules carry _test.go files that no +# workflow ran, so a pull request could break them and still go green. That is +# how tools/docs-version-sync came to have three failing tests on main without +# anyone noticing: they had not run in CI since they were written. +# +# Modules are discovered rather than listed, so a new tool is covered the moment +# it has a go.mod. A list would need editing by exactly the person who is least +# likely to think of it. +# +# Modules with no test files are still built and vetted. That is not the same as +# being tested, and the summary says which is which, because "all modules +# passed" reads as stronger than it is when half of them have no tests. +set -uo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "${repo_root}" + +build_out="$(mktemp -d)" +trap 'rm -rf "${build_out}"' EXIT + +# Modules whose tests are known to fail on main and are not this check's to fix. +# Each entry needs a tracking issue: the point of an exclusion is that someone +# comes back for it, and an unexplained one just becomes permanent. Excluded +# modules are still built and vetted. +# +# docs-version-sync three manifest tests expect bitnami-cassandra in the +# EA-CVE section and it is no longer classified there. +# The data or the classification moved and the tests did +# not, which is exactly the drift this check exists to +# prevent from recurring. See NVIDIA/nvcf#1223. +skip_tests=( + "tools/docs-version-sync" +) + +skipped() { + local m="$1" s + for s in "${skip_tests[@]}"; do [ "${m}" = "${s}" ] && return 0; done + return 1 +} + +modules=() +while IFS= read -r f; do + modules+=("$(dirname "${f}")") +done < <(find tools -name go.mod -not -path '*/vendor/*' | sort) + +if [ "${#modules[@]}" -eq 0 ]; then + echo "error: no Go modules found under tools/; has the layout changed?" >&2 + exit 1 +fi + +failures=0 +tested=0 +built_only=0 +excluded=0 +declaration_only=0 + +for m in "${modules[@]}"; do + # tools/go-toolchain declares the Go version and contains no source at all, on + # purpose: rules_go's from_file requires a file named exactly go.mod. Building + # it errors with "matched no packages", which is not a failure, so a module + # with nothing to build is recognised rather than special-cased by name. + if [ -z "$(go list -C "${m}" ./... 2>/dev/null)" ]; then + echo "ok ${m} (declaration only, no packages to build)" + declaration_only=$((declaration_only + 1)) + continue + fi + # -o a scratch directory. Plain `go build ./...` writes each main package's + # executable into the working directory, which with -C is the module itself, + # so running this check would leave binaries behind and, for + # tools/changelog-site, rewrite one that is committed. + if ! out=$(go build -C "${m}" -o "${build_out}/" ./... 2>&1); then + echo "FAIL ${m}: build" >&2 + printf '%s\n' "${out}" >&2 + failures=$((failures + 1)) + continue + fi + if ! out=$(go vet -C "${m}" ./... 2>&1); then + echo "FAIL ${m}: vet" >&2 + printf '%s\n' "${out}" >&2 + failures=$((failures + 1)) + continue + fi + + if skipped "${m}"; then + echo "ok ${m} (built and vetted; tests excluded, see the list in this script)" + excluded=$((excluded + 1)) + continue + fi + + # A module with no test files is a pass for build and vet, and nothing more. + # Saying so keeps the summary honest. + # find, not a glob: bash needs globstar for ** and it is off by default, so + # the nested case would silently look like "no tests". tools/byoo keeps its + # only test file one directory down. + if [ -z "$(find "${m}" -name '*_test.go' -not -path '*/vendor/*' -print -quit)" ]; then + echo "ok ${m} (built and vetted; no tests)" + built_only=$((built_only + 1)) + continue + fi + + if ! out=$(go test -C "${m}" ./... 2>&1); then + echo "FAIL ${m}: tests" >&2 + printf '%s\n' "${out}" >&2 + failures=$((failures + 1)) + continue + fi + echo "ok ${m} (built, vetted, tested)" + tested=$((tested + 1)) +done + +echo +echo "${#modules[@]} modules: ${tested} tested, ${built_only} without tests, ${excluded} with tests excluded, ${declaration_only} declaration only, ${failures} failed" + +if [ "${failures}" -gt 0 ]; then + exit 1 +fi From f522a0872a78abc582b0710b0cd256498c9b3fda Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 10:26:23 -0700 Subject: [PATCH 2/2] fix(ci): fail check-go-tools when a module cannot be read Two findings from review, both real. A module whose go.mod is malformed was reported as "ok, declaration only" and the check exited 0. `go list ./...` exits non-zero and prints nothing both for a module with no source and for one it cannot read, so testing its output or its status alone cannot tell them apart. `go mod edit -json` can: it succeeds on a valid go.mod with no source, and fails on a malformed one. A broken module passing silently is the exact failure this check exists to catch. The check had no committed test. tools/ci/test-check-go-tools adds nineteen assertions over fixture repositories rather than the real one, so they do not move when a tool is added under tools/. It covers a healthy module, a declaration-only module, a malformed go.mod, a failing test, a build break, a vet-only problem, a module with no tests, a test one directory down, an empty tools tree, and the check leaving no build output behind. Writing it found a third bug: `go build -o ./...` fails with "no main packages to build" on a library-only module, which is a perfectly valid shape under tools/. Every module there happens to have a main package today, so nothing surfaced it. Modules with a main package build to a scratch directory, and library-only modules get a plain build whose object output Go discards. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/build-test.yml | 3 + tools/ci/check-go-tools | 46 +++++++-- tools/ci/test-check-go-tools | 164 +++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 10 deletions(-) create mode 100755 tools/ci/test-check-go-tools diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 11cd62a89..ad7d89c55 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -133,6 +133,9 @@ jobs: # The repo tooling modules carry tests that no workflow ran, so a pull # request could break them and still go green. tools/docs-version-sync # reached main with three failing tests that way. + - name: Test the checker + run: tools/ci/test-check-go-tools + - name: Build, vet, and test the Go modules under tools/ run: tools/ci/check-go-tools diff --git a/tools/ci/check-go-tools b/tools/ci/check-go-tools index 94867c2c9..7fe6ec506 100755 --- a/tools/ci/check-go-tools +++ b/tools/ci/check-go-tools @@ -61,20 +61,46 @@ excluded=0 declaration_only=0 for m in "${modules[@]}"; do + # A module must at least be readable. `go mod edit -json` is what separates + # the two ways a module can have nothing to list: it succeeds on a valid + # go.mod with no source, and fails on a malformed one. + # + # `go list ./...` cannot make that distinction. It exits non-zero and prints + # nothing in BOTH cases, so testing its output or its status alone reports a + # module with a broken go.mod as "declaration only" and passes it. That is the + # exact failure this check exists to catch. + if ! out=$(go mod edit -C "${m}" -json 2>&1); then + echo "FAIL ${m}: go.mod is not readable" >&2 + printf '%s\n' "${out}" >&2 + failures=$((failures + 1)) + continue + fi + # tools/go-toolchain declares the Go version and contains no source at all, on - # purpose: rules_go's from_file requires a file named exactly go.mod. Building - # it errors with "matched no packages", which is not a failure, so a module - # with nothing to build is recognised rather than special-cased by name. - if [ -z "$(go list -C "${m}" ./... 2>/dev/null)" ]; then - echo "ok ${m} (declaration only, no packages to build)" + # purpose: rules_go's from_file requires a file named exactly go.mod. + if [ -z "$(find "${m}" -name '*.go' -not -path '*/vendor/*' -print -quit)" ]; then + echo "ok ${m} (declaration only, no Go source)" declaration_only=$((declaration_only + 1)) continue fi - # -o a scratch directory. Plain `go build ./...` writes each main package's - # executable into the working directory, which with -C is the module itself, - # so running this check would leave binaries behind and, for - # tools/changelog-site, rewrite one that is committed. - if ! out=$(go build -C "${m}" -o "${build_out}/" ./... 2>&1); then + + # Where the build output goes depends on whether the module has a main + # package, and getting it wrong breaks one case or the other. + # + # With a main package, plain `go build ./...` writes the executable into the + # working directory, which under -C is the module itself. That leaves binaries + # behind and, for tools/changelog-site, rewrites one that is committed, so the + # output goes to a scratch directory. + # + # With no main package, `go build -o ./...` is itself an error: "no main + # packages to build". A library-only module under tools/ is perfectly valid, + # so it gets a plain build, whose object output Go discards on its own. + if [ -n "$(go list -C "${m}" -f '{{if eq .Name "main"}}x{{end}}' ./... 2>/dev/null)" ]; then + out=$(go build -C "${m}" -o "${build_out}/" ./... 2>&1) + else + out=$(go build -C "${m}" ./... 2>&1) + fi + if [ $? -ne 0 ]; then echo "FAIL ${m}: build" >&2 printf '%s\n' "${out}" >&2 failures=$((failures + 1)) diff --git a/tools/ci/test-check-go-tools b/tools/ci/test-check-go-tools new file mode 100755 index 000000000..378bf6558 --- /dev/null +++ b/tools/ci/test-check-go-tools @@ -0,0 +1,164 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Behavioral test for tools/ci/check-go-tools. +# +# A check that only ever passes is indistinguishable from one that does nothing, +# so every case below asserts a specific failure or a specific classification. +# The important one is the malformed go.mod: `go list ./...` exits non-zero and +# prints nothing both for a module with no source and for one it cannot read, so +# an implementation that looks at either alone reports a broken module as +# "declaration only" and passes it. +set -uo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +check="${here}/check-go-tools" +fail=0 + +# Each case runs the check against a fixture repository, not the real one, so +# the assertions do not move when a tool is added to tools/. +scratch="$(mktemp -d)" +trap 'rm -rf "${scratch}"' EXIT + +# new_repo -> echoes a repo root with tools/ci/check-go-tools in place +new_repo() { + local root="${scratch}/$1" + mkdir -p "${root}/tools/ci" + cp "${check}" "${root}/tools/ci/check-go-tools" + chmod +x "${root}/tools/ci/check-go-tools" + echo "${root}" +} + +# add_module [main.go body] [test body] +add_module() { + local root="$1" name="$2" + mkdir -p "${root}/tools/${name}" + printf '%s\n' "$3" > "${root}/tools/${name}/go.mod" + [ -n "${4:-}" ] && printf '%s\n' "$4" > "${root}/tools/${name}/main.go" + [ -n "${5:-}" ] && printf '%s\n' "$5" > "${root}/tools/${name}/main_test.go" + return 0 +} + +GOOD_MOD='module good + +go 1.26' +GOOD_MAIN='package main + +func main() {}' +GOOD_TEST='package main + +import "testing" + +func TestPasses(t *testing.T) {}' + +run() { # run -> sets $out, returns the check's exit code + out="$(cd "$1" && ./tools/ci/check-go-tools 2>&1)" +} + +expect_exit() { # expect_exit + local desc="$1" want="$2" root="$3" + run "${root}"; local got=$? + if [ "${got}" = "${want}" ]; then + printf 'ok %s\n' "${desc}" + else + printf 'FAIL %s: want exit %s, got %s\n%s\n' "${desc}" "${want}" "${got}" "${out}" + fail=1 + fi +} + +expect_output() { # expect_output + local desc="$1" pat="$2" root="$3" + run "${root}" + case "${out}" in + *"${pat}"*) printf 'ok %s\n' "${desc}" ;; + *) printf 'FAIL %s: output lacked %s\n%s\n' "${desc}" "${pat}" "${out}"; fail=1 ;; + esac +} + +# A healthy module passes and is reported as tested. +r="$(new_repo healthy)" +add_module "${r}" good "${GOOD_MOD}" "${GOOD_MAIN}" "${GOOD_TEST}" +expect_exit "a healthy module passes" 0 "${r}" +expect_output "and is counted as tested" "1 modules: 1 tested" "${r}" + +# A module with a valid go.mod and no source is declaration only, not a failure. +# tools/go-toolchain is exactly this shape. +r="$(new_repo declonly)" +add_module "${r}" decl "module decl + +go 1.26" +expect_exit "a module with no Go source is not a failure" 0 "${r}" +expect_output "and is reported as declaration only" "declaration only, no Go source" "${r}" + +# The case that motivates the whole discriminator. `go list ./...` behaves +# identically here and above; only reading go.mod separates them. +r="$(new_repo malformed)" +add_module "${r}" broken "this is not a valid go.mod" "${GOOD_MAIN}" +expect_exit "a malformed go.mod fails" 1 "${r}" +expect_output "and is named as unreadable, not skipped" "go.mod is not readable" "${r}" + +# A failing test must fail the check. +r="$(new_repo failingtest)" +add_module "${r}" bad "${GOOD_MOD}" "${GOOD_MAIN}" 'package main + +import "testing" + +func TestFails(t *testing.T) { t.Fatal("injected") }' +expect_exit "a failing test fails the check" 1 "${r}" +expect_output "and says which module" "FAIL tools/bad: tests" "${r}" + +# A build break must fail the check. +r="$(new_repo buildbreak)" +add_module "${r}" bad "${GOOD_MOD}" 'package main + +func main() { undefinedSymbol() }' +expect_exit "a build break fails the check" 1 "${r}" +expect_output "and says which module" "FAIL tools/bad: build" "${r}" + +# A vet-only problem must fail: building is not enough. +r="$(new_repo vetbreak)" +add_module "${r}" bad "${GOOD_MOD}" 'package main + +import "fmt" + +func main() { fmt.Printf("%d\n", "not a number") }' +expect_exit "a vet-only problem fails the check" 1 "${r}" +expect_output "and says which module" "FAIL tools/bad: vet" "${r}" + +# A module without tests is built and vetted, and counted separately. Reporting +# it as "tested" would overstate what the check proved. +r="$(new_repo notests)" +add_module "${r}" bare "${GOOD_MOD}" "${GOOD_MAIN}" +expect_exit "a module without tests passes" 0 "${r}" +expect_output "and is not counted as tested" "1 modules: 0 tested, 1 without tests" "${r}" + +# Tests one directory down still count. A ** glob would miss these, because bash +# needs globstar and has it off by default. +r="$(new_repo nested)" +mkdir -p "${r}/tools/nest/sub" +printf 'module nest\n\ngo 1.26\n' > "${r}/tools/nest/go.mod" +printf 'package sub\n' > "${r}/tools/nest/sub/sub.go" +printf 'package sub\n\nimport "testing"\n\nfunc TestNested(t *testing.T) { t.Fatal("injected") }\n' \ + > "${r}/tools/nest/sub/sub_test.go" +expect_exit "a test one directory down is found and run" 1 "${r}" +expect_output "and is reported" "FAIL tools/nest: tests" "${r}" + +# No modules at all means the layout moved; passing silently would hide it. +r="$(new_repo empty)" +expect_exit "no modules found is an error" 1 "${r}" +expect_output "and says the layout may have changed" "no Go modules found" "${r}" + +# The check must not leave build output behind. `go build ./...` writes each +# main package's executable into the working directory. +r="$(new_repo clean)" +add_module "${r}" good "${GOOD_MOD}" "${GOOD_MAIN}" +run "${r}" +if [ -e "${r}/tools/good/good" ]; then + printf 'FAIL the check left a binary in the module directory\n'; fail=1 +else + printf 'ok the check leaves no build output behind\n' +fi + +[ "${fail}" -eq 0 ] && echo "check-go-tools: all checks passed" || { + echo "check-go-tools: FAILED" >&2; exit 1; }