-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
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 |
---|---|---|
|
@@ -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() | ||
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. 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(), | ||
|
@@ -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
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. nit: de-duplicate error messages 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. It reminds me that I can do this: |
||
// 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) = ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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 | ||
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. 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") | ||
|
@@ -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(); | ||
} |
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 need to make sure we're tracking 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.
How about add it to the tracking issue or make a new issue for it?