[libcxx-commits] [PATCH] D115398: [libcxx] Use LIBCXX_EXECUTOR, LIBCXX_TEST_COMPILER_FLAGS and LIBCXX_TEST_LINKER_FLAGS in new test configs

Fri Dec 10 09:34:59 PST 2021

ldionne added inline comments.

================
Comment at: libcxx/test/configs/cmake-bridge.cfg.in:33-34
+config.substitutions.append(('%{executor}', '@LIBCXX_EXECUTOR@'))
+config.substitutions.append(('%{user_compile_flags}', '@LIBCXX_TEST_COMPILER_FLAGS@'))
+config.substitutions.append(('%{user_link_flags}', '@LIBCXX_TEST_LINKER_FLAGS@'))
----------------
mstorsjo wrote:
> ldionne wrote:
> > mstorsjo wrote:
> > > ldionne wrote:
> > > > About these two -- I would actually love to get rid of them. Instead, you could create your own simple `.cfg.in` file for MingGW where you set the required flags.
> > > > 
> > > > I loathe forwarding flags from CMake to the Lit config, IMO that's just unnecessary plumbing.
> > > I know you're not a fan of such settings, but I find them vital. I, on the other hand, loathe setups where it's only possible to build and run things in a finite, fixed set of configurations, instead of passing individual options for configuring one's build.
> > > 
> > > Sure we can only test and support a finite, fixed set of configurations, but it should be possible for vendors to build and test in other configs by just configuring things with additional flags on top of the existing configs, as long as it's not different enough warranting a separate config.
> > > 
> > > As a very concrete example; for the clang-cl builds, we currently build and test with setting `-DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128"`. I've considered extending it so we'd test both in the default configuration (with filesystem disabled) and this config (with int128 disabled so that msvc environments can test filesystem). That would require adding a separate test config file for that too.
> > > 
> > > And when such a detail is configured by adding things to `CMAKE_CXX_FLAGS`, it's best if the corresponding flag for tests can be passed in tandem right next to it, via `LIBCXX_TEST_COMPILER_FLAGS` to cmake, instead of using a separate test config for it.
> > Needing to pass additional compiler flags to the test suite is always a way to hack around some deficiency somewhere else. For instance, `_LIBCPP_HAS_NO_INT128` comes from:
> > 
> > ```
> > # 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.
> > ```
> > 
> > If we have a proper option that is not a hack, then it should be supported 1st class in `features.py` or in `params.py`. I don't want to bloat the number of base substitutions required to run the libc++ test suite in order to facilitate doing hacks -- I'm fine with these hacks remaining in the state of "being a temporary hack".
> > 
> Right, `_LIBCPP_HAS_NO_INT128` is indeed a bit of a workaround.
> 
> >  I don't want to bloat the number of base substitutions required to run the libc++ test suite
> 
> Well, technically, these substitutions aren't base substitutions needed for running the libc++ test suite - you could write a test config from scratch that don't use them, but it's more of a contract between `cmake-bridge` and the individual configs. But this is splitting hairs...
> 
> Anyway, back on track constructively:
> 
> - `LIBCXX_EXECUTOR` isn't essential for the windows test configs, but it's just something I'd appreciate: I'm not a fan of needing to make a duplicate copy of a file (which will be left out of sync when the original changes) just to adjust an option that is supposed to be user settable, when one could just specify it among with the rest of options for the build on the cmake command line.
> 
> - For setting defines like `_LIBCPP_HAS_NO_INT128`, if one sets something like that via `CMAKE_CXX_FLAGS` when building the library, any user of the library should be setting it in the same way too - so that's brittle indeed. I know this particular case is a bit of a workaround, but more generalized, I think a vendor should be able to easily add custom defines into `__config_site`. That way, such a custom configuration gets applied consistently both for the library itself and for users of it. Would you be ok with such a patch, adding a cmake option like `LIBCXX_EXTRA_SITE_DEFINES`?
> 
> - Just like for `LIBCXX_EXECUTOR`, one could want to set specific linker flags with `LIBCXX_TEST_LINKER_FLAGS`, not about how to link libc++ itself, but for how to link other things (libunwind, libc etc) for the specific testing scenario. (E.g. for cross testing, one wants to link as much as possible statically.) Mainly a case of me wanting to specify things on the cmake command line instead of making a duplicate copy of a test config file that I'd tweak.
I like the idea of `LIBCXX_EXTRA_SITE_DEFINES` because it solves the problem of specifying macros while building the library and while running the test suite (aka when using the library if you're a user) in a single place. I dislike that it makes it easier to keep hacks around, but I guess that's just a side effect. So I'd welcome exploring that.

I'm also fine with `LIBCXX_EXECUTOR` -- the set of executors is very finite so this is not the same as opening the door for arbitrary craziness.

I don't think `LIBCXX_TEST_LINKER_FLAGS` has a good reason to exist, and I think we shouldn't support it in this patch -- and we should remove it in a different patch.

The goal of these from-scratch configuration is specifically that they are easy to author. Instead of making them more customizable, and hence more complicated, I want to simplify them. For instance, I know it's really annoying to have to manually include the `test/support` directory and stuff like that -- we should work on removing that duplication and making from-scratch configs easier to author instead of making them more customizable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115398