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

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Jan 23, 2024

What does this PR try to resolve?

Part of #13308, include:

  • Switching the cargo-features to a -Z
  • Warning if public is used without -Z and setting it to false

These had not done yet:

  • We should also warn if the data type changes but that is less likely to happen and could possibly be skipped
  • Ensuring the published version of the package does not have public

How should we test and review this PR?

All test case should be pass.

Additional information

r? @epage

@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2024

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support A-workspaces Area: workspaces Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2024
@rustbot rustbot assigned epage and unassigned ehuss Jan 23, 2024
@linyihai linyihai changed the title feat: Add "-Zpublic-denpendency" for public-dependency feature. feat: Add "-Zpublic-dependency" for public-dependency feature. Jan 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2024

Could not assign reviewer from: epage.
User(s) epage are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@linyihai linyihai marked this pull request as ready for review January 23, 2024 11:40
Copy link
Contributor

@epage epage left a 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!

src/cargo/core/features.rs Outdated Show resolved Hide resolved
Comment on lines 915 to 917
// FIXME: Turn this on at some point in the future
//Some(vec!["-D exported_private_dependencies".to_string()])
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?

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Jan 23, 2024

@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 -Z flag (or environment variable) will need to be added (anywhere in the bootstrap code that runs cargo, like vendoring, etc.). Worst case it will generate warnings, but I think it would be good to avoid those.

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 cargo-features? I realize that might make the code more complicated, just checking how hard that would be.

Is it intentional that public=false does not generate a warning?

@epage
Copy link
Contributor

epage commented Jan 23, 2024

@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

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 cargo-features then I'm game. Our main interest was to avoid an MSRV bump when using this.

@weihanglo
Copy link
Member

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 cargo metadata to analyze libstd from sysroot, we may want to have a short transition period to avoid too many warnings. One or two versions is sufficient I guess.

@linyihai

This comment was marked as resolved.

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Feb 20, 2024

☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 223 to 233
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()
}
Copy link
Contributor

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.

Copy link
Contributor

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

  1. Validate and error like we do here
  2. Validate but warn on stable
  3. 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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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)

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 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.

@linyihai
Copy link
Contributor Author

☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts.

Conficts had been addressed.

@linyihai linyihai closed this Feb 23, 2024
@linyihai linyihai reopened this Feb 23, 2024
@bors
Copy link
Collaborator

bors commented Feb 23, 2024

☔ The latest upstream changes (presumably #13461) made this pull request unmergeable. Please resolve the merge conflicts.

@linyihai linyihai force-pushed the Z-public branch 2 times, most recently from e426186 to 01de3e0 Compare February 27, 2024 10:37
@epage
Copy link
Contributor

epage commented Feb 27, 2024

@bors r+

Thanks for your work on this and patience as we iterated

@bors
Copy link
Collaborator

bors commented Feb 27, 2024

📌 Commit 01de3e0 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2024
@bors
Copy link
Collaborator

bors commented Feb 27, 2024

⌛ Testing commit 01de3e0 with merge 8964c8c...

@bors
Copy link
Collaborator

bors commented Feb 27, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 8964c8c to master...

@bors bors merged commit 8964c8c into rust-lang:master Feb 27, 2024
21 checks passed
Comment on lines +2025 to +2026
(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()),
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()),

{
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

.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.

@epage
Copy link
Contributor

epage commented Feb 27, 2024

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.

weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 27, 2024
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.
@weihanglo
Copy link
Member

In case I forget, please remind me to update rust-lang/rust#121710 when this PR gets into beta release :)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
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)
@rustbot rustbot added this to the 1.78.0 milestone Feb 28, 2024
epage added a commit to epage/cargo that referenced this pull request Sep 6, 2024
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.
bors added a commit that referenced this pull request Sep 7, 2024
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
dingxiangfei2009 pushed a commit to dingxiangfei2009/cargo that referenced this pull request Sep 17, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support A-workspaces Area: workspaces Command-package S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants