[libc++] Install modules. #75741

mordante · 2023-12-17T15:53:09Z

Installs the source files of the experimental libc++ modules. These
source files (.cppm) are used by the Clang to build the std and
std.compat modules.

The design of this patch is based on a discussing in SG-15 on
12.12.2023. (SG-15 is the ISO C++ Tooling study group):

The modules are installed at a location, that is not known to build
systems and compilers.
Next to the library there will be a module manifest json file.
This json file contains the information to build the module from the
libraries sources. This information includes the location where the
sources are installed. @ruoso supplied the specification of this json
file.
If possible, the compiler has an option to give the location of the
module manifest file
([clang][modules] Print library module manifest path. #76451).

Currently there is no build system support, but it expected to be added
in the future.

Fixes: #73089

llvmbot · 2023-12-17T15:53:37Z

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

The patch is based on the discussion in SG-15 on 12.12.2023.

Fixes: #73089

Full diff: https://github.com/llvm/llvm-project/pull/75741.diff

16 Files Affected:

(modified) libcxx/CMakeLists.txt (+3)
(modified) libcxx/cmake/caches/Generic-cxx26.cmake (+1)
(modified) libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-exceptions.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-experimental.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-filesystem.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-localization.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-random_device.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-threads.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-unicode.cmake (+1)
(modified) libcxx/cmake/caches/Generic-no-wide-characters.cmake (+1)
(modified) libcxx/docs/Modules.rst (-1)
(modified) libcxx/docs/ReleaseNotes/18.rst (+7)
(modified) libcxx/modules/CMakeLists.txt (+50)
(added) libcxx/modules/modules.json.in (+21)
(modified) libcxx/src/CMakeLists.txt (+3)

diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 75cb63222da35c..f6e18e4b303988 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -177,6 +177,7 @@ set(LIBCXX_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
     "Define suffix of library directory name (32/64)")
 option(LIBCXX_INSTALL_HEADERS "Install the libc++ headers." ON)
 option(LIBCXX_INSTALL_LIBRARY "Install the libc++ library." ON)
+option(LIBCXX_INSTALL_MODULES "Install the libc++ C++20 modules (experimental)." OFF)
 cmake_dependent_option(LIBCXX_INSTALL_STATIC_LIBRARY
   "Install the static libc++ library." ON
   "LIBCXX_ENABLE_STATIC;LIBCXX_INSTALL_LIBRARY" OFF)
@@ -424,6 +425,8 @@ set(LIBCXX_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/c++/v1" CACHE STRING
     "Path where target-agnostic libc++ headers should be installed.")
 set(LIBCXX_INSTALL_RUNTIME_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
     "Path where built libc++ runtime libraries should be installed.")
+set(LIBCXX_INSTALL_MODULE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/../modules/c++/v1" CACHE STRING
+    "Path where target-agnostic libc++ modules should be installed.")
 
 set(LIBCXX_SHARED_OUTPUT_NAME "c++" CACHE STRING "Output name for the shared libc++ runtime library.")
 set(LIBCXX_STATIC_OUTPUT_NAME "c++" CACHE STRING "Output name for the static libc++ runtime library.")
diff --git a/libcxx/cmake/caches/Generic-cxx26.cmake b/libcxx/cmake/caches/Generic-cxx26.cmake
index f48d72d493c2f5..2252fe99ed9388 100644
--- a/libcxx/cmake/caches/Generic-cxx26.cmake
+++ b/libcxx/cmake/caches/Generic-cxx26.cmake
@@ -1,3 +1,4 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_TEST_PARAMS "std=c++26" CACHE STRING "")
 set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
diff --git a/libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake b/libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake
index 0487377d4e9ba2..85758ec9557b9d 100644
--- a/libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake
+++ b/libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_HARDENING_MODE "extensive" CACHE STRING "")
diff --git a/libcxx/cmake/caches/Generic-no-exceptions.cmake b/libcxx/cmake/caches/Generic-no-exceptions.cmake
index f405f7fe993752..83c7af4f340a2c 100644
--- a/libcxx/cmake/caches/Generic-no-exceptions.cmake
+++ b/libcxx/cmake/caches/Generic-no-exceptions.cmake
@@ -1,3 +1,4 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_EXCEPTIONS OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_EXCEPTIONS OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-experimental.cmake b/libcxx/cmake/caches/Generic-no-experimental.cmake
index fe14e7afed7b96..579242eb59383f 100644
--- a/libcxx/cmake/caches/Generic-no-experimental.cmake
+++ b/libcxx/cmake/caches/Generic-no-experimental.cmake
@@ -1,3 +1,4 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_TEST_PARAMS "enable_experimental=False" CACHE STRING "")
 set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
diff --git a/libcxx/cmake/caches/Generic-no-filesystem.cmake b/libcxx/cmake/caches/Generic-no-filesystem.cmake
index db62f86854d941..55b1c1424c458d 100644
--- a/libcxx/cmake/caches/Generic-no-filesystem.cmake
+++ b/libcxx/cmake/caches/Generic-no-filesystem.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_FILESYSTEM OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-localization.cmake b/libcxx/cmake/caches/Generic-no-localization.cmake
index 54a7ec3f1f5b36..94f05dea477a42 100644
--- a/libcxx/cmake/caches/Generic-no-localization.cmake
+++ b/libcxx/cmake/caches/Generic-no-localization.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_LOCALIZATION OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-random_device.cmake b/libcxx/cmake/caches/Generic-no-random_device.cmake
index adfa2458a8edf6..a01407a44c5ce9 100644
--- a/libcxx/cmake/caches/Generic-no-random_device.cmake
+++ b/libcxx/cmake/caches/Generic-no-random_device.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_RANDOM_DEVICE OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-threads.cmake b/libcxx/cmake/caches/Generic-no-threads.cmake
index 2aeab22915e00c..b79c3541c1647b 100644
--- a/libcxx/cmake/caches/Generic-no-threads.cmake
+++ b/libcxx/cmake/caches/Generic-no-threads.cmake
@@ -1,4 +1,5 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_THREADS OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_THREADS OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_MONOTONIC_CLOCK OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-unicode.cmake b/libcxx/cmake/caches/Generic-no-unicode.cmake
index 880e2d502ad91b..75213687a27006 100644
--- a/libcxx/cmake/caches/Generic-no-unicode.cmake
+++ b/libcxx/cmake/caches/Generic-no-unicode.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_UNICODE OFF CACHE BOOL "")
diff --git a/libcxx/cmake/caches/Generic-no-wide-characters.cmake b/libcxx/cmake/caches/Generic-no-wide-characters.cmake
index 5036f6abd52e83..c5bf594b77bff4 100644
--- a/libcxx/cmake/caches/Generic-no-wide-characters.cmake
+++ b/libcxx/cmake/caches/Generic-no-wide-characters.cmake
@@ -1,2 +1,3 @@
 set(LIBCXX_ENABLE_STD_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
+set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
 set(LIBCXX_ENABLE_WIDE_CHARACTERS OFF CACHE BOOL "")
diff --git a/libcxx/docs/Modules.rst b/libcxx/docs/Modules.rst
index 5099e6095582cf..068d83035c12dd 100644
--- a/libcxx/docs/Modules.rst
+++ b/libcxx/docs/Modules.rst
@@ -69,7 +69,6 @@ Some of the current limitations
  * The path to the compiler may not be a symlink, ``clang-scan-deps`` does
    not handle that case properly
  * Libc++ is not tested with modules instead of headers
- * The module ``.cppm`` files are not installed
  * Clang supports modules using GNU extensions, but libc++ does not work using
    GNU extensions.
  * Clang:
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index 9e509db6359c4a..172245b2c088fd 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -81,6 +81,13 @@ Improvements and New Features
 - The ``_LIBCPP_ENABLE_CXX26_REMOVED_STRING_RESERVE`` macro has been added to make
   the function ``std::basic_string<...>::reserve()`` available.
 
+- The experimental standard library modules can now be installed. By default,
+  they are not installed. This can be enabled by configuring CMake with
+  ``-DLIBCXX_INSTALL_MODULES=ON``. The installation directory can be configured
+  with the CMake option ``-DLIBCXX_INSTALL_MODULE_DIR=<path>``. The default
+  location is ``modules/c++/v1`` in the same prefix as the ``include``
+  directory.
+
 
 Deprecations and Removals
 -------------------------
diff --git a/libcxx/modules/CMakeLists.txt b/libcxx/modules/CMakeLists.txt
index fae6448a7eec84..7942dbca68b2d2 100644
--- a/libcxx/modules/CMakeLists.txt
+++ b/libcxx/modules/CMakeLists.txt
@@ -210,3 +210,53 @@ add_custom_target(generate-cxx-modules
   ALL DEPENDS
     ${_all_modules}
 )
+
+# Use the relative path between the installation and the module in the json
+# file. This allows moving the entire installation to a different to a
+# different location.
+file(RELATIVE_PATH LIBCXX_MODULE_RELATIVE_PATH
+	${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_LIBRARY_DIR}
+	${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_MODULE_DIR})
+configure_file(
+  "modules.json.in"
+  "${LIBCXX_LIBRARY_DIR}/modules.json"
+  @ONLY
+)
+
+if (LIBCXX_INSTALL_MODULES)
+  foreach(file ${LIBCXX_MODULE_STD_SOURCES} ${LIBCXX_MODULE_STD_COMPAT_SOURCES})
+    get_filename_component(dir ${file} DIRECTORY)
+    install(FILES ${file}
+      DESTINATION "${LIBCXX_INSTALL_MODULE_DIR}/${dir}"
+      COMPONENT cxx-modules
+      PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
+    )
+  endforeach()
+
+  # Install the generated module files.
+  install(FILES
+      "${LIBCXX_GENERATED_MODULE_DIR}/std.cppm"
+      "${LIBCXX_GENERATED_MODULE_DIR}/std.compat.cppm"
+    DESTINATION "${LIBCXX_INSTALL_MODULE_DIR}"
+    COMPONENT cxx-modules
+    PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
+  )
+
+  # Install the module manifest.
+  install(FILES
+      "${LIBCXX_LIBRARY_DIR}/modules.json"
+    DESTINATION "${LIBCXX_INSTALL_LIBRARY_DIR}"
+    COMPONENT cxx-modules
+    PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
+  )
+
+  if (NOT CMAKE_CONFIGURATION_TYPES)
+    add_custom_target(install-cxx-modules
+                      DEPENDS cxx-modules
+                      COMMAND "${CMAKE_COMMAND}"
+                              -DCMAKE_INSTALL_COMPONENT=cxx-modules
+                              -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
+    # Stripping is a no-op for modules
+    add_custom_target(install-cxx-modules-stripped DEPENDS install-cxx-headers)
+  endif()
+endif()
diff --git a/libcxx/modules/modules.json.in b/libcxx/modules/modules.json.in
new file mode 100644
index 00000000000000..97eb83f3ab7899
--- /dev/null
+++ b/libcxx/modules/modules.json.in
@@ -0,0 +1,21 @@
+{
+  "version": 1,
+  "revision": 1,
+  "modules": [
+    {
+      "logical-name": "std",
+      "source-path": "@LIBCXX_MODULE_RELATIVE_PATH@/std.cppm",
+      "system-include-directories": "@LIBCXX_MODULE_RELATIVE_PATH@/std",
+      "is-standard-library": true
+    },
+    {
+      "logical-name": "std.compat",
+      "source-path": "@LIBCXX_MODULE_RELATIVE_PATH@/std.compat.cppm",
+      "system-include-directories": [
+        "@LIBCXX_MODULE_RELATIVE_PATH@/std",
+        "@LIBCXX_MODULE_RELATIVE_PATH@/std.compat"
+     ],
+      "is-std-library": true
+    }
+  ]
+}
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index be0113e6b0a585..597715af71f852 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -396,6 +396,9 @@ if (NOT CMAKE_CONFIGURATION_TYPES)
     endif()
     if(LIBCXX_INSTALL_HEADERS)
       set(header_install_target install-cxx-headers)
+    endif()
+	if(LIBCXX_INSTALL_MODULES)
+      set(header_install_target install-cxx-modules)
     endif()
     add_custom_target(install-cxx
                       DEPENDS ${lib_install_target}

mordante · 2023-12-17T15:53:45Z

Note this does not make changes to Clang to print the location of the module manifest.

ChuanqiXu9 · 2023-12-17T16:11:10Z

May it be a reasonable expectation that we can see std.cppm in the installed path after we run something like apt install libc++-18? I am curious since the default value of LIBCXX_INSTALL_MODULES is false. Then it looks like we need to teach someone else (the packagers?) to enable that explicitly.

mordante · 2023-12-17T16:21:32Z

May it be a reasonable expectation that we can see std.cppm in the installed path after we run something like apt install libc++-18? I am curious since the default value of LIBCXX_INSTALL_MODULES is false. Then it looks like we need to teach someone else (the packagers?) to enable that explicitly.

The feature is still considered experimental and typically we don't enable experimental features by default. I don't see modules stable in LLVM-18.

If vendors want to ship it they have to enable it. They also need to adjust their packaging to ship the new files. I'm not sure whether Debian will put it in the libc++ dev package or a new experimental package.

ChuanqiXu9 · 2023-12-17T16:28:18Z

May it be a reasonable expectation that we can see std.cppm in the installed path after we run something like apt install libc++-18? I am curious since the default value of LIBCXX_INSTALL_MODULES is false. Then it looks like we need to teach someone else (the packagers?) to enable that explicitly.

The feature is still considered experimental and typically we don't enable experimental features by default. I don't see modules stable in LLVM-18.

If vendors want to ship it they have to enable it. They also need to adjust their packaging to ship the new files. I'm not sure whether Debian will put it in the libc++ dev package or a new experimental package.

Thanks. Fine enough : )

ruoso · 2023-12-17T17:11:32Z

The system-include-directories should be inside a local-arguments object, according to the initial proposal.

mordante · 2023-12-17T19:02:50Z

The system-include-directories should be inside a local-arguments object, according to the initial proposal.

Thanks! I'll fix it.

The patch is based on the discussion in SG-15 on 12.12.2023. Fixes: llvm#73089

mordante · 2023-12-18T17:13:41Z

From mordante@b23193d#commitcomment-135229396

The experimental standard library modules can now be installed. By default, they are not installed. This can be enabled by configuring CMake with -DLIBCXX_INSTALL_MODULES=ON. The installation directory can be configured with the CMake option -DLIBCXX_INSTALL_MODULE_DIR<path>. The default location is modules/c++/v1 in the same prefix as the include directory.

Hm, so they will be disabled by default, which means that downstream package maintainers (Debian, Fedora, etc) will most likely leave them disabled. Is there a reason why they cannot be enabled by default? Merely installing a few extra files is unlikely to break anything existing.

As replied above. In general libc++ disables experimental features by default.

Also, the default installation directory will be /usr/modules/... which will clearly cause friction for downstream, will most likely require input from FHS and, as was mentioned in SG15 discussions, will likely be rejected by FHS (understandably). Is there any reason why it cannot be something less contentious by default, say, /usr/include/modules/... or /usr/include/c++/v1/modules/?

This is the location we discussed in SG15 and the FHS issue was mentioned. Since this is an RFC I like to get some feedback on this location.

ruoso · 2023-12-18T17:58:12Z

I would suggest to keep the module sources in a location private to the library, instead of commiting to a general purpose location before we have a consensus on that.

ldionne · 2023-12-18T20:32:11Z

libcxx/CMakeLists.txt

@@ -424,6 +428,8 @@ set(LIBCXX_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/c++/v1" CACHE STRING
    "Path where target-agnostic libc++ headers should be installed.")
 set(LIBCXX_INSTALL_RUNTIME_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
    "Path where built libc++ runtime libraries should be installed.")
+set(LIBCXX_INSTALL_MODULE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/../modules/c++/v1" CACHE STRING


Following @ruoso 's comment in #75741 (comment), I would suggest making this something much less pretty than modules/c++/v1 so as to not give the impression that folks can rely on this as a stable name.

Suggestions from our live review just now:

$prefix/usr/include/c++/v1/__private_modules/ $prefix/usr/modules/c++/v1/ $prefix/usr/share/libc++/v1/ (or whatever was agreed upon on the Discourse RFC) $prefix/usr/share/c++/v1/

@jwakely What do you think about this? Any opinion? This path technically doesn't matter because it should always be extracted from the JSON file, but it would be nice if both GCC and Clang had semi-consistent paths.

The JSON file itself is always going to be next to the dylib in e.g.$prefix/usr/lib/libc++.module.json (and in theory always accessed via something like clang -print-file-name=libc++.module.json).

usr is a $prefix, so probably intended as

$prefix/include/c++/v1/__private_modules/ $prefix/include/c++/v1/ $prefix/usr/share/libc++/v1/ etc

I see very few things in my usr/include that aren't textual inclusion for C or C++. There's some evidence that fortran may put mod files there, but it looks like rust puts similar information in share/cargo, which isn't a choice for C++.

I've removed the usr.

I see very few things in my usr/include that aren't textual inclusion for C or C++. There's some evidence that fortran may put mod files there, but it looks like rust puts similar information in share/cargo, which isn't a choice for C++.

If only C++ had one package manager ;-)

ldionne · 2023-12-18T20:33:21Z

libcxx/CMakeLists.txt

@@ -177,6 +177,10 @@ set(LIBCXX_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
    "Define suffix of library directory name (32/64)")
 option(LIBCXX_INSTALL_HEADERS "Install the libc++ headers." ON)
 option(LIBCXX_INSTALL_LIBRARY "Install the libc++ library." ON)
+cmake_dependent_option(LIBCXX_INSTALL_MODULES


Can we install the modules if-and-only-if LIBCXX_ENABLE_STD_MODULES is ON? In other words, we might not need a separate option for installing the modules, I would think that enabling modules with LIBCXX_ENABLE_STD_MODULES should take care of that.

As discussed privately; this option currently needs a modern CMake version (3.26 or newer). We discussed the way to remove this dependency. So for now I keep it and work on that other solution first.

What did 3.26 introduce that this relies on?

I used the CMake module support to build the std module. This is used in the libc++ test suite.

libcxx/src/CMakeLists.txt

ruoso · 2023-12-19T16:42:06Z

Also, I don't think we need the include directories in the metadata.

The local-arguments are meant to cover things that are only needed when translating the module sources, but not needed when translating something that imports the module.

The paths to the standard library headers are part of the "basic toolchain configuration arguments" and must match across both the BMI and the TU performing the import.

Please see P2581R2 for an extended discussion of that problem.

ldionne · 2023-12-19T16:52:13Z

libcxx/CMakeLists.txt

@@ -424,6 +428,8 @@ set(LIBCXX_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/c++/v1" CACHE STRING
    "Path where target-agnostic libc++ headers should be installed.")
 set(LIBCXX_INSTALL_RUNTIME_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
    "Path where built libc++ runtime libraries should be installed.")
+set(LIBCXX_INSTALL_MODULE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/../modules/c++/v1" CACHE STRING


@jwakely What do you think about this? Any opinion? This path technically doesn't matter because it should always be extracted from the JSON file, but it would be nice if both GCC and Clang had semi-consistent paths.

The JSON file itself is always going to be next to the dylib in e.g.$prefix/usr/lib/libc++.module.json (and in theory always accessed via something like clang -print-file-name=libc++.module.json).

libcxx/modules/CMakeLists.txt

tschuett · 2023-12-19T17:12:57Z

As long as it is experimental, I would suggest to have no default path and force the user to provide the installation path.

mordante · 2023-12-19T18:52:31Z

Also, I don't think we need the include directories in the metadata.

The local-arguments are meant to cover things that are only needed when translating the module sources, but not needed when translating something that imports the module.

The paths to the standard library headers are part of the "basic toolchain configuration arguments" and must match across both the BMI and the TU performing the import.

Please see P2581R2 for an extended discussion of that problem.

In libc++ the std module has parts of its implementation in <header>.inc files. These should be included during the building of the module. Typically compilers will look in the path of the .cpp file, but I feel it's better to do that explicitly. This is not the path of the standard library header files. I indeed expect compiler to find that path.

mordante · 2023-12-19T18:59:54Z

As long as it is experimental, I would suggest to have no default path and force the user to provide the installation path.

The goal is to find a good place, which our vendors also like. So I like to have a location.

ruoso · 2023-12-19T19:46:53Z

In libc++ the std module has parts of its implementation in <header>.inc files. These should be included during the building of the module.

Ok. In that case that's exactly what the local-arguments are for.

ruoso · 2023-12-19T19:54:21Z

The goal is to find a good place, which our vendors also like. So I like to have a location.

My suggestion would be to do something like $PREFIX/share/libc++/v1/, as that is a private location for the libc++ package within the scope of the FHS as it stands today.

tahonermann · 2023-12-19T21:47:06Z

libcxx/CMakeLists.txt

+cmake_dependent_option(LIBCXX_INSTALL_MODULES
+  "Install the libc++ C++20 modules (experimental)." OFF
+  "LIBCXX_ENABLE_STD_MODULES" OFF
+)


The terminology used here is unclear regarding what is being installed. "C++20 modules" is a language feature and thus not something that is installable. What is being installed is source code files for the module interface units.

Suggested change

cmake_dependent_option(LIBCXX_INSTALL_MODULES

"Install the libc++ C++20 modules (experimental)." OFF

"LIBCXX_ENABLE_STD_MODULES" OFF

)

cmake_dependent_option(LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCES

"Install the libc++ C++20 module interface unit source files (experimental)." OFF

"LIBCXX_ENABLE_STD_MODULES" OFF

)

tahonermann · 2023-12-19T21:54:48Z

libcxx/CMakeLists.txt

+set(LIBCXX_INSTALL_MODULE_DIR "usr/share/libc++/v1" CACHE STRING
+    "Path where target-agnostic libc++ modules should be installed.")


I think the installation location needs a better default. For myself, I build with cmake -DCMAKE_INSTALL_PREFIX=/my/own/path and a usr/share scheme doesn't fit in with what that produces.

Suggested change

set(LIBCXX_INSTALL_MODULE_DIR "usr/share/libc++/v1" CACHE STRING

"Path where target-agnostic libc++ modules should be installed.")

set(LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCE_DIR "usr/share/libc++/v1" CACHE STRING

"Path where libc++ C++20 module interface unit source files should be installed.")

From https://cmake.org/cmake/help/v3.28/module/GNUInstallDirs.html, I would suggest ${CMAKE_INSTALL_DATAROOTDIR}/libc++/v1, which would be $prefix/share/libc++/v1. Vendors would generally set CMAKE_INSTALL_PREFIX=/usr, which would give /usr/share/libc++/v1.

I did not add the "C++20", that can be confusing.
C++20 added modules
C++23 added the std and std.compat module, (which the major vendors make available in C++20).

tahonermann · 2023-12-19T21:57:40Z

libcxx/docs/ReleaseNotes/18.rst

+- The experimental standard library modules can now be installed. By default,
+  they are not installed. This can be enabled by configuring CMake with
+  ``-DLIBCXX_INSTALL_MODULES=ON``. The installation directory can be configured
+  with the CMake option ``-DLIBCXX_INSTALL_MODULE_DIR=<path>``. The default
+  location is ``modules/c++/v1`` in the same prefix as the ``include``
+  directory.


Suggested change

- The experimental standard library modules can now be installed. By default,

they are not installed. This can be enabled by configuring CMake with

``-DLIBCXX_INSTALL_MODULES=ON``. The installation directory can be configured

with the CMake option ``-DLIBCXX_INSTALL_MODULE_DIR=<path>``. The default

location is ``modules/c++/v1`` in the same prefix as the ``include``

directory.

- The experimental standard library C++20 module interface unit source files can

now be installed. By default, they are not installed. This can be enabled by

configuring CMake with ``-DLIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCES=ON``.

The installation directory can be configured with the CMake option

``-DLIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCE_DIR=<path>``. The default

location is ``modules/c++/v1`` in the same prefix as the ``include`` directory.

The location in that last sentence doesn't appear to be consistent with the default specified in libcxx/CMakeLists.txt.

I noticed that too, that was an older proposed location.

libcxx/modules/CMakeLists.txt

mordante · 2023-12-20T19:28:24Z

@llvm/libcxx-vendors can you have a look and provide feedback on the proposed installation location?

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. llvm#75741 adds this feature in libc++.

mordante · 2023-12-27T16:40:10Z

#76451 implements the Clang related changes.

libcxx/modules/modules.json.in

cjdb · 2024-01-02T19:15:31Z

Thanks for working on this. Is there a way to detect that modules have been installed using a build system?

ChuanqiXu9 · 2024-01-03T02:30:55Z

#76451

IIUC, it is the goal of #76451

mordante · 2024-01-03T18:19:22Z

Thanks for working on this. Is there a way to detect that modules have been installed using a build system?

#76451

IIUC, it is the goal of #76451

That's indeed the place to detect its presence.

cjdb · 2024-01-03T18:26:15Z

Thanks for working on this. Is there a way to detect that modules have been installed using a build system?

#76451

IIUC, it is the goal of #76451

That's indeed the place to detect its presence.

Excellent, thank you! I'm excited for this to ship.

ldionne

Thanks a lot for driving this effort Mark, this is going to have a huge impact on the ecosystem and this is the starting point for standard modules being more widely usable.

The patch looks good to me once my comments have been addressed.

libcxx/cmake/caches/Generic-no-threads.cmake

ldionne · 2024-01-16T16:59:04Z

libcxx/docs/ReleaseNotes/18.rst

+- The experimental standard library module module interface unit source files
+  can now be installed. By default, they are not installed. This can be enabled
+  by configuring CMake with


Suggested change

- The experimental standard library module module interface unit source files

can now be installed. By default, they are not installed. This can be enabled

by configuring CMake with

- The experimental standard library module module interface unit source files

(`.cppm` files) can now be installed. By default, they are not installed. This can be enabled

by configuring CMake with

I've slightly reworded the text, but it keeps the intent of making the text easier to understand for non-module experts.

libcxx/docs/ReleaseNotes/18.rst

ldionne · 2024-01-16T17:02:29Z

libcxx/CMakeLists.txt

@@ -177,6 +177,10 @@ set(LIBCXX_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
    "Define suffix of library directory name (32/64)")
 option(LIBCXX_INSTALL_HEADERS "Install the libc++ headers." ON)
 option(LIBCXX_INSTALL_LIBRARY "Install the libc++ library." ON)
+cmake_dependent_option(LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCES


I'll be 100% honest, LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCES reads like jibberish to me. I understand it's the correct term, but it is long and harder to read. I don't think LIBCXX_INSTALL_STD_MODULES was confusing. This comment is non-blocking, but personally I like the simple and "dumb" LIBCXX_INSTALL_STD_MODULES better.

FWIW I think part of my dislike for LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCES is that it's so long that it's difficult to distinguish from LIBCXX_INSTALL_MODULE_INTERFACE_UNIT_SOURCE_DIR, and it's also not clear that it's a boolean option (since there's the word SOURCES in there). LIBCXX_INSTALL_STD_MODULES makes it clear that this is a ON/OFF toggle.

libcxx/modules/CMakeLists.txt

libcxx/modules/modules.json.in

libcxx/src/CMakeLists.txt

ldionne · 2024-01-16T17:24:33Z

libcxx/src/CMakeLists.txt

@@ -397,6 +397,9 @@ if (NOT CMAKE_CONFIGURATION_TYPES)
    if(LIBCXX_INSTALL_HEADERS)


In your commit message, please explain what the patch does. In particular, please explain the location where we install the .cppm files and the manifest file, and mention that the manifest file is supposed to live alongside the dylib, and that there's a contract with Clang. Also mention the command-line that can be used to find the manifest file from clang.

You referenced the SG15 discussion, but I would like us to capture the important points of that discussion in the commit message.

The patch is based on the discussion in SG-15 on 12.12.2023. Fixes: llvm#73089

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. #75741 adds this feature in libc++.

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. llvm#75741 adds this feature in libc++. This reverts commit 82f424f. Disables the tests on non-X86 platforms as suggested.

mathstuf · 2024-02-24T13:32:39Z

libcxx/modules/modules.json.in

+    {
+      "logical-name": "std.compat",
+      "source-path": "@LIBCXX_MODULE_RELATIVE_PATH@/std.compat.cppm",
+      "is-std-library": true,


This isn't consistent with is-standard-library above. Which is it?

Could we please use valid python identifiers for keys? That way we can load the json into typed models.

Or has that ship sailed?

FWIW, P1689 uses kebab-case as well. Note that 18 is in rc3; I don't know how much "Python APIs are easier" is to make a release blocker versus "the file is not consistent with itself". Something for release managers to decide.

Note that there's nothing stopping a typed model from acting on it as-is because a function can transform these into identifier names (IME, the explicitness from things like #[serde(rename = "…")] are better than forcing the interchange format to bend to a specific language's ergonomics).

Sure. I agree that this probably isn't important enough to do anything about. And standardizing behind a specific format is more important.

But I also don't think it's unreasonable to prefer interacting with the data in a typed/schema-enforced way, and that treating keys as identifiers rather than strings is preferable to achieve that.

However, if they're no tooling that yet depends on the format, I think it's worth changing. Because consistency isn't everything, but it's nice to have, and we might lose the ability to have it later.

That said, this can land after the release, it's just a little uglier of a scenario.

I've no strong opinion about the naming. However as mentioned in the commit message the naming is based on a discussion in SG15 and as @mathstuf mentioned is used in P1689. (clang-scan-deps and CMake (and probably others too) have implemented this paper.) So IMO changing the naming should be discussed in SG15.

This isn't consistent with is-standard-library above. Which is it?

Thanks for catching this. I should have used copy-paste programming ;-)

Thanks for catching this. I should have used copy-paste programming ;-)

No problem. Was very confused when "no such key" errors came up. Changed it to the one in the line it complained about…still errored. Finally saw the difference. Just glad I got around to testing this before the final release.

btw does that mean you're working on implementing this is CMake?

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. #75741 adds this feature in libc++. This reverts commit 82f424f. Disables the tests on non-X86 platforms as suggested.

…2160) This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. llvm#75741 adds this feature in libc++. This reverts commit 82f424f. Disables the tests on non-X86 platforms as suggested.

llvm/llvm-project#75741

mordante requested a review from a team as a code owner December 17, 2023 15:53

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 17, 2023

mordante mentioned this pull request Dec 17, 2023

Please consider installing std.cppm in lib++ #73089

Closed

mordante force-pushed the GH_libcxx_install_modules branch from 553d9ba to b23193d Compare December 17, 2023 16:35

mordante referenced this pull request in mordante/llvm-project Dec 18, 2023

[RFC][libc++] Install modules.

b23193d

The patch is based on the discussion in SG-15 on 12.12.2023. Fixes: llvm#73089

ldionne reviewed Dec 18, 2023

View reviewed changes

ldionne reviewed Dec 19, 2023

View reviewed changes

libcxx/src/CMakeLists.txt Outdated Show resolved Hide resolved

ldionne reviewed Dec 19, 2023

View reviewed changes

tahonermann reviewed Dec 19, 2023

View reviewed changes

mordante mentioned this pull request Dec 27, 2023

[clang][modules] Print library module manifest path. #76451

Merged

tschuett reviewed Dec 31, 2023

View reviewed changes

libcxx/modules/modules.json.in Show resolved Hide resolved

ldionne self-assigned this Jan 10, 2024

ldionne approved these changes Jan 16, 2024

View reviewed changes

mordante added 2 commits January 18, 2024 19:24

[RFC][libc++] Install modules.

e020e08

The patch is based on the discussion in SG-15 on 12.12.2023. Fixes: llvm#73089

Address review comments.

26f262d

mordante force-pushed the GH_libcxx_install_modules branch from ae89648 to 26f262d Compare January 18, 2024 18:36

Addresses more review comments.

9620519

mordante changed the title ~~[RFC][libc++] Install modules.~~ [libc++] Install modules. Jan 18, 2024

waruqi mentioned this pull request Jan 19, 2024

Improve c++ modules support xmake-io/xmake#4629

Closed

8 tasks

mordante merged commit 8b47bb6 into llvm:main Jan 21, 2024
45 of 46 checks passed

mordante deleted the GH_libcxx_install_modules branch January 21, 2024 11:16

a858438680 mentioned this pull request Feb 1, 2024

Libcxx's document of C++ std library modules is outdated #80231

Closed

mordante mentioned this pull request Feb 18, 2024

Reland "[clang][modules] Print library module manifest path." #82160

Merged

mathstuf reviewed Feb 24, 2024

View reviewed changes

mathstuf mentioned this pull request Feb 24, 2024

modules;json: inconsistent keys: is-standard-library vs is-std-library #82879

Closed

huangqinjin added a commit to huangqinjin/cxxmodules that referenced this pull request Apr 29, 2024

clang import std

5dd9daf

llvm/llvm-project#75741

		set(LIBCXX_INSTALL_MODULE_DIR "usr/share/libc++/v1" CACHE STRING
		"Path where target-agnostic libc++ modules should be installed.")

		@@ -397,6 +397,9 @@ if (NOT CMAKE_CONFIGURATION_TYPES)
		if(LIBCXX_INSTALL_HEADERS)

[libc++] Install modules. #75741

[libc++] Install modules. #75741

Conversation

mordante commented Dec 17, 2023 • edited Loading

llvmbot commented Dec 17, 2023

mordante commented Dec 17, 2023

ChuanqiXu9 commented Dec 17, 2023

mordante commented Dec 17, 2023

ChuanqiXu9 commented Dec 17, 2023

ruoso commented Dec 17, 2023

mordante commented Dec 17, 2023

mordante commented Dec 18, 2023

ruoso commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruoso commented Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

tschuett commented Dec 19, 2023

mordante commented Dec 19, 2023

mordante commented Dec 19, 2023

ruoso commented Dec 19, 2023 • edited Loading

ruoso commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordante commented Dec 20, 2023

mordante commented Dec 27, 2023

cjdb commented Jan 2, 2024

ChuanqiXu9 commented Jan 3, 2024

mordante commented Jan 3, 2024

cjdb commented Jan 3, 2024

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordante commented Dec 17, 2023 •

edited

Loading

ruoso commented Dec 19, 2023 •

edited

Loading

ruoso commented Dec 19, 2023 •

edited

Loading