-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Stabilize -Zremap-path-scope
#147611
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?
Stabilize -Zremap-path-scope
#147611
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1384,6 +1384,28 @@ bitflags::bitflags! { | |
} | ||
} | ||
|
||
pub(crate) fn parse_remap_path_scope( | ||
early_dcx: &EarlyDiagCtxt, | ||
matches: &getopts::Matches, | ||
) -> RemapPathScopeComponents { | ||
if let Some(v) = matches.opt_str("remap-path-scope") { | ||
let mut slot = RemapPathScopeComponents::empty(); | ||
for s in v.split(',') { | ||
slot |= match s { | ||
"macro" => RemapPathScopeComponents::MACRO, | ||
"diagnostics" => RemapPathScopeComponents::DIAGNOSTICS, | ||
"debuginfo" => RemapPathScopeComponents::DEBUGINFO, | ||
"object" => RemapPathScopeComponents::OBJECT, | ||
"all" => RemapPathScopeComponents::all(), | ||
_ => early_dcx.early_fatal("argument for `--remap-path-scope` must be a comma separated list of scopes: `macro`, `diagnostics`, `debuginfo`, `object`, `all`"), | ||
Comment on lines
+1395
to
+1400
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: hm, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so, otherwise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have at least two options here:
Either way, we need to be clear that the it is going to remap as many thing as possible, not just equivalent to some subset of options. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the documentation is already clear about what
It's true that |
||
} | ||
} | ||
slot | ||
} else { | ||
RemapPathScopeComponents::all() | ||
} | ||
} | ||
|
||
#[derive(Clone, Debug)] | ||
pub struct Sysroot { | ||
pub explicit: Option<PathBuf>, | ||
|
@@ -1420,18 +1442,18 @@ pub fn host_tuple() -> &'static str { | |
|
||
fn file_path_mapping( | ||
remap_path_prefix: Vec<(PathBuf, PathBuf)>, | ||
unstable_opts: &UnstableOptions, | ||
remap_path_scope: &RemapPathScopeComponents, | ||
) -> FilePathMapping { | ||
FilePathMapping::new( | ||
remap_path_prefix.clone(), | ||
if unstable_opts.remap_path_scope.contains(RemapPathScopeComponents::DIAGNOSTICS) | ||
if remap_path_scope.contains(RemapPathScopeComponents::DIAGNOSTICS) | ||
&& !remap_path_prefix.is_empty() | ||
{ | ||
FileNameDisplayPreference::Remapped | ||
} else { | ||
FileNameDisplayPreference::Local | ||
}, | ||
if unstable_opts.remap_path_scope.is_all() { | ||
if remap_path_scope.is_all() { | ||
FileNameEmbeddablePreference::RemappedOnly | ||
} else { | ||
FileNameEmbeddablePreference::LocalAndRemapped | ||
|
@@ -1473,6 +1495,7 @@ impl Default for Options { | |
cli_forced_codegen_units: None, | ||
cli_forced_local_thinlto_off: false, | ||
remap_path_prefix: Vec::new(), | ||
remap_path_scope: RemapPathScopeComponents::all(), | ||
real_rust_source_base_dir: None, | ||
real_rustc_dev_source_base_dir: None, | ||
edition: DEFAULT_EDITION, | ||
|
@@ -1499,7 +1522,7 @@ impl Options { | |
} | ||
|
||
pub fn file_path_mapping(&self) -> FilePathMapping { | ||
file_path_mapping(self.remap_path_prefix.clone(), &self.unstable_opts) | ||
file_path_mapping(self.remap_path_prefix.clone(), &self.remap_path_scope) | ||
} | ||
|
||
/// Returns `true` if there will be an output file generated. | ||
|
@@ -1940,6 +1963,14 @@ pub fn rustc_optgroups() -> Vec<RustcOptGroup> { | |
"Remap source names in all output (compiler messages and output files)", | ||
"<FROM>=<TO>", | ||
), | ||
opt( | ||
Stable, | ||
Opt, | ||
"", | ||
"remap-path-scope", | ||
"Defines which scopes of paths should be remapped by `--remap-path-prefix`", | ||
"<macro,diagnostics,debuginfo,object,all>", | ||
), | ||
opt(Unstable, Multi, "", "env-set", "Inject an environment variable", "<VAR>=<VALUE>"), | ||
]; | ||
options.extend(verbose_only.into_iter().map(|mut opt| { | ||
|
@@ -2838,6 +2869,7 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M | |
let externs = parse_externs(early_dcx, matches, &unstable_opts); | ||
|
||
let remap_path_prefix = parse_remap_path_prefix(early_dcx, matches, &unstable_opts); | ||
let remap_path_scope = parse_remap_path_scope(early_dcx, matches); | ||
|
||
let pretty = parse_pretty(early_dcx, &unstable_opts); | ||
|
||
|
@@ -2901,7 +2933,7 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M | |
early_dcx.early_fatal(format!("Current directory is invalid: {e}")); | ||
}); | ||
|
||
let file_mapping = file_path_mapping(remap_path_prefix.clone(), &unstable_opts); | ||
let file_mapping = file_path_mapping(remap_path_prefix.clone(), &remap_path_scope); | ||
let working_dir = file_mapping.to_real_filename(&working_dir); | ||
|
||
let verbose = matches.opt_present("verbose") || unstable_opts.verbose_internals; | ||
|
@@ -2938,6 +2970,7 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M | |
cli_forced_codegen_units: codegen_units, | ||
cli_forced_local_thinlto_off: disable_local_thinlto, | ||
remap_path_prefix, | ||
remap_path_scope, | ||
real_rust_source_base_dir, | ||
real_rustc_dev_source_base_dir, | ||
edition, | ||
|
This file was deleted.
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.
Thanks for driving the stabilization!
"There are no outstanding bugs" probably?
Don't want to block the stabilization. However, this sounds a bit odd that there is no major use cases beyond Cargo and rustc is heading toward stabilization alone. That may imply t-cargo should review again whether
--remap-path-scope
is useful and cover what they want at least (of course this is on me as I worked on Cargo side of this)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 far as I know
--remap-path-scope
has no bugs. We have test for all the scopes, with/without dependency and with mixed scopes between crates.It doesn't really surprises me that there are no public use of it. It's quite a pain currently to setup
--remap-path-prefix
and-Zremap-path-scope
in Cargo, as there is currently no support for it, which is what we are trying to solve. I suspect most users are usingRUSTFLAGS
which I don't think is typically committed.Well, be able to see the not-remapped paths in diagnostics is quite nice thing.
The RFC specifically states that the default should be
object
for therelease
profile.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 meant the "no" is missing in the original PR description.
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.
The other thing is that do we want to stabilize all options all at once. I remember that the RFC said we could optionally stabilize some of them. I am not sure whether
macros
anddebuginfo
standalone are useful, as well asdiagnostics
. The most useful thing as you mentioned is probablyobject
and the original behaviorall
.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 don't know that
macros
ordebuginfo
would need to be useful on their own to be worth stabilising, we're able to support them relatively straightforwardly and they may be useful in combination with other options.