Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Homebrew Bundle’s Cargo integration to preserve user Cargo/Rustup environment configuration across Homebrew’s environment filtering, matching the existing approach used for other toolchains (e.g., Go).
Changes:
- Pass through
CARGO_HOME,CARGO_INSTALL_ROOT, andRUSTUP_HOMEby exporting them asHOMEBREW_*variables inbin/brew. - Apply those variables when running
cargo install --listandcargo installin the Bundle Cargo extension via a sharedcargo_envhelper.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bin/brew |
Allows Cargo/Rustup-related env vars to survive env -i filtering by copying them to HOMEBREW_*. |
Library/Homebrew/bundle/extensions/cargo.rb |
Wraps Cargo invocations with with_env(cargo_env(...)) so Cargo sees the intended install/home/toolchain dirs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sig { params(cargo: Pathname).returns(T::Hash[String, T.nilable(String)]) } | ||
| def cargo_env(cargo) | ||
| { | ||
| "CARGO_HOME" => ENV.fetch("HOMEBREW_CARGO_HOME", nil), | ||
| "CARGO_INSTALL_ROOT" => ENV.fetch("HOMEBREW_CARGO_INSTALL_ROOT", nil), | ||
| "PATH" => "#{cargo.dirname}:#{ENV.fetch("PATH")}", | ||
| "RUSTUP_HOME" => ENV.fetch("HOMEBREW_RUSTUP_HOME", nil), | ||
| } | ||
| end |
There was a problem hiding this comment.
The new env pass-through logic (CARGO_HOME/CARGO_INSTALL_ROOT/RUSTUP_HOME via cargo_env and with_env) isn’t covered by tests. Since this file already has specs, please add assertions that these variables are set/passed through when corresponding HOMEBREW_* vars are present (and that they don’t leak outside the block).
| sig { params(cargo: Pathname).returns(T::Hash[String, T.nilable(String)]) } | ||
| def cargo_env(cargo) | ||
| { | ||
| "CARGO_HOME" => ENV.fetch("HOMEBREW_CARGO_HOME", nil), | ||
| "CARGO_INSTALL_ROOT" => ENV.fetch("HOMEBREW_CARGO_INSTALL_ROOT", nil), | ||
| "PATH" => "#{cargo.dirname}:#{ENV.fetch("PATH")}", | ||
| "RUSTUP_HOME" => ENV.fetch("HOMEBREW_RUSTUP_HOME", nil), | ||
| } |
There was a problem hiding this comment.
| sig { params(cargo: Pathname).returns(T::Hash[String, T.nilable(String)]) } | |
| def cargo_env(cargo) | |
| { | |
| "CARGO_HOME" => ENV.fetch("HOMEBREW_CARGO_HOME", nil), | |
| "CARGO_INSTALL_ROOT" => ENV.fetch("HOMEBREW_CARGO_INSTALL_ROOT", nil), | |
| "PATH" => "#{cargo.dirname}:#{ENV.fetch("PATH")}", | |
| "RUSTUP_HOME" => ENV.fetch("HOMEBREW_RUSTUP_HOME", nil), | |
| } | |
| sig { params(cargo: Pathname).returns(T::Hash[String, String]) } | |
| def cargo_env(cargo) | |
| { | |
| "CARGO_HOME" => ENV.fetch("HOMEBREW_CARGO_HOME", nil), | |
| "CARGO_INSTALL_ROOT" => ENV.fetch("HOMEBREW_CARGO_INSTALL_ROOT", nil), | |
| "PATH" => "#{cargo.dirname}:#{ENV.fetch("PATH")}", | |
| "RUSTUP_HOME" => ENV.fetch("HOMEBREW_RUSTUP_HOME", nil), | |
| }.compact |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Agreed with comments, otherwise looks good!
brew lgtm(style, typechecking and tests) with your changes locally?Similar to #20866 for go, this passes through cargo env variables that affect
cargo install --listandcargo installCARGO_HOMEandCARGO_INSTALL_ROOTtell cargo where to put binariesRUSTUP_HOMEtells a rustup-provided cargo where to find its default toolchain (which will otherwise fail withrustup could not choose a version of cargo to run)