[libc++] Make __config_site modular #135432
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Ian Anderson (ian-twilightcoder) ChangesThis patch makes the __config_site header modular, which solves various problems with non-modular headers. This requires going back to generating the modulemap file, since we only know how to make __config_site modular when we're not using the per-target runtime dir. The patch also adds a test that we support -Wnon-modular-include-in-module, which warns about non-modular includes from modules. Full diff: https://github.com/llvm/llvm-project/pull/135432.diff 7 Files Affected:
diff --git a/libcxx/docs/Contributing.rst b/libcxx/docs/Contributing.rst
index 5bcd6e3a7f03e..6aaa70764c2fa 100644
--- a/libcxx/docs/Contributing.rst
+++ b/libcxx/docs/Contributing.rst
@@ -116,7 +116,7 @@ sure you don't forget anything:
- Did you add all new named declarations to the ``std`` module?
- If you added a header:
- - Did you add it to ``include/module.modulemap``?
+ - Did you add it to ``include/module.modulemap.in``?
- Did you add it to ``include/CMakeLists.txt``?
- If it's a public header, did you update ``utils/libcxx/header_information.py``?
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index c225131101ce2..ca8364962cf55 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -1025,7 +1025,6 @@ set(files
mdspan
memory
memory_resource
- module.modulemap
mutex
new
numbers
@@ -1665,8 +1664,16 @@ set(files
configure_file("__config_site.in" "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site" @ONLY)
configure_file("${LIBCXX_ASSERTION_HANDLER_FILE}" "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler" COPYONLY)
+# We generate the modulemap file so that we can include __config_site in it. For now, we don't know how to
+# make __config_site modular when per-target runtime directories are used.
+if (NOT LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)
+ set(LIBCXX_CONFIG_SITE_MODULE_ENTRY "textual header \"__config_site\"")
+endif()
+configure_file("module.modulemap.in" "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap" @ONLY)
+
set(_all_includes "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}/__config_site"
- "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler")
+ "${LIBCXX_GENERATED_INCLUDE_DIR}/__assertion_handler"
+ "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap")
foreach(f ${files})
set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
set(dst "${LIBCXX_GENERATED_INCLUDE_DIR}/${f}")
@@ -1724,6 +1731,12 @@ if (LIBCXX_INSTALL_HEADERS)
PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
COMPONENT cxx-headers)
+ # Install the generated modulemap file to the generic include dir.
+ install(FILES "${LIBCXX_GENERATED_INCLUDE_DIR}/module.modulemap"
+ DESTINATION "${LIBCXX_INSTALL_INCLUDE_DIR}"
+ PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
+ COMPONENT cxx-headers)
+
if (NOT CMAKE_CONFIGURATION_TYPES)
add_custom_target(install-cxx-headers
DEPENDS cxx-headers
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap.in
similarity index 99%
rename from libcxx/include/module.modulemap
rename to libcxx/include/module.modulemap.in
index 65123eb268150..60240723a8940 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap.in
@@ -1,6 +1,7 @@
// This module contains headers related to the configuration of the library. These headers
// are free of any dependency on the rest of libc++.
module std_config [system] {
+ @LIBCXX_CONFIG_SITE_MODULE_ENTRY@ // generated via CMake
textual header "__config"
textual header "__configuration/abi.h"
textual header "__configuration/availability.h"
diff --git a/libcxx/test/libcxx/Wnon_modular_include_in_module.compile.pass.cpp b/libcxx/test/libcxx/Wnon_modular_include_in_module.compile.pass.cpp
new file mode 100644
index 0000000000000..1946d24a36109
--- /dev/null
+++ b/libcxx/test/libcxx/Wnon_modular_include_in_module.compile.pass.cpp
@@ -0,0 +1,20 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: target={{.*}}-apple-{{.*}}
+
+// This test ensures that libc++ supports being compiled with modules enabled and with
+// -Wnon-modular-include-in-module. This effectively checks that we don't include any
+// non-modular header from the library.
+//
+// Since most underlying platforms are not modularized properly, this test currently only
+// works on Apple platforms.
+
+// ADDITIONAL_COMPILE_FLAGS: -Wnon-modular-include-in-module -Wsystem-headers-in-module=std -fmodules -fcxx-modules
+
+#include <vector>
diff --git a/libcxx/test/libcxx/headers_in_modulemap.sh.py b/libcxx/test/libcxx/headers_in_modulemap.sh.py
index 51b14377ba89b..d4a18a20c8926 100644
--- a/libcxx/test/libcxx/headers_in_modulemap.sh.py
+++ b/libcxx/test/libcxx/headers_in_modulemap.sh.py
@@ -4,7 +4,7 @@
sys.path.append(sys.argv[1])
from libcxx.header_information import all_headers, libcxx_include
-with open(libcxx_include / "module.modulemap") as f:
+with open(libcxx_include / "module.modulemap.in") as f:
modulemap = f.read()
isHeaderMissing = False
diff --git a/libcxx/test/libcxx/lint/lint_headers.sh.py b/libcxx/test/libcxx/lint/lint_headers.sh.py
index c5e582cb0f7cb..ab237c968da7e 100644
--- a/libcxx/test/libcxx/lint/lint_headers.sh.py
+++ b/libcxx/test/libcxx/lint/lint_headers.sh.py
@@ -11,7 +11,7 @@
def exclude_from_consideration(path):
return (
path.endswith(".txt")
- or path.endswith(".modulemap")
+ or path.endswith(".modulemap.in")
or os.path.basename(path) == "__config"
or os.path.basename(path) == "__config_site.in"
or os.path.basename(path) == "libcxx.imp"
diff --git a/libcxx/utils/libcxx/header_information.py b/libcxx/utils/libcxx/header_information.py
index 9811b42d510ca..a505d37b65b81 100644
--- a/libcxx/utils/libcxx/header_information.py
+++ b/libcxx/utils/libcxx/header_information.py
@@ -15,7 +15,7 @@
def _is_header_file(file):
"""Returns whether the given file is a header file, i.e. not a directory or the modulemap file."""
return not file.is_dir() and not file.name in [
- "module.modulemap",
+ "module.modulemap.in",
"CMakeLists.txt",
"libcxx.imp",
"__config_site.in",
|
|
Plain rebase of #134699 with no other changes, I'm not sure why GitHub doesn't think it can merge that one 🤷♂️ |
|
@ian-twilightcoder You should be able to push on Louis' branch. |
How do I do that? It's in his fork |
|
Also is there something wrong with the Android bots? We keep getting "agent lost" errors. |
|
That happens sometimes. Just ignore it, they'll be happy again soon. |
This part doesn't appear to have been done: "Only the user who created the pull request can give you permission to push commits to the user-owned fork." |
That's allowed. See the "Maintainers are allowed to edit this pull request." of both this PR and the original one? (See also https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) |
🤔 ok well if the clone ever finishes I'll give it a try. |
It won't let me do it, maybe I'm not listed as a project maintainer. |
|
I think this is an unrelated error. |
This patch makes the __config_site header modular, which solves various problems with non-modular headers. This requires going back to generating the modulemap file, since we only know how to make __config_site modular when we're not using the per-target runtime dir. The patch also adds a test that we support -Wnon-modular-include-in-module, which warns about non-modular includes from modules.
ian-twilightcoder
force-pushed
the
review/make-config-site-modular
branch
from
a10b1ec to
be86a01
Compare
But @var-const does, he's doing this. |
4 participants
This patch makes the __config_site header modular, which solves various problems with non-modular headers. This requires going back to generating the modulemap file, since we only know how to make __config_site modular when we're not using the per-target runtime dir.
The patch also adds a test that we support -Wnon-modular-include-in-module, which warns about non-modular includes from modules.