[libcxx] [ci] Test mingw environments with msvcrt.dll, too #115783
Conversation
On Windows, there are many alternative C runtimes that a process can use; the modern default choice is UCRT, which is what we use and test so far. MinGW toolchains can also use msvcrt.dll, which traditionally was the default within MinGW environments, while UCRT is the new default. msvcrt.dll is a much less featureful C runtime than UCRT; a number of things don't work quite as they should (in particular, locales are essentially entirely broken within msvcrt.dll). Each release of llvm-mingw is built targeting both UCRT and msvcrt.dll; therefore, it is valuable to test libcxx upstream with this configuration as well. In most cases, both C runtime choices use the exact same codepaths, but we have one ifdef regarding this, see the current handling of strftime_l in src/support/win32/locale_win32.cpp. This adds build test coverage of this piece of code. As a number of tests fail in this configuration, only test building libcxx in this configuration for now. After adding suitable XFAILs, we can enable running the tests as well.
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxx Author: Martin Storsjö (mstorsjo) ChangesOn Windows, there are many alternative C runtimes that a process can use; the modern default choice is UCRT, which is what we use and test so far. MinGW toolchains can also use msvcrt.dll, which traditionally was the default within MinGW environments, while UCRT is the new default. msvcrt.dll is a much less featureful C runtime than UCRT; a number of things don't work quite as they should (in particular, locales are essentially entirely broken within msvcrt.dll). Each release of llvm-mingw is built targeting both UCRT and msvcrt.dll; therefore, it is valuable to test libcxx upstream with this configuration as well. In most cases, both C runtime choices use the exact same codepaths, but we have one ifdef regarding this, see the current handling of strftime_l in As a number of tests fail in this configuration, only test building libcxx in this configuration for now. After adding suitable XFAILs, we can enable running the tests as well. Full diff: https://github.com/llvm/llvm-project/pull/115783.diff 2 Files Affected:
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 2184ddd49537b5..107d23e3573f66 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -250,10 +250,11 @@ jobs:
- { config: clang-cl-no-vcruntime, mingw: false }
- { config: clang-cl-debug, mingw: false }
- { config: clang-cl-static-crt, mingw: false }
- - { config: mingw-dll, mingw: true }
- - { config: mingw-static, mingw: true }
- - { config: mingw-dll-i686, mingw: true }
- - { config: mingw-incomplete-sysroot, mingw: true }
+ - { config: mingw-dll, mingw: true, crt: ucrt }
+ - { config: mingw-static, mingw: true, crt: ucrt }
+ - { config: mingw-dll-i686, mingw: true, crt: ucrt }
+ - { config: mingw-incomplete-sysroot, mingw: true, crt: ucrt }
+ - { config: mingw-msvcrt, mingw: true, crt: msvcrt }
steps:
- uses: actions/checkout@v4
- name: Install dependencies
@@ -267,7 +268,7 @@ jobs:
- name: Install llvm-mingw
if: ${{ matrix.mingw == true }}
run: |
- curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20240606/llvm-mingw-20240606-ucrt-x86_64.zip
+ curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20240606/llvm-mingw-20240606-${{matrix.crt}}-x86_64.zip
powershell Expand-Archive llvm-mingw*.zip -DestinationPath .
del llvm-mingw*.zip
mv llvm-mingw* c:\llvm-mingw
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 3df7b00a8aa09d..4bc9be39d1a386 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -772,6 +772,15 @@ mingw-incomplete-sysroot)
step "Building the runtimes"
${NINJA} -vC "${BUILD_DIR}"
;;
+mingw-msvcrt)
+ # All tests don't pass in msvcrt based configurations (and we don't have
+ # the right XFAIL markings for it yet), thus initially only test building.
+ clean
+ generate-cmake \
+ -C "${MONOREPO_ROOT}/libcxx/cmake/caches/MinGW.cmake"
+ step "Building the runtimes"
+ ${NINJA} -vC "${BUILD_DIR}"
+;;
aix)
clean
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/AIX.cmake" \
|
|
@llvm/pr-subscribers-github-workflow Author: Martin Storsjö (mstorsjo) ChangesOn Windows, there are many alternative C runtimes that a process can use; the modern default choice is UCRT, which is what we use and test so far. MinGW toolchains can also use msvcrt.dll, which traditionally was the default within MinGW environments, while UCRT is the new default. msvcrt.dll is a much less featureful C runtime than UCRT; a number of things don't work quite as they should (in particular, locales are essentially entirely broken within msvcrt.dll). Each release of llvm-mingw is built targeting both UCRT and msvcrt.dll; therefore, it is valuable to test libcxx upstream with this configuration as well. In most cases, both C runtime choices use the exact same codepaths, but we have one ifdef regarding this, see the current handling of strftime_l in As a number of tests fail in this configuration, only test building libcxx in this configuration for now. After adding suitable XFAILs, we can enable running the tests as well. Full diff: https://github.com/llvm/llvm-project/pull/115783.diff 2 Files Affected:
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 2184ddd49537b5..107d23e3573f66 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -250,10 +250,11 @@ jobs:
- { config: clang-cl-no-vcruntime, mingw: false }
- { config: clang-cl-debug, mingw: false }
- { config: clang-cl-static-crt, mingw: false }
- - { config: mingw-dll, mingw: true }
- - { config: mingw-static, mingw: true }
- - { config: mingw-dll-i686, mingw: true }
- - { config: mingw-incomplete-sysroot, mingw: true }
+ - { config: mingw-dll, mingw: true, crt: ucrt }
+ - { config: mingw-static, mingw: true, crt: ucrt }
+ - { config: mingw-dll-i686, mingw: true, crt: ucrt }
+ - { config: mingw-incomplete-sysroot, mingw: true, crt: ucrt }
+ - { config: mingw-msvcrt, mingw: true, crt: msvcrt }
steps:
- uses: actions/checkout@v4
- name: Install dependencies
@@ -267,7 +268,7 @@ jobs:
- name: Install llvm-mingw
if: ${{ matrix.mingw == true }}
run: |
- curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20240606/llvm-mingw-20240606-ucrt-x86_64.zip
+ curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20240606/llvm-mingw-20240606-${{matrix.crt}}-x86_64.zip
powershell Expand-Archive llvm-mingw*.zip -DestinationPath .
del llvm-mingw*.zip
mv llvm-mingw* c:\llvm-mingw
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 3df7b00a8aa09d..4bc9be39d1a386 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -772,6 +772,15 @@ mingw-incomplete-sysroot)
step "Building the runtimes"
${NINJA} -vC "${BUILD_DIR}"
;;
+mingw-msvcrt)
+ # All tests don't pass in msvcrt based configurations (and we don't have
+ # the right XFAIL markings for it yet), thus initially only test building.
+ clean
+ generate-cmake \
+ -C "${MONOREPO_ROOT}/libcxx/cmake/caches/MinGW.cmake"
+ step "Building the runtimes"
+ ${NINJA} -vC "${BUILD_DIR}"
+;;
aix)
clean
generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/AIX.cmake" \
|
|
I have a small stack of patches I can send separately, that adds the necessary XFAILs to make this a build-and-test configuration too, but starting out with build-only seems very straightforward, and can add valuable test coverage for the changes in #115752. |
There was a problem hiding this comment.
I don't want to oppose this change just for the sake of it, but reading the PR description made it look like msvcrt.dll is really a legacy thing that is being moved away from (and has a bunch of things that don't work like locales). This makes me wonder whether we should even strive to support it. What is your take on that?
It's a legacy configuration indeed, and it's not as functional as UCRT. Many users do move away from it - however I don't see being able to entirely drop it within any foreseeable future either. (Note, my toolchain distribution is "upstream-only", I don't carry any downstream patches on anything, as I make sure to work entirely upstream.) So I do want to keep supporting it - with a slightly different definition of "support"; i.e. quite expected that everything doesn't work as it should, but building libcxx should work at the very least, and plain basic things should work. In the vast majority of cases, this configuration isn't something that costs us extra maintainance - within the whole libcxx, we have one single If we keep it as a build-only CI configuration, it's quite cheap wrt CPU time on the CI. If we want to, we can also add running of tests with it; it requires adding a handful of extra |
That sometimes makes sense, and sometimes it doesn't. I think it makes sense for mainstream configurations to be supported upstream, but folks must also be OK with maintaining a bit of downstream diff for non-mainstream configurations. We do that at Apple as well -- we have many downstream configurations that we don't try to contribute upstream to avoid creating a support burden upstream. It's all about finding a balance. I'm not claiming
To be clear, this is a "death by a thousand cuts" scenarios -- we support many platforms and if we're not fairly aggressive about our support policy, we quickly end up in a state where the code base is a mess.
I had actually not noticed that we were only building the runtimes, not testing them. I don't think that's sufficient -- we want to also run the tests, otherwise this clearly isn't worth supporting upstream. |
Sure, that's understandable. In this case, I do hope that it can be taken into account, that while I'm one toolchain vendor that builds it in this configuration, I believe that there are others that also build it in the same configuration, so keeping it buildable without requiring downstream patches is valuable. And in the case of my toolchain, as a policy, I don't do downstream patches.
Ok, that's reasonable - and actually running tests obviously is even more valuable. I do have a couple of patches on top of this that makes the tests run in this configuration - it requires a handful of extra On the other side, if you decide you don't want to take this configuration in, I would still hope that we can keep this build configuration working through refactorings like #115752; cherrypicking this commit into the branch would add the test coverage at least while working on that code while it can be dropped when that's done. (Refactorings like that one is one of the very few cases within libcxx where the distinction of this configuration makes any difference at all.) Alternatively, I can of course also test the PR (when it otherwise seems to work) and let you know what tweaks it may require to keep things working. |
There was a problem hiding this comment.
Sorry for the delay. Upon revisiting this, I think it's reasonable to support but I'd like us to have real testing, i.e. not only build-time testing.
How many XFAILs are we talking about in order to make the test suite pass? Just to know the rough scale of the changes.
Also, we should add this to our documentation.
From looking at my local branch for this, it looks like it's around 40 XFAILs. So not very pretty, but not entirely unsurmountable.
Sure |
On Windows, there are many alternative C runtimes that a process can use; the modern default choice is UCRT, which is what we use and test so far.
MinGW toolchains can also use msvcrt.dll, which traditionally was the default within MinGW environments, while UCRT is the new default.
msvcrt.dll is a much less featureful C runtime than UCRT; a number of things don't work quite as they should (in particular, locales are essentially entirely broken within msvcrt.dll).
Each release of llvm-mingw is built targeting both UCRT and msvcrt.dll; therefore, it is valuable to test libcxx upstream with this configuration as well. In most cases, both C runtime choices use the exact same codepaths, but we have one ifdef regarding this, see the current handling of strftime_l in
src/support/win32/locale_win32.cpp. This adds build test coverage of this piece of code.
As a number of tests fail in this configuration, only test building libcxx in this configuration for now. After adding suitable XFAILs, we can enable running the tests as well.