Skip to content

[libc++] Install modules. #75741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 21, 2024
Merged

[libc++] Install modules. #75741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e020e08
[RFC][libc++] Install modules.
mordante Dec 16, 2023
26f262d
Address review comments.
mordante Jan 18, 2024
9620519
Addresses more review comments.
mordante Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions libcxx/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@ 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 module source files (experimental)." OFF
)
cmake_dependent_option(LIBCXX_INSTALL_STATIC_LIBRARY
"Install the static libc++ library." ON
"LIBCXX_ENABLE_STATIC;LIBCXX_INSTALL_LIBRARY" OFF)
Expand Down Expand Up @@ -425,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_MODULES_DIR "share/libc++/v1" CACHE STRING
"Path where target-agnostic libc++ module source files 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.")
Expand Down
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-cxx20.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_TEST_PARAMS "std=c++20" CACHE STRING "")
set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-cxx23.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_TEST_PARAMS "std=c++23" CACHE STRING "")
set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-cxx26.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
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 "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-hardening-mode-extensive.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_HARDENING_MODE "extensive" CACHE STRING "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-exceptions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
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 "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-experimental.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
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 "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-filesystem.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_ENABLE_FILESYSTEM OFF CACHE BOOL "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-localization.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_ENABLE_LOCALIZATION OFF CACHE BOOL "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-random_device.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_ENABLE_RANDOM_DEVICE OFF CACHE BOOL "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-threads.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
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 "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-unicode.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_ENABLE_UNICODE OFF CACHE BOOL "")
1 change: 1 addition & 0 deletions libcxx/cmake/caches/Generic-no-wide-characters.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
set(LIBCXX_INSTALL_MODULES ON CACHE BOOL "") # TODO MODULES Remove when enabled automatically.
set(LIBCXX_ENABLE_WIDE_CHARACTERS OFF CACHE BOOL "")
1 change: 0 additions & 1 deletion libcxx/docs/Modules.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
7 changes: 7 additions & 0 deletions libcxx/docs/ReleaseNotes/18.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ Improvements and New Features
infrastructure no longer depends on a modern CMake, it works with the minimal
required LLVM version (3.20.0).

- The ``.cppm`` files of 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
``${PREFIX}/share/libc++/v1``.


Deprecations and Removals
-------------------------
Expand Down
54 changes: 54 additions & 0 deletions libcxx/modules/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,57 @@ add_custom_target(generate-cxx-modules
ALL DEPENDS
${_all_modules}
)

# Configure the modules manifest.
# Use the relative path between the installation and the module in the json
# file. This allows moving the entire installation to a different location.
file(RELATIVE_PATH LIBCXX_MODULE_RELATIVE_PATH
${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_LIBRARY_DIR}
${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_MODULES_DIR})
configure_file(
"modules.json.in"
"${LIBCXX_LIBRARY_DIR}/libc++.modules.json"
@ONLY
)

# Dummy library to make modules an installation component.
add_library(cxx-modules INTERFACE)
add_dependencies(cxx-modules generate-cxx-modules)

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_MODULES_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_MODULES_DIR}"
COMPONENT cxx-modules
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
)

# Install the module manifest.
install(FILES
"${LIBCXX_LIBRARY_DIR}/libc++.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-modules)
endif()
endif()
26 changes: 26 additions & 0 deletions libcxx/modules/modules.json.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"version": 1,
"revision": 1,
"modules": [
{
"logical-name": "std",
"source-path": "@LIBCXX_MODULE_RELATIVE_PATH@/std.cppm",
"is-standard-library": true,
"local-arguments": {
"system-include-directories": [
"@LIBCXX_MODULE_RELATIVE_PATH@"
]
}
},
{
"logical-name": "std.compat",
"source-path": "@LIBCXX_MODULE_RELATIVE_PATH@/std.compat.cppm",
"is-std-library": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or has that ship sailed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep :) .

"local-arguments": {
"system-include-directories": [
"@LIBCXX_MODULE_RELATIVE_PATH@"
]
}
}
]
}
5 changes: 5 additions & 0 deletions libcxx/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,18 +398,23 @@ if (NOT CMAKE_CONFIGURATION_TYPES)
endif()
if(LIBCXX_INSTALL_HEADERS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

set(header_install_target install-cxx-headers)
endif()
if(LIBCXX_INSTALL_MODULES)
set(module_install_target install-cxx-modules)
endif()
add_custom_target(install-cxx
DEPENDS ${lib_install_target}
cxx_experimental
${header_install_target}
${module_install_target}
COMMAND "${CMAKE_COMMAND}"
-DCMAKE_INSTALL_COMPONENT=cxx
-P "${LIBCXX_BINARY_DIR}/cmake_install.cmake")
add_custom_target(install-cxx-stripped
DEPENDS ${lib_install_target}
cxx_experimental
${header_install_target}
${module_install_target}
COMMAND "${CMAKE_COMMAND}"
-DCMAKE_INSTALL_COMPONENT=cxx
-DCMAKE_INSTALL_DO_STRIP=1
Expand Down