[libcxx-commits] [PATCH] D99178: [libcxx] Disable c++experimental by default in DLL builds

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D99178#2674584 <https://reviews.llvm.org/D99178#2674584>, @mstorsjo wrote:

> I agree that avoiding platform specific logic in the main cmakefiles would be nice, but I disagree that cmake cache files are the solution here.
>
> Cmake caches capture individual possible ways that you _can_ configure and build the library, but they aren't the only allowed way to do it, as there's far more possible configurations than is sensible to enumerate.
>
> For this particular case, procedural logic in cmake captures it much better.
>
> If building for windows as a DLL, the experimental library should be implicitly disabled if the user expressed no opinion on the matter. If the user requested the library to be enabled, we should tell him why that can't be done. If building for windows but as a static library only, the experimental library works fine and can be enabled by default.

For the record, the logic added in this patch technically doesn't work, because `LIBCXX_ENABLE_SHARED` and `LIBCXX_ENABLE_STATIC` are not mutually exclusive. So, for example, if you set `LIBCXX_ENABLE_SHARED=ON` but also `LIBCXX_ENABLE_STATIC=ON`, you might want to have the experimental library available for use with the static version of libc++ but not the shared version. I don't see a way to make that possible without adding an option like `LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_STATIC_LIBRARY` and `LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY_FOR_SHARED_LIBRARY`, and having only the shared one be defaulted to `OFF` on Windows.

That sort of complexity is exactly the sort of thing that we must not add.

> I'm quite against trying to shoehorn all allowed build configurations into a fixed list of configurations. Sure, we can't guarantee that any random configuration not covered by CI will work though, but building without a presupplied cmake cache should definitely be ok and work.

I understand the appeal to have things work out of the box. I think it can be achieved. What if we loaded a CMake cache by default based on the platform we're building for first thing in the CMake? For example:

  #===============================================================================
  # Setup Project
  #===============================================================================
  cmake_minimum_required(VERSION 3.13.4)
  
  set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON)
  ...
  
  if (WIN32)
    include(cmake/caches/Windows.cmake)
  elseif(APPLE)
    include(cmake/caches/Apple.cmake)
  else()
    ...
  endif()

We'd also need a way to completely skip those if CMake is called with `-C <path-to-some-cache-file>`. If we had something along those lines, we could hide platform specific knowledge into those files and avoid adding complexity to our CMake.



================
Comment at: libcxx/test/libcxx/experimental/lit.local.cfg:6-7
+# linking against the static-only libc++experimental.
+if config.enable_shared and 'windows' in config.available_features:
+  config.unsupported = True
----------------
We already have a way to exclude the experimental tests from the test suite, and it's not acceptable to add another one based on platform specific knowledge. Lit can be passed `--param enable_experimental=False` for that purpose. Whatever we end up doing, we must find a way to pass `--param enable_experimental=False` to Lit when the configuration you're dealing with is used, without having to add that knowledge inside the test suite. Note that this is what `LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF` does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99178