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

Add an MSRV policy to the README #3046

Merged
merged 6 commits into from
Sep 8, 2023
Merged

Add an MSRV policy to the README #3046

merged 6 commits into from
Sep 8, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Aug 25, 2023

As discussed in #3000, this adopts an official MSRV policy for the rust-windowing organization. It uses Debian Testing as the definition of our support. In the future once winit becomes more stable we might want to use Debian Stable instead, in order to allow Debian to use crates like Alacritty in the stable channel.

Outstanding questions:

  • What are the general thoughts on the organizational policy?

  • Does this give us enough leeway to add new features?

  • Are there any other Linux distros that we need to support?

  • Tested on all platforms changed

  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users

  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior

  • Created or updated an example program if it would help users understand this functionality

  • Updated feature matrix, if new features were added or implemented

@notgull notgull requested a review from daxpedda August 25, 2023 01:26
@ids1024
Copy link
Member

ids1024 commented Aug 25, 2023

When does Debian Testing update their Rust package? Maybe updated when something like Firefox requires a new Rust version? (Updating it for a new Firefox version is what Ubuntu was doing; though they've moved to a snap for Firefox.)

If Debian Testing ever updates to the latest Rust release, that would not be a useful MSRV policy, except for users of Debian Testing.

@notgull notgull requested a review from rib August 25, 2023 01:39
@kchibisov
Copy link
Member

I agree that the wording is a bit confusing, I'd suggest to pick a minimal version across the distros packaging winit software, which are not out of date, which means that we target Sid and not Testing in the first place. The same will work with e.g. fedora, like recently released fedora should have a needed version (which is true most of the time these days).

@daxpedda
Copy link
Member

When does Debian Testing update their Rust package? Maybe updated when something like Firefox requires a new Rust version? (Updating it for a new Firefox version is what Ubuntu was doing; though they've moved to a snap for Firefox.)

If Debian Testing ever updates to the latest Rust release, that would not be a useful MSRV policy, except for users of Debian Testing.

I think the wording is fine, because we aren't committing to actually update the MSRV when Debian does, we are just committed to not go further (with an exception).

I do agree with @kchibisov though that it would be more useful to specify more distros then just Debian, preferably ones that actually package Winit.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I think we should also specify a hard policy when we update the MSRV in the context of versioning.

E.g.:

A change will be accompanied by a minor version bump.

We can revisit if we want to require a major version bump for MSRV changes when we reach v1.0.

@ids1024
Copy link
Member

ids1024 commented Aug 25, 2023

I think the wording is fine, because we aren't committing to actually update the MSRV when Debian does, we are just committed to not go further (with an exception).

Such a policy doesn't require updating the MSRV. But if it still has to be held back for other unwritten reasons (other distros, wanting to limit to no more than nth most recent release, etc.) the policy is incomplete and not effectively serving it's purpose.

@notgull
Copy link
Member Author

notgull commented Aug 25, 2023

Such a policy doesn't require updating the MSRV. But if it still has to be held back for other unwritten reasons (other distros, wanting to limit to no more than nth most recent release, etc.) the policy is incomplete and not effectively serving it's purpose.

What other unwritten reasons would there be? IIRC Debian Testing should be a good low water mark for now.

@kchibisov
Copy link
Member

I'd still probably pick Sid, like debian is a bit hard for brand new desktop, and testing lags behind a lot, we already violate its minimum version, unless they've bumped it recently. Sid is like the only thing we could realistically target.

README.md Outdated Show resolved Hide resolved
@ids1024
Copy link
Member

ids1024 commented Aug 25, 2023

What other unwritten reasons would there be?

As I said, what package is in another distribution, or not require the latest n releases.

I don't follow exactly how Rust is updated in Debian (even if I used Debian, I probably wouldn't, unless I were packaging for Debian). But looking at https://wiki.debian.org/DebianTesting:

Packages from Debian Unstable enter the next-stable testing distribution automatically, when a list of requirements is fulfilled:

  • The package has been in "unstable" at least for 2-10 days (depending on the urgency of the upload).
  • The package has been built for all the architectures which the present version in testing was built for.
  • Installing the package into testing will not make the distribution more uninstallable.
  • The package does not introduce new release critical bugs.

So when Rust is updated in sid, it may be updated in testing as well within 2-10 days if there are no issues?

When Rust is updated in sid, I'd presume they update to the latest release? It seems odd to specifically update to an already old Rust release.

So by this logic (correct me if anyone knows better) I'd expect the Rust version in Debian testing to sometimes be the latest Rust release, if Debian has updated the package recently. Which isn't consistently helpful as an MSRV policy for someone not using Debian.

@kchibisov
Copy link
Member

We could do basically min(stable - 2, debian_sid). Or stable - 3.

@notgull
Copy link
Member Author

notgull commented Aug 26, 2023

In my memory I don't think that Sid chooses recent versions of Rust. I'm fine with an n - 3 approach for now, but I think we should aim to be more conservative, as it helps for adoption in the future, especially by distros.

@kchibisov
Copy link
Member

I'm fine with an n - 3 approach for now, but I think we should aim to be more conservative, as it helps for adoption in the future, especially by distros.

I haven't said n - 3 though, I said min(n - 3, sid), so if sid is at 1.66, but stable at 1.71, we pick 1.66.

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Please setup your editor to wrap markdown files, because it's not the first time when your markdown changes are not wrapped.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
notgull and others added 5 commits August 27, 2023 11:38
As discussed in #3000, this adopts an official MSRV policy for the
rust-windowing organization. It uses Debian Testing as the definition of
our support. In the future once winit becomes more stable we might want
to use Debian Stable instead, in order to allow Debian to use crates
like Alacritty in the stable channel.

Outstanding questions:

- What are the general thoughts on the organizational policy?
- Does this give us enough leeway to add new features?
- Are there any other Linux distros that we need to support?

Signed-off-by: John Nunley <[email protected]>
- Use Debian Sid instead of Debian Testing
- Explicitly specify version bump strategy.

Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Besides, maybe we should remove the rust_version field if we split things like that? Or maybe have it dynamic, as we write it on CI, but won't have it in the Cargo.toml otherwise we'll still cap android in some cases?

Comment on lines +44 to +50
exclude:
# Android is tested on stable-3
- toolchain: '1.65.0'
platform: { name: 'Android', target: aarch64-linux-android, os: ubuntu-latest, options: '--package=winit --features=android-native-activity', cmd: 'apk --' }
include:
- toolchain: '1.69.0'
platform: { name: 'Android', target: aarch64-linux-android, os: ubuntu-latest, options: '--package=winit --features=android-native-activity', cmd: 'apk --' }
Copy link
Member

Choose a reason for hiding this comment

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

If you want to special case android like that, you must tell cargo to ignore rust_version in the Cargo.toml, otherwise it won't build iirc even when version is good enough. The flag is --ignore-rust-version.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just change the rust-version field to the Android MSRV? Still useful I would argue.

Copy link
Member

Choose a reason for hiding this comment

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

Ah wait, that would prevent lower version from working as well ...
Yeah, I'm in favor of removing it then.

Copy link
Member

Choose a reason for hiding this comment

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

Actually no ... sorry, it's still useful, just a lower bound is better then nothing.
Apologies, I got confused here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @daxpedda, as it causes a compile error rust-version serves best as a lower bound for the MSRV.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have a rust-version per target and not global? I think it's global, no? That's why I've suggested to sort of dynamically add it into the toml or maybe somehow fix the version....

Copy link
Contributor

Choose a reason for hiding this comment

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

It's always fine to build with a newer version of Rust, and there's no need to use --ignore-rust-version.

--ignore-rust-version would be applicable for the opposite situation where you know that Winit technically can be built with an even older Rust version than is specified in the rust-version.

That said though - removing the rust-version sounds good to me and Winit can instead just rely on CI to check the MSRV because that gives more flexibility to test different versions for different platforms.

Cargo will anyway calculate the real minimum rust-version based on the minimum rust-version across the dependencies selected for a specific platform at build time.

Copy link
Member

Choose a reason for hiding this comment

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

The thing about rust-version is that some crate might force MSRV larger than it can be built with. I guess I just forgot how rust-version works exactly, and if you can build just fine when there's incompat between rust versions, then it sounds fine the way it's now.

README.md Outdated Show resolved Hide resolved
@rib
Copy link
Contributor

rib commented Sep 6, 2023

I think the last issue raised by @kchibisov can be marked as resolved, based on the discussion that followed, right?

It seems like this might be ready to merge?

It would be great to land this and enable the android-activity + ndk crates to bump their rust-version.

@rib
Copy link
Contributor

rib commented Sep 6, 2023

Oh actually the MSRV policy currently says "Changes to the MSRV will be accompanied by a minor version bump."

That doesn't sound desirable. I think Rust projects are generally moving away from trying to conflate the semver and rust-version - it doesn't really scale well to try and encode rust-version bumps in the semver.

The rust-version is related to build tooling requirements, the semver is about the compatibility of your public API - they are two very separate things.

I assume 'minor' version here effectively intends to say that a rust-version bump would be considered a breaking change (from the pov that winit's version is below 1.0) but I think it should be fine to do a patch release that updates the msrv assuming that it still abides by the msrv policy at the time that the change is made. E.g. imagine doing a security fix for an older version of Winit.

@kchibisov
Copy link
Member

kchibisov commented Sep 6, 2023

@rib yeah, but end binaries can't use patch versions when the msrv doesn't match, this is not how packaging works.

So for example, when winit changes the msrv in patch version, alacritty won't be able to use it, because distros won't be able to package it.

@daxpedda
Copy link
Member

daxpedda commented Sep 6, 2023

@rib we have discussed on why we don't want to break MSRV in patch versions here: #3046 (comment).

@rib
Copy link
Contributor

rib commented Sep 6, 2023

So for example, when winit changes the msrv in patch version, alacritty won't be able to use it, because distros won't be able to package it.

The distros will be able to package it though because the msrv is still in line with what Debian requires, so there would only be a bump on a minor version if it would be compatible with Debian.

@rib
Copy link
Contributor

rib commented Sep 6, 2023

@rib we have discussed on why we don't want to break MSRV in patch versions here: #3046 (comment).

Please e.g. see:

https://users.rust-lang.org/t/rust-version-requirement-change-as-semver-breaking-or-not/20980/39

and

rust-lang/api-guidelines#231

In particular:

The API guidelines tentatively suggest that, for libraries, an MSRV increase should
not be considered a semver-breaking change. Specifically, when increasing
MSRV:

for crates past 1.0.0, increment minor version (1.1.3 -> 1.2.0),
for crates before 1.0.0, increment patch version (0.1.3 -> 0.1.4).

This reduces the amount of ecosystem-wide work for MSRV upgrades and prevents incompatibilities. It also is a de-facto practice for many cornerstone crates. This policy gives more power to library consumers to manually select working combinations of library and compiler versions, at the cost of breaking cargo update workflow for older compilers.

I think it would be a mistake for Winit to fall into the trap of trying to reflect rust-version bumps as breaking semver changes

@daxpedda
Copy link
Member

daxpedda commented Sep 6, 2023

I've honestly not read through all of that, but clearly this is a very contested topic, otherwise there would be an official recommendation already, which there isn't.

I agree that we should not use breaking semver changes for MSRV bumps, but in versions below v1 it's not an option because it doesn't allow for MSRV patches, which again, is a sentiment many people share in both links you have shared.

I'm strongly against breaking MSRV in patch versions because of this and I think practically this is not an issue for Winit. We don't really need to break MSRV below v1 in patch versions because we have a regular release cycle until we get to v1 anyway. In v1 we can break MSRV at minor versions, so we don't have this issue then anyway.

@rib
Copy link
Contributor

rib commented Sep 6, 2023

I'm not sure that there's necessarily any issue with bumping the patch version when below 1.0.

The tentative guideline quoted above even gives an example of bumping the patch version for an msrv bump when below 1.0:

for crates before 1.0.0, increment patch version (0.1.3 -> 0.1.4).

I don't even see an issue with bumping the MSRV in a patch version when > 1.0 - the two version numbers can be considered orthogonal but the semver patch has to at least bump to make a new crate release.

It doesn't really seem like it's that contentious - it just feels like one of those things where it initially seems like a good idea to try and reflect msrv bumps in the semver but over time I think the ecosystem is realizing, from having tried that, that in practice that idea doesn't really work out very well.

@daxpedda
Copy link
Member

daxpedda commented Sep 6, 2023

To clarify what the issue is: you can't supply patches to people relying on MSRV.

I have outlined this in detail: #3046 (comment).

Just to take your own references: #1, #2, #3, #4, #5, #6.

It doesn't really seem like it's that contentious - it just feels like one of those things where it initially seems like a good idea to try and reflect msrv bumps in the semver but over time I think the ecosystem is realizing, from having tried that, that in practice that idea doesn't really work out very well.

The main point here is that the problem with requiring breaking change releases to upgrade MSRV is that every library relying on that library has to go upgrade and I absolutely agree, this has been incredibly problematic in the past and many big crates have changed their policy because of this.

But let's address this point practically instead of theoretically: Winit doesn't have this problem. We don't really need to break MSRV in patch versions, the only time we release patch versions is to fix issues. Post v1, I assume, we won't want to break the API that often, so in that case we can break MSRV in minor versions, so we don't hit the problem that the ecosystem has suffered from in the past.

@kchibisov
Copy link
Member

It doesn't really seem like it's that contentious - it just feels like one of those things where it initially seems like a good idea to try and reflect msrv bumps in the semver but over time I think the ecosystem is realizing, from having tried that, that in practice that idea doesn't really work out very well.

We're talking about patch version bumps, it's true that before 1.0 it's a bit weird, but the thing is that we need to somehow deliver patch updates. Your software could be packaged with the rust version which won't be updated on the stable distro and you can't do anything about it, so your critical patch might not be delivered at all, because winit changed msrv and that distro can't package things anymore.

You're probably not aware, but I do backport fixes and resolve msrv conflicts when winit has a patch that doesn't build with the patch version of msrv, it's not a big deal, and that's how it was working for a long time.

@kchibisov
Copy link
Member

kchibisov commented Sep 7, 2023

Probably would need to link other crates to MSRV policy of winit.

@ids1024
Copy link
Member

ids1024 commented Sep 7, 2023

You're probably not aware, but I do backport fixes and resolve msrv conflicts when winit has a patch that doesn't build with the patch version of msrv, it's not a big deal, and that's how it was working for a long time.

I guess that could be documented here too?

@rib
Copy link
Contributor

rib commented Sep 7, 2023

But let's address this point practically instead of theoretically: Winit doesn't have this problem. We don't really need to break MSRV in patch versions, the only time we release patch versions is to fix issues.

I'm not really sure why Winit would be different here. It's a bit higher up the stack compared to many crates but it's still middleware and Winit has pretty long development cycles between major versions whereby it seems very plausible that it could make sense to want to be able to bump the msrv as part of a patch release.

Your software could be packaged with the rust version which won't be updated on the stable distro and you can't do anything about it, so your critical patch might not be delivered at all, because winit changed msrv and that distro can't package things anymore.

The issue of being compatible with stable distros like Debian is covered by the MSRV policy. The assumption is that if the msrv were bumped in a patch release of winit then it would still be abiding by the MSRV policy that ensures it's compatible with distros.

Winit 0.28 was released 7 months ago and in that kind of time frame Debian can pull in a newer version of Rust and a patch release of Winit could updates it's msrv and still be compatible with Debian.

@daxpedda
Copy link
Member

daxpedda commented Sep 7, 2023

But let's address this point practically instead of theoretically: Winit doesn't have this problem. We don't really need to break MSRV in patch versions, the only time we release patch versions is to fix issues.

I'm not really sure why Winit would be different here. It's a bit higher up the stack compared to many crates but it's still middleware and Winit has pretty long development cycles between major versions whereby it seems very plausible that it could make sense to want to be able to bump the msrv as part of a patch release.

Nobody is proposing to only break MSRV on major versions, we are talking about minor versions. The development cycles aren't long enough between minor versions, but we didn't exactly define how long "long" is, so this is just subjective. Practically, I think this never happened before and so I'm arguing we won't need it in the future as well.

Another thing that I already pointed out as well, is a reason why this most likely wasn't needed in the past: we don't actually continue to develop Winit in patch versions, we only backport fixes and fix bugs. It's why I'm saying we should take a practical approach to this because otherwise we are getting lost in speculation. My assumptions here are built on what we have done in the past, so I think this is at least less speculative.

@notgull
Copy link
Member Author

notgull commented Sep 8, 2023

I would personally err more on the side of using a patch version in this case. However, if Alacritty has requirements that prevent us from using that strategy (as I infer from discussion above), then I'm fine with using minor version bumps.

@kchibisov
Copy link
Member

Winit 0.28 was released 7 months ago and in that kind of time frame Debian can pull in a newer version of Rust and a patch release of Winit could updates it's msrv and still be compatible with Debian.

It doesn't matter what debian will update to. When an application releases, it could become a part of the stable distro cut (assume fedora XX or ubuntu XX.04), such distros tend to keep the version of each package as it was when it was released, bumping only patch version, that means that rust might not be ever updated on them, thus the patch would never be delivered.

I'm still not sure what is the issue here, winit was doing this policy implicitly for the last years without any issue. It's not like we come up with something new, it's basically the documentation on how it was. Making it more loose when the current/proposed policy already works doesn't sound healthy.

@rib
Copy link
Contributor

rib commented Sep 8, 2023

Please just take my comment for all it was: feedback on a written MSRV policy for Winit.

Imho, ideally the policy wouldn't stipulate anything about requiring semver bumps for msrv bumps - that essentially just my personal opinion but I tried to share some discussions around this topic for context.

I don't really have a strong opinion on what written policy Winit adopts, except for the clarification that the strict rust-version isn't going to be enforced in CI for the Android backend.

I'd be happy with this landing as is if others feel it's worth keeping the comment about minor version bumps.

@daxpedda daxpedda merged commit c00c1e9 into master Sep 8, 2023
50 checks passed
@daxpedda daxpedda deleted the notgull/msrv branch September 8, 2023 15:34
kchibisov pushed a commit to kchibisov/winit that referenced this pull request Oct 17, 2023
kchibisov pushed a commit that referenced this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants