[libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #85756

mstorsjo · 2024-03-19T08:56:00Z

In other contexts, install directories such as
LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path.

When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too.

…DULES_DIR In other contexts, install directories such as LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path. When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too.

llvmbot · 2024-03-19T08:56:30Z

@llvm/pr-subscribers-libcxx

Author: Martin Storsjö (mstorsjo)

Changes

In other contexts, install directories such as
LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path.

When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too.

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

1 Files Affected:

(modified) libcxx/modules/CMakeLists.txt (+13-2)

diff --git a/libcxx/modules/CMakeLists.txt b/libcxx/modules/CMakeLists.txt
index 0dea8cfca94ac3..08deb452f5579a 100644
--- a/libcxx/modules/CMakeLists.txt
+++ b/libcxx/modules/CMakeLists.txt
@@ -203,12 +203,23 @@ add_custom_target(generate-cxx-modules
     ${_all_modules}
 )
 
+
+function(make_absolute OUT_VAR INPUT BASE)
+  if (IS_ABSOLUTE ${INPUT})
+    set(${OUT_VAR} "${INPUT}" PARENT_SCOPE)
+  else()
+    set(${OUT_VAR} "${BASE}/${INPUT}" PARENT_SCOPE)
+  endif()
+endfunction()
+
 # 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.
+make_absolute(ABS_LIBRARY_DIR "${LIBCXX_INSTALL_LIBRARY_DIR}" "${CMAKE_INSTALL_PREFIX}")
+make_absolute(ABS_MODULES_DIR "${LIBCXX_INSTALL_MODULES_DIR}" "${CMAKE_INSTALL_PREFIX}")
 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"

mordante

Thanks for your patch! In general I like it, but I've one question.

mordante · 2024-03-19T11:41:44Z

libcxx/modules/CMakeLists.txt

 # 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.
+make_absolute(ABS_LIBRARY_DIR "${LIBCXX_INSTALL_LIBRARY_DIR}" "${CMAKE_INSTALL_PREFIX}")
+make_absolute(ABS_MODULES_DIR "${LIBCXX_INSTALL_MODULES_DIR}" "${CMAKE_INSTALL_PREFIX}")


Why do you implement this function instead of using CMake facilities?
https://cmake.org/cmake/help/latest/command/cmake_path.html#absolute-path

I wasn't aware of this function, and it retained the old functionality as it was (just concatenating it manually) for non-absolute paths. But I can try to use this instead.

Updated to use cmake_path(ABSOLUTE_PATH ... now instead, thanks for the suggestion!

mordante

Thanks! LGTM!

mstorsjo · 2024-03-20T08:46:11Z

/cherry-pick 272d1b4

…DULES_DIR (llvm#85756) In other contexts, install directories such as LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path. When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too. (cherry picked from commit 272d1b4)

llvmbot · 2024-03-20T08:51:20Z

/pull-request #85907

ilovepi · 2024-03-20T15:30:28Z

Hi, we're seeing the following error in our CI after this patch ... I'm not totally sure this is completely the right approach. We see this on all platforms(linux, windows, mac).

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:209 (cmake_path):
  Error after keyword "BASE_DIRECTORY":

    missing required value

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:212 (cmake_path):
  Error after keyword "BASE_DIRECTORY":

    missing required value

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:215 (file):
  file RELATIVE_PATH called with incorrect number of arguments

https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64-rbe/b8752958259472772145/overview

Can you take a look and revert if it will be hard to fix quickly?

mstorsjo · 2024-03-20T15:47:11Z

Hi, we're seeing the following error in our CI after this patch ... I'm not totally sure this is completely the right approach. We see this on all platforms(linux, windows, mac).
CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:209 (cmake_path):
  Error after keyword "BASE_DIRECTORY":

    missing required value

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:212 (cmake_path):
  Error after keyword "BASE_DIRECTORY":

    missing required value

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:215 (file):
  file RELATIVE_PATH called with incorrect number of arguments
https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64-rbe/b8752958259472772145/overview

Can you take a look and revert if it will be hard to fix quickly?

Hm, I guess this is possible if CMAKE_INSTALL_PREFIX is unset/empty. I'll push a potential fix for that, and if that doesn't help, let's revert.

…L_PREFIX This should hopefully fix the issue brought up at #85756 (comment).

mstorsjo · 2024-03-20T15:51:14Z

I pushed a fix in d209d13 now.

Let's update the backport PR with this as well:
/cherry-pick 272d1b4 d209d13

…DULES_DIR (llvm#85756) In other contexts, install directories such as LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path. When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too. (cherry picked from commit 272d1b4)

…L_PREFIX This should hopefully fix the issue brought up at llvm#85756 (comment). (cherry picked from commit d209d13)

mstorsjo · 2024-03-20T16:05:28Z

Argh, I see that this still is failing, if CMAKE_INSTALL_PREFIX is empty, with errors like this:

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:215 (file):
  file RELATIVE_PATH must be passed a full path to the directory:
  lib/x86_64-pc-windows-msvc

Let's revert it on main and restart with a new PR, as this will need a bit more modifications.

…STALL_MODULES_DIR (#85756)" This reverts commit 272d1b4, and the follow-up fix in d209d13. Even after the follow-up fix, building with an empty CMAKE_INSTALL_PREFIX errors out with errors like this: CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:215 (file): file RELATIVE_PATH must be passed a full path to the directory: lib/x86_64-pc-windows-msvc

ilovepi · 2024-03-20T16:13:57Z

@mstorsjo Thanks for the quick response. I'll try to sync w/ @petrhosek to see if he knows a way to work around the issue.

mstorsjo · 2024-03-20T16:17:26Z

@mstorsjo Thanks for the quick response. I'll try to sync w/ @petrhosek to see if he knows a way to work around the issue.

Thanks, that's appreciated.

It's probably not hard, we'd just need to use a dummy / as the base directory, if CMAKE_INSTALL_PREFIX is empty (which would mimic what happened before anyway), so that file(RELATIVE_PATH doesn't error out, even if the output is seemingly bogus.

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

…STALL_MODULES_DIR (#86020) 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 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)

…DULES_DIR (llvm#85756) In other contexts, install directories such as LIBCXX_INSTALL_LIBRARY_DIR and LIBCXX_INSTALL_MODULES_DIR can be specified either as a relative path, relative to CMAKE_INSTALL_PREFIX, or as an absolute path. When calculating the relative path between the two, account for the fact that LIBCXX_INSTALL_MODULES_DIR and LIBCXX_INSTALL_LIBRARY_DIR can be absolute paths too.

…L_PREFIX This should hopefully fix the issue brought up at llvm#85756 (comment).

…STALL_MODULES_DIR (llvm#85756)" This reverts commit 272d1b4, and the follow-up fix in d209d13. Even after the follow-up fix, building with an empty CMAKE_INSTALL_PREFIX errors out with errors like this: CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libcxx/modules/CMakeLists.txt:215 (file): file RELATIVE_PATH must be passed a full path to the directory: lib/x86_64-pc-windows-msvc

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

…L_PREFIX This should hopefully fix the issue brought up at llvm/llvm-project#85756 (comment). NOKEYCHECK=True GitOrigin-RevId: d209d1340b99d4fbd325dffb5e13b757ab8264ea

mstorsjo requested a review from mordante March 19, 2024 08:56

mstorsjo requested a review from a team as a code owner March 19, 2024 08:56

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 19, 2024

mordante self-assigned this Mar 19, 2024

mordante reviewed Mar 19, 2024

View reviewed changes

Use cmake_path(ABSOLUTE_PATH) instead of a custom function

a97b2fb

mordante approved these changes Mar 19, 2024

View reviewed changes

mstorsjo merged commit 272d1b4 into llvm:main Mar 20, 2024
51 checks passed

mstorsjo deleted the libcxx-modules-abs-path branch March 20, 2024 08:45

mstorsjo added this to the LLVM 18.X Release milestone Mar 20, 2024

mstorsjo added a commit that referenced this pull request Mar 20, 2024

[libcxx] [cmake] Fix cmake_path(ABSOLUTE_PATH) for empty CMAKE_INSTAL…

d209d13

…L_PREFIX This should hopefully fix the issue brought up at #85756 (comment).

mstorsjo mentioned this pull request Mar 20, 2024

Reapply [libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #86020

Merged

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024

[libcxx] [cmake] Fix cmake_path(ABSOLUTE_PATH) for empty CMAKE_INSTAL…

eca146d

…L_PREFIX This should hopefully fix the issue brought up at llvm#85756 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #85756

[libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #85756

mstorsjo commented Mar 19, 2024

llvmbot commented Mar 19, 2024

mordante left a comment

mordante Mar 19, 2024

mstorsjo Mar 19, 2024

mstorsjo Mar 19, 2024

mordante left a comment

mstorsjo commented Mar 20, 2024

llvmbot commented Mar 20, 2024

ilovepi commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

ilovepi commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

[libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #85756

[libcxx] [modules] Fix relative paths with absolute LIBCXX_INSTALL_MODULES_DIR #85756

Conversation

mstorsjo commented Mar 19, 2024

llvmbot commented Mar 19, 2024

mordante left a comment

Choose a reason for hiding this comment

mordante Mar 19, 2024

Choose a reason for hiding this comment

mstorsjo Mar 19, 2024

Choose a reason for hiding this comment

mstorsjo Mar 19, 2024

Choose a reason for hiding this comment

mordante left a comment

Choose a reason for hiding this comment

mstorsjo commented Mar 20, 2024

llvmbot commented Mar 20, 2024

ilovepi commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

mstorsjo commented Mar 20, 2024

ilovepi commented Mar 20, 2024

mstorsjo commented Mar 20, 2024