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

[mstorsjo]

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]

@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]

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

[mstorsjo]

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.

[mstorsjo]

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

[mordante]

Thanks! LGTM!

[mstorsjo]

/cherry-pick 272d1b4

[llvmbot]

/pull-request #85907

[ilovepi]

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]

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.

[mstorsjo]

I pushed a fix in d209d13 now.

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

[mstorsjo]

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.

[ilovepi]

@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]

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