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

Tracking issue for RFC 1977: public & private dependencies #44663

Open
14 of 27 tasks
Tracked by #61
withoutboats opened this issue Sep 17, 2017 · 85 comments
Open
14 of 27 tasks
Tracked by #61

Tracking issue for RFC 1977: public & private dependencies #44663

withoutboats opened this issue Sep 17, 2017 · 85 comments
Assignees
Labels
A-maybe-future-edition Something we may consider for a future edition. B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. F-public_private_dependencies feature: public_private_dependencies S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-impl-incomplete Status: The implementation is incomplete. S-tracking-needs-documentation Status: Needs documentation. S-tracking-needs-migration-lint Status: This item needs a migration lint. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@withoutboats
Copy link
Contributor

withoutboats commented Sep 17, 2017

Summary

RFC (original, superseded): #1977
RFC: #3516
Cargo tracking issue: rust-lang/cargo#6129
Issues: https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AF-public_private_dependencies
Cargo issues: https://github.com/rust-lang/cargo/issues?q=is%3Aopen+is%3Aissue+label%3AZ-public-dependency
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#public-dependency

This feature enables the ability to track which dependencies are publicly exposed through a library's interface. It has two sides to the implementation: rustc (lint and --extern flag), and cargo (Cargo.toml syntax, and passing --extern flags).

This feature was originally specified in rust-lang/rfcs#1977, but was later down-scoped in rust-lang/rfcs#3516.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Discussion comments will get marked as off-topic or deleted.
Repeated discussions on the tracking issue may lead to the tracking issue getting locked.

Unresolved Questions

Steps

Non-blocking further improvements

Changes from RFC

cc @rust-lang/cargo @epage

@withoutboats withoutboats added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. labels Sep 17, 2017
@Boscop
Copy link

Boscop commented Oct 27, 2017

Maybe it's useful to look at how this problem is approached in Haskell.
Here are some relevant slides: https://wiki.haskell.org/wikiupload/b/b4/HIW2011-Talk-Loeh.pdf

Here is an example of a subtle issue that occurs with private dependencies:

Btw, I found it through this post: http://harry.garrood.me/blog/purescript-why-bower/

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 24, 2018

Would Rust2018 be a good opportunity to make the braking changes to Cargo.toml?

@SimonSapin
Copy link
Contributor

Maybe. Though such changes should be designed and proposed soon, have an easy migration path, and preferably only affect a minority of existing crates.

@Nemo157
Copy link
Member

Nemo157 commented Nov 29, 2018

Thought I just had related to ecosystems like futures that provide a "core" stable crate along with less stable utilities re-exported from a "facade": items from a public dependency re-exported through a private dependency should be treated as public. (This is likely how the implementation would behave anyway, but having a testcase for this would be nice). Basically how I see using a tightly coupled ecosystem like that work is that you would declare both the "facade" crate and "core" crate as dependencies, but only the "core" crate as public:

[dependencies]
futures = { version = "10.17", public = false }
futures-core = { version = "1.2", public = true }

But then, in code you would never import items from the "core" crate, instead you would simply have public APIs that return types which have been re-exported through the "facade" from the "core" crate and rely on the compiler to warn you if you accidentally use something that didn't originate in the "core" crate.

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 29, 2018

I don't quite understand something about your example. Can you elaborate on what the Toml and the code in the futures crate look like?

@Nemo157
Copy link
Member

Nemo157 commented Nov 30, 2018

@Eh2406 the setup's a little annoying to just explain, so here's a full compiling set of example crates: https://github.com/Nemo157/rust-44663-facade-example

In this example futures-core exports a single trait Future, futures re-exports this trait from futures-core along with a "utility" Foo.

Now mylib wants to use futures, including returning some Futures from its public API. But futures itself intends to have breaking changes to update utilities, users that care about interoperability should only use types from futures-core in their public APIs. Even as new major versions of futures are released they will all link against the same version of futures-core so the underlying traits will be interoperable.

So, to have the compiler enforce this restriction the developers of mylib want to use the public/private dependencies feature, they have both futures and futures-core as dependency but only futures-core marked as public. But for ergonomics it's nicer to just use the re-exports directly from futures, instead of having to remember which parts of the API need to be imported from futures and which from futures-core.

@Aaron1011
Copy link
Member

I'd like to work on this.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 16, 2018

Thanks for offering to work on this! @alexcrichton, do you have any mentoring advice?

I can at any time get a branch ready for the resolver part of cargo, it will be good enough to start experimentation, although not good enough to be ready to stabilize.

@alexcrichton
Copy link
Member

Sure! @Aaron1011 so this is split roughly into two features, one being rustc knowing what crates are in the "public interface" and a second being the support in Cargo's resolver to make various decisions differently. Are you interested in one of those in particular?

@Aaron1011
Copy link
Member

@alexcrichton: I've started working on the rustc part in a branch: https://github.com/Aaron1011/rust/commits/feature/pub-priv-dep, and I plan to submit a (WIP) PR soon. I might also be interested in working on the cargo part as well.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 17, 2018

@Aaron1011 please cc me when you make that PR. I know nothing about the internals of Rust, so will be of no help, but I do want to follow the discussion.

I will redouble my efforts to get the Cargo's resolver part mergeable. I have been enjoying ( and hating ) it because it is the hardest algorithmic problem I have ever worked on. I would be happy for some help, perhaps a new perspective will get me un-stuck.

@alexcrichton
Copy link
Member

@Aaron1011 ok and it seems like you're off to a great start! I'd be up for reviewing that PR, and if you've got any questions along the way just let me know

@Nemo157
Copy link
Member

Nemo157 commented Jan 25, 2019

There was another usecase for this mentioned on i.rl.o, if cargo doc had a form of "local development" mode to build the documentation you care about when working on a crate, this would allow that mode to know exactly which dependencies it needs to produce the documentation for (all direct dependencies + their public dependencies).

Similarly, if you're self-hosting documentation for a crate somewhere you may want to be able to generate a documentation bundle including the crate and all its public dependencies so that it is fully self-contained.

bors added a commit that referenced this issue Feb 1, 2019
Implement public/private dependency feature

Implements #44663

The core implementation is done - however, there are a few issues that still need to be resolved:

- [x] The `EXTERNAL_PRIVATE_DEPENDENCY` lint currently does notthing when the `public_private_dependencies` is not enabled. Should mentioning the lint (in an `allow` or `deny` attribute) be an error if the feature is not enabled? (Resolved- the feature was removed)
- [x] Crates with the name `core` and `std` are always marked public, without the need to explcitily specify them on the command line. Is this what we want to do? Do we want to allow`no_std`/`no_core` crates to explicitly control this in some way? (Resolved - private crates are now explicitly specified)
- [x] Should I add additional UI tests? (Resolved - added more tests)
- [x] Does it make sense to be able to allow/deny the `EXTERNAL_PRIVATE_DEPENDENCY` on an individual item? (Resolved - this is implemented)
Aaron1011 added a commit to Aaron1011/crates.io that referenced this issue Mar 21, 2019
This implements the crates.io side of rust-lang/rust#44663

This is fully backwards-compatible - 'public' will default to 'false'
for old versions of Cargo that don't include it in their manifest
@linyihai

This comment was marked as resolved.

@epage
Copy link
Contributor

epage commented Dec 29, 2023

Thanks, fixed!

@tmandry
Copy link
Member

tmandry commented Jan 5, 2024

I think the decision to require a key (public = true) that breaks old cargo versions is a mistake. It shouldn't have been implemented in such a way that it produced a hard error in cargo in the first place, but now many crates in the ecosystem will be prevented from using this highly desirable feature for years.

At the same time I understand that public = true is the most ergonomic option. Can we at least provide an alternative key, for crates that care about MSRV?

@epage
Copy link
Contributor

epage commented Jan 5, 2024

I can understand the desire for this to not require an MSRV change. Part of the problem is historical, that the original feature was designed to be very different than what we have today. Part of the problem is not require an MSRV bump is very out of the ordinary with the only cases I can think of being package.rust-version and lints.

However, I don't feel a redundant field would be appropriate.

We could consider dropping the nightly requirement now so we at least have a couple release lead time on this problem (with the risk of stabilzation being even further out than that with the state of the compiler side).

I do think this conversation represents a deeper problem: a push towards stagnation due to MSRV. I'm working on the update for the MSRV resolver RFC that I'm hoping will help lay out a better path for MSRV policies that can help push us out of this stagnation, shifting costs to those that need old versions rather than being old Rust versions being a millstone around the communities neck.

@tmandry
Copy link
Member

tmandry commented Jan 12, 2024

I started on this because I thought public/private deps would weigh on the ability of a library crate to be linked dynamically within a larger build graph, but I don't believe that to be true anymore. So if the cost of not being able to adopt this feature is only local to a crate, it doesn't matter that much.

That being said, I still think we can do better in the future.

Part of the problem is not require an MSRV bump is very out of the ordinary with the only cases I can think of being package.rust-version and lints.

I can throw any random key in my Cargo.toml and will only get a warning that it wasn't used. That is, I believe, what cargo should have continued doing with the public key. Cargo attempted to approach stability issues the same way the language does, by gating features with hard errors, when in fact I believe it needs a different approach.

Your RFC gets this right, by phasing in enforcement as a warning (with opt-in, even) and turning it into a hard error over an edition. I can think of other approaches that use this "two factor" opt-in idea to get away without waiting for an edition.

We could consider dropping the nightly requirement now so we at least have a couple release lead time on this problem (with the risk of stabilzation being even further out than that with the state of the compiler side).

Yes, I think we should do that in any case.

I do think this conversation represents a deeper problem: a push towards stagnation due to MSRV. I'm working on the update for the MSRV resolver RFC that I'm hoping will help lay out a better path for MSRV policies that can help push us out of this stagnation, shifting costs to those that need old versions rather than being old Rust versions being a millstone around the communities neck.

Respectfully, I think we still have to take version skew issues seriously in the design of our package manager. Shifting the cost to the users who want or need the most stability that might help motivate updates in some instances, but in others it will just make Rust a less viable option. Having access to recent versions of ecosystem crates is a security matter.

There is a fundamental tension between stability and stagnation. How confident can you be that your MSRV RFC will modify the revealed preferences of ecosystem libraries and their users? There will always be a cost to updating unless we take a very hard line stance on backward compatibility, which historically we have not been willing to do (and I don't think we should).

@epage
Copy link
Contributor

epage commented Jan 12, 2024

I can throw any random key in my Cargo.toml and will only get a warning that it wasn't used. That is, I believe, what cargo should have continued doing with the public key. Cargo attempted to approach stability issues the same way the language does, by gating features with hard errors, when in fact I believe it needs a different approach.

Its not as simple as "it was ignored before so it should be fine to not error while unstable". Depending on how the key changes behavior, ignoring it would run counter to the users intention. While we can't go back in time to ensure the key was never allowed, the unstable period offers us the chance to ensure users will get a hard error, telling them that the feature they are trying to use is unsupported and cargo cannot fulfill their intent. This also discourages users mixing nightly and stable code where an upgrade of stable can break them because the stabilized form of the feature is different.

To balance this out, I was the one who advocated for [lints] to not error on stable (and I went out of my way in the implementation to deal with nightly users in case the format changed). I also was originally advocating for it here. We should be looking for opportunities where it works to not error while its unstable. Its just not a blanket thing we should do.

@epage
Copy link
Contributor

epage commented Jan 16, 2024

We discussed the blocking feature gate in today's cargo meeting and I've posted rust-lang/cargo#13307 to clarify the documentation to encourage more non-blocking gates and I've opened rust-lang/cargo#13308 to change our feature gate here.

@tmandry
Copy link
Member

tmandry commented Feb 14, 2024

Thanks @epage, I really appreciate your receptiveness here!

@weihanglo
Copy link
Member

See #121710 (comment).

The non-blocking gate -Zpublic-dependency turns out blocking bootstrapping rustc, with a flood of exported_private_dependencies lint errors. That's because cargo-features = ["public-dependency"] is scoped to a certain crate, but with -Zpublic-dependency the entire dependency graph starts passing --extern priv:<crate> unconditionally.

@epage
Copy link
Contributor

epage commented Mar 23, 2024

Probably worth keeping both opt-ins.

@traviscross traviscross added the A-edition-2024 Area: The 2024 edition label Apr 10, 2024
@epage epage self-assigned this Apr 11, 2024
@traviscross traviscross added S-tracking-needs-migration-lint Status: This item needs a migration lint. S-tracking-needs-documentation Status: Needs documentation. labels May 21, 2024
@traviscross
Copy link
Contributor

traviscross commented May 28, 2024

@rustbot labels +E-help-wanted

If this item is going to make it into Rust 2024 (and it's OK if it doesn't), we're going to need some help to check off the remaining items above.

As @davidtwco describes:

I think the compiler's implementation needs to change quite a bit. We currently compute the private-ness of a dependency and then that is used by the lint whenever we see an item from that dependency, regardless of the path you access the item from. I think that's confusing but also if an intermediate dependency changes which of its dependencies it re-exports from then something that didn't lint might start linting and all sorts of things like that. However, the implementation of lint uses our visibility infrastructure, at which point it's far too late to be able to know which path you took to reference an item, that information isn't really available after name resolution. I experimented with trying to stash away some information that I could later use to do this, but nothing felt right.

We'll mark this as E-help-wanted. If you're interested in seeing this happen in time for the edition and pitching in here, please comment on this Zulip thread.

@rustbot rustbot added the E-help-wanted Call for participation: Help is requested to fix this issue. label May 28, 2024
@traviscross traviscross added the S-tracking-at-risk Status: This item is at risk of missing e.g. edition deadlines. label May 28, 2024
@traviscross
Copy link
Contributor

traviscross commented Jun 4, 2024

@rustbot labels -A-edition-2024 +A-maybe-future-edition

Without an owner driving this work forward, this is looking unlikely for this edition, so let's drop that label and mark this as a possibility for a future edition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-maybe-future-edition Something we may consider for a future edition. B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. F-public_private_dependencies feature: public_private_dependencies S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-impl-incomplete Status: The implementation is incomplete. S-tracking-needs-documentation Status: Needs documentation. S-tracking-needs-migration-lint Status: This item needs a migration lint. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
Status: In Progress
Development

No branches or pull requests