rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

RFC: Precise Pre-release Deps

Open Stargateur opened this issue 3 years ago • 16 comments

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 https://github.com/Stargateur/semver/commit/c7098f773a38216c135aafb1a4c79ff8b03066a6)

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

Stargateur avatar May 10 '22 06:05 Stargateur

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 avatar May 10 '22 09:05 Stargateur

@djc

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.

I think this needs some clarification: We don't just have limited time to work on new features but to shepherd the work of others:

the rate of new PRs is beyond our capacity to accept at this time.

(from the above linked blog post)

We specifically ask that people not work on something unless there is an Issue and it has been marked as Accepted:

Due to limited review capacity, the Cargo team is not accepting new features or major changes at this time. Please consult with the team before opening a new PR. Only issues that have been explicitly marked as accepted will be reviewed.

(from our contributing guide)

With that said, I would be willing to review @djc's work on this.

epage avatar May 10 '22 14:05 epage

Thanks for the clarification. I'm aware my PR might not get reviewed anytime soon (and thanks for committing to review!).

djc avatar May 10 '22 14:05 djc

Thanks for the clarification. I'm aware my PR might not get reviewed anytime soon (and thanks for committing to review!).

quick caveat: I'm assuming this won't touch the resolver. If it does, then things are different.

epage avatar May 10 '22 15:05 epage

Hmm, my guess is that this would be in the resolver. However, it seems somewhat likely that the moratorium on Cargo team reviews also comprises RFCs, so let's first see how far we get in getting this RFC moving forward.

djc avatar May 10 '22 15:05 djc

I worked on understanding precisely what define Semver and found additional problem about how we handle semver in Cargo. I also dig up more about npm behavior.

I added more weigh to alternative solution cause I think we should seriously consider it.

Stargateur avatar May 10 '22 21:05 Stargateur

All of this make me think we should maybe stop trying to follow npm. If this situation continue I think we should stop using semver specification cause I think they will only reflect what npm do. We should not use a convention just because a lot of javascript code use it if the convention is wrong.

For better or worse, the way semver is currently implemented in Cargo is now widely ingrained in the crates ecosystem, and changing it without breaking backwards compatibility would require a lot of work for probably limited benefit. I would strongly suggest you focus on the incremental improvement for which there is already some support (notably, from the maintainer of the semver crate).

djc avatar May 11 '22 09:05 djc

I'd also add that it looks like refactors were mixed in. I know you were more doing a PoC but it makes it so any of us trying to following along have to examine all of the code to try to figure out what was just a refactor or what actually had logic changes. I gave up on looking over it. Please either don't refactor or use separate commits.

epage avatar May 11 '22 11:05 epage

@djc I have put already > 30 hours into just this RFC. I think I grap most of the problem and see why they try to add such complex rules. Thus I totally disagree with solution they choice I think there is a lot of good information and use case. I decide that I will make a much bigger RFC, that will propose to clearly define what are the rules Cargo will use. Make small step would be a mistake in my opinion, no one want rules to change every time, one (not so big) change is better. I already begun to sum up all points and solution. I think it will require a lot of work for me I expect quite a long RFC, I expect it would takes me at least one week to write it down. I have a very good feeling about this, it will make the situation way better at the end. The new RFC would propose to close this one if accepted.

@epage yes sorry, I will try to fix it, it was very difficult for me to understand previous code, but I think the most important relevant change is the test file, where I try to keep change at the minimum.

Stargateur avatar May 11 '22 12:05 Stargateur

I have put already > 30 hours into just this RFC

I think I may need to create a bigger new RFC that would fix all of this

Some frank feedback from me as semver crate maintainer, to maybe save you from wasting too much time: the big RFC you've been outlining above and in other GitHub issues has 0 chance of being accepted. The prerelease-related behaviors you're objecting to are mostly exceedingly well justified; https://github.com/semver/semver/pull/584#issuecomment-1123740758 is a coherent explanation of why semver ranges are intentionally designed this way.

My recommendation for an RFC would be to stick to https://internals.rust-lang.org/t/changing-cargo-semver-compatibility-for-pre-releases/14820/10?u=dtolnay which I think has a decent chance of being accepted and would be an improvement to the Cargo ecosystem, in my opinion.

dtolnay avatar May 11 '22 15:05 dtolnay

The prerelease-related behaviors you're objecting to are mostly exceedingly well justified

I didn't say they weren't I say the solution is bad.

Some frank feedback from me as semver crate maintainer, to maybe save you from wasting too much time: the big RFC you've been outlining above and in other GitHub issues has 0 chance of being accepted.

Please refrain from such declaration, your voice weigh too much, let me I least try, here you give me no chance it's really annoying. I put a lot of effort into this. At least wait to read the RFC I will propose.

Stargateur avatar May 11 '22 16:05 Stargateur

I also don't think this RFC is necessary. The main issue called out in the RFC is this:

The current behavior break pre-release build, here a no exhaustive list:

ie. that people write foo = "1.0.0.alpha-0" and accidentally opt into future upgrades.

From skimming the various threads, this also seems to be the main complaint with the current behaviour.

If this is really a footgun for users, then the obvious fix would be to warn or disallow this. In other words, if the user puts foo = "1.0.0.alpha-0" in their Cargo.toml, then a warning is issued to either write foo = "=1.0.0.alpha-0" or foo = "^1.0.0.alpha-0" to retain the old behaviour.

The warning would not be issued for dependencies, and newer versions of Cargo could prevent publishing with such an ambiguous dependency string.

Diggsey avatar May 11 '22 16:05 Diggsey

If this is really a footgun for users, then the obvious fix would be to warn or disallow this. In other words, if the user puts foo = "1.0.0.alpha-0" in their Cargo.toml, then a warning is issued to either write foo = "=1.0.0.alpha-0" or foo = "^1.0.0.alpha-0" to retain the old behaviour.

When clap 3 was being worked on, all version requirements in the workspace had to be switched to = and then all switched back. This might seem small but its enough of a pain, especially when already automating the release process that I'm avoiding pre-release versions as a whole.

It might be said "the process is automated, just automate it" but cargo release won't know which = version reqs are for pre-release and which ones are wanted always so it can't automatically remove them.

When talking about adding cargo add to cargo, we had decided to remove any pre-release features because we felt there were bigger issues with pre-release in cargo that needed addressing before we tried making it easy to deal with pre-releases in cargo add.

epage avatar May 11 '22 16:05 epage

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.

Stargateur avatar May 14 '22 06:05 Stargateur

Shouldn't -rc prereleases be upgradable to releases, unlike -alpha or -beta?

vi avatar May 14 '22 13:05 vi

@vi Please keep comment for the other RFC on the forum, I don't want to spam this thread with other RFC. You can use your github account to log in in the internal forum. Thanks for the comment, I recommand you to post it again on the forum.

Stargateur avatar May 14 '22 14:05 Stargateur

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

ehuss avatar Nov 26 '22 22:11 ehuss

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

epage avatar Nov 27 '22 02:11 epage

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 https://github.com/rust-lang/cargo/issues/2222#issuecomment-174084275 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?

ehuss avatar Nov 29 '22 14:11 ehuss

As this conversation is broader than this RFC, I'm going to reply over at https://github.com/rust-lang/cargo/issues/2222

epage avatar Nov 29 '22 20:11 epage

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

joshtriplett avatar Dec 12 '23 15:12 joshtriplett

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

  • [ ] @Eh2406
  • [x] @Muscraft
  • [ ] @arlosi
  • [x] @ehuss
  • [x] @epage
  • [x] @joshtriplett
  • [x] @weihanglo

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 avatar Dec 12 '23 15:12 rfcbot

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

rfcbot avatar Dec 12 '23 16:12 rfcbot

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 avatar Dec 22 '23 16:12 rfcbot