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.
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
mkl: fix install_name on Darwin #57947
Conversation
f9b4a6a
to
8f7dae3
Compare
There are some
This can be fixed with |
036dbe3
to
1a62a1d
Compare
@bhipple can you please review this PR? |
6266597
to
382d2cf
Compare
Thanks for the fix @veprbl. I've squashed it and force-pushed it. |
After rebasing on master (to update to 2019.3.199), the post-installation phase fails:
I'll look into this. |
Obviously we can't relink the library since Intel only provides binary. Is there a way to fix this? |
The |
In fact the Darwin and Linux derivations do not contain the same libraries:
Note that the |
I've just realised that the MKL contains its own |
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/2 |
@GrahamcOfBorg build mkl |
Fetching the source fails, seems like that's not a stable url.
|
It's working now. Sounds like a hiccup with Intel's website. |
Apologies for taking a while to look at this @smaret!
I'm not sure; I don't have access to a Darwin machine to test. But if you don't need it then certainly we should drop it; this package is huge, which is why we have the fixed output derivation to avoid rebuilds whenever possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can leave this as a fixed output derivation?
done | ||
install_name_tool -change @rpath/libiomp5.dylib $out/lib/libiomp5.dylib $out/lib/libmkl_intel_thread.dylib | ||
install_name_tool -change @rpath/libtbb.dylib $out/lib/libtbb.dylib $out/lib/libmkl_tbb_thread.dylib | ||
install_name_tool -change @rpath/libtbbmalloc.dylib $out/lib/libtbbmalloc.dylib $out/lib/libtbbmalloc_proxy.dylib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we can modify binaries with the ISSL license agreement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ISSL allows to "redistribute the software without modification". I doubt that changing the install_name of the libraries can be considered as a modification of the software. Since we're not installing the libraries in a standard location, we can't redistribute them (at least on Darwin) if we don't change the install name accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that would be a problem we would be pretty much screwed, because we use patchelf quite heavily in most derivations, including proprietary software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a lawyer, but I also would consider the ELF header as part of the actual software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a precedent for build recipes that perform modification of mkl: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=intel-parallel-studio-xe#n523
You don't need it if you only use the MKL as a BLAS/LAPACK provider. But shouldn't we include all libraries for users that need other MKL functionalities? |
@veprbl suggested a way to do this (see his comment above). Personally I'm not convinced it's worth the effort. If I understand correctly, the fixed output derivation avoids rebuilding the package if some of the dependencies (rpmextract or undmg) changes. But my impression is that the MKL itself is updated much more often that its dependencies. Since you need to rebuild the package at each MKL update anyway, I'm not sure that the fixed output makes much of a difference. |
The reverse dependencies that are to change are not just
|
Closes NixOS#57697 Co-Authored-By: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
# fixed-output derivations. | ||
outputHashAlgo = "sha256"; | ||
outputHashMode = "recursive"; | ||
outputHash = "101krzh2mjbfx8kvxim2zphdvgg7iijhbf9xdz3ad3ncgybxbdvw"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping this fixed-output on Linux!
As mentioned by @veprbl, MKL is updated a couple times a year, while |
I agree with the summary by @bhipple. Let's merge this PR so that |
Motivation for this change
Fix MKL install names on Darwin (see #57697).
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)