Skip to content

Consider builds for UCRT64? #3167

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

Closed
Berrysoft opened this issue Apr 5, 2021 · 31 comments
Closed

Consider builds for UCRT64? #3167

Berrysoft opened this issue Apr 5, 2021 · 31 comments

Comments

@Berrysoft
Copy link

It seems that MSYS2 has added arch support for ucrt64, which uses Universal CRT (64-bit) as its C runtime, and provides a full toolchain. Would you consider add builds for that arch?

@rimrul
Copy link
Member

rimrul commented Apr 5, 2021

That isn't really an architecture, but a different CRT. We allready do dynamically load some UCRT functions (like strftime) at runtime.

I went looking how linux package managers handle packages expecting alternative libc implementations and it seems like the answer for both pacman and apt seems to be they pretty much don't. So maybe a diffrent pseudo-arch is not the worst approach.

I don't think we'd want to offer actual release builds linked against UCRT in parallel to the MSVCRT builds, though. It just adds unnesscessary maintenance complexity. So from my perspective we'd want to have UCRT32 builds as well as UCRT64 builds working before we produce any official UCRT releases and then probably offer just UCRT releases.

This raises the interesting side question, what do we link the arm64 builds against? UCRT is probably our only option there, isn't it? That's probably a nice source of hard to pin-point differences between arm and intel builds.

@rimrul
Copy link
Member

rimrul commented Apr 5, 2021

They also currently only seem to do UCRT builds for MinGW-packages, not MSYS2-packages.

Forget what I said. MSYS2-packages link against newlib.

@Berrysoft
Copy link
Author

Well, I know it is not really an architecture, but MSYS2 (or, MinGW-packages) put ucrt64 parallel with mingw64, just like different archs. I'm asking because I'm using msys2 with git-for-windows package source, and wishing to install a package like mingw-w64-ucrt-x86_64-git.

@dscho
Copy link
Member

dscho commented Apr 6, 2021

@Berrysoft could you show us how such a thing would look like (preferably as a PR at git-for-windows/MINGW-packages)?

@Berrysoft
Copy link
Author

@dscho I'm trying so, but it maybe cost some more time because MSYS2 doesn't provide enough packages for ucrt64 now.

@dscho
Copy link
Member

dscho commented Apr 7, 2021

@Berrysoft thank you for researching that. Could you maybe keep an eye out and let us know once enough packages are available to continue on this quest?

@Berrysoft
Copy link
Author

@dscho OK, I'll try to PR after they're avaliable.

@Berrysoft
Copy link
Author

Berrysoft commented Oct 13, 2021

I'd like to know how did you packaged cv2pdb. I didn't find the PKGBUILD in the package repo.

I also would like to know how did you packaged python2, as it is deleted from the offical msys2 repo.

@dscho
Copy link
Member

dscho commented Oct 13, 2021

I'd like to know how did you packaged cv2pdb. I didn't find the PKGBUILD in the package repo.

The build definition is maintained here: https://github.com/git-for-windows/build-extra/tree/main/mingw-w64-cv2pdb

I also would like to know how did you packaged python2, as it is deleted from the offical msys2 repo.

We didn't package it, we consumed it from MSYS2 when it still was offered.

@Berrysoft
Copy link
Author

Well, I have to admit that the process trying to build git for windows in UCRT64 env is frustrating. It seems that there are many places hard coding mingw64 and mingw32, and the build process even stopped at beginning, complained missing syslog.h, which is already in compat/win32.

@Berrysoft
Copy link
Author

I finally succeeded using cmake, with a little fix. Do you consider totally move to cmake? It will simplify a lot.

@dscho
Copy link
Member

dscho commented Oct 13, 2021

We do have a working CMake definition, but I can't really say what we would lose by switching to it. Neither do I see any big win... what does UCRT bring us? Is it available on every Windows version we support? Does it need to be installed manually?

@Berrysoft
Copy link
Author

Anyway you need to ship git with a msys2 runtime, and there is UCRT support in msys2.

UCRT is a much newer C runtime than msvcrt, which is released more than 20 years ago. It contains new features, for example, Unicode support, and is continually updated by MS. However, UCRT is not available on Windows below 10 by default. It needs to be installed.

I'm asking for UCRT64 builds not for replacing the MINGW64 builds, but only asking if it can, and if it could be implemented with little effort. We now know that it could be built with cmake, and so could the CLANG64 and CLANGARM64 ones. If the cmake build system is improved to be able to replace the old build system, we could then get a lot of new types of builds, instead of only 32-bit and 64-bit (linking with msvcrt) ones.

@dscho
Copy link
Member

dscho commented Oct 14, 2021

There is indeed work underway to build ARM64 artifacts, but they will be hybrid. Remember, Git for Windows bundles files from ~150 separate packages. Git is but one of them. All the other packages are built using either the MSYS or the MINGW toolchains, and have currently no support for ARM64. The most obvious challenge is to get the MSYS2 runtime built for ARM64. There is currently no support to bootstrap this, therefore there is no GCC targeting MSYS/ARM64. Since the MSYS2 runtime is a close fork of the Cygwin runtime, we'd have to rely on them to support ARM64 builds. Last time I checked, there was no effort under way to that end.

That, combined with the fact that Git for Windows still needs to support Windows 7, and that Git does not need any of the features available in the UCRT (it uses direct Win32 API calls for Unicode, and does not interact with the Windows GUI in any way) makes me rather unenthusiastic to embark on what I perceive as a multi-year, multi-person project with negligible benefit, at least as far as I understand it.

@Berrysoft
Copy link
Author

Well, no problem. I understand your choice, because there's a lot of work to do besides making the project built.

@rimrul
Copy link
Member

rimrul commented Oct 14, 2021

We do have a working CMake definition, but I can't really say what we would lose by switching to it.

One small thing I know is that Git built through cmake doesn't know the commit it was built from.

However, UCRT is not available on Windows below 10 by default. It needs to be installed.

It is probably installed on most supported systems through windows update.

All the other packages are built using either the MSYS or the MINGW toolchains, and have currently no support for ARM64.

The MSYS2 team are working on building UCRT64 and ARM64 MINGW-Packages. I can't tell you the exact status of that effort, though.

That, combined with the fact that Git for Windows still needs to support Windows 7

Did we ever officially stop supporting Vista? I know we've talked about doing so on multiple occasions if Vista support would block some new feature, but I don't think it ever did.

@Berrysoft
Copy link
Author

A little off-topic: cmake can know the commit with some git commands.

@dscho
Copy link
Member

dscho commented Oct 14, 2021

A little off-topic: cmake can know the commit with some git commands.

Absolutely. Our CMake-based configuration is just not there yet.

All the other packages are built using either the MSYS or the MINGW toolchains, and have currently no support for ARM64.

The MSYS2 team are working on building UCRT64 and ARM64 MINGW-Packages. I can't tell you the exact status of that effort, though.

Well, that's good! At the same time I want to caution that Git requires a Unix shell to work, in our case that's MSYS2's Bash (which is an MSYS program, not a MINGW program, as it requires the MSYS2 runtime to work). And that won't be built for ARM64 any time soon.

Did we ever officially stop supporting Vista?

Whoops, I actually had forgotten about Vista ;-) But yes, according to https://gitforwindows.org/requirements.html we still support Vista.

However, UCRT is not available on Windows below 10 by default. It needs to be installed.

It is probably installed on most supported systems through windows update.

That's something I don't want to rely on. GitHub Desktop bundles MinGit, and has no provision to take care of downloading additional files (such as UCRT). UCRT could not be bundled with it, either, for licensing and practical reasons.

If I see enough benefit to pay the very real price of spending the time with every new release to bundle UCRT-based versions, I will do so, but for the time being I think the GCC-built version needs to stay our principal way to deliver Git for Windows to the users.

Having said all that, I definitely didn't mean to discourage anyone from working on UCRT64-based builds or from improving the CMake configuration.

@dennisameling
Copy link

Just chiming in here for the ARM64-specific bits as I've been working on Git for Windows ARM64 support. In this PR I'm using llvm-mingw with the msvcrt toolchain for building Git for Windows (need that toolchain to be able to cross-compile from x64 to ARM64). That is looking good so far and the basic operations I tested (clone/push/etc.) are working correctly.

I saw the MSYS2 team has been hard at work providing Clang builds of MINGW-packages, including clangarm64, which I'm happily using now in the aforementioned PR. This is a great start, but things like full ARM64 support won't be available until the MSYS2 runtime is available for ARM64 natively, like @dscho mentioned above.

Just for the record, MSYS2 has a good overview of the different environments here: https://www.msys2.org/docs/environments/

For ARM64, using UCRT is not a problem as ARM64 support was only introduced in Windows 10 😊

@Berrysoft
Copy link
Author

I've written a new PKGBUILD using the CMake build system here: msys-git-for-windows. It may be a little simple (and somehow dirty), but works. I'm now using git packaged from this repo. Would you like to try and check it? I hope it gives some help.

@dscho
Copy link
Member

dscho commented May 17, 2022

I've written a new PKGBUILD using the CMake build system

@Berrysoft there are two major problems with this approach:

  1. As stated here, UCRT's malloc() implementation isn't well suited for Git's usage patterns, and who knows what other surprises lurk there if we switch one C runtime out for a completely different one? That's not a risk I am willing to take on the millions of Git for Windows users, not without a careful assessment.
  2. The PKGBUILD file lists several dependencies, but as far as I can tell, they are not at all integrated into the build. Which means that the build will still initialize its own vcpkg system, build all the dependencies from scratch, and use those versions. That's not a good integration into the Pacman package system.

@Berrysoft
Copy link
Author

@dscho

  1. I read the comments and it seems a performance issue. Could you explain clearer? Is there non-standard usage of malloc in git source?
  2. It is built with -DUSE_VCPKG=off, therefore the dependencies are installed from msys2.

@dscho
Copy link
Member

dscho commented May 17, 2022

  1. I read the comments and it seems a performance issue. Could you explain clearer?

I cannot explain more because I failed to get to the root of the problem.

  1. Is there non-standard usage of malloc in git source?

It must have something to do with Git's usage patterns, yes.

2. It is built with -DUSE_VCPKG=off, therefore the dependencies are installed from msys2.

How would this work? How would e.g. Visual Studio find those dependencies when running CMake?

@Berrysoft
Copy link
Author

OK, I still don't know about the problem 1, but I strongly suggest avoiding non-standard usage. Could you kindly point the usage out, so that I can produce a failure when targeting UCRT?

The problem 2: CMake could find dependencies itself. My PKGBUILD is for the MinGW envs (mingw32, mingw64, ucrt64, clang32, clang64) in Msys2, therefore no relation to Visual Studio and vcpkg. However, if there are corresponding installed libraries in the CMake searching path (specified with -DCMAKE_PREFIX_PATH=), it should just work without vcpkg.

@dscho
Copy link
Member

dscho commented May 18, 2022

I still don't know about the problem 1, but I strongly suggest avoiding non-standard usage.

@Berrysoft if by "non-standard usage" you are referring to using API functions that are either rarely used or undocumented: no, Git does not do that.

If you're referring to "using malloc() in a way that apparently hammers UCRT's poor logic, but is totally within legitimate usage patterns, glibc's malloc() simply fares better with said usage pattern", then yes, that's what Git does do.

My PKGBUILD is for the MinGW envs (mingw32, mingw64, ucrt64, clang32, clang64) in Msys2

That's what I missed. I thought you were somehow targeting Linux (based on your PR that adds PCRE2 support).

@Berrysoft
Copy link
Author

Well, UCRT's logic of malloc is just a wrapper of HeapAlloc. I won't be surprised if it performs bad with git. However, if so, msvcrt's malloc shouldn't perform well either. Anyway, although without strict benchmark, I think the short board of the performance is msys2 part (or actually, cygwin part).

I thought you were somehow targeting Linux (based on your PR that adds PCRE2 support).

Originally I added this support because the PKGBUILD is a lot easier with CMake build system, which lacks PCRE2 support. Then I found that this change should apply to all platforms, and opened the pull request to the upstream. Now the PKGBUILD mixes the old and new build system to package all contents.

I hope the new PKGBUILD could be convenient for you maintainers to package builds for new msys2 environments, especially ARM64 ones(clangarm64 in msys2).

Although git-for-windows uses a slightly different msys2 environment, I find it compiles and works well with the original environment. Did I miss something? If not, that should be perfect. Although the attempt years ago that to publish git-for-windows onto the official msys2 packages server failed, I hope it will success with this new attempt.

@dscho
Copy link
Member

dscho commented May 18, 2022

My point of raising the malloc() issue is to caution against expecting me to switch Git for Windows to use a UCRT-based build anytime soon. If already malloc() is a problem, how many more problems do lurk there that we would be fundamentally unable to address?

As to malloc() in particular, we work around that by using nedmalloc, which works much better with Git's usage patterns.

@Berrysoft
Copy link
Author

I've noticed that you've already used nedmalloc. If so, why there's problem to switch to UCRT64? The switch to a new crt won't break the impl of nedmalloc.

@dscho
Copy link
Member

dscho commented May 19, 2022

The switch to a new crt won't break the impl of nedmalloc.

I said "how many more problems do lurk there".

It is such a fundamental (if all-too-frequently ignored) rule in our business that you should not nilly-willy "fix what ain't need fixing", lest you incur a ginormous amount of problems just by using something new whose negative implications you cannot even assess.

You're trying to switch out the mingw-w64/MSVCRT combination, which we know really well, and whose shortcomings we know really well, for a clang/UCRT combination which we do not know well at all, and whose shortcomings we do not even know.

All we know is that there are shortcomings that were not supposed to be there (read on the internets how many people claimed that nedmalloc is not necessary with modern C runtimes, including UCRT, because they outperform nedmalloc).

This fact, that there is a known shortcoming already, even if it was supposed to not be there at all, concerns me and forces me to face the uncomfortable truth that there are most likely more such issues.

Of course, you can feel free to go and simply switch to clang/UCRT and see where that gets you. But I, in my role as Git for Windows maintainer, cannot. That would be irresponsible.

@dscho dscho closed this as not planned Won't fix, can't repro, duplicate, stale Sep 8, 2022
@dscho
Copy link
Member

dscho commented Sep 8, 2022

I've made up my mind, and after weighing the enormous cost of doing this against the benefit (I do not see any), I've decided that I won't consider builds for UCRT64, at least for now.

@Berrysoft
Copy link
Author

Never mind, I've built it myself.

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

4 participants