keg_relocate: Don't relocate dylib IDs containing metavariables.

[woodruffw]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew tests with your changes locally?

---

This is a WIP and still being tested locally, I'll reword the commit when it's ready.

see rust-lang/rust#39870
cc @alexcrichton @ilovezfs

[alexcrichton]

I've confirmed locally that this patch fixes the problem we ran into locally, thanks for the quick fix @woodruffw!

[ilovezfs]

@UniqMartin do you mind taking a look at this?

[ilovezfs]

@tdsmith given https://github.com/Homebrew/homebrew-science/issues/5173 I wonder if we need to think twice before shipping this.

[tdsmith]

@woodruffw says in the rust ticket:

we're careful not to clobber metavariables like @rpath in install names, but we seem to have neglected the same case for the IDs themselves

If I understand that correctly I don't think this directly impacts netcdf, since the netcdf issue is related to using @rpath to specify a linked library and not the dylib's own install_name?

[woodruffw]

If I understand that correctly I don't think this directly impacts netcdf, since the netcdf issue is related to using @rpath to specify a linked library and not the dylib's own install_name?

Yep, that's correct. This PR should only prevent the rewriting of dylib IDs, not change the current behavior w/r/t linked libraries.

[ilovezfs]

🎊

[woodruffw]

Since there's a good chance we'll be reverting this, I'm trying to get to the bottom of why we were skipping metavariables in the first place. Looks like it got introduced here, way back in 2012: bd4f69a

Any recollection of the reason for that change @jacknagel?

[jacknagel]

Mmm, not much. If I had to guess I'd say it was resulting in incorrect linkages, and leaving the rpath alone made it work. I don't remember any specifics though.

Shame on me for not writing a good commit message. 😿

[woodruffw]

If I had to guess I'd say it was resulting in incorrect linkages, and leaving the rpath alone made it work. I don't remember any specifics though.

That helps a bit. I suppose the linkage could be broken pretty easily if the file specifies a different prefix under a LC_RPATH and we ignore that in favor of our own prefix, although that shouldn't happen much (ever?) in practice.

I'll do some sleuthing in my local installation.

Shame on me for not writing a good commit message. 😿

Don't sweat it; I dug up a 5 year old commit on the slim chance that you might remember it 😄

[ilovezfs]

Unfortunately this change broke llvm:

iMac-TMP:homebrew-core joe$ brew test llvm
Testing llvm
==> Using the sandbox
==> /usr/local/Cellar/llvm/3.9.1/bin/llvm-config --prefix
==> /usr/local/Cellar/llvm/3.9.1/bin/clang -L/usr/local/Cellar/llvm/3.9.1/lib -fopenmp -nobuiltininc -I/usr/local/Cellar/llvm/3.9.1/lib/clang/3.9.1/include omptest.c -o omptest
==> ./omptest
dyld: Library not loaded: @rpath/libomp.dylib
  Referenced from: /private/tmp/llvm-test-20170228-37913-v6douy/./omptest
  Reason: image not found
Error: llvm: failed
<0> expected but was
<nil>.

[ilovezfs]

So one option is to change the LLVM test with something like

iMac-TMP:homebrew-core joe$  git diff -- Formula/llvm.rb 
diff --git a/Formula/llvm.rb b/Formula/llvm.rb
index 103aa3b..cc5ee3c 100644
--- a/Formula/llvm.rb
+++ b/Formula/llvm.rb
@@ -301,8 +301,8 @@ class Llvm < Formula
       }
     EOS
 
-    system "#{bin}/clang", "-L#{lib}", "-fopenmp", "-nobuiltininc",
-                           "-I#{lib}/clang/#{version}/include",
+    system "#{bin}/clang", "-L#{lib}", "-Wl,-rpath,#{lib}", "-fopenmp",
+                           "-nobuiltininc", "-I#{lib}/clang/#{version}/include",
                            "omptest.c", "-o", "omptest"
     testresult = shell_output("./omptest")
 
@@ -381,7 +381,7 @@ class Llvm < Formula
               "-I#{MacOS.sdk_path}/usr/include",
               "-L#{lib}",
               "-Wl,-rpath,#{lib}", "test.cpp", "-o", "test"
-      assert_includes MachO::Tools.dylibs("test"), "#{opt_lib}/libc++.1.dylib"
+      assert_includes MachO::Tools.dylibs("test"), "@rpath/libc++.1.dylib"
       assert_equal "Hello World!", shell_output("./test").chomp
     end
   end

[woodruffw]

Hmm. Our ultimate goal should probably be to not exclude metavariables from relocation, so I think that reverting this is the right move.

I can create a PR for that in a bit if you'd like, @ilovezfs.

[fxcoudert]

Same problem with lapack test: Homebrew/homebrew-core#12033

[UniqMartin]

I don't know for sure why things regarding rewriting of library paths are (or have been) the way they are, but I have some ideas, cf. rust-lang/rust#39870 (comment).

To summarize: The rewriting of dylib IDs helps to avoid problems by libraries and software not properly ported to macOS with some collateral damage (broken relocatability of software that gets everything right) that is acceptable per the goals of the Homebrew project.

Also sorry for this incredibly late response. If things have developed further in this area, I'm decently out of touch with more recent developments in Homebrew land …