search

purpleprotocol / mimalloc_rust

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

set needed libs for windows gnu #56

Closed ryancinsight closed 3 years ago

ryancinsight commented 3 years ago

This pull request is to automate setting the libs needed to be linked to on windows gnu/mingw builds only. Furthermore, dependencies were upgraded.

thomcc commented 3 years ago

.cargo/config is workspace local. This won't fix anything outside of this workspace, e.g. anybody depending via crates.io.

ryancinsight commented 3 years ago

You can distribute a .cargo/config file in a crates package and any extra flags will be used on top of what the user has already set. This is an easier way for me to distinguish windows gnu which requires me statically linking bcrypt from other gnu systems. I have already tried it and it works properly with GitHub patch.

thomcc commented 3 years ago

No, that's not true.

The .cargo/config isn't even uploaded to crates.io, you can check this with cargo package --list (You can force it to be included by the package.include property in Cargo.toml, but it still won't be used) Edit: This wasn't true

I have already tried it and it works properly with GitHub patch.

This is probably because path dependencies are considered part of the current workspace. (unsure if that would explain it) I don't know exactly what you mean by worked. Did you just try using it from within mimalloc_rust?

thomcc commented 3 years ago

So, it actually is in the package, it still isn't used though.

Here's a way I can show it's not used: The safe_arch has this in the package https://github.com/Lokathor/safe_arch/blob/main/.cargo/config

However, if I add make a dummy crate that depends on safe_arch, and cargo build --verbose, I don't see those flags

    Updating crates.io index
  Downloaded safe_arch v0.5.2
  Downloaded 1 crate (75.0 KB) in 0.59s
   Compiling safe_arch v0.5.2
     Running `rustc --crate-name safe_arch --edition=2018 /Users/thom/.cargo/registry/src/github.com-1ecc6299db9ec823/safe_arch-0.5.2/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 --cfg 'feature="default"' -C metadata=a310c81b1ffae673 -C extra-filename=-a310c81b1ffae673 --out-dir /Users/thom/scratch/safearch-user/target/debug/deps -L dependency=/Users/thom/scratch/safearch-user/target/debug/deps --cap-lints allow`
   Compiling safearch-user v0.1.0 (/Users/thom/scratch/safearch-user)
     Running `rustc --crate-name safearch_user --edition=2018 src/main.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 -C metadata=24aa196ce8326819 --out-dir /Users/thom/scratch/safearch-user/target/debug/deps -C incremental=/Users/thom/scratch/safearch-user/target/debug/incremental -L dependency=/Users/thom/scratch/safearch-user/target/debug/deps --extern safe_arch=/Users/thom/scratch/safearch-user/target/debug/deps/libsafe_arch-a310c81b1ffae673.rlib`
    Finished dev [unoptimized + debuginfo] target(s) in 2.67s

You'll note that the rustflags don't contain -Ctarget-cpu=native.

Additionally, https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure lists the search path for config files, and it doesn't include that of dependencies.

ryancinsight commented 3 years ago

I do typically work with packages typically arranged in a workspaces on crates, like pyoxidizer has a .cargo/config for workspace and individual package pyembed. It’s not in toml so are you saying it’s because it’s stored as a workspace? It was my understanding toml config was replacing .cargo/config since rust 1.39 but were currently interchangeable. Though I don’t mind the method of how this is done I was just looking for a way to statically link these libs for this crate only and not everything, any alternative suggestions?

ryancinsight commented 3 years ago

That doesn’t count, that is under build you have to download and do cargo build only for that to be used

If you put actual targets like x86_64-pc-windows-msvc than it should run, the other option is install instead of build

thomcc commented 3 years ago

It was my understanding toml config was replacing .cargo/config since rust 1.39 but were currently interchangeable

Yes, the distinction between .cargo/config.toml and .cargo/config isn't important.

I was just looking for a way to statically link these libs for this crate only and not everything, any alternative suggestions?

That's not really how static linking works, but the equivalent way to do what you've done[1] is either

#[cfg(windows)] // might need to be only windows + target_env=gnu?
#[link(name = "bcrypt")]
extern {}

inside the -sys crate (preferrably), or something like println!("cargo:rustc-link-lib=bcrypt"); in the build.rs (although it's somewhat surprising this is needed, since static libs on windows generally know their dependencies — it might not be needed after #58).

That said, I suspect if this is necessary, we probably should be linking a few others too, tbh, and might just be getting lucky that they happen to work. Hrm.

[1]: Well, you do a bunch of things in that set of flags, but the important one is linking bcrypt. Edit: and archive

thomcc commented 3 years ago

Oh, you also link libarchive. Same thing there. (Very surprised that one is needed actually...)

ryancinsight commented 3 years ago

That is correct bcrypt and libarchive are the ones I have issues with and only on windows gnu, I would be happy to revise to your method if that’s preferred

thomcc commented 3 years ago

It looks like we should be linking all of these: https://github.com/microsoft/mimalloc/blob/15220c684331d1c486550d7a6b1736e0a1773816/CMakeLists.txt#L203

Most of those will be linked by libstd, so not a problem, I guess bcrypt is not. That said, we should link all of these so that we work under no_std.

libarchive isn't on that list, so I'm kinda surprised it's needed.

ryancinsight commented 3 years ago

Libarchive may be a Mingw bcrypt issue. When I linked bcrypt I then got errors about libarchive and google search shows similar topics about libarchive on mingw.

ryancinsight commented 3 years ago

No, that's not true.

~The .cargo/config isn't even uploaded to crates.io, you can check this with cargo package --list (You can force it to be included by the package.include property in Cargo.toml, but it still won't be used)~ Edit: This wasn't true

I have already tried it and it works properly with GitHub patch.

~This is probably because path dependencies are considered part of the current workspace.~ (unsure if that would explain it) I don't know exactly what you mean by worked. Did you just try using it from within mimalloc_rust?

In the project using mimalloc_rust I used the git=* to the fork with the .cargo/config with the windows gnu target flags and it worked that way. That’s how I know it works from git. Again it’s dependent on weather you use [build], [install], or [x86_64-pc-windows-gnu] that it will work from crates. Though I believe if you have one in home directly that takes precedence. By work I mean it didn’t give me missing bcrypt or libarchive error.

thomcc commented 3 years ago

Again it’s dependent on weather you use [build], [install], or [x86_64-pc-windows-gnu] that it will work from crates

This is still not true. I just tried with https://github.com/jamwaffles/sh1106, and didn't get their args when compiling for the targets that would have it.

I'm pretty certain there's no way for an external dependency (e.g. from crates.io) to directly change which rustflags are used.

ryancinsight commented 3 years ago

Again it’s dependent on weather you use [build], [install], or [x86_64-pc-windows-gnu] that it will work from crates

This is still not true. I just tried with https://github.com/jamwaffles/sh1106, and didn't get their args when compiling for the targets that would have it.

I'm pretty certain there's no way for an external dependency (e.g. from crates.io) to directly change which rustflags are used.

Regardless, I like your suggestion better but this is going to bug me lol, I’m having a difficult time believing that if it downloads it from crates and target is set that it won’t be used, I do this all the time on local packages and adjust for each one so the distinction of situation bugs me...

thomcc commented 3 years ago

I just tried this and I can't reproduce what you describe in https://github.com/thomcc/test-cargo-config-deps (and https://github.com/thomcc/test-cargo-config-gitdep). Neither the path dependency nor the git dependency influence the .cargo/config of the crate that has them as dependencies.

I can't explain why you say this works, unless you also are setting these in your project's local .cargo/config.toml

That said, the behavior I'm seeing matches what is documented in https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure, which is good. I think the documentation should be improved to be more clear about this, though.

thomcc commented 3 years ago

but this is going to bug me

FWIW I'm going to file a docs bug about this after I get some feedback on zulip about what the docs should say. I agree it should be clearer.

ryancinsight commented 3 years ago

but this is going to bug me

FWIW I'm going to file a docs bug about this after I get some feedback on zulip about what the docs should say. I agree it should be clearer.

I am humbled and appreciate you for correcting me on this! I shall correct based on your feedback soon, probably after checking #58 doesn’t solve the mingw problems first. Best regards!

ryancinsight commented 3 years ago

looks like cc provides better warnings than cmake but it did not correct issue:

` Compiling libmimalloc-sys v0.1.20 (E:\fork\mimalloc_rust\libmimalloc-sys) **warning: c_src/mimalloc/src/random.c:168: warning: ignoring '#pragma comment ' [-Wunknown-pragmas] warning: 168 #pragma comment (lib,"bcrypt.lib") warning: warning: c_src/mimalloc/src/stats.c:453: warning: ignoring '#pragma comment ' [-Wunknown-pragmas] warning: 453 #pragma comment(lib,"psapi.lib") warning: Compiling mimalloc v0.1.24 (E:\fork\mimalloc_rust)** error: linking with x86_64-w64-mingw32-gcc failed: exit code: 1

= note: "x86_64-w64-mingw32-gcc" "-fno-use-linker-plugin" "-Wl,--nxcompat" "-m64" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\rsbegin.o" "-L" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1h9tj6b34usbmvnu.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1jrgchoreamu835y.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1r680wbd6wpvc72o.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1rsr2iqh8j3379dy.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1sxn971gpkxq7cu.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.2tixd4j2vdh6km0j.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.390eo860kagp0rr2.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.3gvf41o1as48yg8t.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.43v137z9stc21ak6.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.4a7h2qaoaf0l3oo9.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.4go7wlraeqg7yhj0.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.4tcnxhevoisy8kfa.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.4u3bs530qntxwg0.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.4zequ7ofkrdx1sv9.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.5dkeucu82d9lfq0d.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.gg4awm0qkvwu2uo.rcgu.o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.pnr9wcq1lrhkbyd.rcgu.o" "-o" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.exe" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\mimalloc-899de694067c2712.1thu0gmy9sg39x5x.rcgu.o" "-Wl,--gc-sections" "-nodefaultlibs" "-L" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps" "-L" "E:\fork\mimalloc_rust\target\debug\deps" "-L" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\build\libmimalloc-sys-fd50a56bf33f412a\out" "-L" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib" "-Wl,-Bstatic" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libtest-6b691ae2f7a468dd.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libterm-31c4b14ee45d62c5.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libgetopts-fc86de0fe7be563d.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libunicode_width-eee464f106bb1692.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\librustc_std_workspace_std-a13e0c0cecc328ed.rlib" "E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\liblibmimalloc_sys-146c87855da4a083.rlib" "-Wl,--start-group" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libstd-8394c9398cbadee5.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libpanic_unwind-886e4f572a6d227c.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libobject-17cf826d0f7d2ed3.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libaddr2line-0dad5a98c26b9739.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libgimli-dfab679e0565d217.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\librustc_demangle-9f6866b4336a1d1c.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libhashbrown-81ade66b9e4c9cd5.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\librustc_std_workspace_alloc-4149a301273ebf0c.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libunwind-738a3de7ad0cd3a3.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libcfg_if-06d418ce9254f841.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\liblibc-7c3291a8d67347b3.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\liballoc-9f2b7229edc9025e.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\librustc_std_workspace_core-4ae0cac901a4e3c1.rlib" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libcore-9e39cfd0b85a4b6e.rlib" "-Wl,--end-group" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\libcompiler_builtins-de70f57e5c136950.rlib" "-Wl,-Bdynamic" "-lkernel32" "-ladvapi32" "-lws2_32" "-luserenv" "-lgcc_eh" "-l:libpthread.a" "-lmsvcrt" "-lmingwex" "-lmingw32" "-lgcc" "-lmsvcrt" "-luser32" "-lkernel32" "C:\Users\RyanM\.rustup\toolchains\stable-x86_64-pc-windows-gnu\lib\rustlib\x86_64-pc-windows-gnu\lib\rsend.o" = note: E:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: E:\fork\mimalloc_rust\target\x86_64-pc-windows-gnu\debug\deps\liblibmimalloc_sys-146c87855da4a083.rlib(random.o): in function os_random_buf': E:\fork\mimalloc_rust\libmimalloc-sys/c_src/mimalloc/src/random.c:171: undefined reference toBCryptGenRandom' collect2.exe: error: ld returned 1 exit status`

ryancinsight commented 3 years ago

using #58 adding println!("cargo:rustc-link-lib=bcrypt"); to the end of build.rs main function allows tests to run properly on windows gnu. if it is due to ignored pragmas println!("cargo:rustc-link-lib=psapi"); worked but didn't appear needed for tests.

`running 6 tests test tests::it_frees_allocated_memory ... ok test tests::it_frees_zero_allocated_big_memory ... ok test tests::it_frees_zero_allocated_memory ... ok test tests::it_frees_allocated_big_memory ... ok test tests::it_frees_reallocated_memory ... ok test tests::it_frees_reallocated_big_memory ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Doc-tests mimalloc

running 3 tests test src\lib.rs - (line 21) ... ignored test src\lib.rs - (line 9) ... ignored test src\lib.rs - MiMalloc (line 56) ... ignored

test result: ok. 0 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out`