Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 17, 2020
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/development/haskell-modules/generic-builder.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${stdenv.hostPlatform.extensions.sharedLibrary}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ln -s "$lib" "$dynamicLinksDir"
done
done
Expand Down