-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Add typo suggestion for a misspelt Cargo environment variable #148559
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
base: master
Are you sure you want to change the base?
Conversation
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
48c2fbb to
1f8b6da
Compare
| const KNOWN_CARGO_VARS: &[&str] = &[ | ||
| // List of known Cargo environment variables that are set for crates (not build scripts, OUT_DIR etc). | ||
| // See: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion (non-blocking): I have two reservations about making this suggestion:
- Wouldn't this depend on the specific cargo version?
- Basically, what I was trying to get at, is that in principle I feel like
rustcshould not make assumptions aboutcargo(that is,rustcshould not "know" aboutcargo's existence), becausecargoisn't the only way to invokerustc.
I guess I could be convinced by "well, most users do build with cargo", so don't consider this a hard-blocking concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm not sure, there is a similar list for this in
rust-analyzer: https://github.com/chenyukang/rust/blob/702bf000a0b5062d8b5ced717a26d70919df6ba0/src/tools/rust-analyzer/crates/project-model/src/env.rs#L9-L49 , I guess there is a better and general way to keep it synced. - I agree
rustcshould not knowing details about cargo, I think it's ok since we need to check the prefix withCARGO_of env:
https://github.com/rust-lang/rust/pull/148559/files#diff-0eaa1095d59f2f3d9c015005d15fd8f7cda13121a8404d77b2955fc5fcd40449R213, and it's a note but not code suggestions, it's maybe incorrect for rare scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems alright, was just curious to hear what you think.
1f8b6da to
ad471f4
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits
ad471f4 to
760e803
Compare
This comment has been minimized.
This comment has been minimized.
760e803 to
553d580
Compare
This comment has been minimized.
This comment has been minimized.
553d580 to
3edd25f
Compare
|
@bors r+ |
Fixes #148439