Reapply [libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #86020
Conversation
…STALL_MODULES_DIR This reapplies 272d1b4 (from llvm#85756), which was reverted in 4079370. In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by appending a slash to CMAKE_INSTALL_PREFIX, so we always get an absolute path, even if CMAKE_INSTALL_PREFIX was empty.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Martin Storsjö (mstorsjo) ChangesThis reapplies 272d1b4 (from #85756), which was reverted in In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by appending a slash to CMAKE_INSTALL_PREFIX, so we always get an absolute path, even if CMAKE_INSTALL_PREFIX was empty. Full diff: https://github.com/llvm/llvm-project/pull/86020.diff 1 Files Affected:
diff --git a/libcxx/modules/CMakeLists.txt b/libcxx/modules/CMakeLists.txt
index 0dea8cfca94ac3..1d9c7518e9f801 100644
--- a/libcxx/modules/CMakeLists.txt
+++ b/libcxx/modules/CMakeLists.txt
@@ -206,9 +206,18 @@ add_custom_target(generate-cxx-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.
+#
+# Using a trailing slash in BASE_DIRECTORY, to produce a seemingly valid
+# absolute path, even if CMAKE_INSTALL_PREFIX is empty.
+cmake_path(ABSOLUTE_PATH LIBCXX_INSTALL_LIBRARY_DIR
+ BASE_DIRECTORY "${CMAKE_INSTALL_PREFIX}/"
+ OUTPUT_VARIABLE ABS_LIBRARY_DIR)
+cmake_path(ABSOLUTE_PATH LIBCXX_INSTALL_MODULES_DIR
+ BASE_DIRECTORY "${CMAKE_INSTALL_PREFIX}/"
+ OUTPUT_VARIABLE ABS_MODULES_DIR)
file(RELATIVE_PATH LIBCXX_MODULE_RELATIVE_PATH
- ${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_LIBRARY_DIR}
- ${CMAKE_INSTALL_PREFIX}/${LIBCXX_INSTALL_MODULES_DIR})
+ ${ABS_LIBRARY_DIR}
+ ${ABS_MODULES_DIR})
configure_file(
"modules.json.in"
"${LIBCXX_LIBRARY_DIR}/libc++.modules.json"
|
libcxx/modules/CMakeLists.txt
Outdated
| # Using a trailing slash in BASE_DIRECTORY, to produce a seemingly valid | ||
| # absolute path, even if CMAKE_INSTALL_PREFIX is empty. | ||
| cmake_path(ABSOLUTE_PATH LIBCXX_INSTALL_LIBRARY_DIR | ||
| BASE_DIRECTORY "${CMAKE_INSTALL_PREFIX}/" |
There was a problem hiding this comment.
I wonder what happens when ${CMAKE_INSTALL_PREFIX} is "/", then we get "//". I'm not sure whether that is a valid path. How about something like
if(${CMAKE_INSTALL_PREFIX} STREQUAL "")
set(BASE_DIRECTORY "/" PARENT_SCOPE)
else()
set(BASE_DIRECTORY ${CMAKE_INSTALL_PREFIX} PARENT_SCOPE)
endif()
There was a problem hiding this comment.
The double slash path here is effectively harmless; after cmake_path(ABSOLUTE_PATH), the output only has a single leading slash even in that case.
But you're right that it's probably nicer to be explicit about this.
There was a problem hiding this comment.
Thanks LGTM! Please wait for @petrhosek's approval before landing.
Sure, although I don't know if we need his extra input here any longer :-) |
I thought Petr had reported the issue with the previous version. It seems I was mistaken it was @ilovepi. Since @ilovepi has approved I'm happy to land it. |
|
Yeah I think we’re good here. We definitely use an empty install directory so the fix makes sense to me. I’ll keep an eye on our ci, but I think our buildout should catch this too. |
|
FWIW, the fuchsia build seems to stay green now, so I'll request a backport of this again. /cherry-pick 50801f1 |
…STALL_MODULES_DIR (llvm#86020) This reapplies 272d1b4 (from llvm#85756), which was reverted in 4079370. In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by constructing a non-empty base directory variable to use for calculating the relative path. (cherry picked from commit 50801f1)
|
/pull-request #86197 |
…STALL_MODULES_DIR (llvm#86020) This reapplies 272d1b4 (from llvm#85756), which was reverted in 4079370. In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by constructing a non-empty base directory variable to use for calculating the relative path.
…STALL_MODULES_DIR (llvm#86020) This reapplies 272d1b4 (from llvm#85756), which was reverted in 4079370. In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by constructing a non-empty base directory variable to use for calculating the relative path. (cherry picked from commit 50801f1)
…STALL_MODULES_DIR (llvm#86020) This reapplies 272d1b4 (from llvm#85756), which was reverted in 4079370. In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH). Avoid this issue by constructing a non-empty base directory variable to use for calculating the relative path. (cherry picked from commit 50801f1)
This reapplies 272d1b4 (from #85756), which was reverted in
4079370.
In the previous attempt, empty CMAKE_INSTALL_PREFIX was handled by quoting them, in d209d13. That made the calls to cmake_path(ABSOLUTE_PATH) succeed, but the output paths of that weren't actually absolute, which was required by file(RELATIVE_PATH).
Avoid this issue by appending a slash to CMAKE_INSTALL_PREFIX, so we always get an absolute path, even if CMAKE_INSTALL_PREFIX was empty.