Skip to content

Conversation

gschulze
Copy link
Contributor

This PR adds a feature flag that allows switching between the latest versions of the v2 and v3 branches of mimalloc. Internally, this is solved by having two submodules for v2 and v3 that point to the same repository URL (https://github.com/microsoft/mimalloc) but different commits. The correct source is chosen in build.rs based on whether the 'v3' feature flag is set or not.

@gschulze gschulze mentioned this pull request Jul 31, 2025
@octavonce
Copy link
Collaborator

Ok, this looks great. Sorry I didn't see this before notifications didn't reach me for some reason. My only issue is that the v3 branch is not currently being tested by CI. Could you please duplicate all the current workflows (build, test, secure, extended, etc) for the v3 feature?

@gschulze
Copy link
Contributor Author

Okay, will do tomorrow!

@gschulze
Copy link
Contributor Author

@octavonce I added support for testing the v3 feature in sys-test and added the missing workflows.

The mi_option_max_segment_reclaim runtime option has been removed in version 3, so I feature-gated it. Moreover, some options have been added in version 3, which probably should also be added to extended.rs at some point. However, there are options in version 2 that are missing in extended.rs, so I left it as it is for now.

@octavonce
Copy link
Collaborator

Good, what you're describing is what I was afraid of would happen when switching from v2 to v3.

The feature flag has to be bulletproof.

I ran the workflows now it seems that there's still issues with it. I didn't look at the code yet as I'm on mobile but yeah we have to fix all of these before we merge.

@gschulze
Copy link
Contributor Author

I tried to fix the workflow errors, but it seems when running the integration tests by invoking cargo run --features v3 -p libmimalloc-sys-test, the CARGO_FEATURE_V3 environment variable is not set in build.rs. However, when changing the working directory to libmimalloc-sys/sys-test and invoking cargo run from there, everything is working fine.

@octavonce
Copy link
Collaborator

It's an issue with passing features to package members.

Try it like this:

RUSTFLAGS='--cfg feature="extended,v3"' cargo run -p libmimalloc-sys-test

@gschulze
Copy link
Contributor Author

It turns out that features passed via RUSTFLAGS are not accessible in build scripts. I adjusted Cargo.toml and build.rs to explicitly handle the features set in ci.yml, that is, secure, extended and v3. There, these features are set explicitly for the libmimalloc-sys-test crate.

@octavonce
Copy link
Collaborator

Ok, looks great! Clippy seems to be complaining again but we can merge after that's fixed. Great work!

@gschulze
Copy link
Contributor Author

The linter errors were due to unused imports in the generated test code, likely caused by not always enabling the extended feature anymore. I inspected the generated all.rs, and it is still covering all basic functionality. I fixed the Clippy errors by allowing unused imports for the generated test code.

After this is merged, would it be possible to have a release soon?

@octavonce octavonce merged commit 31607bf into purpleprotocol:master Aug 26, 2025
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