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

Add a CI job to check the minimal version constraints of direct dependencies #124

Merged

Conversation

carols10cents
Copy link
Contributor

@carols10cents carols10cents commented Oct 16, 2023

I'm seeing a few issues that I will fix in a minute, but I want to make sure this test would catch them in CI (if CI is allowed to run on my PR) Ok that has to be approved, but it turns out CI as-is wouldn't have caught my issue anyway, but I'm including it here because it caught a different issue... keep reading for more explanation but I'm happy to take out whatever commits you'd like me to.

What happened

I upgraded a project I work on that uses croaring to move off of the yanked version 1.0.0. I did this by running cargo update -p croaring, forgetting that croaring-sys would need an update too (and also forgetting to pass --aggressive).

The project's build started failing with these errors:

   Compiling croaring v1.0.1
error[E0425]: cannot find function `roaring_bitmap_internal_validate` in crate `ffi`
    --> /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/croaring-1.0.1/src/bitmap/imp.rs:1495:35
     |
1495 |         let valid = unsafe { ffi::roaring_bitmap_internal_validate(&self.bitmap, &mut error_str) };
     |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `ffi`

error[E0699]: cannot call a method on a raw pointer with an unknown pointee type
    --> /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/croaring-1.0.1/src/bitmap/imp.rs:1499:26
     |
1499 |             if error_str.is_null() {
     |                          ^^^^^^^

So really, croaring v1.0.1 depends on at least v1.1.0 of croaring-sys, but cargo update -p croaring didn't take care of that because croaring specifies croaring-sys version = "1.0.0" so the existing croaring-sys v1.0.0 in my project's lockfile was compatible.

I've corrected this problem by updating the version specified in croaring's Cargo.toml to be croaring-sys version = "1.1.0".

I was hoping that building with cargo's unstable minimal versions flag would prevent this from reoccurring in the future, but it looks like because of the path dependency for croaring-sys that's used when building in this repo, cargo uses whatever version is at the path and not the minimal version someone using the crates from crates.io would experience :( I'm going to make sure this issue was raised somewhere in the discussions of the cargo feature.

But when I ran with cargo +nightly check -Zdirect-minimal-versions, it did report a problem with byteorder, so I have included in this PR a commit to enable this check in CI and a commit to properly specify the minimum required byteorder version.

I'm happy to make any changes you'd like, please let me know!

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@saulius
Copy link
Member

saulius commented Oct 18, 2023

So really, croaring v1.0.1 depends on at least v1.1.0 of croaring-sys, but cargo update -p croaring didn't take care of that because croaring specifies croaring-sys version = "1.0.0" so the existing croaring-sys v1.0.0 in my project's lockfile was compatible.

Sorry about this, version bumps for both usually went together.

@saulius saulius merged commit b01b9ce into RoaringBitmap:master Oct 19, 2023
6 checks passed
@saulius
Copy link
Member

saulius commented Oct 19, 2023

Thanks for making croaring-rs better! :)

@carols10cents carols10cents deleted the update-croaring-sys-constraint branch October 19, 2023 13:32
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.

None yet

3 participants