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

RFC: Superseding public/private dependencies #3516

Merged
merged 43 commits into from
Nov 18, 2023
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 16, 2023

#1977 has languished for 6 years, mostly due to the cargo resolver design section.

This updates the RFC by

  • Removing non-essential parts, particularly minimal-version resolution and pub-aware version resolution
  • Updating the template
  • Taking into account new designs that impact it (cfg dependencies, workspace inheritance, renamed dependencies, etc)
  • Updates the rustc design for how it evolved in Tracking issue for RFC 1977: public & private dependencies rust#44663
  • Fully specifies cargo's behavior

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

@epage epage added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Oct 16, 2023
@djc
Copy link
Contributor

djc commented Oct 17, 2023

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 public makes more sense in this context than mirroring the pub from function/type syntax.

Comment on lines 241 to 244
- `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
Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

@Nemo157 Nemo157 Nov 7, 2023

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

Copy link
Contributor Author

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.

Copy link
Member

@weihanglo weihanglo left a 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.

text/3516-public-private-dependencies.md Outdated Show resolved Hide resolved
text/3516-public-private-dependencies.md Outdated Show resolved Hide resolved
text/3516-public-private-dependencies.md Outdated Show resolved Hide resolved
text/3516-public-private-dependencies.md Outdated Show resolved Hide resolved
epage and others added 4 commits November 13, 2023 14:42
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 18, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 18, 2023

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.

@ehuss ehuss merged commit 845d609 into rust-lang:master Nov 18, 2023
@ehuss
Copy link
Contributor

ehuss commented Nov 18, 2023

Huzzah! The @rust-lang/cargo team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/rust#44663

epage added a commit to epage/cargo that referenced this pull request Nov 22, 2023
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.
epage added a commit to epage/cargo that referenced this pull request Nov 22, 2023
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.
@epage epage deleted the pub branch November 23, 2023 01:23
epage added a commit to epage/cargo that referenced this pull request Nov 27, 2023
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.
epage added a commit to epage/cargo that referenced this pull request Nov 27, 2023
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.
epage added a commit to epage/cargo that referenced this pull request Nov 28, 2023
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.
epage added a commit to epage/cargo that referenced this pull request Nov 29, 2023
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.
epage added a commit to epage/cargo that referenced this pull request Nov 29, 2023
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.
bors added a commit to rust-lang/cargo that referenced this pull request Nov 29, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
No open projects
Status: Accepted
Development

Successfully merging this pull request may close these issues.

10 participants