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.

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: Add "-Zpublic-dependency" for public-dependency feature. #13340

Merged
merged 1 commit into from
Feb 27, 2024
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
7 changes: 3 additions & 4 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1506,15 +1506,14 @@ pub fn extern_args(
|dep: &UnitDep, extern_crate_name: InternedString, noprelude: bool| -> CargoResult<()> {
let mut value = OsString::new();
let mut opts = Vec::new();
if unit
let is_public_dependency_enabled = unit
.pkg
.manifest()
.unstable_features()
.require(Feature::public_dependency())
.is_ok()
&& !dep.public
&& unit.target.is_lib()
{
|| 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;
}
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,7 @@ unstable_cli_options!(
panic_abort_tests: bool = ("Enable support to run tests with -Cpanic=abort"),
precise_pre_release: bool = ("Enable pre-release versions to be selected with `update --precise`"),
profile_rustflags: bool = ("Enable the `rustflags` option in profiles in .cargo/config.toml file"),
public_dependency: bool = ("Respect a dependency's `public` field in Cargo.toml to control public/private dependencies"),
publish_timeout: bool = ("Enable the `publish.timeout` key in .cargo/config.toml file"),
rustdoc_map: bool = ("Allow passing external documentation mappings to rustdoc"),
rustdoc_scrape_examples: bool = ("Allows Rustdoc to scrape code examples from reverse-dependencies"),
Expand Down Expand Up @@ -1141,6 +1142,7 @@ impl CliUnstable {
"mtime-on-use" => self.mtime_on_use = parse_empty(k, v)?,
"no-index-update" => self.no_index_update = parse_empty(k, v)?,
"panic-abort-tests" => self.panic_abort_tests = parse_empty(k, v)?,
"public-dependency" => self.public_dependency = parse_empty(k, v)?,
"profile-rustflags" => self.profile_rustflags = parse_empty(k, v)?,
"precise-pre-release" => self.precise_pre_release = parse_empty(k, v)?,
"trim-paths" => self.trim_paths = parse_empty(k, v)?,
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,7 @@ fn run_verify(
.unstable_features()
.require(Feature::public_dependency())
.is_ok()
|| ws.gctx().cli_unstable().public_dependency
{
// FIXME: Turn this on at some point in the future
//Some(vec!["-D exported_private_dependencies".to_string()])
Comment on lines 916 to 917
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make sure we're tracking this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about add it to the tracking issue or make a new issue for it?

Expand Down
41 changes: 35 additions & 6 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,27 @@ pub fn to_real_manifest(
v.unused_keys(),
manifest_ctx.warnings,
);
let mut resolved = resolved;
if let manifest::TomlDependency::Detailed(ref mut d) = resolved {
if d.public.is_some() {
if matches!(dep.kind(), DepKind::Normal) {
if !manifest_ctx
.features
.require(Feature::public_dependency())
.is_ok()
&& !manifest_ctx.gctx.cli_unstable().public_dependency
{
d.public = None;
manifest_ctx.warnings.push(format!(
"Ignoring `public` on dependency {name}. Pass `-Zpublic-dependency` to enable support for it", name = &dep.name_in_toml()
Copy link
Member

Choose a reason for hiding this comment

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

nit: convention is starting with a lowercase letter

))
}
} else {
d.public = None;
}
}
}
linyihai marked this conversation as resolved.
Show resolved Hide resolved

manifest_ctx.deps.push(dep);
deps.insert(
n.clone(),
Expand Down Expand Up @@ -1992,15 +2013,23 @@ fn detailed_dep_to_dependency<P: ResolveToPath + Clone>(
}

if let Some(p) = orig.public {
manifest_ctx
.features
.require(Feature::public_dependency())?;
let public_feature = manifest_ctx.features.require(Feature::public_dependency());
let with_z_public = manifest_ctx.gctx.cli_unstable().public_dependency;
let with_public_feature = public_feature.is_ok();
if !with_public_feature && (!with_z_public && !manifest_ctx.gctx.nightly_features_allowed) {
public_feature?;
}

if dep.kind() != DepKind::Normal {
bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind());
match (with_public_feature, with_z_public) {
(true, _) => bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind()),
(_, true) => bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind()),
Comment on lines +2025 to +2026
Copy link
Member

Choose a reason for hiding this comment

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

nit: de-duplicate error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It reminds me that I can do this:
(true, _) | (_, true) => bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind()),

// If public feature isn't enabled in nightly, we instead warn that.
(false, false) => manifest_ctx.warnings.push(format!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind())),
}
} else {
dep.set_public(p);
}

dep.set_public(p);
}

if let (Some(artifact), is_lib, target) = (
Expand Down
2 changes: 1 addition & 1 deletion tests/build-std/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn enable_build_std(e: &mut Execs, arg: Option<&str>) {
Some(s) => format!("-Zbuild-std={}", s),
None => "-Zbuild-std".to_string(),
};
e.arg(arg);
e.arg(arg).arg("-Zpublic-dependency");
e.masquerade_as_nightly_cargo(&["build-std"]);
}

Expand Down
28 changes: 15 additions & 13 deletions tests/testsuite/cargo/z_help/stdout.term.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
157 changes: 145 additions & 12 deletions tests/testsuite/pub_priv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,15 @@ fn requires_feature() {

p.cargo("check --message-format=short")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_status(101)
.with_stderr(
"\
error: failed to parse manifest at `[..]`

Caused by:
feature `public-dependency` is required

The package requires the Cargo feature called `public-dependency`, \
but that feature is not stabilized in this version of Cargo (1.[..]).
Consider adding `cargo-features = [\"public-dependency\"]` to the top of Cargo.toml \
(above the [package] table) to tell Cargo you are opting in to use this unstable feature.
See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#public-dependency \
for more information about the status of this feature.
[WARNING] Ignoring `public` on dependency pub_dep. Pass `-Zpublic-dependency` to enable support for it
[UPDATING] `[..]` index
[DOWNLOADING] crates ...
[DOWNLOADED] pub_dep v0.1.0 ([..])
[CHECKING] pub_dep v0.1.0
[CHECKING] foo v0.0.1 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run()
Expand Down Expand Up @@ -198,6 +193,45 @@ Caused by:
.run()
}

#[cargo_test]
fn pub_dev_dependency_without_feature() {
Package::new("pub_dep", "0.1.0")
.file("src/lib.rs", "pub struct FromPub;")
.publish();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"

[dev-dependencies]
pub_dep = {version = "0.1.0", public = true}
"#,
)
.file(
"tests/mod.rs",
"
extern crate pub_dep;
pub fn use_pub(_: pub_dep::FromPub) {}
",
)
.build();

p.cargo("check --message-format=short")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr(
"\
[WARNING] 'public' specifier can only be used on regular dependencies, not Development dependencies
Copy link
Member

Choose a reason for hiding this comment

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

This was not introduced by this PR, but capitalized "Development dependencies" doesn't seem ideal.

[UPDATING] `[..]` index
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run()
}

#[cargo_test(nightly, reason = "exported_private_dependencies lint is unstable")]
fn workspace_pub_disallowed() {
Package::new("foo1", "0.1.0")
Expand Down Expand Up @@ -534,3 +568,102 @@ fn publish_package_with_public_dependency() {
)
.run()
}

#[cargo_test(nightly, reason = "exported_private_dependencies lint is unstable")]
fn verify_mix_cargo_feature_z() {
Package::new("dep", "0.1.0")
.file("src/lib.rs", "pub struct FromDep;")
.publish();
Package::new("priv_dep", "0.1.0")
.file("src/lib.rs", "pub struct FromPriv;")
.publish();
Package::new("pub_dep", "0.1.0")
.file("src/lib.rs", "pub struct FromPub;")
.publish();
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["public-dependency"]
[package]
name = "foo"
version = "0.0.1"

[dependencies]
dep = "0.1.0"
priv_dep = {version = "0.1.0", public = false}
pub_dep = {version = "0.1.0", public = true}
"#,
)
.file(
"src/lib.rs",
"
extern crate dep;
extern crate priv_dep;
extern crate pub_dep;
pub fn use_dep(_: dep::FromDep) {}
pub fn use_priv(_: priv_dep::FromPriv) {}
pub fn use_pub(_: pub_dep::FromPub) {}
",
)
.build();

p.cargo("check -Zpublic-dependency --message-format=short")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr_contains(
"\
src/lib.rs:5:13: warning: type `FromDep` from private dependency 'dep' in public interface
src/lib.rs:6:13: warning: type `FromPriv` from private dependency 'priv_dep' in public interface
",
)
.run();
}

#[cargo_test(nightly, reason = "exported_private_dependencies lint is unstable")]
fn verify_z_public_dependency() {
Package::new("dep", "0.1.0")
.file("src/lib.rs", "pub struct FromDep;")
.publish();
Package::new("priv_dep", "0.1.0")
.file("src/lib.rs", "pub struct FromPriv;")
.publish();
Package::new("pub_dep", "0.1.0")
.file("src/lib.rs", "pub struct FromPub;")
.publish();
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"

[dependencies]
dep = "0.1.0"
priv_dep = {version = "0.1.0", public = false}
pub_dep = {version = "0.1.0", public = true}
"#,
)
.file(
"src/lib.rs",
"
extern crate dep;
extern crate priv_dep;
extern crate pub_dep;
pub fn use_dep(_: dep::FromDep) {}
pub fn use_priv(_: priv_dep::FromPriv) {}
pub fn use_pub(_: pub_dep::FromPub) {}
",
)
.build();

p.cargo("check -Zpublic-dependency --message-format=short")
.masquerade_as_nightly_cargo(&["public-dependency"])
.with_stderr_contains(
"\
src/lib.rs:5:13: warning: type `FromDep` from private dependency 'dep' in public interface
src/lib.rs:6:13: warning: type `FromPriv` from private dependency 'priv_dep' in public interface
",
)
.run();
}