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

set CARGO instead of PATH #123227

Closed
onur-ozkan opened this issue Mar 30, 2024 · 3 comments · Fixed by #123386
Closed

set CARGO instead of PATH #123227

onur-ozkan opened this issue Mar 30, 2024 · 3 comments · Fixed by #123386
Assignees
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@onur-ozkan
Copy link
Member

onur-ozkan commented Mar 30, 2024

Previously, clippy was using cargo from PATH, but since rust-lang/rust-clippy#11944, it now prioritizes checking CARGO first. We can now set cargo path into CARGO and avoid appending it to the PATH chain.

In the bootstrap, we have this piece of code

// Set PATH to include the sysroot bin dir so clippy can find cargo.
// FIXME: once rust-clippy#11944 lands on beta, set `CARGO` directly instead.
let path = t!(env::join_paths(
// The sysroot comes first in PATH to avoid using rustup's cargo.
std::iter::once(PathBuf::from(initial_sysroot_bin))
.chain(env::split_paths(&t!(env::var("PATH"))))
));
which adds cargo into PATH. As indicated by the FIXME note, rust-lang/rust-clippy#11944 wasn't merged at the time. Now that we can do so, set CARGO instead of PATH and remove the FIXME note.

@onur-ozkan onur-ozkan added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 30, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 30, 2024
@onur-ozkan onur-ozkan removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 30, 2024
@long-long-float
Copy link
Contributor

@rustbot claim

@RalfJung
Copy link
Member

RalfJung commented Mar 31, 2024

There's more weird stuff going on here...

cmd.env(helpers::dylib_path_var(), env::join_paths(&dylib_path).unwrap());
cmd.env("PATH", path);

On Windows, helpers::dylib_path_var() is PATH, so this sets the PATH and then immediately overwrites it again with a different value...?!?

This almost looks like it may want to call add_dylib_path or add_rustc_lib_path. It's not quite equivalent though. Unfortunately there are no comments explaining why it does what it does... Cc @asquared31415 who wrote that code, but it's been two years so not sure if they remember.^^

@asquared31415
Copy link
Contributor

Yep, sorry, absolutely no clue what that code is meant to do. It's very likely that I copied that from somewhere else during the initial stages of "try everything and see if any of it works", or copied some part of that, and eventually modified to set PATH. I have also unfortunately forgotten all of the context of the bootstrap code here, how it's meant to flow, and what it's meant to set for other programs, so I will not be of any help here :c

I will try to check my messages with jyn, who i was talking to a lot about this at the time, and trying many things at their suggestion, to see if I can find any useful history.

Rajveer100 added a commit to Rajveer100/rust that referenced this issue Apr 2, 2024
Rajveer100 added a commit to Rajveer100/rust that referenced this issue Apr 3, 2024
Rajveer100 added a commit to Rajveer100/rust that referenced this issue Apr 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 3, 2024
…, r=onur-ozkan

Set `CARGO` instead of `PATH` for Rust Clippy

Resolves rust-lang#123227

Previously, clippy was using `cargo` from `PATH`, but since [PR](rust-lang/rust-clippy#11944), it now prioritises checking `CARGO` first.
@bors bors closed this as completed in 66dee4a Apr 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 2024
Rollup merge of rust-lang#123386 - Rajveer100:branch-for-issue-123227, r=onur-ozkan

Set `CARGO` instead of `PATH` for Rust Clippy

Resolves rust-lang#123227

Previously, clippy was using `cargo` from `PATH`, but since [PR](rust-lang/rust-clippy#11944), it now prioritises checking `CARGO` first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants