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

Update Cargo.toml dependencies and release 1.73.0 #240

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

JamyGolden
Copy link
Contributor

No description provided.

@JamyGolden
Copy link
Contributor Author

@johannesvollmer tagging you since no reviewers were added to the PR automatically

@johannesvollmer
Copy link
Owner

johannesvollmer commented Sep 25, 2024

hey, thanks for your time! unfortunately, some of you suggestions should be discussed:

  • new release 1.73 we can do, nice
  • increasing rust version should be based on image crate rust version, we are a dependency of them. they are 1.67.1
  • increasing the other versions should not be done, if I am not mistaken - the ^ only defines a lower bound, which you increased. now fewer versions would match this pattern, even though the old versions worked fine. maybe I got something wrong though?

@JamyGolden
Copy link
Contributor Author

Hey :)

The real issue I want to fix is actually just minz_oxide and change that to version ~0.8.0 to sort out dependency resolution issues, but before we get to that:

I think the Cargo.toml should probably be using the ~ prefixes instead of ^ for 0.x.x (zero major version) dependencies because the minor version number is for both for new features/non-breaking changes and breaking changes. So having ^ prefix on a semver 0 major version package is potentially very unstable.

Are you sure exrs currently builds on anything under rust 1.70.0? I get this when I build exrs master branch using 1.67.1:

🦄 …/exrs on 🌱 master[] via  v1.67.1 ( 01:10)
❯ git log -n 1
commit e018e0dba9a5a1cba3a5ddbe7a05577638fbc9e2 (HEAD -> master, upstream/master)
Merge: 861f0cf d626147
Author: Johannes Vollmer <[email protected]>
Date:   Sun Jul 7 18:38:45 2024 +0200

    Merge pull request #236 from johannesvollmer/iterate-all-attributes

    add helper to iterate all attributes in a header

🦄 …/exrs on 🌱 master[] via  v1.67.1 ( 01:10)
❯ git status
On branch master
nothing to commit, working tree clean

🦄 …/exrs on 🌱 master[] via  v1.67.1 ( 01:10)
❯ rustc --version
rustc 1.67.1 (d5a82bbd2 2023-02-07)

🦄 …/exrs on 🌱 master[] via  v1.67.1 ( 01:10)
❯ cargo build --release
error: package `half v2.4.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.67.1
Either upgrade to rustc 1.70 or newer, or use
cargo update -p [email protected] --precise ver
where `ver` is the latest version of `half` supporting rustc 1.67.1

I'm not sure why half is resolving to 2.4.1 since it's set as 2.1.0 in Cargo.toml

@johannesvollmer
Copy link
Owner

johannesvollmer commented Sep 27, 2024

Oh sorry, I just forgot that the ^ on a 0.x.x version doesn't do the same thing as on normal versions. Sorry for my misunderstanding.

Are you sure exrs currently builds on anything under rust 1.70.0? I get this when I build exrs master branch using 1.67.1:

Our CI has a test for this, but it seems that gave me a false sense of security: it only runs for PRs, but not regularly, so any dependency breaking the build might have gone unnoticed.

@johannesvollmer
Copy link
Owner

johannesvollmer commented Sep 27, 2024

We did previously constrain the version of half because we wanted to avoid requiring rust 1.70.

But that was undone recently in aba88fb in pr #231

It seems we decided to default to rust 1.70 but allowing users to opt-in to the old version of half manually, if desired. Maybe that would resolve the build error for 1.67?

@johannesvollmer
Copy link
Owner

johannesvollmer commented Sep 27, 2024

I think the Cargo.toml should probably be using the ~ prefixes instead of ^ for 0.x.x (zero major version) dependencies because the minor version number is for both for new features/non-breaking changes and breaking changes. So having ^ prefix on a semver 0 major version package is potentially very unstable.

Agree, but not sure if a strict limit would be good. I will look into it.

@johannesvollmer
Copy link
Owner

I apologize for my confusion, my mind has other topics in focus, and this project is only in the background currently, and for a long time now. from my understanding, this is the current state:

  • rust version increase: the goal with rust-version is to keep it as low as possible. what is the reason to increase it? didn't it build otherwise? The CI succeeded on this PR, but does it succeed on main branch?
  • dependencies: let's update those packages that have version upgrades which would not automatically be picked. for example, your new half, smallvec and rayon core versions would be picked by cargo anyways, as they are above V1. but miniz oxide and image we can change.
  • using tilde versions: does it really make a difference in practice? how exactly would it affect our specific set of dependencies? as the caret never modifies the first non-zero version (0.7.3 always below 0.8.x), it seems to have the same effect as the tilde (only allowing patches).
    from my understanding, both strategies are pretty vague, as the behaviour entirely depends on how to dependency authors handle their updates, which seems quite fuzzy to me.

@JamyGolden JamyGolden force-pushed the master branch 3 times, most recently from 38d04f9 to b976c76 Compare October 4, 2024 08:19
Update deps
@JamyGolden
Copy link
Contributor Author

No problem at all with any async responses 😄 Thanks for the reply!

  • Yes it didn't build otherwise, however I wasn't setting the lockfile via the nightly cargo build with the minimal versions flag, so with that it does work now. (I think the https://github.com/johannesvollmer/exrs?tab=readme-ov-file#usage section in the readme could be updated since the non-nightly option didn't work for me)
  • I've changed the PR to only update miniz_oxide and image versions.
  • I'm sorry about the confusion with the ~ and ^. I didn't realise that rust automatically handles ^ as ~ for 0.x.x versions.

@johannesvollmer
Copy link
Owner

Awesome, thanks for the update. Does this new version still solve the version conflict in your project?

@JamyGolden
Copy link
Contributor Author

Yeah it does thanks 😄 miniz_oxide was the issue.

@johannesvollmer johannesvollmer merged commit 7f312be into johannesvollmer:master Oct 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants