fix: allow 'build'/'unit-tests'/'validate-csv' make targets to work on non-linux machines - #2839
fix: allow 'build'/'unit-tests'/'validate-csv' make targets to work on non-linux machines#2839cdesiniotis wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR replaces the shared host Merge Risk: 🔴 Critical · up to Linux builds and unit-test targets cannot compile because the new platform-specific code references an undefined package identifier, so the PR is not merge-ready until that import is added. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: b2310563-779b-4321-a818-e6b1e76d677f
📒 Files selected for processing (5)
cmd/nvidia-validator/main.gocmd/nvidia-validator/main_test.gocmd/nvidia-validator/resolve_linux.gocmd/nvidia-validator/resolve_linux_test.gocmd/nvidia-validator/resolve_unsupported.go
💤 Files with no reviewable changes (2)
- cmd/nvidia-validator/main_test.go
- cmd/nvidia-validator/main.go
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| // the privileged validator never execs a bogus binary from a non-standard path. | ||
| func resolveHostNvidiaSMI(hostRootCtrPath string) (string, error) { | ||
| for _, nvidiaSMIPath := range hostNvidiaSMISearchPaths { | ||
| f, err := pathrs.OpenInRoot(hostRootCtrPath, nvidiaSMIPath) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- resolve_linux.go ---'
cat -n cmd/nvidia-validator/resolve_linux.go
printf '%s\n' '--- module declarations ---'
rg -n 'go-pathrs|pathrs|^module ' go.mod go.sum cmd internal controllers 2>/dev/null | head -120
printf '%s\n' '--- scoped repository guidance ---'
find /tmp/coderabbit-repo-knowledge/nvidia-gpu-operator-eed98df4 -maxdepth 2 -type f -name '*.md' -printRepository: NVIDIA/gpu-operator
Length of output: 3506
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- go.mod dependency context ---'
sed -n '28,48p' go.mod
printf '%s\n' '--- existing pathrs caller ---'
cat -n internal/driver/hostsmi.go
printf '%s\n' '--- pathrs references and local module metadata ---'
rg -n 'OpenInRoot|cyphar.com/go-pathrs|github.com/cyphar/filepath-securejoin/pathrs-lite' --glob '*.go' --glob 'go.mod' --glob 'go.sum' .
if go env GOMODCACHE >/dev/null 2>&1; then
cache="$(go env GOMODCACHE)"
find "$cache" -path '*go-pathrs*' -o -path '*filepath-securejoin*' 2>/dev/null | head -40
fiRepository: NVIDIA/gpu-operator
Length of output: 10179
Import the pathrs package.
Line 42 references an undefined identifier. Add pathrs "github.com/cyphar/filepath-securejoin/pathrs-lite" so Linux builds compile.
🧰 Tools
🪛 GitHub Check: config-checks / validate-csv
[failure] 42-42:
undefined: pathrs
🪛 GitHub Check: config-checks / validate-helm-values
[failure] 42-42:
undefined: pathrs
Source: Linters/SAST tools
caf720f to
9aa8cc6
Compare
15faf78 to
09039be
Compare
…hines
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
do not default to GOOS=linux in Makefile
This reduces friction for running make targets on non-linux machines.
For example, the `validate-csv` target would fail prior to this commit
with the following error message:
```
CGO_ENABLED=0 GOOS=linux \
go build -ldflags "-s -w -X github.com/NVIDIA/gpu-operator/internal/info.gitCommit=9aa8cc642 -X github.com/NVIDIA/gpu-operator/internal/info.version=v26.7.0" github.com/NVIDIA/gpu-operator/cmd/gpuop-cfg
./gpuop-cfg validate csv --input=./bundle/manifests/gpu-operator-certified.clusterserviceversion.yaml
./gpuop-cfg: ./gpuop-cfg: cannot execute binary file
make: *** [validate-csv] Error 126
```
This change should not affect our GHA jobs as they all run on
linux runners, and therefore, GOOS should be set to linux there.
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
09039be to
d271652
Compare
|
Our current CI runs only on Linux, so it will not catch a future linux only dependency being introduced back into shared code. Can we add a non linux build check for this case? For example, |
Description
make build/make unit-testsfails with the below error:This PR moves the linux-only logic to its own build-tagged file, with a stub for other platforms.
This PR also updates our Makefile so that
GOOS=linuxis not configured by default. This reduces friction when running other make targets on non-linux machines. For example, thevalidate-csvtarget would fail prior to this commit with the following error message:This change should not affect our GHA jobs as they all run on linux runners, and therefore, GOOS should be set to linux there.
Checklist
make lint)make validate-generated-assets)make validate-modules)Testing