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

Jump to bottom

giac: remove liblapackWithAtlas dependency #40430

Merged
merged 1 commit into from May 13, 2018
Merged

giac: remove liblapackWithAtlas dependency #40430

merged 1 commit into from May 13, 2018

Conversation

timokau
Copy link
Member

@timokau timokau commented May 13, 2018 •
edited

Motivation for this change

atlas takes ages to build, is optimized for the build machine and doesn't currently build on aarch.

I tried to figure out why the liblapackWithAtlas dependency is needed for giac and I couldn't find a reason. gentoo (at least in its sage overlay, I couldn't find another package) and debian don't have a dependency on giac. I couldn't find any mention of it on giacs homepage either.

Nothing besides sage depends on giac, so I couldn't use that as a test either. So unless the maintainer @symphorien remembers the original reason to include that dependency, I propose we remove it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg
Copy link

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: giac

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: giac

Partial log (click to expand)

strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59/lib  /nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59/bin
patching script interpreter paths in /nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59
/nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59/bin/pgiac: interpreter directive changed from "/usr/bin/perl" to "/nix/store/cxdmh98g0lvl1dyq304c1lq7f90dh01f-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59...
shrinking RPATHs of ELF executables and libraries in /nix/store/jmfzjb9jq4blf3vsrhmr4ck850i0s816-giac-1.4.9-59-doc
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/jmfzjb9jq4blf3vsrhmr4ck850i0s816-giac-1.4.9-59-doc
checking for references to /build in /nix/store/jmfzjb9jq4blf3vsrhmr4ck850i0s816-giac-1.4.9-59-doc...
/nix/store/ybxvnbnskhn3h9p8f14brd33y3125gkh-giac-1.4.9-59

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: giac

Partial log (click to expand)

strip is /nix/store/gp7fylxwn18b7pl2c18ks89hsiaxyfvf-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59/lib  /nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59/bin
patching script interpreter paths in /nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59
/nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59/bin/pgiac: interpreter directive changed from "/usr/bin/perl" to "/nix/store/13fsgz2l89qflb0ycc5lyxgigpral6g6-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59...
shrinking RPATHs of ELF executables and libraries in /nix/store/cykhvwa16lasn30sqmdngckxagzhqip3-giac-1.4.9-59-doc
strip is /nix/store/gp7fylxwn18b7pl2c18ks89hsiaxyfvf-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/cykhvwa16lasn30sqmdngckxagzhqip3-giac-1.4.9-59-doc
checking for references to /build in /nix/store/cykhvwa16lasn30sqmdngckxagzhqip3-giac-1.4.9-59-doc...
/nix/store/wn2lkbl6b8j42gqxw43q9bbhhfd8py1h-giac-1.4.9-59

@symphorien
Copy link
Member

The "sources of truth" I used for dependencies are this page https://www-fourier.ujf-grenoble.fr/~parisse/giac_compile.html which tells nothing about either lapack or atlas, and the INSTALL file in the tarball which says:

2/ Get and install GMP, MPFR, FLTK 1.3.2, readline, optionnaly GSL,
lapack, atlas if you want to do numerical stuff,
PARI 2.3 or above for advanced arithmetic,
NTL for fast polynomial (see configuration compatible with giac below)

Hence my use of lapackWithAtlas

Honestly, I don't even know what lapack and atlas really are beyond "number crunching libraries". Besides, xcas still seems to work with this change. So if you have reasons to think lapack without atlas is better, then go ahead :)

@timokau
Copy link
Member Author

timokau commented May 13, 2018

It (neither liblapack nor atlas) doesn't end up in the result closure either, so I think its safe to remove.

@xeji
Copy link
Contributor

xeji commented May 13, 2018

@GrahamcOfBorg build giac-with-xcas

@GrahamcOfBorg
Copy link

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: giac-with-xcas

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: giac-with-xcas

Partial log (click to expand)

shrinking /nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59/bin/cas_help
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59/lib  /nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59/bin
patching script interpreter paths in /nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59
/nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59/bin/pgiac: interpreter directive changed from "/usr/bin/perl" to "/nix/store/cxdmh98g0lvl1dyq304c1lq7f90dh01f-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/wcay68cjj3cirxnhlif1p3w5yjpa5gcx-giac-with-xcas-1.4.9-59...
shrinking RPATHs of ELF executables and libraries in /nix/store/bfs69ypbk20cj09swwa64aqk12iydp5s-giac-with-xcas-1.4.9-59-doc
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/bfs69ypbk20cj09swwa64aqk12iydp5s-giac-with-xcas-1.4.9-59-doc
checking for references to /build in /nix/store/bfs69ypbk20cj09swwa64aqk12iydp5s-giac-with-xcas-1.4.9-59-doc...

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: giac-with-xcas

Partial log (click to expand)

strip is /nix/store/gp7fylxwn18b7pl2c18ks89hsiaxyfvf-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59/lib  /nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59/bin
patching script interpreter paths in /nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59
/nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59/bin/pgiac: interpreter directive changed from "/usr/bin/perl" to "/nix/store/13fsgz2l89qflb0ycc5lyxgigpral6g6-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59...
shrinking RPATHs of ELF executables and libraries in /nix/store/bxa1wmz6nyr8sar5kzzcymd79c7r0ijb-giac-with-xcas-1.4.9-59-doc
strip is /nix/store/gp7fylxwn18b7pl2c18ks89hsiaxyfvf-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/bxa1wmz6nyr8sar5kzzcymd79c7r0ijb-giac-with-xcas-1.4.9-59-doc
checking for references to /build in /nix/store/bxa1wmz6nyr8sar5kzzcymd79c7r0ijb-giac-with-xcas-1.4.9-59-doc...
/nix/store/pz8p450az687i1vfn8c58f3wgwwil51p-giac-with-xcas-1.4.9-59

@xeji xeji merged commit 5550015 into NixOS:master May 13, 2018
@symphorien
Copy link
Member

it is strange, lapack seems to be used in the code. https://salsa.debian.org/science-team/giac/blob/master/src/vecteur.cc#L13574
Not that it makes any difference, we could remove the --enable-lapack then.

@xeji
Copy link
Contributor

xeji commented May 13, 2018

Or, if lapack-related functionality seems important, use lapack without atlas if possible (I don't know).

@timokau
Copy link
Member Author

timokau commented May 13, 2018

Oh, I missed the --enable-lapack option. Weird that it didn't complain. That should be removed if we no longer use lapack -- @symphorien should I open another PR to remove that option?

We could also make it optional, but theres not much point in doing that if nobody uses it or even knows what it does.

@timokau timokau deleted the giac-liblapack branch May 13, 2018 18:18
@xeji
Copy link
Contributor

xeji commented May 13, 2018

Keep it simple - I don't think an option is necessary. Chances are nobody will ever complain.

@symphorien
Copy link
Member

xeji is right, building giac with or without lapack does not make any difference given the number of potential users. So let's leave it as is.
Just for future reference: it happens that lapack is a static library. So the resulting derivation does not depend at runtime on liblapack. With the current 18.03 build of giac, libgiac.so defines a zgees_ symbol (it has been incorporated by static linking) and with this pr, the symbol is not there anymore, which proves that providing liblapack does technically make a difference.
But as said, probably nobody cares.

@timokau timokau mentioned this pull request May 13, 2018
Merged
8 tasks
@timokau
Copy link
Member Author

timokau commented May 13, 2018

I don't like just leaving that flag lying around, so for completions sake: #40458

timokau added a commit to timokau/nixpkgs that referenced this pull request May 13, 2018
@timokau
giac: re-add lapack without atlas (NixOS#40430)
6f655f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@timokau @GrahamcOfBorg @symphorien @xeji