This is an archive of the discontinued LLVM Phabricator instance.

Fix generation of exported targets in build directory
ClosedPublic

Authored by mcopik on Oct 17 2018, 8:10 AM.

Details

Summary

After a long investigation of my own CMake scripts, I found out that CMake is not able to link with Polly libraries because the exported targets file is not generated correctly in the build directory when CONFIG is not set.

The generated file lib/cmake/polly/PollyExports-all.cmake

set_target_properties(LLVMPolly PROPERTIES
              IMPORTED_LOCATION_ my_build_dir/lib/LLVMPolly.so)
set_target_properties(Polly PROPERTIES
              IMPORTED_LOCATION_ my_build_dir/lib/libPolly.a)

Please notice the additional underscore at the end of IMPORTED_LOCATION. The generation for install directory works correctly since the generator expression used there adds the underscore and config name only when it's defined. This patch simply adds this missing guard for cmake files generated in the build directory.

Diff Detail

Repository
rL LLVM

Event Timeline

mcopik created this revision.Oct 17 2018, 8:10 AM

I suggest to add @philip.pfaffe as a reviewer (who contributed these lines in D30495)

The fix looks alright, but did you test it with CMAKE_BUILD_TYPE set though? It's hard to parse with the naked eye, but it looks like you're missing at least a $ there.

mcopik updated this revision to Diff 170613.Oct 23 2018, 5:35 AM

The fix looks alright, but did you test it with CMAKE_BUILD_TYPE set though? It's hard to parse with the naked eye, but it looks like you're missing at least a $ there.

I took this code directly from line 114 where it is defined for install target. But you were correct, this code is not correct. It's missing a $ in front of UPPER_CASE and it's missing a > in the end. Otherwise the main condition "print if config not empty" is not interpreted as a boolean generator expression. I fixed the patch and uploaded a new one.

That's the new output for CMAKE_BUILD_TYPE set to Release:

build directory:

set_target_properties(LLVMPolly PROPERTIES
              IMPORTED_LOCATION_RELEASE build_dir/lib/LLVMPolly.so)
set_target_properties(Polly PROPERTIES
              IMPORTED_LOCATION_RELEASE build_dir/lib/libPolly.a)

install directory:

set_target_properties(LLVMPolly PROPERTIES
        IMPORTED_LOCATION_RELEASE ${CMAKE_CURRENT_LIST_DIR}/../../LLVMPolly.so)
set_target_properties(Polly PROPERTIES
        IMPORTED_LOCATION_RELEASE ${CMAKE_CURRENT_LIST_DIR}/../../libPolly.a)

Before it was:

set_target_properties(LLVMPolly PROPERTIES
        IMPORTED_LOCATION_<UPPER_CASE:Release ${CMAKE_CURRENT_LIST_DIR}/../../LLVMPolly.so)
set_target_properties(Polly PROPERTIES
        IMPORTED_LOCATION_<UPPER_CASE:Release ${CMAKE_CURRENT_LIST_DIR}/../../libPolly.a)

Names are generated correctly for an empty build type as well.

philip.pfaffe accepted this revision.Oct 23 2018, 6:15 AM

LGTM, thanks!

This revision is now accepted and ready to land.Oct 23 2018, 6:15 AM
mcopik added a comment.EditedNov 6 2018, 6:53 AM

@philip.pfaffe could I ask to commit the patch on my behalf? I don't have write access to the repository.

This revision was automatically updated to reflect the committed changes.