oracle-instantclient: fixes dylib rpath on macos #45157

mikemckibben · 2018-08-16T21:27:05Z

Motivation for this change

Libraries and applications linked against the oracle-instantclient package on MacOS fail with the following error due to rpath not being patched:

dyld: Library not loaded: @rpath/libclntsh.dylib.12.1
  Referenced from: /nix/store/zhrzcdamy5vppxyg5h7w8pbiizqkbmxg-ocilib/lib/libocilib.4.dylib
  Reason: image not found

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- [ x ] NixOS
- [ x ] macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
[ x ] Tested execution of all binary files (usually in ./result/bin/)
[ x ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ x ] Fits CONTRIBUTING.md.

@pesterhazy @flokli

LnL7 · 2018-08-16T21:32:24Z

pkgs/development/libraries/oracle-instantclient/default.nix

    install -Dm755 {sqlplus,adrci,genezi} $out/bin
+    ${optionalString stdenv.isDarwin ''
+      for exe in "$out/bin/"* ; do
+        install_name_tool -add_rpath "$out/lib" "$exe"


Could you explain a bit what's going on here, we generally don't use RPATHs on darwin and the combination of this with fixDarwinDylibNames seems odd.

The oracle instantclient binary zip sources are packaged as a flat structure and contain an RPATH relative to the executable (e.g. sqlplus, etc), so the embedded load commands don't find the associated libs when installed in the nix store under ./bin, ./lib tree structure. For example listing the sqlplus binary in the instantclient-sqlplus-macos.x64-12.2.0.1.0-2.zip source gives this:

$ otool -l sqlplus Load command 12 cmd LC_LOAD_DYLIB cmdsize 48 name @rpath/libsqlplus.dylib (offset 24) time stamp 2 Wed Dec 31 17:00:02 1969 current version 0.0.0 compatibility version 0.0.0 Load command 13 cmd LC_LOAD_DYLIB cmdsize 56 name @rpath/libclntsh.dylib.12.1 (offset 24) time stamp 2 Wed Dec 31 17:00:02 1969 current version 0.0.0 compatibility version 0.0.0 Load command 14 cmd LC_LOAD_DYLIB cmdsize 56 name @rpath/libclntshcore.dylib.12.1 (offset 24) time stamp 2 Wed Dec 31 17:00:02 1969 current version 0.0.0 compatibility version 0.0.0 Load command 15 cmd LC_LOAD_DYLIB cmdsize 48 name @rpath/libnnz12.dylib (offset 24) time stamp 2 Wed Dec 31 17:00:02 1969 current version 0.0.0 compatibility version 0.0.0 Load command 18 cmd LC_RPATH cmdsize 32 path @executable_path/ (offset 12)

I would have thought fixDarwinDylibNames would work on executables similar to how autoPatchelfHook works, but digging into the source reveals that function only works on *.dylib files. Adding the call to install_name_tool was the only way I could get the sqlplus and other executables to run without having to mess around with setting the LD_LIBRARY_PATH or DYLD_LIBRARY_PATH in the environment. I am definitely a nix newb, so I'm not sure if there is a different function than fixDarwinDylibNames that works like autoPatchelfHook. I couldn't find anything in the standard nixpkgs setup-hooks.

I think it's fine adding $out/lib to the rpath of all binaries - we do similar things in other places in Nixpkgs as well.

@mikemckibben I see, changing the LC_LOAD_DYLIB to use absolute paths instead would be nicer IMHO but this works. fixDarwinDylibNames does indeed only handle dylibs and since this isn't built from source we can't fix the rpath referrences before the binaries are linked against the libraries, that would have made this unnecessary.

flokli

Thanks for this PR!
I didn't catch the binaries being broken on darwin, (as I was mostly only using his package via the odpic libraries)

flokli · 2018-08-17T21:53:38Z

pkgs/development/libraries/oracle-instantclient/default.nix

    install -Dm755 {sqlplus,adrci,genezi} $out/bin
+    ${optionalString stdenv.isDarwin ''
+      for exe in "$out/bin/"* ; do
+        install_name_tool -add_rpath "$out/lib" "$exe"


I think it's fine adding $out/lib to the rpath of all binaries - we do similar things in other places in Nixpkgs as well.

flokli · 2018-08-17T21:54:33Z

pkgs/development/libraries/oracle-instantclient/default.nix

-  nativeBuildInputs = [ autoPatchelfHook makeWrapper unzip ];
+    ++ optionals (stdenv.isLinux) [ libaio ]
+    ++ optional odbcSupport unixODBC;
+  nativeBuildInputs = [ autoPatchelfHook makeWrapper unzip ] ++ optional stdenv.isDarwin fixDarwinDylibNames;


you don't need fixDarwinDylibNames - install_name_tool should be available without it too.

That's not correct this is a setup-hook. It's necessary for anything that links against these libraries.

@LnL7 it already worked, as it's (only?) consumer, odpic, loads it via dlopen and we add rpaths in postFixup - but no reason not to set the install names absolute :-)

flokli · 2018-08-17T21:55:56Z

pkgs/development/libraries/oracle-instantclient/default.nix

-    ln -s $out/lib/libclntsh.so.12.1 $out/lib/libclntsh.so
+    if [ \! -e "$out/lib/libclntsh${extLib}" ]; then
+      ln -s $out/lib/libclntsh${extLib}.12.1 $out/lib/libclntsh${extLib}
+    fi


libclntsh${extLib}.12.1 should be available on linux as well, no need to check for existence here.

sure, I'll remove the test. I must have had a weird store state locally, as during testing I ran into a link already exists error.

mikemckibben · 2018-08-18T03:19:31Z

Ok, this should be good to go. Let me know if you see anything else or want me to squash the commits. Thanks!

flokli · 2018-08-18T09:12:19Z

pkgs/development/libraries/oracle-instantclient/default.nix

@@ -61,9 +63,7 @@ in stdenv.mkDerivation rec {
    install -Dm644 sdk/demo/* $out/share/${name}/demo

    # PECL::oci8 will not build without this
-    if [ \! -e "$out/lib/libclntsh${extLib}" ]; then
-      ln -s $out/lib/libclntsh${extLib}.12.1 $out/lib/libclntsh${extLib}
-    fi


Uff, the symlink already seems to exist in the darwin zipfile, but not in the linux one, so the build on darwin would now fail.
We indeed need to check for existence, sorry for the confusion. Could you add these back in, plus a comment above that this symlink only exists in zipfiles for some platforms?

What about just overwriting it with ln -sfn?

Works for me too

flokli · 2018-08-18T09:13:38Z

pkgs/development/libraries/oracle-instantclient/default.nix

@@ -42,7 +42,9 @@ in stdenv.mkDerivation rec {
  buildInputs = [ stdenv.cc.cc.lib ]
    ++ optionals (stdenv.isLinux) [ libaio ]
    ++ optional odbcSupport unixODBC;
-  nativeBuildInputs = [ autoPatchelfHook makeWrapper unzip ] ++ optional stdenv.isDarwin fixDarwinDylibNames;
+
+  nativeBuildInputs = [ autoPatchelfHook makeWrapper unzip ]


Cosmetics: We can put the autoPatchelfHook into a optional stdenv.isLinux too.

flokli · 2018-08-18T09:16:01Z

Some more comments, thanks for the work so far!
I'd prefer squashing the commits together, as we might want to backport this to 18.03 too.

mikemckibben · 2018-08-18T12:34:22Z

ok, added changes from latest comments and squashed the commits. Should be good.

flokli · 2018-08-18T13:40:38Z

LGTM, verified sqlplus binary runs and connectivity via sqlalchemy/cx_oracle on both x86_64-linux and x86_64-darwin.

LnL7

Thanks!

GrahamcOfBorg added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 16, 2018

LnL7 reviewed Aug 16, 2018

View reviewed changes

flokli requested changes Aug 17, 2018

View reviewed changes

flokli reviewed Aug 18, 2018

View reviewed changes

oracle-instantclient: fixes dylib rpath on macos

0aa26ae

LnL7 approved these changes Aug 18, 2018

View reviewed changes

LnL7 merged commit 3c5c504 into NixOS:master Aug 18, 2018

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oracle-instantclient: fixes dylib rpath on macos #45157

oracle-instantclient: fixes dylib rpath on macos #45157

mikemckibben commented Aug 16, 2018

LnL7 Aug 16, 2018

mikemckibben Aug 16, 2018 •

edited

flokli Aug 17, 2018

LnL7 Aug 17, 2018

flokli left a comment

flokli Aug 17, 2018

flokli Aug 17, 2018

LnL7 Aug 17, 2018

flokli Aug 18, 2018

flokli Aug 17, 2018

mikemckibben Aug 17, 2018

mikemckibben commented Aug 18, 2018

flokli Aug 18, 2018

LnL7 Aug 18, 2018

flokli Aug 18, 2018

flokli Aug 18, 2018

flokli commented Aug 18, 2018

mikemckibben commented Aug 18, 2018

flokli commented Aug 18, 2018

LnL7 left a comment

oracle-instantclient: fixes dylib rpath on macos #45157

oracle-instantclient: fixes dylib rpath on macos #45157

Conversation

mikemckibben commented Aug 16, 2018

Motivation for this change

Things done

Choose a reason for hiding this comment

mikemckibben Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemckibben commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented Aug 18, 2018

mikemckibben commented Aug 18, 2018

flokli commented Aug 18, 2018

LnL7 left a comment

Choose a reason for hiding this comment

mikemckibben Aug 16, 2018 •

edited