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: Precise Pre-release Deps #3263

Closed
wants to merge 8 commits into from
Closed

Conversation

Stargateur
Copy link

@Stargateur Stargateur commented May 10, 2022

Rendered

Internals thread

I think there is two major solution either:

  • Cargo will not use the more compatible version for pre-release by default, this mean that 1.0.0-alpha.0 will be interpreted as =1.0.0-alpha.0 by cargo instead of ^1.0.0-alpha.0.
  • We could decide that since Semver doesn't specify compatible rule for pre-release and say there is no compatible version for pre-release. 1.0.0-alpha.0 would never match any other requirement that exact same version. That mean that ^1.0.0-alpha.0 could only match 1.0.0-alpha.0 version. This have the major benefit to not introduce inconsistency with pre-release and release in Cargo resolve. It's also make a lot of sense why pre-release would have any compatibility expectation ? (attempt to implement it Stargateur/semver@c7098f7)

The RFC 3266 is some sort of alternative much deeper solution.

@Stargateur Stargateur force-pushed the master branch 2 times, most recently from 2bc7c7e to a7554f1 Compare May 10, 2022 06:33
@Stargateur Stargateur force-pushed the master branch 2 times, most recently from a827a64 to 471b7e8 Compare May 10, 2022 06:44
@Stargateur Stargateur changed the title Pre Release Sticky RFC: Pre Release Sticky May 10, 2022
@Stargateur Stargateur force-pushed the master branch 11 times, most recently from 46e8542 to 3254a5e Compare May 10, 2022 07:56
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this, would be great to see some progress here.

That said, be aware that the Cargo team recently announced that they don't have a lot of time/energy to work on new features.

That said, if this is accepted I would be willing to implement it in Cargo.

text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
text/3263-pre-release-sticky.md Outdated Show resolved Hide resolved
@Stargateur Stargateur force-pushed the master branch 4 times, most recently from 2f0b059 to 14f20e9 Compare May 10, 2022 09:35
@Stargateur
Copy link
Author

Stargateur commented May 10, 2022

Thank a lot for the good advice, I marked resolved obvious thing but let you mark as resolve for other points if you think there are resolve. Or just make another review ^^.

I think I improve a lot the RFC, for mistake spelling in English I suggest to send me a PR on my fork, cause I expect there is a lot of them. Or send me the fixed file and I will fix the PR myself.

@Stargateur Stargateur force-pushed the master branch 3 times, most recently from 637fcc8 to afe1d7c Compare May 13, 2022 04:44
@Stargateur
Copy link
Author

Stargateur commented May 14, 2022

Related RFC: https://internals.rust-lang.org/t/rfc-rust-semver-2-alpha/16632 if you are interested. If you want to post a comment to critic the linked RFC, please, use the forum, thanks in advance. This thread is more focus for the RFC: Precise Pre-release Deps, however you can comment here about on what solution is the better in your opinion.

@vi

This comment was marked as off-topic.

@Stargateur

This comment was marked as off-topic.

@Stargateur Stargateur mentioned this pull request May 17, 2022
@ehuss
Copy link
Contributor

ehuss commented Nov 26, 2022

The Cargo team discussed this RFC, and we think the following may be a viable path forward:

  1. Generate a warning if a dependency version has a pre-release version without a SemVer operator. For example, "2.0.0-beta.1" would generate a warning. The warning can be silenced by using an explicit operator like "=2.0.0-beta.1". The warning should recommend the = form. This is to make it a more explicit and conscious choice on how to use pre-releases.
  2. In the next Edition, Cargo will require prereleases to have an operator (changes the warning to an error).

There are some risks and drawbacks with this approach that may need some mitigations:

  • Users may get stuck in a scenario where they cannot update Cargo.lock because multiple packages are using a shared dependency with = pre-release versions, and they are out of sync. Using pre-release dependencies can be an inherently risky thing to do, so hopefully they are not used pervasively enough that it affects shared dependencies too often. I also feel that usage of pre-releases should be a temporary thing that should hopefully not be used for too long.

  • When using = dependencies, users are unable to force an update of a transitive dependency, even when they know it is compatible and safe. The package using the = dependency will need to make a new release to grab any new versions, and this can introduce significant friction and lag that can make using pre-releases frustrating. I suspect it is unlikely Cargo will gain support for some kind of forced-override in the foreseeable future, so this leaves the burden on users to publish new versions to stay up-to-date with new pre-releases (and switching to the final release once it leaves the pre-release stage).

  • Updating Cargo.toml to pick up a new prelease can be a tedious process without any sort of tooling support. cargo update currently doesn't provide any help here. I'm personally hopeful that cargo update could gain some kind of flag to also update Cargo.toml as discussed at https://internals.rust-lang.org/t/feedback-on-cargo-upgrade-to-prepare-it-for-merging/17101. This may also require another flag to force the = upgrade.

  • This will reduce the visibility of the availability of new pre-releases, which could normally be discovered with cargo update. I'm hopeful that maybe something like cargo outdated could be upstreamed to make it easier to see when newer versions are available.

  • cargo add should probably force the use of = (either implicitly add it, or generate an error telling the user to add it) when adding pre-releases.

  • We also discussed the possibility of more strongly encouraging users to mark their package as a pre-release if they have any pre-release dependencies. This could be done with a warning message, or possibly a warning during the cargo publish phase, or perhaps just more strongly worded recommendations in the documentation. I'm somewhat uncertain about this particular point, and I'm not sure it is something to gate the RFC on, but I think would be worth mentioning as something to explore (or at least have more discussion here).

@epage
Copy link
Contributor

epage commented Nov 27, 2022

@ehuss my main concern with requiring an operator is scalability as it can't be automated. Take cargo-release of clap. I can make cargo release automatically add a = on switching to a pre-release but when doing the official release, I have no idea what the intended operator is. clap needs to use = on clap_derive independent of pre-release while I use the implicit operator for everything else. I'd need to go through and individually set the version requirement in each case. This is a 7 crate workspace. I would expect there are larger ones that would like to use pre-release. I feel like this creates enough friction that pre-release will continue to not be practical in cargo.

@ehuss
Copy link
Contributor

ehuss commented Nov 29, 2022

Yea, that's another drawback where the switch is lossy. A few thoughts on that:

  • If using workspace inheritance, there should only be one place to fix these.
  • Presumably publishing pre-releases is a relatively rare event, so I wouldn't expect it to happen too often.
  • If automation is important, it may be possible to add some information on how to "revert" the change. For example it could add a comment, maybe something like:
    foo = { path = "foo", version="=2.0.0-alpha.1" } # cargo-release: pre-release-keep-equals
  • I think it might be good to more seriously consider the "channels" concept discussed at Pre-release version numbers cargo#2222 (comment) and similar to what is mentioned in the "Future possibilities" in this RFC. That is, a pre-releases will be considered compatible if the first component is the same, but incompatible otherwise (2.0.0-alpha.2 is compatible with 2.0.0-alpha.1, but not 2.0.0-beta.1). That would avoid the need for doing something like =, and provide an avenue for authors to make breaking changes. I think that could still be challenging, since many authors currently treat pre-releases as completely unstable, and won't know about these compatibility requirements which would be somewhat unique to cargo. Perhaps that could be alleviated if cargo gains something like semver checking during publish?

@epage
Copy link
Contributor

epage commented Nov 29, 2022

As this conversation is broader than this RFC, I'm going to reply over at rust-lang/cargo#2222

@joshtriplett
Copy link
Member

We discussed this at length in today's @rust-lang/cargo meeting. We felt that while the current pre-release behavior is really not ideal, changing the interpretation of ^ to have the same meaning as = would potentially break existing packages.

Right now, our current inclination is to add a lint on pre-release dependencies without an operator, hinting that people might want to use = or a range with pre-release dependencies. We can also work on defining ^ in a more reasonable manner in parallel, but that's a more complex step and not as high on the priority list.

Meanwhile, we're going to postpone this.

@rfcbot postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 12, 2023

Team member @joshtriplett has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Dec 12, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 12, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@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 Dec 22, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 22, 2023

The final comment period, with a disposition to postpone, 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 is now postponed.

@rfcbot rfcbot added postponed RFCs that have been postponed and may be revisited at a later time. and removed disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Dec 22, 2023
@rfcbot rfcbot closed this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
No open projects
Status: Postponed
Development

Successfully merging this pull request may close these issues.

10 participants