commonmark / cmark

CommonMark parsing and rendering library and program in C
Other
1.63k stars 544 forks source link

build: Tell cmake to set 'rpath' #531

Closed mfwitten closed 5 months ago

mfwitten commented 7 months ago

Before this commit, the cmark executable didn’t inform the dynamic linker where to look for the libcmark shared object; this becomes an irritation when libcmark is installed in an unorthodox location:

$ ldd /path/to/installed/files/bin/cmark
        linux-gate.so.1 (0xb7ed7000)
        libcmark.so.0.31.0 => not found
        libc.so.6 => /usr/lib/libc.so.6 (0xb7cf3000)
        /lib/ld-linux.so.2 => /usr/lib/ld-linux.so.2 (0xb7ed8000)

Because of this commit, the cmark executable’s rpath[0][1] setting (or equivalent) is set upon installation, thereby providing the required search directory:

$ ldd /path/to/installed/files/bin/cmark
        linux-gate.so.1 (0xb7ef2000)
        libcmark.so.0.31.0 => /path/to/installed/files/lib/libcmark.so.0.31.0 (0xb7e95000)
        libc.so.6 => /usr/lib/libc.so.6 (0xb7cbc000)
        /lib/ld-linux.so.2 => /usr/lib/ld-linux.so.2 (0xb7ef3000)

In particular, rpath is set to the following value:

"${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}"

Of course, this will only help on a system that supports an rpath feature known to cmake. Windows has no such feature anyway, apparently.

nwellnhof commented 7 months ago

The rpath should be set to ${CMAKE_INSTALL_FULL_LIBDIR} to make absolute libdirs work.

Then we should consider not adding an rpath for system library directories, see FIND CMAKE_PLATFORM_IMPLICIT_LINK_DIRECTORIES in the CMake Wiki. Alternatively, we could make this feature optional.

mfwitten commented 7 months ago

Thank you for the help, @nwellnhof.

I forcibly pushed 2 commits to replace the old one:

The following text was added to the original commit message:

[...]

There is some intelligence behind whether 'rpath' is set at all:

  * If a shared object (e.g., 'libcmark') is going to be installed
    in a standard location, then such location is not added to 'rpath'.

  * A non-standard installation will cause 'rpath' to include at least
    the following path provided by cmake:

      "${CMAKE_INSTALL_FULL_LIBDIR}"

  * Also, cmake has been instructed to append any non-standard path
    to 'rpath' if cmake is aware of an external dependency from
    such a path.

Of course, this will only help on a system that supports an 'rpath'
feature known to cmake; for example, Windows has no such feature,
and so all of this will presumably be ignored when building under
that system.

EDIT: In the commit log, I accidentally wrote CMAKE_INSTALL_PREFIX_FULL_LIBDIR instead of CMAKE_INSTALL_FULL_LIBDIR. I have addressed the mistake, and forcibly pushed the fixed commit here.

mfwitten commented 6 months ago

I think I have hit all the buttons this time. 🤞

nwellnhof commented 6 months ago

Looks good to me.

mfwitten commented 5 months ago

I pushed a slightly modified version; as described in the commit log:

If CMAKE_INSTALL_RPATH is set, then it is used; this allows the user to set a single, overriding value the expected way.

Essentially:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9170bf3..21c1763 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -69,6 +69,9 @@ set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)

 set(CMAKE_INCLUDE_CURRENT_DIR ON)

+if (DEFINED CMAKE_INSTALL_RPATH)
+  set(Base_rpath "${CMAKE_INSTALL_RPATH}")
+else()
   if(BUILD_SHARED_LIBS)
     set(p "${CMAKE_INSTALL_FULL_LIBDIR}")
     list(FIND CMAKE_PLATFORM_IMPLICIT_LINK_DIRECTORIES "${p}" i)
@@ -76,6 +79,7 @@ if(BUILD_SHARED_LIBS)
       set(Base_rpath "${p}")
     endif()
   endif()
+endif()

 # Append non-standard external dependency directories, if any.
 set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
jgm commented 5 months ago

Sorry, I forgot about this. @nwellnhof if it still looks good to you I will merge this.

nwellnhof commented 5 months ago

Looks good.