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.

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

Add GNU threading and Fortran libraries to MKL package #75526

Merged
merged 3 commits into from Jan 4, 2020

Conversation

@robertodr
Copy link
Contributor

@robertodr robertodr commented Dec 11, 2019

Motivation for this change

I have added the libraries needed to link using the GNU Fortran compiler and GNU threading libraries. These are the ones suggested by the MKL link line advisor.
Moreover, I also unpack the static version of all the libraries included.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @bhipple

Copy link
Contributor

@bhipple bhipple left a comment

The mkl package is getting quite big at this point. It might make sense to consider making a split-output derivation with a dev package that has the static libs, headers, and pcfiles, but it looks like your PR only takes it from 1.4G to 1.6G, so I could see that being done in a follow-up.

We install all the libraries on Darwin, so doing so on Linux as well seems like a good change to me.

CC @markuskowa if you have any opinions.

@robertodr
Copy link
Contributor Author

@robertodr robertodr commented Dec 12, 2019

Copy link
Contributor

@c0bw3b c0bw3b left a comment

The commit message should mention mkl

@robertodr robertodr force-pushed the robertodr:more-mkl branch from 6d2741e to 689a5e6 Dec 12, 2019
@robertodr robertodr changed the title Add GNU threading and Fortran libraries Add GNU threading and Fortran libraries to MKL package Dec 12, 2019
@robertodr
Copy link
Contributor Author

@robertodr robertodr commented Dec 12, 2019

@c0bw3b I have amended the commit message.

@ofborg ofborg bot requested a review from bhipple Dec 12, 2019
@markuskowa
Copy link
Member

@markuskowa markuskowa commented Dec 13, 2019

I would not package the static libraries by default. Could we make them optional (e.g. enableStatic)?

@bhipple It would make sense at some point in the future to split the components MKL into separate derivations (for example mkl-base, mkl-scalapack, mkl-fftw, etc.) to make it smaller. One rarely needs all of the components at once.

@veprbl
Copy link
Member

@veprbl veprbl commented Dec 14, 2019

@c0bw3b I have amended the commit message.

See https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md
It should be something like "mkl: add GNU threading and Fortran libraries"

@veprbl
Copy link
Member

@veprbl veprbl commented Dec 14, 2019

I would not package the static libraries by default. Could we make them optional (e.g. enableStatic)?

And that would need to be disabled by default and enabled in an override in the pkgsStatic overlay.

@robertodr robertodr force-pushed the robertodr:more-mkl branch from 689a5e6 to 743e08f Dec 15, 2019
@robertodr
Copy link
Contributor Author

@robertodr robertodr commented Dec 15, 2019

The comments should have now been addressed.

robertodr added 3 commits Dec 11, 2019
Signed-off-by: Roberto Di Remigio <roberto.diremigio@gmail.com>
@robertodr robertodr force-pushed the robertodr:more-mkl branch from 1a392ea to 153597a Dec 19, 2019
@robertodr
Copy link
Contributor Author

@robertodr robertodr commented Dec 19, 2019

@bhipple comment addressed.

@ofborg ofborg bot requested a review from bhipple Dec 19, 2019
@markuskowa markuskowa merged commit 1c55261 into NixOS:master Jan 4, 2020
15 checks passed
15 checks passed
mkl on aarch64-linux No attempt
Details
mkl on x86_64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@robertodr robertodr deleted the robertodr:more-mkl branch Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.