Bottling GNU awk fails when using Ruby-MachO

[UniqMartin]

The failed build of PR Homebrew/homebrew-core#2339 revealed some subtly different behavior in the relocation code based on cctools and the one based on ruby-macho. The failure is actually a combination of multiple factors:

1. The gawk formula installs two Mach-O binaries in bin/, namely gawk and gawk-4.1.3. Turns out these are not only identical, but actually a hard link of each other. This means that by the time the first file has been relocated (its install names rewritten), there's nothing left to do on the second file.
2. The code in the RubyMachO module caches its data in the associated Pathname instance, thus by the time the relocation code starts enumerating the dylibs the second binary is linked against, it is operating on stale data that has been cached at the time when the files were enumerated (and before the first file was modified).
3. All of the above only shows because ruby-macho has stricter failure behavior when compared with install_name_tool. The former throws an exception when attempting to perform a modification that doesn't make sense, the latter silently ignores changes to the dylib ID or the install names, if they don't make sense (e.g. when attempting to set a dylib ID on a bundle or trying to change an install name that doesn't exist).

I'm not sure about the best way to fix this problem, but probably a combination of the following items would be desirable:

1. Make the code that enumerates Mach-O files aware of hard links and only return one of possibly many paths with identical inode.
2. Avoid caching Mach-O data in the RubyMachO module or find a way to safely and efficiently invalidate the cached data to avoid operating on stale data. A similar problem could affect CctoolsMachO, but doesn't in practice because the information about install names and the dylib ID happens to be cached separately from the other Mach-O specific data. (Thus its purely coincidental that CctoolsMachO doesn't run into a similar issue.) It would be interesting to evaluate the performance impact if we avoided caching for the sake of correctness.
3. More closely resemble semantics of install_name_tool and silently ignore certain error conditions instead of raising an exception. (I'm not sure that's something we actually want and it shouldn't be necessary if we address the other two items, but I wanted to list this for the sake of completeness.)

Questions, comments, and ideas for solutions are more than welcome!

cc @woodruffw

---

Here are the necessary steps to reproduce the problem locally. Assuming a pristine Homebrew installation, prepare with:

$ export HOMEBREW_DEVELOPER=1
$ export HOMEBREW_RUBY_MACHO=1
$ brew install --only-dependencies gawk
$ brew install --build-bottle --verbose gawk

The error shows when trying to bottle the formula:

$ brew bottle --verbose --json gawk
==> Determining gawk bottle revision...
==> Bottling gawk-4.1.3_1.el_capitan.bottle.1.tar.gz...
Error: No such dylib name: @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib

Adding --debug reveals that the error happens when relocating gawk-4.1.3, as it is a hard link to gawk and thus was already relocated:

$ brew bottle --verbose --debug --json gawk
[…snip…]
==> Bottling gawk-4.1.3_1.el_capitan.bottle.1.tar.gz...
Changing install name in /opt/local/Cellar/gawk/4.1.3_1/bin/gawk
  from /usr/local/opt/readline/lib/libreadline.6.dylib
    to @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk
  from /usr/local/opt/mpfr/lib/libmpfr.4.dylib
    to @@HOMEBREW_PREFIX@@/opt/mpfr/lib/libmpfr.4.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk
  from /usr/local/opt/gmp/lib/libgmp.10.dylib
    to @@HOMEBREW_PREFIX@@/opt/gmp/lib/libgmp.10.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk-4.1.3
  from /usr/local/opt/readline/lib/libreadline.6.dylib
    to @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk
  from @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib
    to /usr/local/opt/readline/lib/libreadline.6.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk
  from @@HOMEBREW_PREFIX@@/opt/mpfr/lib/libmpfr.4.dylib
    to /usr/local/opt/mpfr/lib/libmpfr.4.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk
  from @@HOMEBREW_PREFIX@@/opt/gmp/lib/libgmp.10.dylib
    to /usr/local/opt/gmp/lib/libgmp.10.dylib
Changing install name in /usr/local/Cellar/gawk/4.1.3_1/bin/gawk-4.1.3
  from @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib
    to /usr/local/opt/readline/lib/libreadline.6.dylib
Error: No such dylib name: @@HOMEBREW_PREFIX@@/opt/readline/lib/libreadline.6.dylib
/usr/local/Library/Homebrew/vendor/macho/macho/macho_file.rb:240:in `change_install_name'
/usr/local/Library/Homebrew/vendor/macho/macho/tools.rb:33:in `change_install_name'
/usr/local/Library/Homebrew/os/mac/ruby_keg.rb:13:in `change_install_name'
/usr/local/Library/Homebrew/keg_relocate.rb:45:in `block (3 levels) in relocate_install_names'
/usr/local/Library/Homebrew/keg_relocate.rb:130:in `each'
/usr/local/Library/Homebrew/keg_relocate.rb:130:in `each_install_name_for'
/usr/local/Library/Homebrew/keg_relocate.rb:38:in `block (2 levels) in relocate_install_names'
/usr/local/Library/Homebrew/extend/pathname.rb:371:in `ensure_writable'
/usr/local/Library/Homebrew/keg_relocate.rb:32:in `block in relocate_install_names'
/usr/local/Library/Homebrew/keg_relocate.rb:31:in `each'
/usr/local/Library/Homebrew/keg_relocate.rb:31:in `relocate_install_names'
/usr/local/Library/Homebrew/cmd/bottle.rb:259:in `block (2 levels) in bottle_formula'
/usr/local/Library/Homebrew/utils.rb:468:in `ignore_interrupts'
/usr/local/Library/Homebrew/cmd/bottle.rb:257:in `ensure in block in bottle_formula'
/usr/local/Library/Homebrew/cmd/bottle.rb:257:in `block in bottle_formula'
/usr/local/Library/Homebrew/keg.rb:245:in `block in lock'
/usr/local/Library/Homebrew/formula_lock.rb:29:in `with_lock'
/usr/local/Library/Homebrew/keg.rb:241:in `lock'
/usr/local/Library/Homebrew/cmd/bottle.rb:193:in `bottle_formula'
/usr/local/Library/Homebrew/cmd/bottle.rb:450:in `block in bottle'
/usr/local/Library/Homebrew/cmd/bottle.rb:449:in `each'
/usr/local/Library/Homebrew/cmd/bottle.rb:449:in `bottle'
/usr/local/Library/brew.rb:87:in `<main>'

[woodruffw]

Oh well, it wouldn't be a rollout if there wasn't at least one bug 😄

Could we fix this in Keg#mach_o_files? We currently have this exclusion logic:

path.find do |pn|
      next if pn.symlink? || pn.directory?
      mach_o_files << pn if pn.dylib? || pn.mach_o_bundle? || pn.mach_o_executable?
end

One possible approach is the one used in Pathname#compute_disk_usage, where a set of scanned files is built and only added to when a file's device ID/inode pair is unique.

Thoughts?

[ilovezfs]

I'm not sure that's something we actually want and it shouldn't be necessary if we address the other two items, but I wanted to list this for the sake of completeness

Are the problems it can catch actually likely to occur? If not, it seems like mimicking the "old" behavior is the path of least resistance.

[woodruffw]

The two-line change I propose in #400 should fix the problem for both cctools and ruby-macho (since both use Pathname#mach_o_files), even though only ruby-macho actually complains about it.

[ilovezfs]

You need to be checking st_dev too as here https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/pathname.rb#L40

[woodruffw]

Updated it!

[tdsmith]

The fix in #400 looks reasonable to me; thanks for your clear explanation!

[tdsmith]

Presuming fixed by #400 but please reopen at will.

[UniqMartin]

@ilovezfs @tdsmith Thanks a bunch for your help with this during my absence!

Oh well, it wouldn't be a rollout if there wasn't at least one bug 😄

Indeed! 😀 It's a rather subtle issue, as can be seen from my report. Absolutely not trying to blame you here, as none of us was able to foresee this.