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

feat: implement RFC 3127 -Ztrim-paths #12625

Merged
merged 8 commits into from
Oct 31, 2023
Merged

feat: implement RFC 3127 -Ztrim-paths #12625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
decd020
doc(trim-paths): unstable doc
weihanglo Sep 1, 2023
28b169b
feat(trim-paths): add unstable feature gate
weihanglo Sep 1, 2023
4d29af1
feat(trim-paths): parsing in mainfest and config
weihanglo Sep 1, 2023
08c5e35
test(trim-paths): parsing in mainfest and config
weihanglo Sep 1, 2023
f4a1a03
feat(rustc): parse and store `commit-hash`
weihanglo Sep 1, 2023
fd893b2
feat(trim-paths): default value for release profile
weihanglo Sep 12, 2023
557fc4f
test(trim-paths): verify current behavior with `-Ztrim-paths`
weihanglo Oct 25, 2023
63cef2c
feat(trim-paths): rustc invocation integration
weihanglo Sep 6, 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
74 changes: 74 additions & 0 deletions src/cargo/core/compiler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ use crate::util::errors::{CargoResult, VerboseError};
use crate::util::interning::InternedString;
use crate::util::machine_message::{self, Message};
use crate::util::toml::TomlDebugInfo;
use crate::util::toml::TomlTrimPaths;
use crate::util::{add_path_args, internal, iter_join_onto, profile};
use cargo_util::{paths, ProcessBuilder, ProcessError};
use rustfix::diagnostics::Applicability;
Expand Down Expand Up @@ -950,6 +951,7 @@ fn build_base_args(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, unit: &Unit)
incremental,
strip,
rustflags: profile_rustflags,
trim_paths,
..
} = unit.profile.clone();
let test = unit.mode.is_any_test();
Expand Down Expand Up @@ -1028,6 +1030,10 @@ fn build_base_args(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, unit: &Unit)
}
}

if let Some(trim_paths) = trim_paths {
trim_paths_args(cmd, cx, unit, &trim_paths)?;
}
Comment on lines +1033 to +1035
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be included in rustdoc to handle the diagnostics stripping (and same with doctests)?

(Though I still don't 100% understand the purpose of diagnostic remapping.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is such an option for rustdoc. Did I miss something?
Maybe this could be put in "unresolved questions" in RFC 3127 tracking issue.

$ rustdoc +nightly -vV
rustdoc 1.75.0-nightly (608e9682f 2023-10-29)
binary: rustdoc
commit-hash: 608e9682f0a6482903de8d3332770104a0ad943c
commit-date: 2023-10-29
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.3

$ rustc --remap-path-prefix
error: Argument to option 'remap-path-prefix' missing

$ rustdoc --remap-path-prefix
error: Unrecognized option: 'remap-path-prefix'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, leaving that as an open issue sounds good. The rustdoc argument parser would just need to be updated.

Though, before that I would like to better understand why the diagnostic option exists and when someone would want to turn it on. I can't really follow the discussion from rust-lang/rust#87745 and how that relates to cargo. If we end up not stabilizing "diagnostic" as an option, then I don't have any motivation to do that.

Copy link
Member

@Urgau Urgau Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the diagnostics option I think it's related to build-systems that create VFS (Virtual File System) or temporary location when building, throwing the all diagnostics path to the wrong location.


cmd.args(unit.pkg.manifest().lint_rustflags());
cmd.args(&profile_rustflags);
if let Some(args) = cx.bcx.extra_args_for(unit) {
Expand Down Expand Up @@ -1162,6 +1168,74 @@ fn features_args(unit: &Unit) -> Vec<OsString> {
args
}

/// Generates the `--remap-path-scope` and `--remap-path-prefix` for [RFC 3127].
/// See also unstable feature [`-Ztrim-paths`].
///
/// [RFC 3127]: https://rust-lang.github.io/rfcs/3127-trim-paths.html
/// [`-Ztrim-paths`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#profile-trim-paths-option
fn trim_paths_args(
cmd: &mut ProcessBuilder,
cx: &Context<'_, '_>,
unit: &Unit,
trim_paths: &TomlTrimPaths,
) -> CargoResult<()> {
if trim_paths.is_none() {
return Ok(());
}

// feature gate was checked during mainfest/config parsing.
cmd.arg("-Zunstable-options");
cmd.arg(format!("-Zremap-path-scope={trim_paths}"));

let sysroot_remap = {
let sysroot = &cx.bcx.target_data.info(unit.kind).sysroot;
let mut remap = OsString::from("--remap-path-prefix=");
remap.push(sysroot);
Copy link
Member Author

Choose a reason for hiding this comment

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

What's the correct remap prefix for sysroot, and how to setup a test case for it?

I've checked rustup, Debian, and Fedora. All of them put the rust-src under [sysroot]/lib/rustlib/src/rust. From what I can tell, -Zbuild-std is also hardcoded with this path.

@Urgau do you have any insight?

Copy link
Member

Choose a reason for hiding this comment

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

What's the correct remap prefix for sysroot

rustc --print=sysroot

and how to setup a test case for it?

Well you could have a Rust fn main() { panic!("something"); } with RUST_BACKTRACE=1 and see the resulting backtrace and assert that it doesn't contain the un-remapped sysroot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip!

Unfortunately I've tried panic! backtrace and it has already remapped even without -Ztrim-paths 😞.

   0: std::panicking::begin_panic
             at /rustc/cd674d61790607dfb6faa9d754bd3adfa13aea7c/library/std/src/panicking.rs:638:12

Copy link
Member

Choose a reason for hiding this comment

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

Hum, the mapping is done by bootstrap here, I wonder if we should remove/disable it since Cargo will now do it.

Or maybe instead of disabling it you could do the inverse and demap in debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

The desired behavior is mentioned in the RFC

To follow the RFC, yes, we might want to disable it in bootstrap.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me and Urgau had a chat on this. Urgau pointed out changing the bootstrap this may require -Ztrim-paths being stabilized, and may leak CI details into all pre-built std, core…. This is something requiring to collaborate with T-bootstrap. We'll postpone it for this pull request.

remap.push("/lib/rustlib/src/rust"); // See also `detect_sysroot_src_path()`.
remap.push("=");
remap.push("/rustc/");
// This remap logic aligns with rustc:
// <https://github.com/rust-lang/rust/blob/c2ef3516/src/bootstrap/src/lib.rs#L1113-L1116>
if let Some(commit_hash) = cx.bcx.rustc().commit_hash.as_ref() {
remap.push(commit_hash);
} else {
remap.push(cx.bcx.rustc().version.to_string());
}
remap
};
cmd.arg(sysroot_remap);

let package_remap = {
let pkg_root = unit.pkg.root();
let ws_root = cx.bcx.ws.root();
let is_local = unit.pkg.package_id().source_id().is_path();
let mut remap = OsString::from("--remap-path-prefix=");
// Remapped to path relative to workspace root:
//
// * path dependencies under workspace root directory
Copy link
Member Author

@weihanglo weihanglo Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The RFC only mentions about mapping the current package, which doesn't take workspaces into account:

For the the current package (where the current working directory is in), from the the absolute path of the package root to empty string. For other packages, from the absolute path of the package root to [package name]-[package version].

Here we expand the RFC a bit: always remap from the workspace root to empty string. When dealing with compilations, Cargo always handle the workspace as a whole instead of a single member. We might not want an exception for -Ztrim-paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, the main question of whether to be relative to workspace or package root is if there'd be ambiguity. Since we have name+version, I'm guessing not.

//
// Remapped to `<pkg>-<version>`
//
// * registry dependencies
// * git dependencies
// * path dependencies outside workspace root directory
if is_local && pkg_root.strip_prefix(ws_root).is_ok() {
remap.push(ws_root);
remap.push("="); // empty to remap to relative paths.
} else {
remap.push(pkg_root);
remap.push("=");
remap.push(unit.pkg.name());
remap.push("-");
remap.push(unit.pkg.version().to_string());
}
remap
};
cmd.arg(package_remap);

Ok(())
}

/// Generates the `--check-cfg` arguments for the `unit`.
/// See unstable feature [`check-cfg`].
///
Expand Down
5 changes: 5 additions & 0 deletions src/cargo/core/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,9 @@ features! {

// Support for 2024 edition.
(unstable, edition2024, "", "reference/unstable.html#edition-2024"),

// Allow setting trim-paths in a profile to control the sanitisation of file paths in build outputs.
(unstable, trim_paths, "", "reference/unstable.html#profile-trim-paths-option"),
}

pub struct Feature {
Expand Down Expand Up @@ -755,6 +758,7 @@ unstable_cli_options!(
separate_nightlies: bool = (HIDDEN),
skip_rustdoc_fingerprint: bool = (HIDDEN),
target_applies_to_host: bool = ("Enable the `target-applies-to-host` key in the .cargo/config.toml file"),
trim_paths: bool = ("Enable the `trim-paths` option in profiles"),
unstable_options: bool = ("Allow the usage of unstable options"),
);

Expand Down Expand Up @@ -1089,6 +1093,7 @@ impl CliUnstable {
"no-index-update" => self.no_index_update = parse_empty(k, v)?,
"panic-abort-tests" => self.panic_abort_tests = parse_empty(k, v)?,
"profile-rustflags" => self.profile_rustflags = parse_empty(k, v)?,
"trim-paths" => self.trim_paths = parse_empty(k, v)?,
"publish-timeout" => self.publish_timeout = parse_empty(k, v)?,
"rustdoc-map" => self.rustdoc_map = parse_empty(k, v)?,
"rustdoc-scrape-examples" => self.rustdoc_scrape_examples = parse_empty(k, v)?,
Expand Down
28 changes: 24 additions & 4 deletions src/cargo/core/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@
use crate::core::compiler::{CompileKind, CompileTarget, Unit};
use crate::core::dependency::Artifact;
use crate::core::resolver::features::FeaturesFor;
use crate::core::Feature;
use crate::core::{PackageId, PackageIdSpec, Resolve, Shell, Target, Workspace};
use crate::util::interning::InternedString;
use crate::util::toml::TomlTrimPaths;
use crate::util::toml::TomlTrimPathsValue;
use crate::util::toml::{
ProfilePackageSpec, StringOrBool, TomlDebugInfo, TomlProfile, TomlProfiles,
};
Expand Down Expand Up @@ -80,7 +83,9 @@ impl Profiles {
rustc_host,
};

Self::add_root_profiles(&mut profile_makers, &profiles);
let trim_paths_enabled = ws.unstable_features().is_enabled(Feature::trim_paths())
|| config.cli_unstable().trim_paths;
Self::add_root_profiles(&mut profile_makers, &profiles, trim_paths_enabled);

// Merge with predefined profiles.
use std::collections::btree_map::Entry;
Expand Down Expand Up @@ -123,6 +128,7 @@ impl Profiles {
fn add_root_profiles(
profile_makers: &mut Profiles,
profiles: &BTreeMap<InternedString, TomlProfile>,
trim_paths_enabled: bool,
) {
profile_makers.by_name.insert(
InternedString::new("dev"),
Expand All @@ -131,7 +137,10 @@ impl Profiles {

profile_makers.by_name.insert(
InternedString::new("release"),
ProfileMaker::new(Profile::default_release(), profiles.get("release").cloned()),
ProfileMaker::new(
Profile::default_release(trim_paths_enabled),
profiles.get("release").cloned(),
),
);
}

Expand Down Expand Up @@ -556,6 +565,9 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
if let Some(flags) = &toml.rustflags {
profile.rustflags = flags.iter().map(InternedString::from).collect();
}
if let Some(trim_paths) = &toml.trim_paths {
profile.trim_paths = Some(trim_paths.clone());
}
profile.strip = match toml.strip {
Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")),
None | Some(StringOrBool::Bool(false)) => Strip::None,
Expand Down Expand Up @@ -599,6 +611,9 @@ pub struct Profile {
#[serde(skip_serializing_if = "Vec::is_empty")] // remove when `rustflags` is stablized
// Note that `rustflags` is used for the cargo-feature `profile_rustflags`
pub rustflags: Vec<InternedString>,
// remove when `-Ztrim-paths` is stablized
#[serde(skip_serializing_if = "Option::is_none")]
pub trim_paths: Option<TomlTrimPaths>,
}

impl Default for Profile {
Expand All @@ -619,6 +634,7 @@ impl Default for Profile {
panic: PanicStrategy::Unwind,
strip: Strip::None,
rustflags: vec![],
trim_paths: None,
}
}
}
Expand All @@ -628,7 +644,7 @@ compact_debug! {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let (default, default_name) = match self.name.as_str() {
"dev" => (Profile::default_dev(), "default_dev()"),
"release" => (Profile::default_release(), "default_release()"),
"release" => (Profile::default_release(false), "default_release()"),
_ => (Profile::default(), "default()"),
};
[debug_the_fields(
Expand All @@ -647,6 +663,7 @@ compact_debug! {
panic
strip
rustflags
trim_paths
)]
}
}
Expand Down Expand Up @@ -688,11 +705,13 @@ impl Profile {
}

/// Returns a built-in `release` profile.
fn default_release() -> Profile {
fn default_release(trim_paths_enabled: bool) -> Profile {
let trim_paths = trim_paths_enabled.then(|| TomlTrimPathsValue::Object.into());
Profile {
name: InternedString::new("release"),
root: ProfileRoot::Release,
opt_level: InternedString::new("3"),
trim_paths,
..Profile::default()
}
}
Expand All @@ -713,6 +732,7 @@ impl Profile {
self.rpath,
(self.incremental, self.panic, self.strip),
&self.rustflags,
&self.trim_paths,
)
}
}
Expand Down
14 changes: 14 additions & 0 deletions src/cargo/util/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ pub struct Rustc {
pub version: semver::Version,
/// The host triple (arch-platform-OS), this comes from verbose_version.
pub host: InternedString,
/// The rustc full commit hash, this comes from `verbose_version`.
pub commit_hash: Option<String>,
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
cache: Mutex<Cache>,
}

Expand Down Expand Up @@ -80,6 +82,17 @@ impl Rustc {
verbose_version
)
})?;
let commit_hash = extract("commit-hash: ").ok().map(|hash| {
debug_assert!(
hash.chars().all(|ch| ch.is_ascii_hexdigit()),
"commit hash must be a hex string"
);
debug_assert!(
hash.len() == 40 || hash.len() == 64,
"hex string must be generated from sha1 or sha256"
);
hash.to_string()
});

Ok(Rustc {
path,
Expand All @@ -88,6 +101,7 @@ impl Rustc {
verbose_version,
version,
host,
commit_hash,
cache: Mutex::new(cache),
})
}
Expand Down
Loading