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

Fix windows support #99

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Fix windows support #99

wants to merge 12 commits into from

Conversation

roblabla
Copy link

@roblabla roblabla commented Aug 2, 2024

The windows targets weren't working at all for me:

  1. The wrong target was being used. i686-pc-win32 doesn't exist as far as jemalloc's build system is concerned, only i686-pc-mingw32 does (and if jemalloc detects the compiler is MSVC, it will automatically switch to an MSVC-based build).
  2. The library wasn't getting found, because it was called jemalloc_s instead of just jemalloc
  3. The unprefixed_on_supported_target feature was broken because jemalloc automatically prefixes macos and windows targets for some reason.

Finally, I also added support for the win7 target, which is a tier3 target supporting windows 7.

Note that this is tested manually on my end using a bit of a bespoke cross-compilation toolchain. The crate doesn't successfully build on a more "normal" toolchain due to autoconf failing to build when the compiler path contains spaces. I'd like to add a CI, but I'd need to first need to find a way to fix that issue...

@BusyJay
Copy link
Member

BusyJay commented Aug 2, 2024

Interesting, you can also update the action files to add windows target and prove it works.

@roblabla
Copy link
Author

roblabla commented Aug 2, 2024

So as I said, I have a bit of a weird setup where I use a cross-compiling toolchain to build the crate, and that works.

However, when I tried to add it in CI, but doing the naive thing of just running cargo test in a windows runner doesn't work, because it tries to run with CC=C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\bin\HostX64\x64\cl.exe, and that doesn't work because of the space in the "Program Files". See https://github.com/JustRustThings/jemallocator/actions/runs/10164835677/job/28111216397

This might be fixable by entering a visual studio shell so the cl.exe program is in the PATH. I'll give it a try.

@BusyJay
Copy link
Member

BusyJay commented Aug 3, 2024

I don't think space in path is the problem.

checking for x86_64-pc-mingw32-gcc... C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\bin\HostX64\x64\cl.exe

The line shows that full compiler path is used. However the target it tries doesn't match the compiler it uses.

@roblabla
Copy link
Author

roblabla commented Aug 3, 2024

The space is absolutely the problem, but the logs don't show it clearly. If I run the build locally on a windows vm and check the config.log, it has an error to the tune of "C:\Program: no such file or directory", which stops the build.

Autoconf does not support spaces in the compiler path. It's a rather well-known problem. I just had a pretty solid idea to avoid it though: I can use the short path syntax (c:\progra~2) to avoid the space 👀.

The target is not wrong. As I explain in the post, the way you do an msvc build with jemalloc is by building for mingw/cygwin with an msvc compiler in CC. The autoconf script will detect that and set use_msvc to yes, at which point the msvc codepaths will be used.

jemalloc-sys/build.rs Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
jemalloc-sys/build.rs Show resolved Hide resolved
@roblabla
Copy link
Author

roblabla commented Sep 23, 2024

Finally found the time to push this across the finish line, this PR now builds correctly in CI: https://github.com/roblabla/jemallocator/actions/runs/10995602458

Had to fix a compilation issue in jemalloc when using MSVC 2022, which I submitted both:

I then had to update the submodule to point to the new commit, and regenerate the vendored configure script.

With all this in place, windows support is now working properly.

I also cleaned up the commit a little, removing the DBG commits and providing more description to some otherwise "bare" commits.

To successfully build jemalloc for msvc-windows, we need to pretend
we're doing a mingw build. The configure script will then check if the
compiler is actually an MSVC compiler (by checking if it defines
_MSC_VER), and if so switch to an MSVC build (which enables a bunch of
compatibility hacks, such as removing the need for pthread).

Signed-off-by: roblabla <[email protected]>
On windows, the generated library is called jemalloc_s instead of
jemalloc_pic.

Signed-off-by: roblabla <[email protected]>
It turns out, jemalloc doesn't support having unprefixed malloc on these
targets. When --with-jemalloc-prefix is not specified, jemalloc will
implicitly add a `je_` prefix regardless.

Signed-off-by: roblabla <[email protected]>
Signed-off-by: roblabla <[email protected]>
When invoking MSVC CC, a handful of environment variables must be set
for the compiler to find its supporting files (such as system includes,
libraries, etc...). When invoking it through cc-rs, these environment
variables are automatically detected and set.

However, we currently don't set those environment variables when running
`./configure` or `make`, leading to those commands failing due to the
compiler not having access to any of the system include/libraries.

To fix this, we ask cc-rs to give us the environment variables to set,
and set them whenever running `configure` and `make`.

Signed-off-by: roblabla <[email protected]>
Autoconf does not support spaces in the compiler path. This is not
usually a problem, as on most platforms, the compiler will reside in a
path without spaces (such as /usr/bin or /opt). However, on windows, the
MSVC compiler is usually found under C:\Program Files, which causes all
hell to break loose.

Similarly, the jemalloc Makefile also assumes the shell (stored in the
SHELL variable) is in a folder without spaces, and is used unquoted all
throughout the makefile, causing issues if the shell is in C:\Program
Files, such as when using Git Bash (which installs in C:\Program
Files\Git\bin\bash).

Thankfully, windows has something called "Short Paths", which we can use
to turn C:\Program Files into C:\PROGRA~2, eliminating the path.

If the build script detects the compiler path has a space, and is
running on windows, it will attempt to turn the path into a short path
and run with this.

Signed-off-by: roblabla <[email protected]>
Signed-off-by: roblabla <[email protected]>
Background Threads are unsupported on windows in jemalloc, due to
requiring pthreads (which aren't available on windows, at least on the
msvc target). Trying to enable it will cause a runtime error causing the
program to immediately abort.

As such, to avoid problems, we set windows as a target that does not
support background_threads.

Signed-off-by: roblabla <[email protected]>
Profiling is not currently supported by jemalloc on windows, as it
requires a profiling backend based on libraries available on Windows.

As such, we disable the profiling-related tests when running on windows.

Signed-off-by: roblabla <[email protected]>
@Aerocatia
Copy link

Hi, I tested this branch and it does not work when targeting windows-gnu from a Linux host. It does not work on the upstream version either. It complains about finding the static library and background_threads_runtime_support.

@Aerocatia
Copy link

Giving it a quick look I would say the issue is build.rs assuming the target for itself is the actual build target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants