Skip to content
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

Build script is broken for many use cases #40

Open
thomcc opened this issue Oct 12, 2020 · 2 comments
Open

Build script is broken for many use cases #40

thomcc opened this issue Oct 12, 2020 · 2 comments

Comments

@thomcc
Copy link
Contributor

thomcc commented Oct 12, 2020

  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
Copy link
Collaborator

@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
Copy link

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

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

No branches or pull requests

3 participants