mimalloc_rust icon indicating copy to clipboard operation
mimalloc_rust copied to clipboard

Build script is broken for many use cases

Open thomcc opened this issue 5 years ago • 2 comments

  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)

thomcc avatar Oct 12 '20 20:10 thomcc

@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.

octavonce avatar Oct 18 '20 14:10 octavonce

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

RivenSkaye avatar Nov 17 '23 14:11 RivenSkaye