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.

Sign up for GitHub

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

Optimize usage under rustup. #11917

Merged
merged 6 commits into from
May 5, 2023
Merged

Optimize usage under rustup. #11917

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52317aa
Add some tests for simulating behavior under rustup.
ehuss Mar 10, 2023
d4b0b49
Add an optimization when running under rustup.
ehuss Mar 11, 2023
cdf60e3
Add an assert to confirm that this function is only used with proxies.
ehuss Apr 8, 2023
42ef94f
Check for toolchains names with slashes and use the slow path in that…
ehuss Apr 8, 2023
4f79ac3
Change `get_tool` to use an enum to constrain which values it accepts.
ehuss May 3, 2023
b9993bd
Switch to then_some.
ehuss May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Check for toolchains names with slashes and use the slow path in that… 
… case.
  • Loading branch information
@ehuss
ehuss committed May 3, 2023
commit 42ef94f38405cf1c4301bdac1d9291eda691c82f
5 changes: 5 additions & 0 deletions src/cargo/util/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,6 +1664,11 @@ impl Config {
//
// First, we must be running under rustup in the first place.
let toolchain = self.get_env_os("RUSTUP_TOOLCHAIN")?;
// This currently does not support toolchain paths.
// This also enforces UTF-8.
if toolchain.to_str()?.contains(&['/', '\\']) {
return None;
}
// If the tool on PATH is the same as `rustup` on path, then
// there is pretty good evidence that it will be a proxy.
Copy link
Contributor

@rbtcollins rbtcollins Apr 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an exact list of the proxies we offer btw. I think it is a good idea to only take the fastpath for things we are known to proxy. I'm happy to commit to keeping a copy of that list in Cargo up to date. New proxies are very rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo is currently hard-coded to only use this for rustc and rustdoc. I added an assert to validate that requirement, and I think we can extend it in the future if needed. I don't think we quite yet need to have an exhaustive list for all the proxies (just to keep things simple for now).

let tool_resolved = paths::resolve_executable(Path::new(tool)).ok()?;
Expand Down