Fix dev-env-setup for macOS: replace wget with curl, install missing tools#1126
Fix dev-env-setup for macOS: replace wget with curl, install missing tools#1126ShreyaGupta08 wants to merge 5 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughUpdated the dev-environment setup script to prefer curl for downloads, load tool versions (including KO_VERSION) from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/setup-dev-env.sh (1)
433-434:⚠️ Potential issue | 🟠 MajorBlack installation on macOS may fail with PEP 668 error.
Poetry installation was updated to use
pipxto avoid PEP 668 "externally-managed-environment" errors on macOS, but Black installation at line 434 still usespip3 installdirectly without--break-system-packagesor--userfallback. This will likely fail on modern macOS with Homebrew Python.🐛 Proposed fix to handle PEP 668 for Black on macOS
if prompt_continue; then if [[ "${OS}" == "darwin" ]]; then - pip3 install black=="${BLACK_VERSION}" + pip3 install --break-system-packages black=="${BLACK_VERSION}" 2>/dev/null || \ + pip3 install --user black=="${BLACK_VERSION}" elif [[ "${OS}" == "linux" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-env.sh` around lines 433 - 434, The macOS Black install line uses pip3 install directly and can trigger PEP 668; update the darwin branch where pip3 installs Black (the pip3 install black=="${BLACK_VERSION}" invocation) to either install via pipx (preferred) like pipx install "black==${BLACK_VERSION}" or, if pipx is not available, run pip3 with a safe fallback: first try pip3 install --break-system-packages "black==${BLACK_VERSION}" and if that fails fallback to python3 -m pip install --user "black==${BLACK_VERSION}" so installations on Homebrew macOS don't hit the externally-managed-environment error.
🧹 Nitpick comments (1)
scripts/setup-dev-env.sh (1)
671-678: Shell profile suggestion could be more universal.Line 677 only suggests
~/.zshrc, but users may use bash (~/.bashrc) or other shells. Consider detecting the current shell or providing a more generic suggestion.♻️ Proposed fix for more universal shell guidance
if [[ ":${PATH}:" != *":${GOPATH_BIN}:"* ]]; then export PATH="${PATH}:${GOPATH_BIN}" log_warning "Added ${GOPATH_BIN} to PATH for this session" log_info "To make this permanent, add to your shell profile:" - log_info " echo 'export PATH=\"\$PATH:\$(go env GOPATH)/bin\"' >> ~/.zshrc" + SHELL_RC="${HOME}/.$(basename "${SHELL}")rc" + log_info " echo 'export PATH=\"\$PATH:\$(go env GOPATH)/bin\"' >> ${SHELL_RC}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-env.sh` around lines 671 - 678, The suggestion to append GOPATH_BIN to a single shell profile (~/.zshrc) is too narrow; update the logic around GOPATH_BIN and the log_info messages to detect the current shell (using $SHELL or $0) and print a tailored, universal instruction — e.g., show the appropriate profile file (~/.bashrc, ~/.zshrc, ~/.profile) or print multiple options — by modifying the block that sets GOPATH_BIN and calls log_warning/log_info so the log_info lines reference the detected profile(s) instead of hardcoding ~/.zshrc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-dev-env.sh`:
- Around line 403-410: After calling pipx ensurepath (before running pipx
install and pipx inject), ensure the pipx user-bin directory is added to the
current shell PATH so poetry is available in this session; detect the pipx bin
dir (e.g., use python3 -m site --user-base and append /bin, or fallback to
$HOME/.local/bin), export it into PATH for the current process, then proceed to
pipx install "poetry==${POETRY_VERSION}" and pipx inject. This change should be
applied near the pipx ensurepath/pipx install/pipx inject block so the later
summary correctly detects poetry.
- Around line 319-325: The macOS branch installs an unpinned Go via "brew
install go" causing version drift; change the macOS flow in the OS == "darwin"
block (where command_exists and log_success/log_error are used) to call the
Makefile target "make install-go-ci" so the Go version is pinned to GO_VERSION
from .versions.yaml (same behavior as the Linux/CI path) and adjust the
success/error log messages accordingly.
---
Outside diff comments:
In `@scripts/setup-dev-env.sh`:
- Around line 433-434: The macOS Black install line uses pip3 install directly
and can trigger PEP 668; update the darwin branch where pip3 installs Black (the
pip3 install black=="${BLACK_VERSION}" invocation) to either install via pipx
(preferred) like pipx install "black==${BLACK_VERSION}" or, if pipx is not
available, run pip3 with a safe fallback: first try pip3 install
--break-system-packages "black==${BLACK_VERSION}" and if that fails fallback to
python3 -m pip install --user "black==${BLACK_VERSION}" so installations on
Homebrew macOS don't hit the externally-managed-environment error.
---
Nitpick comments:
In `@scripts/setup-dev-env.sh`:
- Around line 671-678: The suggestion to append GOPATH_BIN to a single shell
profile (~/.zshrc) is too narrow; update the logic around GOPATH_BIN and the
log_info messages to detect the current shell (using $SHELL or $0) and print a
tailored, universal instruction — e.g., show the appropriate profile file
(~/.bashrc, ~/.zshrc, ~/.profile) or print multiple options — by modifying the
block that sets GOPATH_BIN and calls log_warning/log_info so the log_info lines
reference the detected profile(s) instead of hardcoding ~/.zshrc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c6b30ce-2028-4941-8557-8bdedca3c1c4
📒 Files selected for processing (1)
scripts/setup-dev-env.sh
…tools - Replace all wget calls with curl (wget not available on macOS by default) - Install Go via Homebrew on macOS instead of just printing a manual hint - Install addlicense (required by license-headers-lint but was missing) - Add GOPATH/bin to PATH with permanent setup guidance - Use pipx for Poetry on macOS to avoid PEP 668 externally-managed-environment errors Fixes NVIDIA#1125 Signed-off-by: Shreya Gupta <shregupta@nvidia.com>
- Install ko at pinned version from .versions.yaml (required by Tilt to build Go services) - Add Helm chart repos (jetstack, prometheus-community, sigs-kwok) needed by make dev-env - Add ko to summary tools checklist Signed-off-by: Shreya Gupta <shregupta@nvidia.com>
94ecd6a to
c08bc42
Compare
|
/ok to test 5db3cd3 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/setup-dev-env.sh (2)
320-323:⚠️ Potential issue | 🟠 MajorUse the pinned Go installer on macOS.
brew install goignoresGO_VERSION, so macOS can drift from.versions.yamland CI.Makefile:201-268already supports darwin ininstall-go-ci, so calling that here keeps both platforms on the same version.Suggested fix
- if command_exists brew; then - brew install go - log_success "Go installed via Homebrew" + if command_exists brew; then + make install-go-ci + log_success "Go ${GO_VERSION} installed" else log_error "Homebrew not found. Please install Go manually or install Homebrew first." fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-env.sh` around lines 320 - 323, The macOS branch currently runs "brew install go" which ignores GO_VERSION; replace that call so macOS uses the same pinned installer as CI by invoking the Makefile's install-go-ci target instead (call the Makefile target used elsewhere, e.g., install-go-ci) so the script uses the pinned GO_VERSION for darwin; update the block that references brew (in scripts/setup-dev-env.sh) to call the Makefile target (install-go-ci) for darwin rather than brew install.
404-411:⚠️ Potential issue | 🟠 MajorExport the pipx bin dir before installing Poetry.
pipx ensurepathupdates shell config files for later sessions; it does not makepoetryavailable in the current shell, so the summary at the end can still report Poetry as missing in the same run. (pipx.pypa.io)Suggested fix
if ! command_exists pipx; then log_info "Installing pipx first (required for Poetry on macOS)..." brew install pipx pipx ensurepath fi + PIPX_BIN="$(python3 -m site --user-base 2>/dev/null || echo "${HOME}/.local")/bin" + if [[ ":${PATH}:" != *":${PIPX_BIN}:"* ]]; then + export PATH="${PIPX_BIN}:${PATH}" + log_info "Added ${PIPX_BIN} to PATH for this session" + fi pipx install "poetry==${POETRY_VERSION}" pipx inject poetry "poetry-plugin-export==${POETRY_PLUGIN_EXPORT_VERSION}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-env.sh` around lines 404 - 411, The script calls pipx ensurepath but does not make the pipx user-bin dir available in the current shell before running pipx install, so export the pipx bin directory into PATH immediately after pipx ensurepath and before pipx install "poetry==${POETRY_VERSION}"; you can compute the user bin with python3 -m site --user-base and append "/bin" (or fall back to ~/.local/bin on Linux/macOS) and export PATH="$USER_BASE_BIN:$PATH", then run pipx install and pipx inject as currently done (refer to pipx ensurepath, pipx install "poetry==${POETRY_VERSION}", pipx inject poetry "poetry-plugin-export==${POETRY_PLUGIN_EXPORT_VERSION}").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-dev-env.sh`:
- Around line 635-640: The script currently prints log_success "ko installed"
regardless of whether the go install ran; move the success message inside the
branch that actually runs go install (the if block using command_exists and
invoking go install github.com/google/ko@"${KO_VERSION}") and only emit
log_success after the install command completes successfully (check its exit
status); keep the log_warning "Go not available — cannot install ko. Install Go
first." in the else branch and do not emit the success message there.
- Around line 698-703: The script currently appends GOPATH_BIN to PATH which
lets system tools override pinned versions; change it to prepend GOPATH_BIN by
exporting PATH="${GOPATH_BIN}:${PATH}" (update the export that references
GOPATH_BIN and the log_warning message if needed) and update the profile hint
lines (the log_info echo that writes export PATH) so it suggests echo 'export
PATH="${GOPATH_BIN}:$PATH"' >> ~/.zshrc (or use the original go env expression)
to ensure GOPATH/bin takes precedence at login and runtime; adjust references to
GOPATH_BIN, PATH export, and the profile hint messages accordingly.
---
Duplicate comments:
In `@scripts/setup-dev-env.sh`:
- Around line 320-323: The macOS branch currently runs "brew install go" which
ignores GO_VERSION; replace that call so macOS uses the same pinned installer as
CI by invoking the Makefile's install-go-ci target instead (call the Makefile
target used elsewhere, e.g., install-go-ci) so the script uses the pinned
GO_VERSION for darwin; update the block that references brew (in
scripts/setup-dev-env.sh) to call the Makefile target (install-go-ci) for darwin
rather than brew install.
- Around line 404-411: The script calls pipx ensurepath but does not make the
pipx user-bin dir available in the current shell before running pipx install, so
export the pipx bin directory into PATH immediately after pipx ensurepath and
before pipx install "poetry==${POETRY_VERSION}"; you can compute the user bin
with python3 -m site --user-base and append "/bin" (or fall back to ~/.local/bin
on Linux/macOS) and export PATH="$USER_BASE_BIN:$PATH", then run pipx install
and pipx inject as currently done (refer to pipx ensurepath, pipx install
"poetry==${POETRY_VERSION}", pipx inject poetry
"poetry-plugin-export==${POETRY_PLUGIN_EXPORT_VERSION}").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3e2cb68-8173-45af-b5b6-6e8a8f377e5d
📒 Files selected for processing (1)
scripts/setup-dev-env.sh
- Pin Go version on macOS: use make install-go-ci instead of brew install go, which installs unpinned latest. The Makefile target already supports darwin. - Add pipx bin to PATH for current session after Poetry install, so the summary check correctly detects poetry. - Fix ko install: only log success when go install actually runs, not when Go is unavailable. - Prepend GOPATH/bin to PATH instead of appending, so version-pinned tools in GOPATH/bin take precedence over any system-installed versions. Signed-off-by: Shreya Gupta <shregupta@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/setup-dev-env.sh (1)
306-321:⚠️ Potential issue | 🟠 MajorAuto-remediate Go version drift instead of continuing with the wrong toolchain.
If
gois present but does not match.versions.yaml, this branch only prints a manual command and keeps going. The latergo installsteps then run against the wrong compiler, sodev-env-setupno longer leaves the environment in the pinned state promised by the PR.Suggested fix
if command_exists go; then CURRENT_GO=$(go version | grep -o 'go[0-9]\+\.[0-9]\+\.[0-9]\+' | sed 's/go//' || echo "unknown") log_info "Current Go version: ${CURRENT_GO}" if [[ "${CURRENT_GO}" == "${GO_VERSION}"* ]]; then log_success "Go ${GO_VERSION} already installed" else log_warning "Go version mismatch (current: ${CURRENT_GO}, target: ${GO_VERSION})" - log_info "To install Go ${GO_VERSION}, run: make install-go-ci" + log_info "Installing Go ${GO_VERSION}..." + if prompt_continue; then + make install-go-ci + log_success "Go ${GO_VERSION} installed" + fi fi else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dev-env.sh` around lines 306 - 321, The script currently logs a Go version mismatch (using CURRENT_GO vs GO_VERSION) but continues; change the mismatch branch so the setup auto-remediates by invoking make install-go-ci (or prompting via prompt_continue and then calling make install-go-ci) and aborts or retries until the expected version is installed; update the branch that currently logs "Go version mismatch" to call make install-go-ci (using prompt_continue if interactive) and verify CURRENT_GO again (re-run go version) before proceeding so the rest of the script runs with the pinned GO_VERSION.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-dev-env.sh`:
- Around line 642-648: The helm repo add calls in the setup script are masking
failures by redirecting stderr and forcing success; update the block around
command_exists/helm to run each helm repo add without `2>/dev/null || true`,
capture its exit status and stderr, and only treat failures as non-fatal when
the stderr/output explicitly indicates the repo "already exists"; for other
non-zero exits (e.g., network errors) log the error with log_error including the
captured stderr and exit or return a non-zero status so the script stops rather
than incorrectly logging "Helm repositories configured"; reference the
command_exists check and the log_info/log_success calls to locate where to
insert the error handling and early exit logic.
- Around line 682-691: The script currently skips installation if any addlicense
binary exists (command_exists), which allows an unpinned system copy to satisfy
the check; change the logic to detect the installed addlicense version and
compare it to ADDLICENSE_VERSION: if addlicense is present, invoke it to obtain
its version string (e.g., call the binary with its version flag and parse
output), compare against the value in ADDLICENSE_VERSION, and only skip
installation when they match exactly; if the binary is missing or the version
differs, call go install github.com/google/addlicense@"${ADDLICENSE_VERSION}"
(as currently done inside the prompt_continue block) and update the log messages
in this block (references: ADDLICENSE_VERSION, command_exists, prompt_continue,
and the go install invocation).
---
Outside diff comments:
In `@scripts/setup-dev-env.sh`:
- Around line 306-321: The script currently logs a Go version mismatch (using
CURRENT_GO vs GO_VERSION) but continues; change the mismatch branch so the setup
auto-remediates by invoking make install-go-ci (or prompting via prompt_continue
and then calling make install-go-ci) and aborts or retries until the expected
version is installed; update the branch that currently logs "Go version
mismatch" to call make install-go-ci (using prompt_continue if interactive) and
verify CURRENT_GO again (re-run go version) before proceeding so the rest of the
script runs with the pinned GO_VERSION.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7626a7c6-c719-4808-83cf-23cac80f46ed
📒 Files selected for processing (1)
scripts/setup-dev-env.sh
| if command_exists helm; then | ||
| log_info "Adding required Helm chart repositories..." | ||
| helm repo add jetstack https://charts.jetstack.io 2>/dev/null || true | ||
| helm repo add prometheus-community https://prometheus-community.github.io/helm-charts 2>/dev/null || true | ||
| helm repo add sigs-kwok https://kwok.sigs.k8s.io/charts 2>/dev/null || true | ||
| helm repo update | ||
| log_success "Helm repositories configured" |
There was a problem hiding this comment.
Do not mask Helm repository add failures.
2>/dev/null || true hides every helm repo add failure, not just the idempotent "already exists" case. A transient network error here still ends with Helm repositories configured, leaving later chart installs to fail much later.
Suggested fix
if command_exists helm; then
log_info "Adding required Helm chart repositories..."
- helm repo add jetstack https://charts.jetstack.io 2>/dev/null || true
- helm repo add prometheus-community https://prometheus-community.github.io/helm-charts 2>/dev/null || true
- helm repo add sigs-kwok https://kwok.sigs.k8s.io/charts 2>/dev/null || true
+ helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'jetstack' || \
+ helm repo add jetstack https://charts.jetstack.io
+ helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'prometheus-community' || \
+ helm repo add prometheus-community https://prometheus-community.github.io/helm-charts
+ helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'sigs-kwok' || \
+ helm repo add sigs-kwok https://kwok.sigs.k8s.io/charts
helm repo update
log_success "Helm repositories configured"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if command_exists helm; then | |
| log_info "Adding required Helm chart repositories..." | |
| helm repo add jetstack https://charts.jetstack.io 2>/dev/null || true | |
| helm repo add prometheus-community https://prometheus-community.github.io/helm-charts 2>/dev/null || true | |
| helm repo add sigs-kwok https://kwok.sigs.k8s.io/charts 2>/dev/null || true | |
| helm repo update | |
| log_success "Helm repositories configured" | |
| if command_exists helm; then | |
| log_info "Adding required Helm chart repositories..." | |
| helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'jetstack' || \ | |
| helm repo add jetstack https://charts.jetstack.io | |
| helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'prometheus-community' || \ | |
| helm repo add prometheus-community https://prometheus-community.github.io/helm-charts | |
| helm repo list | awk 'NR > 1 {print $1}' | grep -qx 'sigs-kwok' || \ | |
| helm repo add sigs-kwok https://kwok.sigs.k8s.io/charts | |
| helm repo update | |
| log_success "Helm repositories configured" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-dev-env.sh` around lines 642 - 648, The helm repo add calls in
the setup script are masking failures by redirecting stderr and forcing success;
update the block around command_exists/helm to run each helm repo add without
`2>/dev/null || true`, capture its exit status and stderr, and only treat
failures as non-fatal when the stderr/output explicitly indicates the repo
"already exists"; for other non-zero exits (e.g., network errors) log the error
with log_error including the captured stderr and exit or return a non-zero
status so the script stops rather than incorrectly logging "Helm repositories
configured"; reference the command_exists check and the log_info/log_success
calls to locate where to insert the error handling and early exit logic.
| # Install addlicense (needed for license-headers-lint) | ||
| ADDLICENSE_VERSION=$(yq '.linting.addlicense' .versions.yaml) | ||
| if command_exists addlicense; then | ||
| log_success "addlicense already installed" | ||
| else | ||
| log_info "Installing addlicense ${ADDLICENSE_VERSION}..." | ||
| if prompt_continue; then | ||
| go install github.com/google/addlicense@"${ADDLICENSE_VERSION}" | ||
| log_success "addlicense installed" | ||
| fi |
There was a problem hiding this comment.
Presence-only checking is not enough for addlicense.
This skips installation as soon as any addlicense binary is found, even if it is an older system copy. That breaks the PR’s “exact versions from .versions.yaml” goal and can leave make license-headers-lint running against an unpinned tool.
Suggested fix
# Install addlicense (needed for license-headers-lint)
ADDLICENSE_VERSION=$(yq '.linting.addlicense' .versions.yaml)
- if command_exists addlicense; then
- log_success "addlicense already installed"
- else
- log_info "Installing addlicense ${ADDLICENSE_VERSION}..."
- if prompt_continue; then
- go install github.com/google/addlicense@"${ADDLICENSE_VERSION}"
- log_success "addlicense installed"
- fi
+ log_info "Installing addlicense ${ADDLICENSE_VERSION}..."
+ if prompt_continue; then
+ go install github.com/google/addlicense@"${ADDLICENSE_VERSION}"
+ log_success "addlicense installed"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-dev-env.sh` around lines 682 - 691, The script currently skips
installation if any addlicense binary exists (command_exists), which allows an
unpinned system copy to satisfy the check; change the logic to detect the
installed addlicense version and compare it to ADDLICENSE_VERSION: if addlicense
is present, invoke it to obtain its version string (e.g., call the binary with
its version flag and parse output), compare against the value in
ADDLICENSE_VERSION, and only skip installation when they match exactly; if the
binary is missing or the version differs, call go install
github.com/google/addlicense@"${ADDLICENSE_VERSION}" (as currently done inside
the prompt_continue block) and update the log messages in this block
(references: ADDLICENSE_VERSION, command_exists, prompt_continue, and the go
install invocation).
Summary
wgetcalls withcurl—wgetis not available on macOS by default, causingmake dev-env-setupto fail mid-scriptmake install-go-citar extraction fails on macOS)addlicense— required bymake license-headers-lintbut was not installed by the setup script (only CI installed it)GOPATH/binto PATH automatically during setup, with guidance for making it permanentpipxfor Poetry installation on macOS to avoid PEP 668externally-managed-environmenterrorsTest plan
make dev-env-setupon a fresh macOS (Apple Silicon) machine and verify it completes without errorsmake dev-env-setupon Linux and verify no regressions (curl replaces wget, all other logic unchanged)make lint-test-allafter setup completes and verify all lint/test stages pass without manual tool installationaddlicense,protoc-gen-go,protoc-gen-go-grpc,golangci-lint,gotestsum,poetryare all available on PATH after setupFixes #1125
Summary by CodeRabbit