Page MenuHomePhabricator

[cmake] Fix DESTDIR support in compiler-rt build

Authored by fjricci on Dec 19 2017, 10:08 AM.



cmake's install function will automatically use the correct install
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.

Event Timeline

fjricci created this revision.Dec 19 2017, 10:08 AM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald TranscriptDec 19 2017, 10:08 AM
smeenai requested changes to this revision.Dec 19 2017, 10:17 AM
smeenai added a subscriber: 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.

This revision now requires changes to proceed.Dec 19 2017, 10:17 AM
fjricci edited the summary of this revision. (Show Details)Dec 19 2017, 10:25 AM
smeenai resigned from this revision.Dec 19 2017, 10:28 AM

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 accepted this revision.Dec 19 2017, 10:28 AM

(gonna resign afterwards to clear out my request changes)

This revision is now accepted and ready to land.Dec 19 2017, 10:28 AM
smeenai resigned from this revision.Dec 19 2017, 10:28 AM
This revision now requires review to proceed.Dec 19 2017, 10:28 AM

ping (particularly looking for input from @beanz)

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

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

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

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!

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

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

D41673 is what I had in mind.

That patch fixes our build for me.

beanz added a comment.Jan 3 2018, 10:29 AM

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 abandoned this revision.Jan 8 2018, 1:51 PM

Abandoning in favor of D41706