[PATCH] D25304: cmake: Set the proper rpath in add_llvm_executable and llvm_add_library

[Chris Bieneman via llvm-commits]

beanz added a comment.

First, the two blocks of code are doing the same thing, so they should be a function, not duplicated. Aside from that I have some feedback on the specific implementation, but I'm only going to write it on one set of the diffs.



> AddLLVM.cmake:482
> +      set_target_properties( ${name} PROPERTIES INSTALL_NAME_DIR "@rpath" )
> +      set_target_properties( ${name} PROPERTIES INSTALL_RPATH "@executable_path/../lib" )
> +    else(UNIX)

In OS X 10.4 @loader_path was introduced as a more rubust rpath option, and it is equivolent to @executable_path in trivial situations but also supports bundles (which is important to LLDB), so we should use that here instead of @executable_path. I'm pretty sure nobody cares about deploying to 10.3 anymore.

> AddLLVM.cmake:487
> +        if(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")
> +          set_property(TARGET ${name} APPEND_STRING PROPERTY LINK_FLAGS " -Wl,-z,origin")
> +        endif()

Just for safety sake, please put an extra space at the end of the flag too.

> AddLLVM.cmake:491
> +    endif()
> +    if (${CMAKE_PROJECT_NAME} MATCHES "LLVM")
> +      set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH ON )

This is definately not right. This condition will be false in clang and other sub-projects regardless of whether or not you're building against an installed LLVM. It might be sufficient to compare `CMAKE_INSTALL_PREFIX` against `LLVM_INSTALL_PREFIX`. The later is defined by LLVMConfig.cmake which is used by out-of-tree project builds. Writing the compare that way would also result in not having unnecissary extra rpaths.

> AddLLVM.cmake:494
> +    else()
> +      set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH OFF )
> +      set_property(TARGET ${name} APPEND_STRING PROPERTY INSTALL_RPATH ":${LLVM_LIBRARY_DIR}")

Why are you setting this off? This doesn't seem right to me.

> AddLLVM.cmake:495
> +      set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH OFF )
> +      set_property(TARGET ${name} APPEND_STRING PROPERTY INSTALL_RPATH ":${LLVM_LIBRARY_DIR}")
> +    endif()

This should be semi-colon separated not colon-separated. When CMake interprets INSTALL_RPATH it expects a list of entries that it converts to the correct platform specific linker flags. Colon-separated may be correct for ELF platforms, but it isn't for Mach-o.

https://reviews.llvm.org/D25304