Delete last rpath when requested (#556)

Deleting the first rpath is a good default because it mirrors the
behaviour of `install_name_tool`. However, it's not useful behaviour
when deleting duplicate rpaths, because it changes the order in which
the runtime paths are searched.

We delete duplicate rpaths in `brew`, so being able to delete the last
instance of the requested rpath instead of the first one would be
useful.

Co-authored-by: William Woodruff <[email protected]>

lib/macho/macho_file.rb

@@ -418,15 +418,24 @@ def add_rpath(path, _options = {})
  # @param options [Hash]
  # @option options [Boolean] :uniq (false) if true, also delete
  # duplicates of the requested path. If false, delete the first
- # instance (by offset) of the requested path.
+ # instance (by offset) of the requested path, unless :last is true.
+ # Incompatible with :last.
+ # @option options [Boolean] :last (false) if true, delete the last
+ # instance (by offset) of the requested path. Incompatible with :uniq.
  # @return void
  # @raise [RpathUnknownError] if no such runtime path exists
+ # @raise [ArgumentError] if both :uniq and :last are true
  def delete_rpath(path, options = {})
  uniq = options.fetch(:uniq, false)
- search_method = uniq ? :select : :find
+ last = options.fetch(:last, false)
+ raise ArgumentError, "Cannot set both :uniq and :last to true" if uniq && last
+
+ search_method = uniq || last ? :select : :find
+ rpath_cmds = command(:LC_RPATH).public_send(search_method) { |r| r.path.to_s == path }
+ rpath_cmds = rpath_cmds.last if last
 
  # Cast rpath_cmds into an Array so we can handle the uniq and non-uniq cases the same way
- rpath_cmds = Array(command(:LC_RPATH).method(search_method).call { |r| r.path.to_s == path })
+ rpath_cmds = Array(rpath_cmds)
  raise RpathUnknownError, path if rpath_cmds.empty?
 
  # delete the commands in reverse order, offset descending.

test/test_macho.rb

@@ -520,12 +520,57 @@ def test_delete_rpath_uniq
  assert_operator file.ncmds, :<, orig_ncmds
  assert_operator file.sizeofcmds, :<, orig_sizeofcmds
  assert_operator file.rpaths.size, :<, orig_npaths
+ # libdupe rpaths: ["foo", "bar", "foo"]
+ assert_equal file.rpaths, ["bar"] if filename.end_with?("libdupe.dylib")
 
  file.write(actual)
  # ensure we can actually re-load and parse the modified file
  modified = MachO::MachOFile.new(actual)
 
- assert_empty modified.rpaths
+ assert_empty modified.rpaths unless filename.end_with?("libdupe.dylib")
+ assert_equal file.serialize.bytesize, modified.serialize.bytesize
+ assert_operator modified.ncmds, :<, orig_ncmds
+ assert_operator modified.sizeofcmds, :<, orig_sizeofcmds
+ assert_equal file.rpaths.size, modified.rpaths.size
+ assert_operator modified.rpaths.size, :<, orig_npaths
+ end
+ ensure
+ groups.each do |_, actual|
+ delete_if_exists(actual)
+ end
+ end
+
+ def test_delete_rpath_last
+ groups = SINGLE_ARCHES.map do |arch|
+ ["hello.bin", "hello_actual.bin"].map do |fn|
+ fixture(arch, fn)
+ end
+ end
+
+ groups << ["libdupe.dylib", "libdupe_actual.dylib"].map do |fn|
+ fixture(:x86_64, fn)
+ end
+
+ groups.each do |filename, actual|
+ file = MachO::MachOFile.new(filename)
+
+ refute_empty file.rpaths
+ orig_ncmds = file.ncmds
+ orig_sizeofcmds = file.sizeofcmds
+ orig_npaths = file.rpaths.size
+
+ file.delete_rpath(file.rpaths.first, :last => true)
+ assert_operator file.ncmds, :<, orig_ncmds
+ assert_operator file.sizeofcmds, :<, orig_sizeofcmds
+ assert_operator file.rpaths.size, :<, orig_npaths
+ # libdupe rpaths: ["foo", "bar", "foo"]
+ assert_equal file.rpaths, ["foo", "bar"] if filename.end_with?("libdupe.dylib")
+
+ file.write(actual)
+ # ensure we can actually re-load and parse the modified file
+ modified = MachO::MachOFile.new(actual)
+
+ assert_empty modified.rpaths unless filename.end_with?("libdupe.dylib")
  assert_equal file.serialize.bytesize, modified.serialize.bytesize
  assert_operator modified.ncmds, :<, orig_ncmds
  assert_operator modified.sizeofcmds, :<, orig_sizeofcmds