search

purpleprotocol / mimalloc_rust

A Rust wrapper over Microsoft's MiMalloc memory allocator
MIT License
486 stars 42 forks source link

Build script is broken for many use cases #40

Open thomcc opened 3 years ago

thomcc commented 3 years ago

  1. I'm pretty sure these were copied blindly without thought (they make no sense for rust) but https://github.com/purpleprotocol/mimalloc_rust/blob/master/libmimalloc-sys/build.rs#L107 is totally broken (/MDd causes use of the debug CRT, and rust always uses the release one. On windows, if you statically link to a library, a number of compilation settings must be identical, but a very important one is which CRT you use).

    I'm assuming that: https://github.com/purpleprotocol/mimalloc_rust/blob/master/libmimalloc-sys/build.rs#L153-L154 not specifying static means we don't actually statically link, which is why this works at all (assuming it does).

    We also don't support target_feature=+crt-static (for this and other reasons), and I suspect we're broken under windows-gnu.

  2. All the code that checks cfg!(all(windows, target_env = "msvc")) is broken for cross compilation, because that's checking the target of the build script, which is the host.

  3. In general we do a ton of work to make cmake function, I think we're better off using cc (which ensures everything works perfectly) and mimic the . This makes upgrades of mimalloc that change build logic harder, but is the only real way to get correct behavior here.

I think at the end of the day our best option here is actually just to use cc and mimic the cmake logic. This will also allow us to avoid requiring cmake to be installed to use mimalloc.

We can allow falling back to cmake for some cases if desirable.

I'm working on this here https://github.com/thomcc/mimalloc_rust/tree/targfix but will PR when done.

(P.S. Apparently I hadn't been watching this repo, sorry — I promised I'd keep the docs up to date for new releases and failed to do so)

octavonce commented 3 years ago

@thomcc Regarding your points: 1) I haven't had my hands on a Windows machine for years now. So feel free to adjust these as necessary. 2) Same as point 1. 3) I agree. We should migrate to cc.

RivenSkaye commented 8 months ago

Looking at the current code, this seems to all be fixed already? The crate works fine on windows on both gnu and msvc toolchains and I see no mention of either the platform or the cfg checks, save for explicitly disabling MI_OVERRIDE which looks to be the sane choice