[libcxx-commits] [PATCH] D99093: [libcxx] [ci] Add a Windows CI buildkite configuration

Tue Mar 23 06:12:49 PDT 2021

mstorsjo added inline comments.

================
Comment at: libcxx/utils/ci/run-buildbot:430-437
+    # -D_LIBCPP_HAS_NO_INT128 allows building filesystem with a MSVC
+    # setup that lacks the necessary builtins for int128.
+
+    # -loldnames is needed for some tests unless using Clang 12 (the
+    # CI runner has Clang 11 currently).
+
+    # -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO is needed, as the testsuite
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > ldionne wrote:
> > > mstorsjo wrote:
> > > > ldionne wrote:
> > > > > 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?
> > > > 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.
> > > 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*.
> > 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?
> With D99178 we can get rid of the hardcoded disabling of the experimental library (and with D99177 one can actually run tests with it).
D99177 which turns out to be a bit of mess isn't strictly needed for dropping the hardcoded flag here, only D99178 is.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99093/new/

https://reviews.llvm.org/D99093