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

[Q&A] Is it possible to target edition=2018 instead? #1699

Closed
Byron opened this issue Apr 18, 2022 · 14 comments
Closed

[Q&A] Is it possible to target edition=2018 instead? #1699

Byron opened this issue Apr 18, 2022 · 14 comments
Labels
question Further information is requested

Comments

@Byron
Copy link

Byron commented Apr 18, 2022

When integrating gitoxide with other packages, some of them set a minimum supported rust version (MSRV), which may also constrain the maximum supported edition. However, when integrating with vergen in particular this now fails only for the reason of requiring a higher edition in windows than is supported by their MSRV.

Currently the windows crates require edition = 2021 and I wonder if it's technically possible to switch them to edition 2018, maybe because no features that would require edition 2021 are actually used.

If edition 2021 is indeed required, this issue can be closed. Otherwise I think there is value in supporting a lower edition for greater compatibility.

Note that gitoxide was setting higher editions and the latest compiler features for no other reason except for 'novelty', but learned why that would be undesirable when integrating with vergen. That's the only reason I post this issue here, there might be a chance :).

Byron added a commit to GitoxideLabs/gitoxide that referenced this issue Apr 18, 2022
Now that the `windows` crate is part of the dependency tree it must
be tested specifically for MSRV support.

Related to microsoft/windows-rs#1699, let's
hope that edition 2018 can be supported or we have to wait.
@kennykerr
Copy link
Collaborator

kennykerr commented Apr 18, 2022

The Rust edition does not impact dependents, only the version itself matters.

Here's the windows crate:

rust-version = "1.59"

And the windows-sys crate:

rust-version = "1.56"

The Rust edition does affect the min version for the crate itself only because older versions of Cargo don't know how to handle newer Rust editions, which seems like an unfortunate oversight.

@kennykerr kennykerr added the question Further information is requested label Apr 18, 2022
@kennykerr
Copy link
Collaborator

So yes, I think it might be possible to use 2018 instead. I switched to 2021 because it simplified a few things and because the docs indicate that interop is seamless. Having switched, I'm not sure what impact it would have to switch back... I'm guessing it will be just as seamless. 😄

@riverar
Copy link
Collaborator

riverar commented Apr 18, 2022

I think the real issue here is that the pipeline in question is using a version of rustc that falls below MSRV. https://github.com/Byron/vergen/runs/6058258829?check_suite_focus=true#step:10:31

Once that's fixed on the vergen side, everything will be good to go. Will close this as there are no changes needed to the windows crate, but feel free to re-open if I missed something!

@riverar riverar closed this as completed Apr 18, 2022
@Byron
Copy link
Author

Byron commented Apr 19, 2022

Thanks for your responses, let me summarize what I understand.

  1. it would indeed be possible to switch back to edition 2018, even though there are concerns about downstream breakage.
  2. everyone is expected to use the latest compiler so that markers for editions are understood. Using an older Rust compiler to verify the MSRV like vergen does is a bug to be fixed.

Regarding 1) and the concern of breakage when switching to 2018, it's my experience that switching back to older editions is never a problem as backwards compatibility is always maintained. From my experience, breakage when doing that is not a concern. From my interactions with the cargo team I myself trust in this assumption.

As I am not an authority in the matter at all let me share some information I gathered while researching point 2 to work up towards my conclusion.

I thought using the winapi crate as research subject might be most fitting as the windows crates probably should replace it one day. winapi is widely adopted so would have to deal with the needs of a lot of downstream packages. How do they deal with the MSRV? It turns out that they do test with toolchains on different versions as well, as stated in their pipeline.

The excerpt below is edited to highlight they test with Rust 1.6.0 as their MSRV.


    runs-on: windows-latest
    strategy:
      matrix:
        toolchain: [1.6.0-x86_64-msvc, nightly-x86_64-msvc, nightly-i686-msvc, nightly-x86_64-gnu, nightly-i686-gnu
]

According to the opinion stated in 2) this might be a bug to fix. If that were true and everybody would use the latest version of Rust, it seems there would be no need for an MSRV either.

My second research subject was cargo-msrv and their cargo msrv book. If somebody goes through the length of creating a tool for checking the MSRV and write documentation in book/manual form, they probably did more research than I would have time for.

The book starts like this, the highlights are mine.

cargo-msrv is a program which can help you find the MSRV for a Rust crate.
MSRV stands for 'Minimum Supported Rust Version', which is exactly what it says on the tin: the earliest Rust release which a given Rust crate promises to support. Most often support for earlier Rust versions is limited by newly introduced Rust language features, library functions or Rust editions.

This again indicates that the MSRV is indeed about the actual Rust version used, while also acknowledging the rust-version field in a later paragraph. This seems to support once more that using an older Rust version is the intended use-case of an MSRV. I couldn't find a crate stating an MSRV without testing it, but that might be confirmation bias.

As gitoxide now is a downstream crate of windows which in turn is downstream of crates with MSRV which do test with older Rustc versions, gitoxide has to play along in order to support them or loose reach by adopting the 'use the latest compiler' of the windows crates.

For me as a maintainer this would mean I have to use the winapi crate as they have a compatible MSRV and test for it in order to support vergen. This, however, is nothing I want to do as I'd want to build on crates directly by the manufacturer if possible, and I know that windows comes with great support, too :).

I hope there is still a chance for windows to adopt an MSRV policy that allows me to use it. Thanks for your consideration.

@kennykerr
Copy link
Collaborator

Thanks for the detailed analysis. To summarize, are you looking for a specific Rust version or specific edition or both?

@Byron
Copy link
Author

Byron commented Apr 19, 2022

Thanks for asking :). In order to support vergen, I'd need the edition lowered to 2018. The rust-version field is ignored so the next test would be to see if Rust 1.54 compiles windows in its current form. If it doesn't, I'd not want to be the one to incur cost of 'downgrading' as you have good reason to require more recent features for sure. I'd rather wait until vergen grows into supporting windows over time. As a matter of fact, my PR to adopt gitoxide is still waiting for feedback.

In the grander scheme, I'd hope that windows can adopt a clear policy about how to handle the MSRV. I suggest to go with the standards set by crates that are highly depended on.

  • test the MSRV on CI with the respective toolchain
  • consider changing the MSRV a breaking change (similar to what winapi does) - otherwise one could lay waste to part of the downstream crates.

Just now I had an interesting conversation with @joshtriplett who is actually knowledgable in the matter (compared to me) and he highlighted that the MSRV of course should be motivated by an actual need. For gitoxide the need is vergen, and to me it ends there as I don't really want to go lower than 1.54, and that assumes that transitively vergen has legitimate needs as well. For windows it's probably a similar balancing act but ultimately there is always the choice of upping the MSRV along with a breaking semver to not be held back forever.

Lastly, let me express my gratitude to all the people involved for participating in this discussion as I think it's valuable and necessary for crates who aspire to be a cornerstone of the crate ecosystem. gitoxide aims to be the better git2 (9m downloads), and I am sure that windows should be replacing winapi one day (48m downloads).

@joshtriplett
Copy link

I agree that whatever the MSRV is declared as needs to be tested in CI, to make sure it actually works. I don't personally tend to consider MSRV changes a breaking change, though; I'm not even sure I agree with the folks who treat it as a minor-semver bump, but I definitely don't think it's a major bump. I've seen a few projects that treat it as a semver-minor bump, but almost no projects that treat it as semver-major.

I do think that it'd be reasonable to check with vergen first to see if their current MSRV is motivated by a specific need/user who requires that version, or if it's just trying to be as minimal as possible for the features they require. If the latter, and they're interested in switching to gitoxide, they might be willing to raise their MSRV to use gitoxide. I'd also check, in general, what their policy for upgrading MSRV is, because you may not want to tie your project to their MSRV requirements if they're much more conservative about MSRV than you want to be.

@kennykerr
Copy link
Collaborator

First up, we do in fact check MSRV during CI. Here's the windows crate:

name: Check windows
runs-on: windows-2019
strategy:
matrix:
rust: [1.59.0, stable, nightly]

And here's the windows-sys crate:

name: Check windows-sys
runs-on: windows-2019
strategy:
matrix:
rust: [1.56.0, stable, nightly]

The rust-version I mentioned above just ensures that compiler errors will be less confusing if an older compiler encounters unrecognized language/library features.

But regarding winapi and Rust 1.6 well that's not something we're interested in going anywhere near. 😄 As I've said before, Rust 1.6 doesn't even support unions which are quite important to the Windows API. The windows crate aims to provide relatively modern Rust bindings.

@Byron
Copy link
Author

Byron commented Apr 19, 2022

I don't personally tend to consider MSRV changes a breaking change, though; I'm not even sure I agree with the folks who treat it as a minor-semver bump, but I definitely don't think it's a major bump.

This is an interesting stance. Particularly because MSRV changes are breaking downstream, so I don't understand why it wouldn't be considered a breaking change. As a library maintainer I think it's of utmost importance to not break downstream whether by own changes or by those more subtle ones that happen easily with the MSRV in conjunction with one's own dependency tree. I don't even claim I have a full grasp on it and how to properly secure downstream, using semver and version pins in my own dependency as as good as it gets. Pinning versions is probably another topic as those also have side-effects downstream, but they don't mean automatic breakage as far as I know.

If the latter, and they're interested in switching to gitoxide, they might be willing to raise their MSRV to use gitoxide. I'd also check, in general, what their policy for upgrading MSRV is, because you may not want to tie your project to their MSRV requirements if they're much more conservative about MSRV than you want to be.

That's true, libraries with MSRV might be a reason for potential adopting libraries to raise their MSRV for that, or to opt-out due to a mismatch in policies.

@kennykerr I am sorry this came across as 'the MSRV is not tested', I didn't check and I didn't assume it. But I did think the MSRV isn't as prominent as I thought it should be for a crate of this caliber. I also don't think a Rust version as old as 1.6 is desirable at all, that field can be left to winapi for sure. As @joshtriplett implied, it's certainly desirable to have a clearly stated MSRV policy so adopters can see if the windows crates suit their needs. Adopters which are application probably don't care, but libraries have to.

Again, for me this means that I hope the edition can be lowered to what's actually, and if it's not feasible that's alright too. Time will bring the windows crate into range. What I'd be interested in knowing is whether MSRV changes are considered breaking or not. If not, this might mean that adopters have to pin their windows version to avoid breakage due to MSRV, also opting out of patches in future.

Whenever the MSRV topic comes up I think it's a very relevant topic to the crate ecosystem, but somewhat underspecified given its importance (especially in terms of breakage that it can cause). I hope also with @joshtriplett 's presence here that this feeling will change over time within the entire community. Maybe, it's just me though 😅.

@riverar
Copy link
Collaborator

riverar commented Apr 19, 2022

@Byron When is vergen/gitoxide going to upgrade from 1.54 (July 29, 2021)?

@Byron
Copy link
Author

Byron commented Apr 19, 2022

@riverar It's really a question for vergen which I didn't ask yet, as it's the sole motivation for gitoxide to strive for that particular MSRV and it was the source of me learning about this entire topic (the hard way) :D. That's probably not the answer you were seeking, so I asked there in case you want to subscribe.

As for gitoxide, I'd like to migrate some more widely used crates myself and improve gitoxide in the process. The MSRV in vergen was entirely accidental but valuable, yet in future I will definitely watch out for this and avoid lowering it. Having too low of an MSRV can probably be an issue as well especially if there are many dependencies of your own (quite an a risk for gitoxide I feel). So my general stance is to generously increase the minor version while in the initial development phase if an MSRV increase is in order.

I hope that helps.

@riverar
Copy link
Collaborator

riverar commented Apr 19, 2022

@Byron Great, thanks! I ask because it seems odd to anchor to that old version. The Rust team does a lot of work to maintain semver, so I find it hard to believe there's some breaking issue in 1.55 and 1.56 preventing upgrade. Maybe a small nudge is all they need!

@Byron
Copy link
Author

Byron commented Apr 19, 2022

It's a good idea to ask the question of what would motivate support for old versions in the first place, which seems to be the premise for this entire thread. It's something I assumed to be the inability to upgrade not because one's concern over the Rust compiler breaking compatibility, but ones inability to upgrade for reasons related to package managers. It's an assumption though, and I'd think one can always upgrade by using rustup.

@joshtriplett
Copy link

Particularly because MSRV changes are breaking downstream, so I don't understand why it wouldn't be considered a breaking change.

Because Rust itself almost never makes breaking changes (other than occasional soundness fixes or exceptionally careful crater-proven changes), precisely so that people can confidently upgrade Rust. A top-level project that doesn't want to use newer versions of Rust can also just not use newer versions of crates; as long as they keep their working Cargo.lock file that worked with that older version, it'll continue to work. If you cargo update, either do a rustup upgrade at the same time, or carefully choose which dependencies you update.

(In general, if there were more people depending on older MSRVs I'd suggest that they develop more tooling to help with the problem of "upgrade to the newest crates that build with the specified MSRV".)

I think at this point it'd be a good idea to wait for vergen to identify why they have their current MSRV and what their policy for updating it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants