build: Tell cmake to set 'rpath'

[mfwitten]

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]

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]

Thank you for the help, @nwellnhof.

I forcibly pushed 2 commits to replace the old one:

• Removed explicit enabling of MACOSX_RPATH.
• Addressed the things you talked about.

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

[nwellnhof]

Both of these variables should already be set to FALSE by default. I think they are only mentioned in the CMake Wiki for clarity.

[mfwitten]

Well, exactly. They are mentioned for clarity.

• Defaults are great, until they're not; convention is a terrible foundation.

• As it turns out, cmake has a lot of knobs related to rpath; one might as well set the knobs explicitly, and in one place, so as to help anybody in the future who has some unforeseen need.

Nevertheless:

• The reason it made sense to me to remove the explicit setting of the target property MACOSX_RPATH is that such an option never made much sense, anyway.

• If you still don’t agree with setting some of these variables explicitly, then perhaps they can be simply commented out. Would that be acceptable?

[nwellnhof]

I think it's misleading to set variables to their default values.

[mfwitten]

Of course, it's not misleading at all. Quite the opposite.

However, I'll choose to interpret your reply as "Look, just remove them, OK?"

[nwellnhof]

libcmark has no external dependencies, so this is unnecessary.

[mfwitten]

Of course libcmark has external dependencies.

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

For example:

cd "$repo"
d=/tmp/custom
p='
    {print}
    /^include\(GNUInstallDirs\)$/ {
      print("link_directories(\"'"$d"'\")")
    }
'
f=CMakeLists.txt; t=$f.tmp; <"$f" awk >"$t" "$p" && mv -f "$t" "$f"
git diff | cat
echo '================='
i='/path/to/installed/files'
make INSTALL_PREFIX="$i" install | grep 'Set runtime path'

Here are the results of running those commands:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 054f8d4..22d9506 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -24,6 +24,7 @@ include(CMakePackageConfigHelpers)
 include(CTest)
 include(GenerateExportHeader)
 include(GNUInstallDirs)
+link_directories("/tmp/custom")
 
 if(NOT MSVC OR CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
   set(CMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_NO_WARNINGS ON)
=================
-- Set runtime path of "/path/to/installed/files/bin/cmark" to "/path/to/installed/files/lib:/tmp/custom"
-- Set runtime path of "/path/to/installed/files/lib/libcmark.so.0.31.0" to "/path/to/installed/files/lib:/tmp/custom"

Then:

• Default to System Library

  $ ldd "$i"/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)
  

• Custom Library

  $ mkdir -p "$d"
  $ cp -a -t "$d" /usr/lib/libc.so.6 /usr/lib/libc-2.29.so
  $ ldd "$i"/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 => /tmp/custom/libc.so.6 (0xb7cbc000)
          /lib/ld-linux.so.2 => /usr/lib/ld-linux.so.2 (0xb7ef3000)
  

In other words:

• By simply adding link_directories("/tmp/custom") to the CMakeLists.txt, the directory /tmp/custom got added to the rpath.

• The only reason this works is because set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE).

It's not unreasonable to think that at some point, in the future, an external dependency might be possible; maybe Nvidia will release a driver for offloading markdown parsing to the GPU. :-P

[nwellnhof]

Well, obviously the C library is a dependency but it doesn't seem useful to support this use case. Maybe we should also consider to only set the INSTALL_RPATH target property for the cmark executable.

[mfwitten]

• Shared libraries have dependencies, too.

    $ ldd "$i"/lib/libcmark.so
            linux-gate.so.1 (0xb7fa3000)
            libc.so.6 => /tmp/custom/libc.so.6 (0xb7da3000)
            /usr/lib/ld-linux.so.2 (0xb7fa4000)
  

• However, it's straightforward enough to set only cmark’s rpath to an unorthodox installation path, while still allowing cmake to set libcmark’s rpath if it deems necessary (as described above); then, in most cases (pretty much always), libcmark’s rpath will be the empty string, even when installed in an unorthodox location.

• The goal isn’t to support that use case.

  (However, it would indeed now be supported; I mean, maybe someone wants to build against musl or something, and has put everything in a separate, non-standard hierarchy.)

  Rather, the goal is to support a logical policy in general, namely:

  If cmake is ever aware of an external dependency (or search directory) that is not covered by the standard locations, then cmake should add the relevant path to the rpath.

  That sounds like a sensible policy; it sounds like a sensible default.

[nwellnhof]

I'm not sure if we need a separate function.

[mfwitten]

I would have much preferred to write:

block()
  set(p "${CMAKE_INSTALL_FULL_LIBDIR}")
  list(FIND CMAKE_PLATFORM_IMPLICIT_LINK_DIRECTORIES "${p}" i)
  if("${i}" STREQUAL "-1")
    set(CMAKE_INSTALL_RPATH "${p}" PARENT_SCOPE)
  endif()
endblock()

However, the CMake language only got [anonymous] block scoping in version 3.25, while this project currently sets 3.7 as the minimum version; I cannot use the block() construct.

This is why I added the comment:

# CMake 3.25 has a better scoping solution: 'block()'

My hope was that it would be a hint to some future janitor to use the block() construct when available.

[nwellnhof]

There are only five relevant Iines in that block and I think it's clearer to add them directly in CMakeLists.txt without adding an additional file.

[mfwitten]

Actually, it would be much clearer to move even more stuff into their own files, but I guess we have different tastes. I will move it all into CMakeLists.txt, as that is what you prefer.

[mfwitten]

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

• There is no new file.
• There is no function scope.
• There is no redundant default.
• No standard path is repeated in an rpath.
• libcmark will essentially never have an rpath.
• cmark will basically have no rpath when statically linked.
• Yet, cmake may still add paths to each rpath as necessary.

[nwellnhof]

Looks good to me.

[mfwitten]

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]

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

[nwellnhof]

Looks good.