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 libsqlite3-sys to allow 0.31 as well #218

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

weiznich
Copy link
Contributor

  • I agree to follow the project's code of conduct.
  • I added an entry to the project's change log file if knowledge of this change could be valuable to users.
    • Usually called CHANGES.md or CHANGELOG.md
    • Prefix changelog entries for breaking changes with "BREAKING: "

@weiznich
Copy link
Contributor Author

weiznich commented Jan 20, 2025

Seems like CI fails due to the new libsqlite3 version requiring a newer rust version. What would need to be done to bump that version?

@lnicola
Copy link
Member

lnicola commented Jan 20, 2025

We've just bumped the MSRV, I'm all for doing it again.

@urschrei
Copy link
Member

What would our new MSRV need to be? geo is "officially" current - 2 right?

@lnicola
Copy link
Member

lnicola commented Jan 20, 2025

stable - 3, or currently 1.81. I see #[expect] in the CI logs, which was stabilized in 1.81.

EDIT: might actually be 1.82 for unsafe extern.

@weiznich
Copy link
Contributor Author

weiznich commented Jan 20, 2025

Technically this wouldn't need a msrv change, as proj still would support the older libsqlite3-sys versions, so anyone that depends on older rust versions could just use an older libsqlite3-sys version. The question is more how do you want to express that in your CI setup. For diesel I just do the minimal rust version build with cargo minimal-versions to enforce that it uses the minimal possible dependency versions that are most likely to build on older rust versions.

@weiznich
Copy link
Contributor Author

It seems like it's currently impossible for me to work on the CI as it's not possible to run jobs here without approval and I also cannot run jobs on a fork as its impossible to pull images there

@urschrei
Copy link
Member

Your PR runs here should no longer require approval. We're aware of the image-pull annoyance, I just haven't had time to figure out how to make the images public so they work in forks.

@urschrei
Copy link
Member

@weiznich your fork should now be able to run its Actions. lmk if it can't.

@weiznich weiznich force-pushed the bump/libsqlite_0.31 branch 7 times, most recently from cbb5566 to 205e3ce Compare January 29, 2025 13:11
* Use `cargo minimal-version` for checking the MSRV builds
* Fix some crate dependency minimal versions
@weiznich weiznich force-pushed the bump/libsqlite_0.31 branch from 205e3ce to 12bbc14 Compare January 29, 2025 13:24
@weiznich
Copy link
Contributor Author

It should be ready to review now.

Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

TBH I would have gone for a MSRV bump instead of the extra complexity, but I guess it looks fine otherwise

@weiznich
Copy link
Contributor Author

Given that the 2024 edition will make resolver = "3" the default case + that resolver will use declared supported rust versions of dependencies it's meaningful in my opinion to test for the minimal supported versions of dependencies to make sure that this not accidentally breaks under resolver="3"

@urschrei
Copy link
Member

FYI (reviewers, future selves etc) this is the crate being used in the new action in the PR: https://github.com/taiki-e/cargo-hack

@urschrei
Copy link
Member

urschrei commented Feb 5, 2025

@lnicola wdyt? I'm inclined to merge this.

@lnicola
Copy link
Member

lnicola commented Feb 5, 2025

Yeah, should be good.

@urschrei urschrei added this pull request to the merge queue Feb 5, 2025
Merged via the queue into georust:main with commit 952c4d0 Feb 5, 2025
28 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.

3 participants