Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo install ignores the rust-toolchain.toml or uses the wrong one #11036

Open
sabify opened this issue Aug 31, 2022 · 5 comments
Open

cargo install ignores the rust-toolchain.toml or uses the wrong one #11036

sabify opened this issue Aug 31, 2022 · 5 comments
Labels
A-rustup Area: rustup interaction C-bug Category: bug Command-install S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@sabify
Copy link

sabify commented Aug 31, 2022

Problem

Hello,

Cargo simply ignores rust-toolchain.toml overrides when used by --git option to install a crate and also it can wrongly use the current directory rust-toolchain.toml (even not related to the desired installing git repo) with or without --git option.

Steps

The first issue: install any crate from a git repo with rust-toolchain.toml in it, cargo ignores the overrides. For example:

cargo install --git=https://github.com/helix-editor/helix helix-term

The second issue: if you are in a rust repo directory that has rust-toolchain.toml, trying to install another crate with or without --git option, uses the current directory rust-toolchain.toml overrides:

git clone https://github.com/nushell/nushell && cd nushell
cargo install --git=https://github.com/helix-editor/helix helix-term # still uses the nushell toolchain overrides
# or
cargo install du-dust # still uses the nushell toolchain overrides

Possible Solution(s)

No response

Notes

This also applies to rustup override which is stored in a user config file.

Version

cargo 1.63.0 (fd9c4297c 2022-07-01)
release: 1.63.0
commit-hash: fd9c4297ccbee36d39e9a79067edab0b614edb5a
commit-date: 2022-07-01
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Ubuntu 20.04 (focal) [64-bit]
@sabify sabify added the C-bug Category: bug label Aug 31, 2022
@epage
Copy link
Contributor

epage commented Aug 31, 2022

I believe rust-toolchain.toml is handled by rustup, the toolchain manager, and is processed before even calling the real cargo (the cargo you run is a rustup shim to lookup and select the right cargo to run).

Another angle on this is that rust-toolchain.toml (similar to .cargo/config.toml is environment configuration and not package configuration, so the version next to a package is ignored while the version in your CWD is respected, regardless of --git, --manifest-path, or any other mechanism (short of -C support for selecting a package.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. A-rustup Area: rustup interaction labels May 24, 2023
@weihanglo
Copy link
Member

See also #7312

webern added a commit to webern/bottlerocket that referenced this issue Sep 18, 2023
Unfortunately the cargo install command does not seem to respect the
rust-toolchain.toml and .cargo/config.toml settings when installing from
a git reference.

rust-lang/cargo#11036

We need to fix this by adding +nightly and specifying -Z bindeps in the
cargo install command for twoliter.
stmcginnis pushed a commit to stmcginnis/bottlerocket that referenced this issue Oct 4, 2023
Unfortunately the cargo install command does not seem to respect the
rust-toolchain.toml and .cargo/config.toml settings when installing from
a git reference.

rust-lang/cargo#11036

We need to fix this by adding +nightly and specifying -Z bindeps in the
cargo install command for twoliter.

(cherry picked from commit 8323260)
@epage
Copy link
Contributor

epage commented Jan 16, 2024

btw I was wrong about .cargo/config.toml, we do reload it based on where we are installing from

See

if let Some(path) = &path {
config.reload_rooted_at(path)?;
} else {
// TODO: Consider calling set_search_stop_path(home).
config.reload_rooted_at(config.home().clone().into_path_unlocked())?;
}

This was then mirrored in "cargo script". "cargo script" would likely benefit from a solution to this as well but .we need someone to take the time to design this.

@epage
Copy link
Contributor

epage commented Jan 16, 2024

Some options

  • If we are under rustup and a recursive call flag isn't set then invoke rustup to re-run the command, telling it to use a different directory for rustup toolchain overrides and setting the recursive flag call
    • Requires coordination from cargo and rustup which is made worse by the fact that cargo would need to detect if a new enough rustup is installed
    • Bit of a performance hit which is likely not bad for cargo install, especially if we limit it to only when its needed. My big concern is extending this to "cargo script"
  • Make rustup toolchain lookup a library and have cargo do the lookup and re-run if needed
    • Are there risks with there being logic skew between the current rustup's logic and the current cargo's logic (they ship independently)
  • If a path is used, re-run cargo install from that path (ie convert --path foo to --path . -C foo).
    • I worry about any other options that are path relative
    • Can't apply to "cargo install" since the user args will be path relative.
      • What if we only invoke the other cargo for the compilation and then run the binary using the current cargo? Then it is likely only the same problem as cargo install.

@epage epage added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jan 16, 2024
@ehuss ehuss removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jan 16, 2024
@epage
Copy link
Contributor

epage commented Jan 16, 2024

We talked about this in today's cargo team meeting.

There is some concern about breaking the abstraction layer. We do so now but its mostly for errors and performance.

The .cargo/config.toml logic is limited to --path (and falls back to $CARGO_HOME for all else). If we extended this to any cargo install package, there would be concern about using rust-toolchain.toml files from arbitrary sources that might do strange things (path locations, pulling in version with security vulnerabilities, etc).

This would only be limited for when being run under rustup, so there wouldn't be concern about Debian (or related) policies about puling in outside versions.

If we went the route of cargo doing a toolchain lookup, there is concern about the toolchain specifying older versions that may have bugs or different behavior, especially if it can lead to a infinite recursion.

If we wanted to explore going down this route, a likely first step would be to provide a warning that the toolchain/override is being ignored with a suggestion on how to override it (+ syntax). The main thing we'd need to decide is how much override lookup logic we need (rust-toolchain.toml only, settings.toml, env variables, etc). Depending on that and in conjunction with the rustup team, we'd then need to decide whether to re-implement or pull the rustup logic out as a library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustup Area: rustup interaction C-bug Category: bug Command-install S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
Archived in project
Development

No branches or pull requests

4 participants