Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
117 changes: 62 additions & 55 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1874,68 +1874,75 @@ pub fn extern_args(
let no_embed_metadata = build_runner.bcx.gctx.cli_unstable().no_embed_metadata;

// Closure to add one dependency to `result`.
let mut link_to =
|dep: &UnitDep, extern_crate_name: InternedString, noprelude: bool| -> CargoResult<()> {
let mut value = OsString::new();
let mut opts = Vec::new();
let is_public_dependency_enabled = unit
.pkg
.manifest()
.unstable_features()
.require(Feature::public_dependency())
.is_ok()
|| build_runner.bcx.gctx.cli_unstable().public_dependency;
if !dep.public && unit.target.is_lib() && is_public_dependency_enabled {
opts.push("priv");
*unstable_opts = true;
}
if noprelude {
opts.push("noprelude");
*unstable_opts = true;
}
if !opts.is_empty() {
value.push(opts.join(","));
value.push(":");
}
value.push(extern_crate_name.as_str());
value.push("=");

let mut pass = |file| {
let mut value = value.clone();
value.push(file);
result.push(OsString::from("--extern"));
result.push(value);
};
let mut link_to = |dep: &UnitDep,
extern_crate_name: InternedString,
noprelude: bool,
nounused: bool|
-> CargoResult<()> {
let mut value = OsString::new();
let mut opts = Vec::new();
let is_public_dependency_enabled = unit
.pkg
.manifest()
.unstable_features()
.require(Feature::public_dependency())
.is_ok()
|| build_runner.bcx.gctx.cli_unstable().public_dependency;
if !dep.public && unit.target.is_lib() && is_public_dependency_enabled {
opts.push("priv");
*unstable_opts = true;
}
if noprelude {
opts.push("noprelude");
*unstable_opts = true;
}
if nounused {
opts.push("nounused");
*unstable_opts = true;
}
Comment on lines +1899 to +1902
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the path to stabilizing this look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also wonder if we should start tracking all of the different unstable features of rustc build-std is relying on to not get surprised when we stabilize it.

CC @davidtwco

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does the path to stabilizing this look like?

It's tied to stabilising -Zbuilt-std itself. This should become default behaviour with no flag needed, since it corrects a warning given by cargo's implicitily injected dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we'll eventually remove the use of nounused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. i feel that when -Zbuild-std stabilises, cargo shouldnt inject shouldnt be injecting crates that trigger linting errors that the user didnt ask for. I think nounused should be a temporary workaround till that isnt the case, unless there's a flaw in this thinking that im not seeing?

Copy link
Contributor

Choose a reason for hiding this comment

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

How would that work? Especially the build-std config variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if RFC 3875 could have a way of doing this? It talks of users explicitly declaring what std lib crates to inject. That, in addition to #16675, which aims to add builtin and opaque deps. Wouldn't builtin by default replace nounused?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where explicit dependencies are used. Build-std will be stabilized without that with rust-lang/rfcs#3874

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay so nounused would need to be stabilised too. im sorry i missed this. The path to stabilisation would be through Rust #98400, but that is itself blocked by Rust #98405 harmonise --extern syntax with command line syntax used for native library modifiers.
Once Rust #98400 unblocks, Cargo could remove unstable_opts = true for nounused?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that! I've noted this in the PR to draw attention to it.

if !opts.is_empty() {
value.push(opts.join(","));
value.push(":");
}
value.push(extern_crate_name.as_str());
value.push("=");

let mut pass = |file| {
let mut value = value.clone();
value.push(file);
result.push(OsString::from("--extern"));
result.push(value);
};

let outputs = build_runner.outputs(&dep.unit)?;
let outputs = build_runner.outputs(&dep.unit)?;

if build_runner.only_requires_rmeta(unit, &dep.unit) || dep.unit.mode.is_check() {
// Example: rlib dependency for an rlib, rmeta is all that is required.
let output = outputs
.iter()
.find(|output| output.flavor == FileFlavor::Rmeta)
.expect("failed to find rmeta dep for pipelined dep");
pass(&output.path);
} else {
// Example: a bin needs `rlib` for dependencies, it cannot use rmeta.
for output in outputs.iter() {
if output.flavor == FileFlavor::Linkable {
pass(&output.path);
}
// If we use -Zembed-metadata=no, we also need to pass the path to the
// corresponding .rmeta file to the linkable artifact, because the
// normal dependency (rlib) doesn't contain the full metadata.
else if no_embed_metadata && output.flavor == FileFlavor::Rmeta {
pass(&output.path);
}
if build_runner.only_requires_rmeta(unit, &dep.unit) || dep.unit.mode.is_check() {
// Example: rlib dependency for an rlib, rmeta is all that is required.
let output = outputs
.iter()
.find(|output| output.flavor == FileFlavor::Rmeta)
.expect("failed to find rmeta dep for pipelined dep");
pass(&output.path);
} else {
// Example: a bin needs `rlib` for dependencies, it cannot use rmeta.
for output in outputs.iter() {
if output.flavor == FileFlavor::Linkable {
pass(&output.path);
}
// If we use -Zembed-metadata=no, we also need to pass the path to the
// corresponding .rmeta file to the linkable artifact, because the
// normal dependency (rlib) doesn't contain the full metadata.
else if no_embed_metadata && output.flavor == FileFlavor::Rmeta {
pass(&output.path);
}
}
Ok(())
};
}
Ok(())
};

for dep in deps {
if dep.unit.target.is_linkable() && !dep.unit.mode.is_doc() {
link_to(dep, dep.extern_crate_name, dep.noprelude)?;
link_to(dep, dep.extern_crate_name, dep.noprelude, dep.nounused)?;
}
}
if unit.target.proc_macro() {
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ fn attach_std_deps(
// TODO: Does this `public` make sense?
public: true,
noprelude: true,
nounused: true,
}));
found = true;
}
Expand Down Expand Up @@ -911,6 +912,7 @@ fn new_unit_dep_with_profile(
dep_name,
public,
noprelude: false,
nounused: false,
})
}

Expand Down
16 changes: 13 additions & 3 deletions src/cargo/core/compiler/unit_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ pub struct UnitDep {
pub public: bool,
/// If `true`, the dependency should not be added to Rust's prelude.
pub noprelude: bool,
/// If `true`, the dependency will not trigger Rust lint.
pub nounused: bool,
}

const VERSION: u32 = 1;
Expand Down Expand Up @@ -74,6 +76,9 @@ struct SerializedUnitDep {
// This is only set on nightly since it is unstable.
#[serde(skip_serializing_if = "Option::is_none")]
noprelude: Option<bool>,
// This is only set on nightly since it is unstable.
#[serde(skip_serializing_if = "Option::is_none")]
nounused: Option<bool>,
// Intentionally not including `unit_for` because it is a low-level
// internal detail that is mostly used for building the graph.
}
Expand Down Expand Up @@ -101,16 +106,21 @@ pub fn emit_serialized_unit_graph(
.iter()
.map(|unit_dep| {
// https://github.com/rust-lang/rust/issues/64260 when stabilized.
let (public, noprelude) = if gctx.nightly_features_allowed {
(Some(unit_dep.public), Some(unit_dep.noprelude))
let (public, noprelude, nounused) = if gctx.nightly_features_allowed {
(
Some(unit_dep.public),
Some(unit_dep.noprelude),
Some(unit_dep.nounused),
)
} else {
(None, None)
(None, None, None)
};
SerializedUnitDep {
index: indices[&unit_dep.unit],
extern_crate_name: unit_dep.extern_crate_name,
public,
noprelude,
nounused,
}
})
.collect();
Expand Down
43 changes: 43 additions & 0 deletions tests/build-std/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,49 @@ fn test_proc_macro() {
.run();
}

#[cargo_test(build_std_real)]
fn build_std_does_not_warn_about_implicit_std_deps() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "buildstd_test"
version = "0.1.0"
edition = "2021"

[dependencies]
bar = { path = "bar" }
"#,
)
.file("src/main.rs", "fn main() {}")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
edition = "2021"
"#,
)
.file("bar/src/lib.rs", "")
.build();

p.cargo("build")
.build_std()
.target_host()
.env("RUSTFLAGS", "-W unused-crate-dependencies")
.with_stderr_data(
str![[r#"
[WARNING] extern crate `bar` is unused in crate `buildstd_test`
[WARNING] `buildstd_test` (bin "buildstd_test") generated 1 warning
...
"#]]
.unordered(),
)
.run();
}

#[cargo_test(build_std_real)]
fn default_features_still_included_with_extra_build_std_features() {
// This is a regression test to ensure when adding extra `build-std-features`,
Expand Down
3 changes: 3 additions & 0 deletions tests/testsuite/unit_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ fn simple() {
"extern_crate_name": "b",
"index": 1,
"noprelude": false,
"nounused": false,
"public": false
}
],
Expand Down Expand Up @@ -106,6 +107,7 @@ fn simple() {
"extern_crate_name": "c",
"index": 2,
"noprelude": false,
"nounused": false,
"public": false
}
],
Expand Down Expand Up @@ -189,6 +191,7 @@ fn simple() {
"extern_crate_name": "a",
"index": 0,
"noprelude": false,
"nounused": false,
"public": false
}
],
Expand Down