This is an archive of the discontinued LLVM Phabricator instance.

[cmake] [libcxx] Call llvm_setup_rpath() when adding shared libraries.
ClosedPublic

Authored by hintonda on Jan 23 2018, 9:15 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hintonda created this revision.Jan 23 2018, 9:15 PM
hintonda retitled this revision from [cmake] Call llvm_setup_rpath() when adding shared libraries. to [cmake] [libcxx] Call llvm_setup_rpath() when adding shared libraries..Jan 23 2018, 9:16 PM
compnerd accepted this revision.Jan 24 2018, 7:50 PM
This revision is now accepted and ready to land.Jan 24 2018, 7:50 PM
This revision was automatically updated to reflect the committed changes.
hintonda reopened this revision.Jan 25 2018, 11:28 AM
hintonda added a reviewer: smeenai.

Reopen after rollback.

@smeenai pointed out LLVM may not be available in standalone builds

This revision is now accepted and ready to land.Jan 25 2018, 11:29 AM
hintonda requested review of this revision.Jan 25 2018, 12:11 PM

I probably should have put this in the summary, but the reason for this patch isn't just to clean up the cmake files, it's to fix a cmake configuration failure when cross-compiling for Linux on Darwin:

CMake Error at /Users/dhinton/projects/llvm_project/libcxx/lib/CMakeLists.txt:224 (add_library):

The install of the cxx_shared target requires changing an RPATH from the
build tree, but this is not supported with the Ninja generator unless on an
ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH variable may be set
to avoid this relinking step.
hintonda updated this revision to Diff 131506.Jan 25 2018, 1:44 PM

Only call llvm_setup_rpath() if LLVM_FOUND is true.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 25 2018, 5:38 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

@hintonda Is there a reason why this was done except for "standardizing the usage across projects"?

I'm asking because this change prevents us from being able to avoid having a rpath completely when building libc++. That is the behavior we want when building libc++ as a system library, in which case we specify the install name dir to be /usr/lib, and we don't want any rpath to appear.

@hintonda Is there a reason why this was done except for "standardizing the usage across projects"?

It was just for standardization. Please feel free to rollback or modify as necessary.

@hintonda Is there a reason why this was done except for "standardizing the usage across projects"?

It was just for standardization. Please feel free to rollback or modify as necessary.

Ok, thanks a lot for the information. If things worked before, I would like to revert this change as we are working around it in our internal build, and it just bit me today. I'll send a review.

I proposed a revert here for those who are interested: https://reviews.llvm.org/D91099