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

Amend MSRV policy for special cases to handle forward breakages #3706

Open
sffc opened this issue Jul 19, 2023 · 12 comments
Open

Amend MSRV policy for special cases to handle forward breakages #3706

sffc opened this issue Jul 19, 2023 · 12 comments
Labels
C-meta Component: Relating to ICU4X as a whole needs-approval One or more stakeholders need to approve proposal

Comments

@sffc
Copy link
Member

sffc commented Jul 19, 2023

ICU4X versions 1.0 and 1.1 compile on Rust versions 1.61 through 1.68.2; they break on 1.69.

I would like to amend the MSRV policy to state that we guarantee the existence of a stable MSRV that compiles all versions of ICU4X within a major release stream. This can help with testing of ICU4X across version numbers: you pin Rust once, at the designated version number, and then you can swap out lockfiles to test ICU4X versions.

@sffc sffc added C-meta Component: Relating to ICU4X as a whole needs-approval One or more stakeholders need to approve proposal labels Jul 19, 2023
@Manishearth
Copy link
Member

I'm fine with this policy with the caveat that updating patch releases is fine. Basically, if 1.1 doesn't build on 1.69 but if performing a patch-only lockfile update (1.1 -> 1.1.1, or zerovec 0.5.0 -> 0.5.1, or whatever) that fixes the breakage works, 1.69 counts. This serves the purpose of testing just fine, IMO. We can refine the caveat to say that the patch bump should not contain any behavior changes other than "make it compile" fixes. (other "minor" changes like fixing Cargo.toml features are fine too)

If that caveat is not acceptable I would rather decide this on a per-breakage level rather than a blanket policy: breakages like the 1.69 one ought to be extremely rare (and ideally, completely avoidable), and will have unique per-situation implications.

(for example for the current 1.69 breakage I suspect this policy doesn't affect us at all, since 2.0 is likely to happen before we are able to update past 1.69)

@sffc
Copy link
Member Author

sffc commented Jul 19, 2023

My position is that we should be able to test any lockfile frozen during a major release stream to replicate the behavior at that point in time; allowing patch updates does not enable us to test the world at arbitrary points in time.

@sffc
Copy link
Member Author

sffc commented Jul 19, 2023

I'm fine adopting a special policy that the 1.x stream will never upgrade MSRV beyond 1.68.2 without making a broader policy for future major release streams if we can't agree on the more general policy right now.

@Manishearth
Copy link
Member

allowing patch updates does not enable us to test the world at arbitrary points in time

I think to some extent "the world" is not static, anyone using software has to be aware of their dependencies and when they should be updating things. It is unrealistic for any user of software to want to pin things in perpetuity.

We definitely can take it on ourselves to make that as pleasant as possible, using scoped patch releases. And we should. I would very much like to adopt policies that better define when we want to do that.

Like, the main situation where this happens is soundness fixes in rustc: i do not think we should encourage people to stay on an old rustc with a potentially-broken ICU4X.

(Yes, I'm aware that in this case the rustc fix did not actually deal with soundness issues in ICU4X, but in the "common" case for the hopefully-uncommon event, it will typically be actual soundness fixes, and in future cases where rustc is incorrectly erroring with future-incompat soundness errors on things that are not soundness; i think it is very easy to prevent rustc from making such a breakage)

@sffc
Copy link
Member Author

sffc commented Nov 30, 2023

We are approaching the time when our MSRV policy permits upgrading MSRV beyond 1.68.2. I'm seeking consensus that we do not upgrade MSRV beyond 1.68.2 until 2.0. I am not seeking consensus on any other general policy change, only this one narrow case.

Needs approval from:

@hsivonen
Copy link
Member

hsivonen commented Dec 1, 2023

We are approaching the time when our MSRV policy permits upgrading MSRV beyond 1.68.2. I'm seeking consensus that we do not upgrade MSRV beyond 1.68.2 until 2.0. I am not seeking consensus on any other general policy change, only this one narrow case.

Do we really need to commit to this? 2.0 might turn out to be further away than it seems right now.

@robertbastian
Copy link
Member

This can help with testing of ICU4X across version numbers

Who does this help? We don't do this, who does this?

1.68 and earlier has a soundness bug in the compiler. Why do we need to support it?

I'd be happy with a 1.1.1 patch release that makes ICU4X buildable on 1.69 - today. However, again, who does this help?

@sffc
Copy link
Member Author

sffc commented Dec 1, 2023

The use case is @sven-oly's conformance test suite which tests the output of ICU4X across its version numbers. It wants to install Rust once and then use it to run all versions of ICU4X. (@sven-oly does not currently test 1.1 but we should be able to add it back without complicating the Rust installation.)

@Manishearth
Copy link
Member

I do somewhat consider that an internal use case, so it's something where we have some wiggle room on UX.

However i am still fine with the proposed policy for 1.x.

@robertbastian
Copy link
Member

robertbastian commented Dec 7, 2023

I'd rather publish patch releases to fix 1.0 and 1.1 than to impose restrictions on 1.5. However, there's nothing in 1.69 that we need; 1.70 contains OnceCell, but we'll be approaching 2.0 then anyway.

(this means I agree with #3706 (comment))

@Manishearth
Copy link
Member

I'd rather publish patch releases to fix 1.0 and 1.1 than to impose restrictions on 1.5.

(This is also my position, but yeah I'm fine with this for 1.0)

@robertbastian
Copy link
Member

As of today, 1.70 is n-6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-meta Component: Relating to ICU4X as a whole needs-approval One or more stakeholders need to approve proposal
Projects
None yet
Development

No branches or pull requests

4 participants