Skip to content

Specify MSRV and test support for it in CI #479

Closed
@link2xt

Description

@link2xt
Contributor

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.

Activity

Mingun

Mingun commented on Sep 13, 2022

@Mingun
Collaborator

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

link2xt commented on Sep 13, 2022

@link2xt
ContributorAuthor

Example is chatmail/core#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

adamreichold commented on Sep 13, 2022

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

adamreichold commented on Sep 13, 2022

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

Mingun commented on Sep 13, 2022

@Mingun
Collaborator

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

adamreichold commented on Sep 13, 2022

@adamreichold
Contributor

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

link2xt commented on Sep 13, 2022

@link2xt
ContributorAuthor

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

Mingun commented on Sep 14, 2022

@Mingun
Collaborator

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

dralley commented on Sep 18, 2022

@dralley
Collaborator

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.

added 6 commits that reference this issue on Sep 18, 2022
6dddefb
59271fd
b96954b
61c3673
a73a9c3
865dc63
self-assigned this
on Sep 18, 2022
added 7 commits that reference this issue on Sep 19, 2022
18e62b8
8da81e0
0fac212
9f01260
44fb918
0b62061
b8cdc1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @Mingun@dralley@adamreichold@link2xt

      Issue actions

        Specify MSRV and test support for it in CI · Issue #479 · tafia/quick-xml