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

qrupdate: refactor, and assert blas && lapack compatibility #98499

Merged
merged 3 commits into from Nov 5, 2020

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 22, 2020

Motivation for this change

Implement in Nix code stuff I learned at https://savannah.gnu.org/bugs/?57591 .

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 nixpkgs-review --run "nixpkgs-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.

@doronbehar
Copy link
Contributor Author

Makeconf is included into the Makefile so you should be able to set PREFIX, BLAS and LAPACK in makeFlags.

Yea that would have been nicer, but apparently the flags are not passed to ld:

extra flags after to /nix/store/p792j5f44l3f0xi7ai5jllwnxqwnka88-binutils-2.31.1/bin/ld:
  -rpath
  /nix/store/9df65igwjmf2wbw0gbrrgair6piqjgmi-glibc-2.31/lib
  -rpath
  /nix/store/z5g0y84g2iknwwgfhw9wslbbzgw1w22k-gfortran-9.3.0-lib/lib
/nix/store/p792j5f44l3f0xi7ai5jllwnxqwnka88-binutils-2.31.1/bin/ld: /build/ccOyKBv6.o: in function `ztest_':
tqr1up.f:(.text+0x18d): undefined reference to `zgerc_'
/nix/store/p792j5f44l3f0xi7ai5jllwnxqwnka88-binutils-2.31.1/bin/ld: /build/ccOyKBv6.o: in function `ctest_':

Though it does work if I use NIX_LDFLAGS. Thanks for the idea.

@jtojnar
Copy link
Contributor

jtojnar commented Sep 22, 2020

Also please do not ping people in commit messages, that causes them to be notified every time the commit is rebased. It is better to ping in PRs and just use names without @ sign in the commit message.

Use `pname` and `version`. Use my preferred indentation style. Use
makeFlagsArray in preBuild instead of overriding configurePhase, per:
https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/explicit-phases.md

Assert that lapack and blas are compatible regarding 64 bit indexing, do
it near evaluation of preBuild, per jtojnar's explanation:
NixOS#94892 (comment)

Use gpl3Plus, as gpl3 is unclear and deprecated.
@doronbehar
Copy link
Contributor Author

@jtojnar is everything good now?

@doronbehar doronbehar mentioned this pull request Oct 1, 2020
10 tasks
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Looks okay.

@doronbehar doronbehar merged commit 4c817f9 into NixOS:master Nov 5, 2020
@doronbehar doronbehar deleted the pkg/octave/qrupdate branch November 5, 2020 11:36
doronbehar added a commit to doronbehar/nixpkgs that referenced this pull request Dec 19, 2020
Apparently, undetected in NixOS#98499, trying to use the flags written makes
the build fail due to -O3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants