Skip to content

Update to CRoaring v4.2.1 for #21 #22

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

Closed
wants to merge 18 commits into from

Conversation

wti
Copy link

@wti wti commented Feb 19, 2025

Update to CRoaring release v4.2.1 for #21

  • Aggregated .c and .h files copied from release (first commit)
  • Migrated .c off two deprecated API (see below)
  • Minor test additions to improve coverage and minimize or bracket print output
  • README updated to remove unsupported code (now manually verified)

re: Swift interface:

  • Swift 4 is not really supported any more.
  • The Swift interface includes inlined functions which don't appear in coverage reports, but tests hit all others. (Coverage of CRoaring is marginal but not at issue here.)

re: CRoaring

  • Fixing the two deprecated API's creates a delta with the release code. Let me know if you want instead to leave those as-is.
  • There are ~40 warnings about implicit casts. I started to address some in a separate branch, but I stopped because I'm not the one to validate their correctness and deltas are unwelcome.

@lemire
Copy link
Member

lemire commented Feb 19, 2025

Running tests.

@wti
Copy link
Author

wti commented Feb 20, 2025

test-macOS.yml is passing, but test-linux.yml has header conflicts.

@wti wti marked this pull request as draft February 20, 2025 01:40
@wti
Copy link
Author

wti commented Feb 20, 2025

Converted PR to DRAFT. I'll stop investigating for now.

  • macOS CI is working
  • Linux CI fails to build SwiftRoaring's CRoaring module, even on ubuntu-latest (where CRoaring project CI passes).
  • It builds/tests fine on my local stock Ubuntu 24.04 with Swift 6.0.3.
  • Unclear if the issue is extrinsic library/build configuration/setup or true problems with amalgamated intrinsics header inclusion in this context.

Let me know if instead you'd like to merge and possibly release, deferring CI fixes.

Sorry for the noise+hassle of an incomplete PR.

@lemire
Copy link
Member

lemire commented Feb 20, 2025

I think we ought to figure out the issue prior to merging.

Do you understand the error message? Something to do with intrinsics... ?

@wti
Copy link
Author

wti commented Feb 24, 2025

Sorry, I didn't realize this would result in so much CI noise in the main repo. I should probably delete the PR if/since it will take CI config fiddling to sort it out.

Do you understand the error message? Something to do with intrinsics... ?

No, I don't understand the cause. I presume there's some delta in include processing due to different library/compiler setup, but perhaps migrating from the deprecated API bumped into the problem.

Factors:

  • Builds/tests fine locally on x86 or ARM in macOS, and on VM ARM in Ubuntu 24.04.
  • I adopted the official swiftlang action to configure the image.
  • I migrated from the deprecated API.
  • Also, it's unclear where the amalgamated sources are built in CI to start from a known-working build.

@lemire
Copy link
Member

lemire commented Feb 24, 2025

@wti

The problem has been reported upstream by other folks: swiftlang/swift#69311

I reproduce it with a minimal PR: #26

@lemire
Copy link
Member

lemire commented Feb 24, 2025

@wti Thanks for reporting the issue. CRoaring has been updated.

I am closing this PR. Feel free to issue a new one. Please don't patch CRoaring here, issue a pull request upstream.

@lemire lemire closed this Feb 24, 2025
@wti
Copy link
Author

wti commented Feb 25, 2025

Understood. Thank you for picking up the slack, but sorry again for breaking things.

@lemire
Copy link
Member

lemire commented Feb 25, 2025

@wti Don't hesitate to issue another PR.

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