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

Specify MSRV and test support for it in CI #479

Closed
link2xt opened this issue Sep 13, 2022 · 9 comments
Closed

Specify MSRV and test support for it in CI #479

link2xt opened this issue Sep 13, 2022 · 9 comments

Comments

@link2xt
Copy link
Contributor

link2xt commented Sep 13, 2022

Currently minimum supported rust version is not specified and bumped without changelog entries.

PR #467 depends on const_fn_trait_bound (rust-lang/rust#93706) feature which was stabilized only in Rust 1.61.0:
https://blog.rust-lang.org/2022/05/19/Rust-1.61.0.html

As a result, quickxml 0.24.0 and 0.25.0 requires Rust 1.61.0, while quickxml 0.23.0 worked with at least Rust 1.56, but this is not reflected in the changelog.

@Mingun
Copy link
Collaborator

Mingun commented Sep 13, 2022

quick-xml does not use const functions in traits, I specially checked that. All consts should work fine on rust 1.41.1 which is effectively our MSRV because of that is MSRV for memchr crate (I've not checked that, but all constness have the same shape that exists in the std since 1.3x.y versions). Do you have any concrete examples where 1.41.1 is not enough?

I'm not against testing MSRV on CI, but I have no strong feeling about the policy. If we need new features, we just use them. After all, why someone should use non-supported compiler when building their apps (all compilers with version <= stable are non-supporting by definition)?

@link2xt
Copy link
Contributor Author

link2xt commented Sep 13, 2022

Example is deltachat/deltachat-core-rust#3599

error[E0658]: trait bounds other than `Sized` on const fn parameters are unstable
  --> .../.cargo/registry/src/github.com-1ecc6299db9ec823/quick-xml-0.25.0/src/writer.rs:62:6
   |
62 | impl<W: Write> Writer<W> {
   |      ^
63 |     /// Creates a `Writer` from a generic writer.
64 |     pub const fn new(inner: W) -> Writer<W> {
   |     --------------------------------------- function declared as const here
   |
   = note: see issue #93706 <https://github.com/rust-lang/rust/issues/93706> for more information

@adamreichold
Copy link
Contributor

Do you have any concrete examples where 1.41.1 is not enough?
I'm not against testing MSRV on CI,

From personal experience, I would say that it is practically impossible to have an MSRV without testing it in the CI and would therefore strongly recommend doing that. Additionally, using the rust-version key in the Cargo.toml manifest is a nice courtesy for downstream projects.

@adamreichold
Copy link
Contributor

(all compilers with version <= stable are non-supporting by definition)?

They may not be supported by the upstream Rust project but for example when building distribution packages, one often targets older Rust versions which are supported by those distributions.

@Mingun
Copy link
Collaborator

Mingun commented Sep 13, 2022

They may not be supported by the upstream Rust project but for example when building distribution packages, one often targets older Rust versions which are supported by those distributions.

That's true for applications, but I don't hear that someone packages libraries distributed as source code and which needs to be compiled. Usually, every second project installs more modern tooling than provided by their distribution and built with them. So when you need to build application for conservative distribution, you just install modern compiler and build with them. Win-win -- your clients use the stable distribution, you use the supported compiler.

@adamreichold
Copy link
Contributor

adamreichold commented Sep 13, 2022

I don't hear that someone packages libraries distributed as source code and which needs to be compiled.

This is exactly how Debian packages Rust crates. The packages are purely used as development dependencies and contain the source code which is used to statically link the leaf packages containing cdylibs or binaries, c.f. https://salsa.debian.org/rust-team/debcargo

So when you need to build application for conservative distribution, you just install modern compiler and build with them.

Not if you want to package that application for the distribution as this means that it must build from source using the toolchain provided by the distribution.

Win-win -- your clients use the stable distribution, you use the supported compiler.

Whether being able to build Rust crates using distribution-provided (or otherwise restrained) toolchain version is something you want to support is of course up to you.

What would be nice in any case, is to make the MSRV explicit by documenting it and if it isn't a trivial policy like always-use-stable, testing it and providing the relevant Cargo metadata.

@link2xt
Copy link
Contributor Author

link2xt commented Sep 13, 2022

Not if you want to package that application for the distribution as this means that it must build from source using the toolchain provided by the distribution.

This is the reason we are conservative about bumping MSRV. We tried to just use latest stable, but then users have problems when they try to just write a PKGBUILD or APKBUILD (Alpine and postmarketOS) with system rust as their build dependency. We don't build with Rust 1.56 ourselves, but it's nice that building just works when users build with any version >=1.56.

@Mingun
Copy link
Collaborator

Mingun commented Sep 14, 2022

Not if you want to package that application for the distribution as this means that it must build from source using the toolchain provided by the distribution.

Very strange restriction with unclear goals, but OK. In any case, it seems to applicable only to some specific binary repositories, because I cannot imagine how this can be enforced, for example, for ppa repositories.

What would be nice in any case, is to make the MSRV explicit by documenting it and if it isn't a trivial policy like always-use-stable, testing it and providing the relevant Cargo metadata.

Agree, PR is welcome. It is not in my priority list for now, so it is unlikely that I'll put my efforts here. I only want to suggest not only set MSRV in Cargo.toml, but also add a build target with -Z minimal-versions and try to relax dependency restrictions in the same PR. MSRV can be also lowered as well if it will require trivial changes, such as deleting const in unsupported places, or replace matches! with ordinary match.

In other cases I would prefer to use power of new versions rather then fanatically keep MSRV as low as possible.

@dralley
Copy link
Collaborator

dralley commented Sep 18, 2022

I can vouch that this is pretty common for any binary-based distribution, Fedora or CentOS for example would have the same issue. All the packages for the distribution are built in an offline buildroot for security reasons, with only sources available, and a standard set of build tooling.

PPAs and COPRs work the same way as far as I can tell. I doubt there is any way to "enforce" it, it will just fail to build.

I also don't want to keep the MSRV crazy low, but 1.56 seems reasonable. And I don't think const is actually buying us much in this context - in most cases it is literally just a struct initialization.

dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 18, 2022
@dralley dralley self-assigned this Sep 18, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 19, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 20, 2022
dralley added a commit to dralley/quick-xml that referenced this issue Sep 20, 2022
It makes the MSRV overly restrictive, and doesn't provide much benefit.
Also add an MSRV check to the CI

closes tafia#479
dralley added a commit to dralley/quick-xml that referenced this issue Sep 20, 2022
It makes the MSRV overly restrictive, and doesn't provide much benefit.
Also add an MSRV check to the CI

closes tafia#479
dralley added a commit to dralley/quick-xml that referenced this issue Sep 20, 2022
It makes the MSRV overly restrictive, and doesn't provide much benefit.
Also add an MSRV check to the CI

closes tafia#479
dralley added a commit to dralley/quick-xml that referenced this issue Sep 20, 2022
It makes the MSRV overly restrictive, and doesn't provide much benefit.
Also add an MSRV check to the CI

closes tafia#479
@Mingun Mingun closed this as completed in b8cdc1f Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants