Skip to content

Move gz API to libz-rs-sys-cdylib #365

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

Merged
merged 16 commits into from
May 27, 2025

Conversation

brian-pane
Copy link

No description provided.

@brian-pane
Copy link
Author

brian-pane commented May 20, 2025

This currently can compile the gz features with cd libz-rs-sys-cdylib && cargo build --features=gz but the build from the base directory fails due to the C and Rust allocators both being defined.

Copy link

codecov bot commented May 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
test-aarch64-apple-darwin 93.35% <100.00%> (+5.61%) ⬆️
test-x86_64-apple-darwin 91.69% <100.00%> (+4.73%) ⬆️
test-x86_64-unknown-linux-gnu 90.46% <100.00%> (+3.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
libz-rs-sys-cdylib/src/gz.rs 91.09% <100.00%> (ø)
libz-rs-sys/src/lib.rs 82.70% <ø> (+0.83%) ⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brian-pane
Copy link
Author

With the change I just pushed, the basic build and tests work. If this approach looks safe, I'll troubleshoot the remaining issues with Clippy and fuzz tests.

@folkertdev
Copy link
Collaborator

let's explore this. I think one of the CI issues is that the cdylib crate should check in its Cargo.lock. There also appears to be some bug with the coverage profile in the fuzzers, but that is not triggered by this PR.

@bjorn3 do you have objections to this approach?

@brian-pane
Copy link
Author

The cdylib's Cargo.lock is checked in, and I added a cargo fetch, but the cargo cinstall --offline --release --destdir=/tmp/cargo-cbuild-libz-rs step still fails because it can't find the libc crate.

Some of the other CI failures happen because both c-allocator and rust-allocator are enabled, but I don't know how the allocator configuration is supposed to work. This line in test-libz-rs-sys/Cargo.toml seems to turn on both allocators, but I can't see how that would work.

@folkertdev
Copy link
Collaborator

This line in test-libz-rs-sys/Cargo.toml seems to turn on both allocators, but I can't see how that would work.

It does work for zlib-rs (it just means: compile in both versions, rust has precedence but we do make sure it all compiles). It is only in libz-rs-sys that we disallow configuring both allocators.

@brian-pane
Copy link
Author

I got the cargo-c dynamic library build working, but I'm stuck on the Clippy failures, which are due to both c-allocator and rust-allocator being defined. I can't tell where in the various Cargo.toml the conflicting allocators are getting enabled for libz-rs-sys.

@folkertdev folkertdev force-pushed the gz-move branch 2 times, most recently from 4ae1d9f to 2000318 Compare May 22, 2025 23:05
@folkertdev folkertdev force-pushed the gz-move branch 2 times, most recently from c841631 to e18efac Compare May 23, 2025 09:37
@folkertdev folkertdev requested a review from bjorn3 May 23, 2025 10:10
@folkertdev folkertdev marked this pull request as ready for review May 23, 2025 10:19
Comment on lines 6 to 7
// #[cfg(not(panic = "abort"))]
// compile_error!("panic=\"abort\" is mandatory because unwinding to C is undefined behavior");
Copy link
Collaborator

@bjorn3 bjorn3 May 27, 2025

Choose a reason for hiding this comment

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

Does #[cfg(not(any(test, panic = "abort"))] work?
Edit: No, I don't think so because of test-libz-rs-sys. I don't like that this has to be disabled though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, we can add a feature flag for it?

@folkertdev folkertdev force-pushed the gz-move branch 2 times, most recently from fd5d6aa to 57da06b Compare May 27, 2025 14:23
@folkertdev
Copy link
Collaborator

one final change; on windows, our semver-prefix used characters that are not allowed in windows symbol names, apparently. We're now just using underscores, which passes the tests.

Copy link
Collaborator

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

thank you for the work here @brian-pane

@folkertdev folkertdev merged commit 322e8e1 into trifectatechfoundation:main May 27, 2025
22 of 24 checks passed
@brian-pane brian-pane deleted the gz-move branch May 27, 2025 17:16
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.

4 participants