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

Rust 1.48 support broken by resolver field in packaged Cargo.toml #2255

Closed
davidhewitt opened this issue Aug 9, 2022 · 16 comments
Closed

Rust 1.48 support broken by resolver field in packaged Cargo.toml #2255

davidhewitt opened this issue Aug 9, 2022 · 16 comments

Comments

@davidhewitt
Copy link

I have a failure in PyO3 CI which I can also reproduce locally.

error: failed to download `serde v1.0.143`

Caused by:
  unable to get packages from source

Caused by:
  failed to parse manifest at `/Users/david/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/serde-1.0.143/Cargo.toml`

Caused by:
  feature `resolver` is required

Looking at the unpacked Cargo.toml it contains resolver = "1" - I wonder if the cargo package process has somehow added this and broken support for older cargo versions?

@striezel
Copy link
Contributor

striezel commented Aug 9, 2022

Encountered the same issue in one of my projects.

Considering that serde 1.0.142 still worked with rustc 1.48 and seeing that serde 1.0.143 does not, let's take a look at the changes between those two versions: v1.0.142...v1.0.143
There is no change that adds resolver = "1" to the manifest in the code, so I guess this is probably an issue with packaging process. 🤔

striezel added a commit to striezel/corona that referenced this issue Aug 9, 2022
The serde 1.0.143 crate does not build on this project's current
MSRV (rustc 1.48), so it's reverted to the previous version.

See <serde-rs/serde#2255> for the
corresponding upstream issue.
@davidhewitt
Copy link
Author

Looks like this is probably rust-lang/cargo#10954

@TheBlueMatt
Copy link

According to that issue its not in stable, though? So presumably whoever does serde uploads needs to just bump the version and do a re-upload using stable cargo?

@TheBlueMatt
Copy link

I followed up with @dtolnay on twitter DM about this, and his response was that there is no MSRV policy for serde, and thus this is an issue that is not likely to be fixed. I believe this issue can be closed.

@davidhewitt
Copy link
Author

Since serde's own metadata claims to support 1.13+, I'll leave this open until the next serde release is pushed. Should help community members who hit this in the meanwhile.

@TheBlueMatt
Copy link

Yes, his comment to me, specifically, was:

the number in the readme is descriptive, not a policy; i'll update it

@striezel
Copy link
Contributor

striezel commented Aug 11, 2022

I followed up with @dtolnay on twitter DM about this, and his response was that there is no MSRV policy for serde, and thus this is an issue that is not likely to be fixed.

Hmm ... I find that a bit odd. 🤷 Sure, the readme could be argued about. But when even the cargo manifest shows something as rustc 1.13+, only to find out it actually requires something more like rustc 1.50+ when used in a project feels a bit like false advertising to me. So either the readme and manifest should be updated, or the crate should be rebuild with stable cargo which does not have that bug. There even is a dedicated CI job named msrv that tests with rustc 1.13. That does not feel like "there is no MSRV policy for serde" to me. 😕

I am aware that this may sound a bit harsh, especially considering that this issue is boils down to a bug in cargo and not in serde. So the serde developers are not even to blame for this. However, with such an important and widely used crate like serde this could potentially cause trouble for a lot of other projects that depend on serde. In my opinion, deciding not to fix this is not really helpful. So I am still hoping that one of the next serde releases comes with a fix for that - whether its just updating the MSRV or getting rid of that troublesome resolver line.

TheBlueMatt added a commit to TheBlueMatt/serde that referenced this issue Aug 11, 2022
After conversation in Twitter DM re: serde-rs#2255, it became clear that
serde does not, in fact, have a specific MSRV policy, but rather
strives to be buildable with older rustc as only a practical
matter.

This led to some confusion from users, as the `rust-version`
Cargo.toml tag is often used to communicate a specific MSRV policy.

Thus, this PR simply removes references which could be confused for
a specific MSRV  policy, leaving a comment in CI that the test with
older rustc's does not imply a specific policy.
TheBlueMatt added a commit to TheBlueMatt/serde that referenced this issue Aug 11, 2022
After conversation in Twitter DM re: serde-rs#2255, it became clear that
serde does not, in fact, have a specific MSRV policy, but rather
strives to be buildable with older rustc as only a practical
matter.

This led to some confusion from users, as the `rust-version`
Cargo.toml tag is often used to communicate a specific MSRV policy.

Thus, this PR simply removes references which could be confused for
a specific MSRV  policy, leaving a comment in CI that the test with
older rustc's does not imply a specific policy.
asomers added a commit to asomers/nix that referenced this issue Aug 11, 2022
Nix's code hasn't changed.  However, Serde accidentally raised its MSRV
to 1.51.0 in a patch release, due to a Cargo bug.  They don't plan to
change it back.  Nix does not depend on Serde, but it's used by
cargo-hack, which we build as part of our CI process.  So we need to
either raise our MSRV, or else install a separate toolchain during CI
just to build cargo-hack.

serde-rs/serde#2255
@Kijewski
Copy link

Kijewski commented Aug 12, 2022

Where does the resolver = "1" line in the generated Cargo.toml come from, though? Cargo.toml.orig does not contain it. Seems like a bug in cargo package, doesn't it? Or is this because of the absent edition value?

@victorjulien
Copy link

In Suricata we try to support the Rust versions as they are shipped by the major (Linux) distributions, so Suricata can be packaged. For this reason we maintain a table of OS' that are important to us. Debian 11 (current stable) is the lowest, at Rust version 1.48. I think especially projects that are not pure Rust (like Suricata, which is C+Rust) care more about distro packaging and support, since we know our users do.

We're maintaining our table here: https://redmine.openinfosecfoundation.org/issues/4163 (screenshot added for reference)

Screenshot from 2022-08-12 10-22-02

I think would be great if Serde, and all other projects, would define a clear MSRV policy to help users of the crates ecosystem know what to expect.

@Kixunil
Copy link
Contributor

Kixunil commented Aug 12, 2022

Since there's a mismatch between rust-version in manifest and actual MSRV it could cause problems later if cargo becomes MSRV-aware and people start using it. It seems the best thing to do is to publish a new version with MSRV 1.13 and if serde MSRV is to be changed then do it in a single step involving update of the rust-version field. Also possibly yank 1.0.143.

@HamzaYasin1
Copy link

  error: failed to run custom build command for `substrate-runtime v2.0.0 (/Users/hamza/Applications/Documents/blockchain/substrate-git/substrate/runtime)`
  
  Caused by:
    process didn't exit successfully: `/Users/hamza/Applications/Documents/blockchain/substrate-git/substrate/target/debug/build/substrate-runtime-29670f71a495afb9/build-script-build` (exit code: 1)
    --- stderr
        Updating crates.io index
    error: failed to download `serde v1.0.143`
  
    Caused by:
      unable to get packages from source
  
    Caused by:
      failed to parse manifest at `/Users/hamza/Applications/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/serde-1.0.143/Cargo.toml`
  
    Caused by:
      feature `resolver` is required
  
      consider adding `cargo-features = ["resolver"]` to the manifest


cargo 1.65.0-nightly (ce40690a5 2022-08-09)
code base : substrate 2.0.0 

Having the same issue, i started getting from today, it was totally working fine couple of weeks back. I have used other versions of rust as well, but nothing was fruitful.
Command i'm using : cargo +nightly-2020-10-05 build

@taiki-e
Copy link
Contributor

taiki-e commented Aug 12, 2022

As said in #2259, the problem should be automatically resolved when the maintainer creates a new release using the nightly toolchain after nightly-2022-08-13 (tomorrow).

bors bot added a commit to nix-rust/nix that referenced this issue Aug 16, 2022
1792: Raise the MSRV to 1.56.1 r=rtzoeller a=asomers

Nix's code hasn't changed.  However, Serde accidentally raised its MSRV
to 1.51.0 in a patch release, due to a Cargo bug.  They don't plan to
change it back.  Nix does not depend on Serde, but it's used by
cargo-hack, which we build as part of our CI process.  So we need to
either raise our MSRV, or else install a separate toolchain during CI
just to build cargo-hack.

serde-rs/serde#2255

Co-authored-by: Alan Somers <asomers@gmail.com>
@taiki-e
Copy link
Contributor

taiki-e commented Aug 21, 2022

This has been fixed in 1.0.144: https://docs.rs/crate/serde/1.0.144/source/Cargo.toml

@TheBlueMatt
Copy link

I'm still very confused as to the MSRV for serde. Should we be looking to rip serde out to get a consistent MSRV, or is the policy going to be made more clear? I admit I'm a bit confused why bother going to the hassle of making serde compile on rustc 1.1X if that's not going to be an MSRV.

@striezel
Copy link
Contributor

This has been fixed in 1.0.144: https://docs.rs/crate/serde/1.0.144/source/Cargo.toml

Very good. :)

I'm still very confused as to the MSRV for serde. Should we be looking to rip serde out to get a consistent MSRV, or is the policy going to be made more clear?

I think the lesson here is that one can not really uphold a MSRV guarantee in one's own projects without pinning the dependencies down to exact versions via Cargo.lock, even if those dependencies have a certain MSRV themselves.

@TheBlueMatt
Copy link

TheBlueMatt commented Aug 21, 2022

I think the lesson here is that one can not really uphold a MSRV guarantee in one's own projects without pinning the dependencies down to exact versions via Cargo.lock, even if those dependencies have a certain MSRV themselves.

This doesn't help libraries, only binary projects. Libraries which, themselves, wish to provide a concrete MSRV policy cannot do so without ensuring their dependencies meet the same MSRV policy. Up until today, as evidenced by the discussion on this bug, many downstream projects of serde provided an MSRV which clearly was not sustainable given serde's MSRV policy (or lack thereof). Hence my request for clarification.

The rust languages' attempt to work towards this was, at least in part, the rust-version field in Cargo.toml. Sadly it appears serde set this without actually "supporting" the rust-version in that field.

@serde-rs serde-rs locked and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

8 participants