feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

linyihai · 2024-01-23T08:45:26Z

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 · 2024-01-23T08:45:31Z

r? @ehuss

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

rustbot · 2024-01-23T11:01:12Z

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.

epage

Thanks for moving this forward!

src/cargo/core/features.rs

epage · 2024-01-23T16:51:25Z

src/cargo/ops/cargo_package.rs

        // FIXME: Turn this on at some point in the future
        //Some(vec!["-D exported_private_dependencies".to_string()])


We need to make sure we're tracking this

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

src/cargo/util/toml/mod.rs

ehuss · 2024-01-23T17:14:29Z

@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 · 2024-01-23T17:21:31Z

@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 · 2024-01-24T03:15:40Z

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.

src/cargo/util/toml/mod.rs

bors · 2024-02-20T19:58:31Z

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

epage · 2024-02-21T18:43:28Z

tests/testsuite/pub_priv.rs

+    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()
+}


Now that we have a test to more concretely talk about this, I'm going to continue on this comment thread here.

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

that changes in unstable behavior affect users on stable.
Not quite sure how this happened, and could it have been avoided?

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?

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)

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 · 2024-02-23T03:04:53Z

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

Conficts had been addressed.

bors · 2024-02-23T18:44:13Z

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

epage · 2024-02-27T19:21:38Z

@bors r+

Thanks for your work on this and patience as we iterated

bors · 2024-02-27T19:21:41Z

📌 Commit 01de3e0 has been approved by epage

It is now in the queue for this repository.

bors · 2024-02-27T19:22:49Z

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

bors · 2024-02-27T20:08:20Z

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

weihanglo · 2024-02-27T21:00:31Z

src/cargo/util/toml/mod.rs

+                (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()),


nit: de-duplicate error messages

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

weihanglo · 2024-02-27T21:03:47Z

src/cargo/util/toml/mod.rs

+                        {
+                            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()


nit: convention is starting with a lowercase letter

weihanglo · 2024-02-27T21:05:19Z

tests/testsuite/pub_priv.rs

+        .masquerade_as_nightly_cargo(&["public-dependency"])
+        .with_stderr(
+            "\
+[WARNING] 'public' specifier can only be used on regular dependencies, not Development dependencies


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

epage · 2024-02-27T21:36:27Z

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.

weihanglo · 2024-02-27T22:15:09Z

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)

rustbot assigned ehuss Jan 23, 2024

linyihai force-pushed the Z-public branch from 8a2bc71 to 2fff2fd Compare January 23, 2024 08:55

rustbot assigned epage and unassigned ehuss Jan 23, 2024

linyihai force-pushed the Z-public branch from 2fff2fd to a60fc98 Compare January 23, 2024 10:27

linyihai changed the title ~~feat: Add "-Zpublic-denpendency" for public-dependency feature.~~ feat: Add "-Zpublic-dependency" for public-dependency feature. Jan 23, 2024

linyihai force-pushed the Z-public branch from a60fc98 to c829772 Compare January 23, 2024 11:01

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

epage reviewed Jan 23, 2024

View reviewed changes

linyihai force-pushed the Z-public branch from c829772 to 1c77007 Compare January 24, 2024 08:04

This comment was marked as resolved.

Sign in to view

linyihai commented Jan 24, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved

linyihai force-pushed the Z-public branch 4 times, most recently from 528f7c1 to f7776ac Compare January 26, 2024 07:16

linyihai marked this pull request as draft January 26, 2024 07:17

linyihai force-pushed the Z-public branch from f7776ac to b13f167 Compare January 26, 2024 07:43

linyihai force-pushed the Z-public branch 3 times, most recently from b6e66e2 to 32392c8 Compare February 19, 2024 11:18

linyihai commented Feb 19, 2024

View reviewed changes

src/cargo/util/toml/mod.rs Show resolved Hide resolved

epage reviewed Feb 21, 2024

View reviewed changes

linyihai force-pushed the Z-public branch from 32392c8 to 3945cb9 Compare February 23, 2024 02:53

linyihai closed this Feb 23, 2024

linyihai reopened this Feb 23, 2024

feat: Add "-Zpublic-dependency" for public-dependency feature

01de3e0

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

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 merged commit 8964c8c into rust-lang:master Feb 27, 2024
21 checks passed

weihanglo reviewed Feb 27, 2024

View reviewed changes

weihanglo mentioned this pull request Feb 27, 2024

Update cargo rust-lang/rust#121699

Merged

weihanglo mentioned this pull request Feb 27, 2024

[DONT MERGE] fix(libstd): switch to -Zpublic-dependency cargo flag rust-lang/rust#121710

Draft

rustbot added this to the 1.78.0 milestone Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

linyihai commented Jan 23, 2024 •

edited

rustbot commented Jan 23, 2024

rustbot commented Jan 23, 2024

epage left a comment

epage Jan 23, 2024

linyihai Jan 26, 2024

ehuss commented Jan 23, 2024

epage commented Jan 23, 2024

weihanglo commented Jan 24, 2024

This comment was marked as resolved.

bors commented Feb 20, 2024

epage Feb 21, 2024

epage Feb 21, 2024

linyihai Feb 23, 2024

linyihai Feb 26, 2024

epage Feb 26, 2024

linyihai Feb 27, 2024

linyihai commented Feb 23, 2024

bors commented Feb 23, 2024

epage commented Feb 27, 2024

bors commented Feb 27, 2024

bors commented Feb 27, 2024

bors commented Feb 27, 2024

weihanglo Feb 27, 2024

linyihai Feb 28, 2024

weihanglo Feb 27, 2024

weihanglo Feb 27, 2024

epage commented Feb 27, 2024

weihanglo commented Feb 27, 2024

		// FIXME: Turn this on at some point in the future
		//Some(vec!["-D exported_private_dependencies".to_string()])

		(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()),

feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

feat: Add "-Zpublic-dependency" for public-dependency feature. #13340

Conversation

linyihai commented Jan 23, 2024 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jan 23, 2024

rustbot commented Jan 23, 2024

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Jan 23, 2024

epage commented Jan 23, 2024

weihanglo commented Jan 24, 2024

This comment was marked as resolved.

bors commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linyihai commented Feb 23, 2024

bors commented Feb 23, 2024

epage commented Feb 27, 2024

bors commented Feb 27, 2024

bors commented Feb 27, 2024

bors commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Feb 27, 2024

weihanglo commented Feb 27, 2024

linyihai commented Jan 23, 2024 •

edited