-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Superseding public/private dependencies #3516
Conversation
Really excited to see this moving forward, thanks for making this happen! I advocated for an approach like this (cutting out to the resolver parts at first) because I think it'll realize 95% of the value at 20% of the complexity. An alternative suggested phasing in path: in current edition (and earlier?), all dependencies are public by default but you can annotate them as non-public. In 2024 edition (and later?), all dependencies are private by default and you must explicitly make them public. Naming nit: feels to me like going with full-on |
- `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = "<public|private>"` | ||
- The parallel with Rust seemed worth pursuing | ||
- `pub` could be seen as ambiguous with `publish` | ||
- While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it |
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.
Moving @djc's comment here for a more focused conversation
Naming nit: feels to me like going with full-on public makes more sense in this context than mirroring the pub from function/type syntax.
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.
btw what decision we make for naming here will impact #3487
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 talked about this in the cargo team meeting and the tie breaker for pub
is that it allows our no-MSRV-bump approach to work. public
is already reserved from the previous RFC's implementation.
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.
What do you mean by the "no-MSRV-bump"? If public
is reserved from the previous RFC (and assuming that there is substantial overlap between this RFC's meaning and that one's), how does it make sense to use a different keyword now?
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.
The original plan had the lint on-by-default with the pub
field being treated as an unused field without -Z
, much like [lints]
.
However, since we switched to allow-by-default, we can re-evaluate this part. Treating this field as unused without -Z
does mean it can be adopted more quickly though.
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.
Going for the worse name here feels like prioritizing short-term MSRV support over long-term readability, given how many years we've waited to use this is delaying it another year for just those that provide extreme MSRVs such a problem? (And if the hard-error is removed now, that delay would be reduced relative to when it is stabilized).
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 talked about this in the cargo team meeting and decided to force an MSRV bump and switch to public
.
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.
Read, and thanks for reviving the 6-year-old RFC.
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Huzzah! The @rust-lang/cargo team has decided to accept this RFC. To track further discussion, subscribe to the tracking issue here: |
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
fix(resolver): Remove unused public-deps error handling To implement rust-lang/rfcs#3516, we need to decouple the resolver's behavior from the unstable flag. Since the code path is now dead, I went ahead and removed it.
#1977 has languished for 6 years, mostly due to the cargo resolver design section.
This updates the RFC by
cfg
dependencies, workspace inheritance, renamed dependencies, etc)Pub-aware version resolution was such a large part of the previous RFC, that we felt this needed the wider input of an RFC, rather than addressing these in stabilization FCP.
Rendered