-
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
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
Could not assign reviewer from: |
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 moving this forward!
// FIXME: Turn this on at some point in the future | ||
//Some(vec!["-D exported_private_dependencies".to_string()]) |
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?
@weihanglo, just a heads up, I believe this will cause problems in the rust-lang/rust workspace. Can you coordinate with the release team to let them know, and perhaps help with the transition? This will need to be done during the next beta branch when bootstrap is updated. There are potentially several places where this In general I'm concerned that this will still generate a bunch of warnings (such as running cargo manually). I'm curious if it is feasible to still honor Is it intentional that |
If its a problem, we could instead do a transition where we temporarily support both or, as you mention elsewhere, generally support both. We had talked about a transition but were unsure what the impact would be. If there are projects that use this enough that they are fine with the |
Created a zulip topic here: https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/-Zpublic-dependency.20syntax.20change. Supposedly editors tools like intellij-rust, rust-analyzer, and neovim plugins might rely on |
This comment was marked as resolved.
This comment was marked as resolved.
528f7c1
to
f7776ac
Compare
☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts. |
tests/testsuite/pub_priv.rs
Outdated
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: | ||
'public' specifier can only be used on regular dependencies, not Development dependencies | ||
", | ||
) | ||
.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.
Now that we have a test to more concretely talk about this, I'm going to continue on this comment thread here.
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 should probably check the unstable status and report this as a warning that this will fail when stabilized. Or in other words, without the -Z, users should get the experience they have before any work was done on this but be warned what will break
Our options are
- Validate and error like we do here
- Validate but warn on stable
- Do nothing, relying only on the "this isn't stable / unused" warning
This takes (1). I had thought [lints]
went with (2) but it in looking at the code, it went with (3). Unsure why I changed my mind.
We should make a conscious choice about what route we go.
- The downside to errors (1) is that changes in unstable behavior affect users on stable.
- Warning (2) can help people on mixed stable/nightly detect changes earlier
- For stable-only people with an MSRV, the warning (2) will be unactionable and annoying
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.
that changes in unstable behavior affect users on stable.
Not quite sure how this happened, and could it have been avoided?
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.
So those tests show that we are erroring on this unstable feature when we should instead warn that this will error out in the future when its stabilized.
if dep.kind() != DepKind::Normal {
bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind());
Change this to a warning?
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.
Not quite sure how this happened, and could it have been avoided?
If someone is using an unstable feature on a specific nightly but wants it to work generally on stable, we need to be careful. However, that is more about introducing new errors. Removing an error (the main risk here) wouldn't be breaking.
Change this to a warning?
When the feature isn't enabled? That is option (2)
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 had made a new commit.
if a dependency isn't normal dependency, warn public
used in dev-dependency when no feature enabled in nightly. we should to skip set dep.set_public(p)
, and set orig.public = None
.
Conficts had been addressed. |
☔ The latest upstream changes (presumably #13461) made this pull request unmergeable. Please resolve the merge conflicts. |
e426186
to
01de3e0
Compare
@bors r+ Thanks for your work on this and patience as we iterated |
☀️ Test successful - checks-actions |
(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()), |
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.
nit: de-duplicate error messages
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.
It reminds me that I can do this:
(true, _) | (_, true) => bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind()),
{ | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
nit: convention is starting with a lowercase letter
.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 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.
Most of these messages will be removed with stabilization, so at least to me, DRYing is not a priority and can be harmful. I at least also care a little less about the quality of the messages. |
rust-lang/cargo#13340 switches the featurte gate for public-dependency from `cargo-features` in Cargo.toml to CLI flag `-Zpublic-dependency`. `cargo-features` will continue working for 1 to 2 release cycles as a transition period, to make sure that it doesn't break self-rebuilds.
In case I forget, please remind me to update rust-lang/rust#121710 when this PR gets into beta release :) |
Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)
Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)
In rust-lang#13308, we decided to make the feature gate for public/private dependencies non-blocking. Generally, people opt-in to a feature that is non-blocking through `-Z` but some nightly users want an "always on" mode for this, so we offered both in rust-lang#13340. In rust-lang#13340, we made both feature gates work but we did not make them non-blocking for stable, only nightly. This change makes the feature gate non-blocking on stable.
fix(toml): Don't require MSRV bump for pub/priv ### What does this PR try to resolve? In #13308, we decided to make the feature gate for public/private dependencies non-blocking. Generally, people opt-in to a feature that is non-blocking through `-Z` but some nightly users want an "always on" mode for this, so we offered both in #13340. In #13340, we made both feature gates work but we did not make them non-blocking for stable, only nightly. This change makes the feature gate non-blocking on stable. Unfortunately, this means that 1.83 will be the MSRV for people using public dependencies. Good thing the feature is indefinitely blocked in rustc as that gives us more time. ### How should we test and review this PR? ### Additional information
In rust-lang#13308, we decided to make the feature gate for public/private dependencies non-blocking. Generally, people opt-in to a feature that is non-blocking through `-Z` but some nightly users want an "always on" mode for this, so we offered both in rust-lang#13340. In rust-lang#13340, we made both feature gates work but we did not make them non-blocking for stable, only nightly. This change makes the feature gate non-blocking on stable.
What does this PR try to resolve?
Part of #13308, include:
These had not done yet:
How should we test and review this PR?
All test case should be pass.
Additional information
r? @epage