test(priv_dep): add test for `verify public is respected recursively` #13183

linyihai · 2023-12-18T12:29:20Z

What does this PR try to resolve?

This PR want to verify public is respected recursively by rustc.
The original idea came from here: rust-lang/rust#44663 (comment).

How should we test and review this PR?

you can review and test on per commit.

Additional information

test case wait for add:

Add test case from exported_private_dependencies lint only take effect in innermost dependency rust#119428

rustbot · 2023-12-18T12:29:26Z

r? @ehuss

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

tests/testsuite/pub_priv.rs

epage · 2023-12-27T23:00:45Z

Thanks for doing this investigation!

Any thoughts on what test cases are worth merging now vs waiting on rust-lang/rust#119428?

linyihai · 2023-12-28T11:48:43Z

Thanks for doing this investigation!

Any thoughts on what test cases are worth merging now vs waiting on rust-lang/rust#119428?

I think both test cases make sense. IMO, the test case recursive_package_pub_no_warning verifies the recursively public API does not cause warnings. recursive_package_priv_warning It's also a use case and a reminder of our current processing logic. It's ok to be updated recursive_package_priv_warning when this issue rust-lang/rust#119428 has any progress.

And now I have no plans to add more use cases. If there is nothing else missing, I suggest a combined submission

linyihai · 2023-12-28T12:07:17Z

In the commit I add a public field when publish a crate, but this is only for test.

We should also support upload the public field when publish crates to cratess-io and so on.

At least one public needs to be added here: https://github.com/rust-lang/cargo/blob/master/crates/crates-io/lib.rs#L67,
and optional: dep.is_public() in https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/registry/publish.rs#L353C45-L353C45

ehuss · 2024-01-02T14:57:24Z

r? epage

epage · 2024-01-02T20:54:58Z

crates/cargo-test-support/src/registry.rs

@@ -558,6 +558,7 @@ pub struct Dependency {
    package: Option<String>,
    optional: bool,
    default_features: bool,
+    public: bool,


Feel free to break this out into its own PR for us to merge immiedately

epage · 2024-01-02T20:56:16Z

tests/testsuite/pub_priv.rs

@@ -479,3 +479,220 @@ fn allow_priv_in_custom_build() {
        )
        .run()
 }
+
+// A indirectly add D as private dependency.


These tests don't make it too clear that they are not expected behavior but that some help show existing bugs. We should call that out, linking to the issue in question

epage · 2024-01-02T20:58:21Z

tests/testsuite/pub_priv.rs

@@ -479,3 +479,220 @@ fn allow_priv_in_custom_build() {
        )
        .run()
 }
+
+// A indirectly add D as private dependency.


I've been unsure whether these should be merged before the related fix. Thinking on it more, and with rust-lang/rust#119428 being moved to the rust repo, I suspect we should hold off on tests that demonstrate bad behavior. The problem is that the fix would be merged into the rust repo but they can't update the tests in the same commit, adding more frustration to the process of fixing the bugs.

Sorry that I hadn't thought of it before you had put more time into polishing this up.

It is ok to close this PR or add WIP to the title until the issue fixed in rust repo.

linyihai · 2024-01-03T11:33:08Z

In the commit I add a public field when publish a crate, but this is only for test.

We should also support upload the public field when publish crates to cratess-io and so on.

At least one public needs to be added here: https://github.com/rust-lang/cargo/blob/master/crates/crates-io/lib.rs#L67, and optional: dep.is_public() in https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/registry/publish.rs#L353C45-L353C45

and what do you think this? Maybe this is not what you need to do now, because this will add a 'public' field to the user's local file

test: support publish package with a `public` field. ### What does this PR try to resolve? This PR add a `public` alike method to support add a dependency as public/private, ``` Package::new("foo", "0.1.0") .cargo_feature("public-dependency").add_dep(Dependency::new("bar", "0.1.0").public(true)) ``` and then get it from registry in test. This PR was seperated from the #13183. ### How should we test and review this PR? You can review on per commit. After running the test case `publish_package_with_public_dependency`, you can check a "public" field in `./target/tmp/cit/t0/registry/3/b/bar`. ### Additional information r? epage

epage · 2024-01-03T15:06:04Z

and what do you think this? Maybe this is not what you need to do now, because this will add a 'public' field to the user's local file

Sorry, had overlooked that message. Yes, we need to add publish support. Looking at bindeps and artifacts, it looks like we do that for unstable features today.

We will also need to make sure the crates.io side is implemented.

bors · 2024-01-03T15:39:13Z

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

linyihai · 2024-01-08T10:42:25Z

Sorry, had overlooked that message. Yes, we need to add publish support. Looking at bindeps and artifacts, it looks like we do that for unstable features today.

We will also need to make sure the crates.io side is implemented.

After talking in t-crates-io channel, I've gathered the information as below:

the crates-io currently not suport public field.
if they plan to support that they may ignore the public in metadata carried with the publish api.

So is it deserves to make a PR to add public field when publish crates?

rustbot assigned ehuss Dec 18, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2023

epage reviewed Dec 18, 2023

View reviewed changes

tests/testsuite/pub_priv.rs Outdated Show resolved Hide resolved

linyihai force-pushed the recursive_pri_dep branch 2 times, most recently from d5fa7e2 to 10ff613 Compare December 19, 2023 07:47

rustbot added the A-testing-cargo-itself Area: cargo's tests label Dec 19, 2023

linyihai force-pushed the recursive_pri_dep branch from 8dd77a7 to d2b4b70 Compare December 19, 2023 08:39

epage added the Z-public-dependency Nightly: public-dependency label Dec 20, 2023

linyihai force-pushed the recursive_pri_dep branch from f9d2861 to a5647da Compare December 22, 2023 09:20

linyihai mentioned this pull request Dec 29, 2023

exported_private_dependencies lint only take effect in innermost dependency rust-lang/rust#119428

Open

linyihai changed the title ~~test(priv_dep): add test for verify public is respected recursively~~ WIP: test(priv_dep): add test for verify public is respected recursively Dec 29, 2023

linyihai marked this pull request as draft December 29, 2023 02:07

test(priv_dep): add test for verify public is respected recursively

2633985

linyihai force-pushed the recursive_pri_dep branch from 1205b55 to 9398a1b Compare January 2, 2024 14:51

rustbot assigned epage and unassigned ehuss Jan 2, 2024

linyihai force-pushed the recursive_pri_dep branch from 9398a1b to 2633985 Compare January 2, 2024 15:07

linyihai changed the title ~~WIP: test(priv_dep): add test for verify public is respected recursively~~ test(priv_dep): add test for verify public is respected recursively Jan 2, 2024

linyihai marked this pull request as ready for review January 2, 2024 15:45

epage reviewed Jan 2, 2024

View reviewed changes

linyihai mentioned this pull request Jan 3, 2024

test: support publish package with a public field. #13245

Merged

epage added the S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix label Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(priv_dep): add test for `verify public is respected recursively` #13183

test(priv_dep): add test for `verify public is respected recursively` #13183

linyihai commented Dec 18, 2023 •

edited

rustbot commented Dec 18, 2023

epage commented Dec 27, 2023

linyihai commented Dec 28, 2023

linyihai commented Dec 28, 2023 •

edited

ehuss commented Jan 2, 2024

epage Jan 2, 2024

linyihai Jan 3, 2024

epage Jan 2, 2024

epage Jan 2, 2024

linyihai Jan 3, 2024

linyihai commented Jan 3, 2024

epage commented Jan 3, 2024

bors commented Jan 3, 2024

linyihai commented Jan 8, 2024

test(priv_dep): add test for verify public is respected recursively #13183

Are you sure you want to change the base?

test(priv_dep): add test for verify public is respected recursively #13183

Conversation

linyihai commented Dec 18, 2023 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 18, 2023

epage commented Dec 27, 2023

linyihai commented Dec 28, 2023

linyihai commented Dec 28, 2023 • edited

ehuss commented Jan 2, 2024

epage Jan 2, 2024

Choose a reason for hiding this comment

linyihai Jan 3, 2024

Choose a reason for hiding this comment

epage Jan 2, 2024

Choose a reason for hiding this comment

epage Jan 2, 2024

Choose a reason for hiding this comment

linyihai Jan 3, 2024

Choose a reason for hiding this comment

linyihai commented Jan 3, 2024

epage commented Jan 3, 2024

bors commented Jan 3, 2024

linyihai commented Jan 8, 2024

test(priv_dep): add test for `verify public is respected recursively` #13183

test(priv_dep): add test for `verify public is respected recursively` #13183

linyihai commented Dec 18, 2023 •

edited

linyihai commented Dec 28, 2023 •

edited