Skip to content

Fix compilation on MSRV (1.37)#112

Closed
yotamofek wants to merge 2 commits intorayon-rs:mainfrom
yotamofek:msrv
Closed

Fix compilation on MSRV (1.37)#112
yotamofek wants to merge 2 commits intorayon-rs:mainfrom
yotamofek:msrv

Conversation

@yotamofek
Copy link
Contributor

@yotamofek yotamofek commented Feb 17, 2025

Latest versions of serde_derive, syn and proc_macro2 have all raised the MSRV to be incompatible with either's MSRV, which is at 1.37.
This fixes compilation on MSRV (i.e. cargo +1.37 build --all-features)

Personally, I think raising MSRV is a better option. 1.37 was released 5.5 years ago, so it's quite old by now, and this fix will cause downstream users to get syn version 1.x in their dep tree, even though they might be already be using 2.x (and maybe other duplicated crates, or just older versions of non-duplicated crates).

@yotamofek yotamofek mentioned this pull request Feb 17, 2025
Merged
@yotamofek
Copy link
Contributor Author

yotamofek commented Feb 17, 2025 •
edited
Loading

@jswrenn continuing the discussion we had here, this is what happens when building the latest main, with 1.37 (the MSRV), without a Cargo.lock:

➜  either git:(main) rm Cargo.lock 
➜  either git:(main) cargo +1.37 build --all-features
warning: unused manifest key: package.rust-version
    Updating crates.io index
error: failed to download `proc-macro2 v1.0.93`

Caused by:
  unable to get packages from source

Caused by:
  failed to parse manifest at `/home/yotam/.cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.93/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  supported edition values are `2015` or `2018`, but `2021` is unknown

@yotamofek
Copy link
Contributor Author

yotamofek commented Feb 17, 2025

FWIW, without the serde feature (i.e. just no_std), it builds just fine.
I still think that if you purge the CI cache and try to build the latest commit on main again, the 1.37+serde job should fail.

@cuviper
Copy link
Member

cuviper commented Feb 17, 2025

Personally, I think raising MSRV is a better option.

I think that's fine. IIRC itertools was the holdout dependent last time, but that's up to 1.63 now -- a common rallying point since that's what Debian stable has. And that's still 2.5 years old!

this fix will cause downstream users to get syn version 1.x in their dep tree

I would reject this fix (locking versions) regardless, mainly for that reason. My general preference for MSRV CI is to either manually cargo update -p PACKAGE --precise x.y.z in the CI script, or use a commited Cargo.lock with those versions, but never force old versions on the ecosystem.

cuviper
cuviper requested changes Feb 17, 2025
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

I'm just adding a review to formally NAK the current state.

@yotamofek
Copy link
Contributor Author

yotamofek commented Feb 17, 2025

Good, I hoped that raising the MSRV would be the outcome. Let me pull up a PR to do that real quick!

@yotamofek yotamofek closed this Feb 17, 2025
@yotamofek yotamofek deleted the msrv branch February 17, 2025 18:19
@yotamofek
Copy link
Contributor Author

yotamofek commented Feb 17, 2025

Ah yes, didn't even notice this:

either/.github/workflows/ci.yml

Lines 44 to 47 in ef94c4e

cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
cargo update -p quote --precise 1.0.30
cargo update -p proc-macro2 --precise 1.0.65

@yotamofek yotamofek mentioned this pull request Feb 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cuviper cuviper cuviper requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yotamofek @cuviper