Delete rpaths one at a time

[carlocab]

Since delete_rpath differs from the behaviour of otool in that
duplicate rpaths are deleted in a single invocation, we should be
consistent with that and not report duplicates even when they exist.

Otherwise, this can break workflows that iterate through a macho-o
file's rpaths and does something with each of them. I currently have a
workaround for this in brew [1], but I realised this should actually
probably be fixed here.

[1] Homebrew/brew#11392

[carlocab]

The alternative to this is probably to be more like otool and just delete duplicate rpaths one at a time.

[woodruffw]

Yeah, I'm of two minds on this:

• ruby-macho is intended to directly reflect the contents of the parsed Mach-O, i.e.: if there are multiple duplicate RPATHs, then #rpaths should return each of them (reflecting the underlying reality of the binary).

• ruby-macho should behave mostly the same as otool/install_name_tool/libmacho, i.e.: if any of those tools perform modifications or operations in any particular way, then we try to duplicate their technique.

Overall, I'm inclined to say that #rpaths should return the exact list of RPATHs in the Mach-O, not uniq'd. That being said, we should probably add a warning or other notice to the YARD docs for #rpaths and other accessors that can return potentially duplicated strings.

[woodruffw]

Another idea: I'm not opposed to a uniq: true or similar kwarg! That gets us the best of both worlds (the default is still a direct representation of the binary, uniq: true gives a more user-friendly view).

[carlocab]

Either of those is fine, but I think if rpaths are presented as they are contained in the parsed Mach-O, then they should probably also be acted on in the way they are presented. i.e. delete_rpath should only delete one among duplicates, rather than all of them together.

Said differently, if #rpaths distinguishes between distinct LC_RPATH entries that actually refer to the same path, then so should #delete_rpath. Or, at the very least, #delete_rpath should (also?) offer a kwarg! that allows you to act on duplicate rpaths as a group or individually.

[woodruffw]

Said differently, if #rpaths distinguishes between distinct LC_RPATH entries that actually refer to the same path, then so should #delete_rpath. Or, at the very least, #delete_rpath should (also?) offer a kwarg! that allows you to act on duplicate rpaths as a group or individually.

Yep, fully agree. What do you think about this?

1. #rpaths learns a uniq kwarg
2. #delete_rpath also only deletes one RPATH at a time, and learns a multiple kwarg for duplicates

[carlocab]

Makes sense to me! I think this brings things closer in line with otool and install_name_tool, which is a good benchmark to err on.

How do we want to do #change_rpath, though? IIRC this just invokes #delete_rpath then #add_rpath. Should this also change dupes one at a time? This is what install_name_tool does, and seems most consistent with how other commands act. (I guess it also needs to learn a kwarg to align its behaviour with #rpaths and #delete_rpath.)

There might also be room here for an extra command: uniq-ifying duplicate rpaths (if that makes sense).

[woodruffw]

How do we want to do #change_rpath, though? IIRC this just invokes #delete_rpath then #add_rpath. Should this also change dupes one at a time? This is what install_name_tool does, and seems most consistent with how other commands act. (I guess it also needs to learn a kwarg to align its behaviour with #rpaths and #delete_rpath.)

Yeah, changing dupes one at a time sounds good to me 🙂

There might also be room here for an extra command: uniq-ifying duplicate rpaths (if that makes sense).

Absolutely! #uniq_rpaths!, perhaps?

[carlocab]

Sounds good to me. I think all this can be done in three steps:

1. Change the behaviour of #delete_rpath to handle only one rpath at a time. (Since #change_rpath just invokes #delete_rpath, that gives us the desired behaviour of #change_rpath for free.)
2. Teach #rpaths, #change_rpath and #delete_rpath some kwargs.
3. Add #uniq_rpaths!.

I'll rework this PR to do the first step.

[carlocab]

New commit pushed. Not sure how much we want to change the test for rpath deletion here.

[woodruffw]

Not sure how much we want to change the test for rpath deletion here.

Hmm, perhaps unroll the loop, and assert that the count is what we expect at each step?

[carlocab]

Tweaked the test a bit.

[woodruffw]

Awesome. I'll take a look tonight.

[woodruffw]

Hmm, now that we're only deleting one at a time, maybe we should delete the first instead of the last (since that's what I suspect install_name_tool does).

That in turn makes everything a little more efficient, since we can switch from #select to #find for finding the first RPATH. WDYT?

[carlocab]

Sure, makes sense to me. Let me fix that up.

[woodruffw]

Thanks! LGTM.