[libcxx-commits] [PATCH] D154246: [libc++] Add check for building with picolibc

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/utils/ci/run-buildbot:625
+    echo "--- Installing libc++, libc++abi and libunwind to a fake location"
+    ${NINJA} -vC "${BUILD_DIR}" install-cxx install-cxxabi install-unwind
+;;
----------------
ldionne wrote:
> michaelplatings wrote:
> > ldionne wrote:
> > > We're not running the tests? I would expect
> > > 
> > > ```
> > > echo "+++ Running the libc++ tests"
> > > ${NINJA} -vC "${BUILD_DIR}" check-cxx check-cxxabi check-unwind
> > > ```
> > Yes I'd also like to get the tests passing or xfailed but that's going to require a fair bit of work. Off the top of my head:
> > * Add linker scripts.
> > * Figure out how to run the tests via emulation in the Docker containers.
> > * Xfail all the tests that rely on features that picolibc doesn't support.
> > All of these have been done already with various hacks in various downstream repositories, but finding a suitable way to do them properly in the libcxx sources is something I anticipate could take months.
> > 
> > For now I just want to set a baseline of keeping libcxx+picolibc building. Once that's in place we can start ratcheting up the quality bar.
> > For now I just want to set a baseline of keeping libcxx+picolibc building. Once that's in place we can start ratcheting up the quality bar.
> 
> Ratcheting the quality bar is usually done by first setting up the test suite and adding a bunch of `XFAIL` and `UNSUPPORTED` markup that is then gradually removed. In this case, we're not running the test suite at all, and that's insufficient to claim support for it. I hate to be that person, but this patch needs to include whatever's necessary to build and run the test suite before it is merged.
> 
> One thing we could discuss is actually only compiling and linking all the tests, but not running them right away since that might require more complicated setup. I'm not sure we want to set that precedent, but I'd definitely be open to the discussion if we had everything compiling and linking. If we were to do that, we'd replace the `%{exec}` substitution by something like `true` and that would cause the tests not to be executed for real.
I too would like to see `XFAIL` and `UNSUPPORTED`. That gives us more insights in which parts work and which parts don't work. This also helps with future patches to decide how much effort we should spend on getting it to work with picolib. For example getting locales tested properly may take quite a bit of effort; at least that has been my experience. When adding a new locale test it's easier to justify to add a new `XFAIL/UNSUPPORTED`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154246