[libcxx] [ci] Add a Windows CI buildkite configuration

[ldionne]

This is great, thanks a lot for working on it!

libcxx/utils/ci/buildkite-pipeline.yml

293

How far are we from fixing this?

libcxx/utils/ci/run-buildbot

464–471

Shouldn't we fix those issues instead of working around them? Since we're soft-failing the runner anyway, we could add the job without those work arounds and let it soft-fail until we've fixed all issues?

[mstorsjo]

libcxx/utils/ci/buildkite-pipeline.yml

293

This runner uses the same base image as the main llvm premerge checks, and adding bash to the path there breaks the llvm tests. (The breakage is due to odd/untested combinations of windows containers and the posix emulation layer used by bash.) It's fixed on the main branch, but the same runner also tests release branches, so it needs backporting to all old releases it might test. So hopefully no biggie, but might take a little while.

libcxx/utils/ci/run-buildbot

464–471

You mean the experimental lib issue, or all the rest?

The int128 in msvc envs issue is a bit bigger (I'm not planning on working on that one as I mainly focus on mingw environments, and it's quite unclear how to fix best) - the lack of int128 precision doesn't matter for ABI here, and it only affects like 1 test in one file at most, so I'd like to have it tested this way for maximal CI coverage, so we can test fs even in msvc environments.

The experimental lib issue makes every single testcase fail to link, as things stand right now. I can look into it (it might be a pretty small thing) though, but the override here can be removed once that's fixed, too.

[ldionne]

libcxx/utils/ci/run-buildbot

464–471

I meant all of it.

re: int128, can't we detect it properly automatically? We already try to detect it in __config but it looks like we're failing to detect it properly in this instance.

re: -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO, I think we should fix the underlying issue instead.

re: -loldnames, I think it's OK to wait until the CI upgrades to Clang 12

If you think those are unreasonable to wait for, I could be convinced to ship as-is. The only thing is that I dislike shipping something in a state where we know for sure we'll have to change in the future *without leaving a marker for us to find it again in the future*.

[mstorsjo]

libcxx/utils/ci/run-buildbot

464–471

Re int128, the issue is that the compiler does support it, fully (which __config detects), but in MSVC environments we normally don't have clang_rt builtins available, so we lack the runtime support for e.g. division helper functions. This was discussed in D91139 (https://reviews.llvm.org/D91139#2429595 in particular) - the intent from people working on the MSVC configurations is to provide the runtime helpers, somehow, but it's not there yet.

Re the experimental library, the issue is that c++experimental is only built as a static library, not a dynamic one. In MSVC environments (in particular, how libc++ names things), a dynamic lib foo is named foo.lib while a static lib foo is named libfoo.lib. This, combined with how tests are invoked via the GNU style driver (which is kinda foreign for an MSVC environment) means the AddLinkFlag('-lc++experimental') in libcxx/utils/libcxx/test/params.py would need to be an AddLinkFlag(msvc ? '-llibc++experimental' : '-lc++experimental') - and we don't have info about the target at that place.

Re -loldnames, that one shouldn't be too bad IMO, as it can be removed at some fairly clearly defined point in the future, just not quite yet. After Clang 12 is released, there's still a cycle of waiting for the package manager used in the CI docker images to update to provide it, then bothering @goncharov to update the image and the runners again, etc.

I'd much rather have this in, with a few minor warts, so we can start having the benefit of the CI on all the other patches for cleaning up the state of the tests on windows. Contrary to changes in the library itself and/or the tests, the CI setup doesn't affect how things are shipped or anything permanent in that sense.

Regarding finding these issues later, would you feel better if I'd slap a FIXME: Remove when int128 helpers are available in MSVC environments, FIXME: Remove when the test driver can find libc++experimental.lib, FIXME: Remove once the CI runner has Clang 12 and FIXME: Remove once the CI runner has bash in the path on these instances?

[mstorsjo]

libcxx/utils/ci/run-buildbot

464–471

With D99178 we can get rid of the hardcoded disabling of the experimental library (and with D99177 one can actually run tests with it).

[mstorsjo]

libcxx/utils/ci/run-buildbot

464–471

D99177 which turns out to be a bit of mess isn't strictly needed for dropping the hardcoded flag here, only D99178 is.

[goncharov]

That's a lot of failed tests reported. If you want to add this step before fixing all tests I would probably need to modify test result processing to ignore all from the jobs that are "soft failed". Otherwise every review author will see them. WDYT?

[mstorsjo]

In D99093#2646967, @goncharov wrote:

That's a lot of failed tests reported. If you want to add this step before fixing all tests I would probably need to modify test result processing to ignore all from the jobs that are "soft failed". Otherwise every review author will see them. WDYT?

Most of those failures have been silenced with XFAIL: LIBCXX-WINDOWS-FIXME separately in D99095. There's a few other minor mismatches with expectations left (D99096 and some other bits), but it's supposed to be silent by the time this one is merged. I guess I could give this one a new run through CI to reflect the current state of affairs.

[mstorsjo]

Rerun CI to see the number of failures now reported (should be only a handful, and there's patches under discussion for most of them). Rephrased the comments, adding clearer TODO comments for the bits that are non-ideal, with clear comments about what's required for them to be removed.

[curdeius]

LGTM.

[mstorsjo]

Ping @ldionne - while everything isn't in the ideal perfect shape yet, it'd be good to be able to proceed with this (either in this form, or with one TODO removed by merging D99178 first), so we can keep track of other regressions - e.g. D99398 already snuck in new testcases that don't work as-is on Windows.

[mstorsjo]

Removed the soft_fail, as there are no more unexcpected fails/passes.

[mstorsjo]

Rebased past 9abff04e506f.

[mstorsjo]

Thanks! This will be quite valuable for avoiding regressions, and for having one defined reference configuration with a known outcome.

libcxx/utils/ci/buildkite-pipeline.yml

agents:
       queue: "libcxx-builders"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
 
   - label: "Runtimes build"
     command: "libcxx/utils/ci/run-buildbot runtimes-build"
     artifact_paths:
       - "**/test-results.xml"
     agents:
       queue: "libcxx-builders"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
 
+  - label: "Windows"
+    # TODO: The CI runner doesn't have bash in the path currently. Once it
+    # has that, remove the absolute path and just call 'bash' here.
+    command: "\"\\Program Files\\Git\\usr\\bin\\bash\" libcxx/utils/ci/run-buildbot generic-win"
+    artifact_paths:
+      - "**/test-results.xml"
+    agents:
+      queue: "windows"
+    retry:
+      automatic:
+        - exit_status: -1  # Agent was lost
+          limit: 2
+
   # All jobs defined before this `wait` will run whenever a CI job is started.
   # Jobs defined after the `wait` will run only if all the jobs above succeeded.
   # We use this to reduce the load on testers that have more constrained resources
   # and avoid running builds that we know fail anyway.
   - wait
 
   # Build with the configuration we use to generate libc++.dylib on Apple platforms
   - label: "Apple system"

libcxx/utils/ci/run-buildbot

armv7)
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Armv7Arm.cmake"
     check-cxx-cxxabi
 ;;
 armv7-noexceptions)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Armv7Thumb-noexceptions.cmake"
     check-cxx-cxxabi
 ;;
+generic-win)
+    clean
+    # TODO: The CI runner doesn't have bash in the path currently, and it's
+    # needed for running tests. Once it's available out of the box, remove this.
+    export PATH="$PATH:/c/Program Files/Git/usr/bin"
+
+    # TODO: Clang-cl in MSVC configurations don't have access to compiler_rt
+    # builtins helpers for int128 division. See
+    # https://reviews.llvm.org/D91139#2429595 for a comment about longterm
+    # intent for handling the issue. In the meantime, define
+    # -D_LIBCPP_HAS_NO_INT128 (both when building the library itself and
+    # when building tests) to allow enabling filesystem for running tests,
+    # even if it uses a non-permanent ABI.
+
+    # TODO: The CI runner currently uses Clang 11, which doesn't implicitly
+    # link in oldnames.lib (which is needed for some tests) when compiling
+    # with the plain "clang" driver, as the tests do (as opposed to using
+    # the "clang-cl" driver for compiling). When the CI runner runs
+    # Clang 12, the "-loldnames" option can be dropped.
+
+    # TODO: Currently, building with the experimental library breaks running
+    # tests (the test linking look for the c++experimental library with the
+    # wrong name, and the statically linked c++experimental can't be linked
+    # correctly when libc++ visibility attributes indicate dllimport linkage
+    # anyway), thus just disable the experimental library. Remove this
+    # setting when cmake and the test driver does the right thing automatically.
+
+    echo "--- Generating CMake"
+    cmake -S "${MONOREPO_ROOT}/libcxx" \
+          -B "${BUILD_DIR}" \
+          -GNinja -DCMAKE_MAKE_PROGRAM="${NINJA}" \
+          -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+          -DCMAKE_C_COMPILER=clang-cl \
+          -DCMAKE_CXX_COMPILER=clang-cl \
+          -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml" \
+          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO \
+          -DLIBCXX_ENABLE_FILESYSTEM=YES \
+          -DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
+          -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
+          -DLIBCXX_TEST_LINKER_FLAGS="-loldnames"
+    echo "+++ Running the libc++ tests"
+    ${NINJA} -vC "${BUILD_DIR}" check-cxx
+;;
 *)
     echo "${BUILDER} is not a known configuration"
     exit 1
 ;;
 esac