-
Notifications
You must be signed in to change notification settings - Fork 33
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
Significantly simplify sysroot configuration #342
Conversation
let our_exe_filename = std::env::current_exe() | ||
.ok() | ||
.and_then(|p| p.file_stem().map(ToOwned::to_owned)) | ||
.unwrap_or_else(|| "plrustc".into()); | ||
|
||
let wrapper_mode = orig_args | ||
.get(1) | ||
.map(std::path::Path::new) | ||
.and_then(std::path::Path::file_stem) | ||
.map_or(false, |name| { | ||
name == our_exe_filename || name == "plrustc" || name == "rustc" | ||
}); | ||
|
||
if wrapper_mode { | ||
args.remove(1); | ||
} |
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.
note that removing this means that setting RUSTC_WORKSPACE_WRAPPER=plrustc
won't work anymore, but thom tells me you were using RUSTC
instead anyway.
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.
Yes, I don't think we realistically supported that anyway.
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.
It's beautiful.
@@ -52,6 +51,8 @@ impl Callbacks for PlrustcCallbacks { | |||
} | |||
} | |||
|
|||
// TODO: eventually we can replace this with: | |||
// rustc_driver::install_ice_hook("https://github.com/tcdi/plrust/issues/new", |_| ()); |
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.
When? 👀
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.
as soon as you update the nightly toolchain :) rust-lang/rust#110989
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.
We actually don't use nightly >_>
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.
ah. when 1.71 lands on stable in a few weeks, then.
let args = rustc_driver::args::arg_expand_all(&std::env::args().collect::<Vec<_>>()); | ||
let config = PlrustcConfig::from_env_and_args(&args); |
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.
So much less code!
We no longer have to manually search for the sysroot, as of rust-lang/rust#103660 it should be auto-detected from first the arguments passed, followed by locating it based on the location of the rustc_driver dynamic library.
This means we don't support configuring it via the environment anymore, but I don't believe anybody was explicitly using this anyway. Regardless, if they were configuring it to a different value than we locate with this function, it was almost certainly wrong. Still, we should make a note in the next release notes about it.
Thanks to @jyn514 for pointing this out to me!