Review period will end on 2021-04-20 at 08:30:16 UTC.

Is this cmake specific or more general? If it's cmake specific I would like to propose to use cmake_rpath. Else rpath is fine.

It's likely to be used with cmake the most, but it could be used more generally (say, passed to the linker).

There is a def rpath in formula.rb 😅

There is a def rpath in formula.rb 😅

I cannot read 🤦🏻. !

Review period ended.

I didn't see this PR until now. For the future, I'm very interested in getting a review request on PRs that affect Linux bottles and their ability to be relocated.

An RPATH of $ORIGIN/../lib is only correct for files installed in a top-level directory such as bin or libexec. rpath should take two arguments: the location of the executable (or perhaps its directory), the desired library directory, and compute the correct relative path from the executable's directory to that of the library.

Thank you for this work, Carlo. I like the general idea. Here's an alternative. keg_relocate rather than replacing the absolute path #{HOMEBREW_PREFIX} with @@HOMEBREW_PREFX@@ could instead compute the relative path from the executable to each library directory specified in RPATH, and replace each element of RPATH with $ORIGIN and a relative path to the library directory.

For the rpath API, I'd suggest something like lib.rpath bin for executables that are being installed in bin and are looking for libraries installed in lib.

Thanks for the comments, @sjackman! I'll remember to request a review from you in the future for PRs like this.

While I do like the added flexibility you suggest, I don't see where it could be used at the moment. This PR was designed to make it easy to pass a value to CMAKE_INSTALL_RPATH, which doesn't really give you much control over what it's applied to.

I was contemplating a slightly less sophisticated change to this, which, with a little bit more verbosity might let you do something similar to what you suggest, I think: Homebrew/homebrew-core#75566 (comment)

Note that this value of RPATH isn't necessarily just for executables looking for libraries: it's also for libraries looking for other libraries. (See the llvm formula for an instance where this is needed.)

Note that this value of RPATH isn't necessarily just for executables looking for libraries: it's also for libraries looking for other libraries.

Good point! In that case particular case, although $ORIGIN/../lib works, it should probably instead be simply $ORIGIN since the libraries are found in the same directory.

Well the idea is that it should work for both libraries and executables with no intervention :)

My biggest concern is that Formula.rpath makes assumptions and hides complexity that's not evident from its name. It works only for files installed in a directory subdirectory of prefix (like bin, lib, and libexec) and looking for libraries in lib. Although that's certainly the most common case, it's not the only case. The desired functionality is more similar to the existing function Pathname.make_relative_symlink. I'd suggest making a function something like Pathname.make_relative_rpath.

Are there any formulae that could make use of the added flexibility? I agree it makes a big assumption, but it's an assumption that standard build systems make. It's also an assumption that I believe is true for each of the ~40 cases I intend to use it in.

Are there any formulae that could make use of the added flexibility?

Homebrew/homebrew-core#75566 (comment)
You mentioned csound, which has libraries installed in Frameworks.

https://github.com/Homebrew/homebrew-core/blob/bf3cc1388d68d829bfbeb7f9cdfb71ac57b4692c/Formula/standardese.rb#L24
standardese.rb has libraries installed in libexec/lib.

If standardese has libraries installed in libexec/lib that uses RPATH to find other libraries also installed in libexec/lib, then RPATH=@loader_path/../lib would work for those libraries, although RPATH=@loader_path would be simpler.

If it has executables installed in bin that uses RPATH to find libraries installed in libexec/lib, then RPATH=@loader_path/../lib would not work for those executables. It needs RPATH=@loader_path/../libexec/lib

f = Formula["hello"]

# The RPATH for an executable installed in bin looking for a library installed in lib.
p "@loader_path/#{f.lib.relative_path_from f.bin}"
# "@loader_path/../lib"

# The RPATH for an executable installed in bin looking for a library installed in Frameworks.
p "@loader_path/#{f.frameworks.relative_path_from f.bin}"
# "@loader_path/../Frameworks"

# The RPATH for a library installed in lib looking for a library installed in lib.
p "@loader_path/#{f.lib.relative_path_from f.lib}"
# "@loader_path/."

# The RPATH for an executable installed in bin looking for a library installed in libexec/lib.
p "@loader_path/#{(f.libexec/"lib").relative_path_from f.bin}"
# "@loader_path/../libexec/lib"

# The RPATH for an executable installed in libexec/bin looking for a library installed in lib.
p "@loader_path/#{f.lib.relative_path_from f.libexec/"bin"}"
# "@loader_path/../../lib"

A few different cases to consider.

Perhaps something like…

class Pathname
  def relative_rpath_from base_directory
    "@loader_path/#{self.relative_path_from base_directory}"
  end
end

$ otool -l /usr/local/Cellar/standardese/0.5.1/bin/standardese | grep -A2 LC_RPATH
          cmd LC_RPATH
      cmdsize 56
         path /usr/local/opt/standardese/libexec/lib (offset 12)

The RPATH of the executable bin/standardese needs to be set to @loader_path/../libexec/lib.

Homebrew/homebrew-core#75566 (comment)
You mentioned csound, which has libraries installed in Frameworks.

It does, but I don't think we'll be using an rpath helper in csound, as it requires a number of additional hacks to make relocatable.

https://github.com/Homebrew/homebrew-core/blob/bf3cc1388d68d829bfbeb7f9cdfb71ac57b4692c/Formula/standardese.rb#L24
standardese.rb has libraries installed in libexec/lib.

I know. I put them there in Homebrew/homebrew-core@8916862, because I wasn't sure if putting them in lib was the right move. 😄 I have since come to the realisation that I was mistaken. (This was shortly after I opened Homebrew/homebrew-core#75458 and started contemplating the fix in this PR.) I am moving the libraries to lib when I fix standardese.

After looking over the 37 formulae that use CMAKE_INSTALL_RPATH and finding only two exceptions csound and standardese, and you plan to fix the latter, I'm fine with the simpler Formula.rpath API that you've already implemented. I commented too quickly before fully understanding the situation. Sorry for that.

It does make me a tad nervous that if the executable is installed in a subdirectory like say libexec/tools/foobar then it will not be able to find its libraries installed in lib through the RPATH of @loader_path/../lib. If that case were to come up, it'll likely fail tests like brew linkage (though not on Linux where HOMEBREW_PREFIX/lib is in the library search path), and we can cross that bridge when we get there.

Thanks for improving bottle relocation, Carlo!

No apologies necessary! Always happy to discuss changes to make sure they're the right ones to make.

I actually really like the idea of a more flexible rpath API that computes relative paths between binary objects in order to determine the correct RPATH. Unfortunately, I'm not aware of a standard build system that allows you to take advantage of such flexibility. We might one day build it into brew's relocation code, but I don't think we've found need to just yet.

I agree with your concern about breaking stuff though: I'll be careful, and double check the built bottles for stray binary objects living somewhere the current definition of rpath doesn't account for.