[cmake] Fix DESTDIR support in compiler-rt build

[fjricci]

Summary

cmake's install function will automatically use the correct install
path: $ENV{DESTDIR}${CMAKE_INSTALL_PREFIX} when DESTINATION is supplied
a relative path. Don't try to force a custom path when we intend to
respect the user's configuration anyway.

To test, verify that install directory here is /tmp/destdir/install:

cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/install
DESTDIR=/tmp/destdir ninja install

An alternative solution to using '.' as a placeholder would be to not specify
DESTINATION at all for standalone builds, but we need to set this for in-tree
llvm builds, and it doesn't accept an empty argument, so we would need to
duplicate all of our install calls, to check whether COMPILER_RT_INSTALL_PATH is set,
which seems quite messy.

[smeenai]

cmake will expand out relative paths for PATH cache variables, so the . will actually be expanded out to the build directory, which isn't desirable. You need to use the STRING type to avoid this normalization (though that's hacky).

Also, the canonical path is actually $ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}; i.e., DESTDIR is supposed to contain the trailing slash itself.

[smeenai]

Okay, looks like I was mistaken and the normalization only happens when an untyped variable from the command line gets converted to a PATH or FILEPATH variable by a set. Weird.

(Resigning to try to remove my request changes.)

[smeenai]

(gonna resign afterwards to clear out my request changes)

[fjricci]

ping (particularly looking for input from @beanz)

[smeenai]

This does seem like the cleanest solution to me (though I'm not comfortable enough with compiler-rt's build system to approve this).

To give @beanz a bit more context, we're configuring a standalone compiler-rt build where (for various reasons that aren't really easy to change) the install destination is determined after CMake configuration, and so we wanna use DESTDIR to completely control the installation directory. We achieve this by setting CMAKE_INSTALL_PREFIX to / and DESTDIR to our desired install path. compiler-rt is unhappy with this setup, however, since its install destinations of the form ${COMPILER_RT_INSTALL_PATH}/... then end up starting with //, and CMake interprets this as a network path and complains about network paths being incompatible with DESTDIR.

Stripping any trailing slashes from CMAKE_INSTALL_PREFIX won't work in this case because COMPILER_RT_INSTALL_PATH will then end up empty, which also causes CMake errors. Setting CMAKE_INSTALL_PREFIX to /. (to avoid ending up with the problematic //) on our end instead works, but it's pretty hacky, and this patch definitely feels cleaner than that. This patch also avoids absolute paths in the install commands, which appear to be discouraged anyway (judging by the existence of CMAKE_WARN_ON_ABSOLUTE_INSTALL_DESTINATION and CMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION).

[phosek]

@smeenai why not set -DCMAKE_INSTALL_PREFIX="" (you can even omit the quotes altogether)? This is exactly what we do in Fuchsia toolchain build, we set CMAKE_INSTALL_PREFIX to empty string and then use DESTDIR to control the desired install path. This is a pretty standard pattern used by various package systems.

[smeenai]

@phosek over here (as in before this patch), COMPILER_RT_INSTALL_PATH is set to CMAKE_INSTALL_PREFIX, and then we have install commands like install(... DESTINATION ${COMPILER_RT_INSTALL_PATH}). Aren't those installation commands gonna fail for an empty COMPILER_RT_INSTALL_PATH? (I'm not setup to try a compiler-rt build right now, but I did reproduce that error in a test CMake project.)

[fjricci]

Yeah, if I try to use an empty CMAKE_INSTALL_PATH for compiler-rt, I end up with:

CMake Error at cmake/Modules/AddCompilerRT.cmake:441 (install):
  install FILES given no DESTINATION!

[smeenai]

@phosek you're right that an empty CMAKE_INSTALL_PREFIX is more canonical than / though (even if it doesn't make a difference in this case). CMake strips trailing /s from CMAKE_INSTALL_PREFIX before using it, so the / ends up becoming empty anyway.

[phosek]

I see the issue, the resource files are being installed into the COMPILER_RT_INSTALL_PATH rather than a subdirectory as everything else which will fail if you set empty CMAKE_INSTALL_PREFIX. When compiler-rt is built as part of LLVM, COMPILER_RT_INSTALL_PATH is set to lib/clang/${clang_version} so it's not an issue but in your case it'd break. A different solution that I can think of that might be cleaner would be to move these resource files into a subdirectory, e.g. share/.

[phosek]

D41673 is what I had in mind.

[fjricci]

That patch fixes our build for me.

[beanz]

I like @phosek's idea of moving the files into /share, but that needs some accompanying changes to clang to find those files in their new location.

[fjricci]

Abandoning in favor of D41706

cmake/base-config-ix.cmake

else()
     set(COMPILER_RT_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++.exe)
   endif()
 else()
     # Take output dir and install path from the user.
   set(COMPILER_RT_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR} CACHE PATH
     "Path where built compiler-rt libraries should be stored.")
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR}/bin CACHE PATH
     "Path where built compiler-rt executables should be stored.")
-  set(COMPILER_RT_INSTALL_PATH ${CMAKE_INSTALL_PREFIX} CACHE PATH
+  set(COMPILER_RT_INSTALL_PATH . CACHE PATH
     "Path where built compiler-rt libraries should be installed.")
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests." OFF)
   option(COMPILER_RT_ENABLE_WERROR "Fail and stop if warning is triggered" OFF)
   # Use a host compiler to compile/link tests.
   set(COMPILER_RT_TEST_COMPILER ${CMAKE_C_COMPILER} CACHE PATH "Compiler to use for testing")
   set(COMPILER_RT_TEST_CXX_COMPILER ${CMAKE_CXX_COMPILER} CACHE PATH "C++ Compiler to use for testing")
 endif()