fix: respect CARGO_HOME in install.sh#326
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the ci/install.sh installer to honor CARGO_HOME when choosing the default install destination, aligning behavior with user expectations and documented Cargo configuration.
Changes:
- Default
--todestination now resolves to$CARGO_HOME/binwhenCARGO_HOMEis set, otherwise$HOME/.cargo/bin. - Adds stateless journey coverage for both default-destination branches via snapshot tests.
- Adds new failure snapshots for installer output in both scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ci/install.sh |
Adjusts default install destination logic and updates --help text to mention CARGO_HOME. |
tests/stateless-journey.sh |
Adds journey tests that exercise the installer’s default destination selection. |
tests/snapshots/failure-install-script-defaults-to-home-cargo-bin |
Snapshot for installer output when CARGO_HOME is not used. |
tests/snapshots/failure-install-script-defaults-to-cargo-home |
Snapshot for installer output when defaulting to CARGO_HOME/bin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -130,7 +130,7 @@ fi | |||
| say_err "Target: $target" | |||
|
|
|||
| if [ -z $dest ]; then | |||
There was a problem hiding this comment.
dest is optional and may be unset/empty when --to is omitted. In POSIX sh, [ -z $dest ] becomes [ -z ] and typically exits with a "missing argument"/"unary operator expected" error (and with set -e this aborts the script). Quote the variable in the test (and ideally keep it consistently quoted anywhere it’s tested).
| if [ -z $dest ]; then | |
| if [ -z "$dest" ]; then |
| rm -rf /tmp/dua-install-home /tmp/dua-cargo-home | ||
| it "uses CARGO_HOME for the default install location" && { | ||
| WITH_SNAPSHOT="$snapshot/failure-install-script-defaults-to-cargo-home" \ | ||
| PATH="$PWD/fakebin:$PATH" \ | ||
| HOME=/tmp/dua-install-home \ | ||
| CARGO_HOME=/tmp/dua-cargo-home \ | ||
| expect_run ${WITH_FAILURE} sh "$root/../ci/install.sh" \ |
There was a problem hiding this comment.
This test uses fixed global paths under /tmp and rm -rf on them. That can collide under parallel test runs and can delete unrelated data if those paths exist for some other purpose. Prefer using per-sandbox paths (e.g., $PWD/home, $PWD/cargo-home, or a subdir of the sandbox tempdir) and avoid removing global /tmp/... entries.
| it "falls back to HOME/.cargo/bin when CARGO_HOME is unset" && { | ||
| WITH_SNAPSHOT="$snapshot/failure-install-script-defaults-to-home-cargo-bin" \ | ||
| PATH="$PWD/fakebin:$PATH" \ | ||
| HOME=/tmp/dua-install-home \ |
There was a problem hiding this comment.
The “CARGO_HOME is unset” case isn’t forced here: if the runner’s environment already has CARGO_HOME set, it will be inherited by the sh ci/install.sh process and this snapshot will fail. Make the test deterministic by explicitly unsetting CARGO_HOME (or setting it to an empty value, which still exercises the fallback with ${CARGO_HOME:-...}).
| HOME=/tmp/dua-install-home \ | |
| HOME=/tmp/dua-install-home \ | |
| CARGO_HOME= \ |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot!
Let's keep only the one change to install.sh, then I can merge.
Fixes #279.
Before this change, omitting
--toalways defaulted the installer to$HOME/.cargo/bin, even whenCARGO_HOMEwas set. That made the documented installer path disagree with the user's configured Cargo home.This updates the default destination to use
$CARGO_HOME/binwhenCARGO_HOMEis set, and keeps the existing$HOME/.cargo/binfallback otherwise. The journey test coverage now exercises both branches with fixedHOME/CARGO_HOMEvalues.Validation:
HOME/CARGO_HOMEbefore and after the change./tests/stateless-journey.sh ./target/debug/duacargo fmt --all -- --checkcargo clippy -- -D warningscargo test --allstill failed locally in two unrelated interactive size assertions on this macOS environment