haskell generic-builder: fix Darwin regression for lmdb #80191

simonchatts · 2020-02-15T17:43:57Z

Recent updates to the generic builder have caused haskellPackages.lmdb-simple to
fail to build on Darwin, since it cannot see the lmdb C dynamic library included
by its dependent haskellPackages.lmdb.

The C dynamic library has suffix .so not .dylib, so this fix allows for
that.

Closes #80190, but that issue may identify a preferable solution.

Motivation for this change

Things done

Recent updates to the generic builder have caused haskellPackages.lmdb-simple to fail to build on Darwin, since it cannot see the lmdb C dynamic library included by its dependent haskellPackages.lmdb. The C dynamic library has suffix `.so` not `.dylib`, so this fix allows for that. Closes NixOS#80190, but that issue may identify a preferable solution.

cdepillabout · 2020-02-16T01:32:44Z

cc @infinisil

cdepillabout · 2020-02-16T01:33:40Z

@GrahamcOfBorg build haskellPackages.lmdb-simple

FRidh · 2020-02-16T07:39:38Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -384,7 +384,7 @@ stdenv.mkDerivation ({
    done

    for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do
-      for lib in "$d/"*.dylib; do
+      for lib in "$d/"*.{dylib,so}; do


${stdenv.hostPlatform.extensions.sharedLibrary}

Thanks for the review @FRidh! Just to clarify, you're suggesting it should be this:

- for lib in "$d/"*.dylib; do + for lib in "$d/"*{${stdenv.hostPlatform.extensions.sharedLibrary},.so}; do

right? Or maybe a bit clearer like this:

- for lib in "$d/"*.dylib; do + for lib in "$d/"*${stdenv.hostPlatform.extensions.sharedLibrary} "$d/"*.so; do

I'm happy to do either of these, but to my eyes this makes the code more obfuscated. This whole section of code is intensely Darwin-specific, and the code has to know about both .so and .dylib, so there's no generality gained.

So can I just check that this is what you think would be an improvement? If so I'll happily re-test and push.

stdenv.hostPlatform.extensions.sharedLibrary evaluates to .so on Linux and .dylib on Darwin. I doubt you need to perform an action on both .dylib and .so in the same build.

Unfortunately, that is exactly the point of this patch. See #80190 for more details.

I share your intuition that there should be a better way, and would love someone to propose a superior alternative. Right now though, this is the only fix I have for the broken packages.

BTW the Darwin nix-store in front of me (no cross-compilation) has 44421 .dylibs and 778 .sos. Of those .sos, there are two distinct ones that otool -hv reports as DYLIB rather than BUNDLE:

the libstdbuf.so from coreutils

the liblmdb.so from lmdb at issue here

I have no idea whether the real root cause for this issue is something to do with them, but as it stands, there definitely are dylibs with suffix .so on Darwin.

Turns out the liblmdb .so suffix was specified manually in 84bd2f4 over two years ago.
I still do not understand why 8d4509e from a couple of weeks ago should trigger a breakage though.

The install_name needs to point to the full path to the final library location. Perhaps, the library got moved as a result of 8d4509e so the line

nixpkgs/pkgs/development/libraries/lmdb/default.nix

Line 26 in 82945f3

++ stdenv.lib.optional stdenv.isDarwin "LDFLAGS=-Wl,-install_name,$(out)/lib/liblmdb.so";

needs to be updated to reflect the move.

Good thought @veprbl - I've looked, and to the best of my understanding, this isn't the diagnosis. Both working and broken builds end up depending on the exact same LMDB C package (same SHA) where otool -L does indeed show the full path to the final library location.

veprbl · 2020-02-17T02:15:40Z

I think the issue might be with this line

nixpkgs/pkgs/development/haskell-modules/generic-builder.nix

Line 393 in 8d4509e

    
                 sed -i "s,dynamic-library-dirs: .*,dynamic-library-dirs: $dynamicLinksDir," "$f"

It used to add "links" directory and replace other directory or multiple directories. After the 8d4509e it only replaces all directories in the "dynamic-library-dirs:".

This can be demonstrated using an override:

diff --git a/pkgs/development/haskell-modules/configuration-common.nix b/pkgs/development/haskell-modules/configuration-common.nix
index 8f127e9490d..21481682143 100644
--- a/pkgs/development/haskell-modules/configuration-common.nix
+++ b/pkgs/development/haskell-modules/configuration-common.nix
@@ -15,6 +15,15 @@ with haskellLib;
 
 self: super: {
 
+lmdb-simple = super.lmdb-simple.overrideAttrs (old: {
+  setupCompilerEnvironmentPhase =
+    builtins.replaceStrings [
+      ''sed -i "s,dynamic-library-dirs: .*,dynamic-library-dirs: $dynamicLinksDir," "$f"''
+    ] [
+      ''sed -i "s,dynamic-library-dirs: ,dynamic-library-dirs: $dynamicLinksDir ," "$f"''
+    ] old.setupCompilerEnvironmentPhase;
+});
+
   # Arion's test suite needs a Nixpkgs, which is cumbersome to do from Nixpkgs
   # itself. For instance, pkgs.path has dirty sources and puts a huge .git in the
   # store. Testing is done upstream.

veprbl · 2020-02-17T02:35:16Z

Some background on the code in question: 7131e06 #22810

We might be able to remove this logic altogether if the there is no size limit on the recent macOS versions. If that's not the case, this PR is doing the right fix, as far as I understand now.

infinisil · 2020-02-17T03:17:53Z

@veprbl Oh yeah I think you got it. I think the underlying problem is that the dynamic-library-dirs key is essentially processed like this (wherever it's used)

for d in $dynamic-library-dirs; do
  for f in "$d"/*; do
    if is_valid_dynamic_library "$f"; do
      use_dynamic_library "$f";
    fi
  done
done

This means it will catch all files, no matter the extension, even though it should always be .so on Linux and .dylib on macOS. Before 8d4509e, the generic builder would transform

# From dependency A
dynamic-library-dirs: alibs1
                      alibs2
# From dependency B
dynamic-library-dirs: blibs1
                      blibs2

into

# From dependency A
dynamic-library-dirs: link-dir
                      alibs2
# From dependency B
dynamic-library-dirs: link-dir
                      blibs2

Where link-dir contains all .dylibs from alibs1 and blibs1. But after 8d4509e it transforms it into

# From dependency A
dynamic-library-dirs: link-dir
# From dependency B
dynamic-library-dirs: link-dir

Where now link-dir contains all .dylibs from alibs1, alibs2, blibs1 and blibs2.

The problem with this is that all .so files (and others) in alibs2 and blibs2 are now ignored! (in addition to the ones in alibs1 and blibs1 that were ignored before already).

With this said, I think the "correct" way to fix this is to determine what files to link with a is_valid_dynamic_library-like function, instead of relying on extensions. E.g. with otool -hv as mentioned in #80190 to determine whether something is a mach-o library.

However I'd be fine with the fix in this PR too

cdepillabout · 2020-02-17T03:50:43Z

@infinisil What do you want to do with this PR?

I think the "correct" way to fix this is to determine what files to link with a is_valid_dynamic_library-like function

Do you want to merge in this PR? Or fix it as described above in a future PR?

infinisil

Eh this should be fine. I don't think we'll encounter any other extensions being marketed as dylib's, and if so it's not really our fault anyways.

veprbl · 2020-02-17T08:12:34Z

Mass rebuild shouldn't be going to master...

infinisil · 2020-02-17T08:37:59Z

Reverted in cfc21ad

infinisil · 2020-02-17T08:45:46Z

Opened #80337 for having this in staging

kirelagin · 2020-04-03T21:52:19Z

Unfortunately, it looks like you need to link .as as well, because this new directory overrides everything else. So linking will fail if your haskell package will try to link with a library that only exists in the static version.

simonchatts requested review from basvandijk and cdepillabout as code owners February 15, 2020 17:43

ofborg bot added 6.topic: darwin Running or building packages on Darwin 6.topic: haskell 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 labels Feb 15, 2020

FRidh requested changes Feb 16, 2020

View reviewed changes

simonchatts mentioned this pull request Feb 16, 2020

haskell generic-builder: breakage from recent Darwin changes #80190

Closed

veprbl requested a review from infinisil February 16, 2020 22:50

veprbl approved these changes Feb 17, 2020

View reviewed changes

infinisil approved these changes Feb 17, 2020

View reviewed changes

infinisil merged commit b7bf4ce into NixOS:master Feb 17, 2020

infinisil mentioned this pull request Feb 17, 2020

haskell generic-builder: fix Darwin regression for lmdb #80337

Merged

10 tasks

simonchatts deleted the lmdb branch February 21, 2020 11:30

kirelagin mentioned this pull request Apr 3, 2020

Allow .so dynamic libraries on Darwin input-output-hk/haskell.nix#534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

haskell generic-builder: fix Darwin regression for lmdb #80191

haskell generic-builder: fix Darwin regression for lmdb #80191

simonchatts commented Feb 15, 2020

cdepillabout commented Feb 16, 2020

cdepillabout commented Feb 16, 2020

FRidh Feb 16, 2020

simonchatts Feb 16, 2020

FRidh Feb 16, 2020

simonchatts Feb 16, 2020

simonchatts Feb 16, 2020

simonchatts Feb 16, 2020

veprbl Feb 16, 2020

simonchatts Feb 16, 2020

veprbl commented Feb 17, 2020 •

edited

veprbl commented Feb 17, 2020

infinisil commented Feb 17, 2020

cdepillabout commented Feb 17, 2020

infinisil left a comment

veprbl commented Feb 17, 2020

infinisil commented Feb 17, 2020

infinisil commented Feb 17, 2020

kirelagin commented Apr 3, 2020

haskell generic-builder: fix Darwin regression for lmdb #80191

haskell generic-builder: fix Darwin regression for lmdb #80191

Conversation

simonchatts commented Feb 15, 2020

Motivation for this change

Things done

cdepillabout commented Feb 16, 2020

cdepillabout commented Feb 16, 2020

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

Choose a reason for hiding this comment

veprbl commented Feb 17, 2020 • edited

veprbl commented Feb 17, 2020

infinisil commented Feb 17, 2020

cdepillabout commented Feb 17, 2020

infinisil left a comment

Choose a reason for hiding this comment

veprbl commented Feb 17, 2020

infinisil commented Feb 17, 2020

infinisil commented Feb 17, 2020

kirelagin commented Apr 3, 2020

veprbl commented Feb 17, 2020 •

edited