fflas-ffpack: Add darwin support #45013
Conversation
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
3 times, most recently
from
2fc87cc
to
d07a7e1
Compare
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
2 times, most recently
from
c5671e3
to
c9164ad
Compare
|
This actually isn't working: it stills fails tests, but they weren't being run before because |
|
The problem seems to be with Is there a known issue with |
|
Not that I know of, what is the error? Common practice would be to leave the PR open but add a [WIP] prefix to the title. |
|
|
||
| # doInstallCheck needed for darwin | ||
| doInstallCheck = true; | ||
| installCheckTarget = "check"; |
There was a problem hiding this comment.
Same as the other tickets, please expand and report upstream. Does the same issue actually affect all 3 packages? I could understand linbox and givaro since they are from the same people, but this seems weird.
There was a problem hiding this comment.
No this was my mistake, I saw a segfault and assumed it was the same problem, but it was actually openblas. But then it wasn't running the tests so it was looking like it was working.
| nativeBuildInputs = [ | ||
| autoreconfHook | ||
| pkgconfig | ||
| ] ++ stdenv.lib.optionals doCheck checkInputs; | ||
| ] ++ stdenv.lib.optionals doInstallCheck checkInputs; | ||
|
|
There was a problem hiding this comment.
Not sure what github is telling me here. Are you adding whitespace at the start of the line / changing spaces to tabs or something?
|
There is this, but they don't say anything about darwin. Might be relevant, might not be. |
|
Still I'd avoid switching to atlas if possible. |
|
So I should just use regular blas instead of openblas over atlas? |
jbaum98
changed the title
|
I don't think there is such a thing as regular blas (might be wrong). blas just defines an interface and openblas and atlas are the two main implementations. It should stay with openblas if possible. openblas seems generally preferred, has more features and is better supported on nix (more optimizations). What error are you getting? |
|
There is a package called blas that I used that works. It's segfaulting on
a test, and according to my debugger it's happening in a call to an
openblas method. Will post more details soon.
…On Tue, Aug 14, 2018, 5:17 PM Timo Kaufmann ***@***.***> wrote:
I don't think there is such a thing as regular blas (might be wrong). blas
just defines an interface and openblas and atlas are the two main
implementations. It should stay with openblas if possible. openblas seems
generally preferred, has more features and is better supported on nix (more
optimizations).
What error are you getting?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#45013 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFCgl8JhstGH4P13vKO7ivyC1p1nc1cyks5uQz5qgaJpZM4V71hc>
.
|
|
Oh, didn't know about that one. Probably that is the one that started the standard. I always thought the first one was proprietary for some reason. Apparently this is the upstream issue that caused debian to temporarily switch to atlas. You could try if downgrading openblas to 0.3.1 fixes the issue. Edit: actually this is upstream |
|
From your history it looks like you're trying to get sage to build on darwin, is that right? |
|
Yup, you figured me out.
…On Tue, Aug 14, 2018, 6:05 PM Timo Kaufmann ***@***.***> wrote:
From your history it looks like you're trying to get sage to build on
darwin, is that right?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#45013 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFCgl4JCsLTaXCO394tTWNM1ORv8rXJ3ks5uQ0msgaJpZM4V71hc>
.
|
|
Awesome! I'm the sage maintainer. If you need help or review, ping me. |
|
Okay here's the situation. There are three (maybe four) I looked at
This may or may not be a bug, because the README says it is, but the issue it links to is closed, so I'm confused. So, then, we settle for |
|
Did you try to build with an older version of |
|
I tried with a newer |
|
Maybe |
Good question, I don't think you can. You could just make the value dependent on
Again, its a bit confusing 😀 |
|
So if you want to double check what the reason for the failures is, you could write an |
|
If you'd like to keep it the way it is now, please add a comment to the package file somewhere explaining that openblas with osx causes issues, linking to the upstream issue and mentioning that After that if you're happy with it and tests pass on both platforms, I'd say its good to go. |
|
For the record, I did ask on irc if there is a better way for the lib name, but apparently there is not. |
pkgs/top-level/all-packages.nix
Outdated
| @@ -9169,7 +9169,16 @@ with pkgs; | |||
|
|
|||
| ffcast = callPackage ../tools/X11/ffcast { }; | |||
|
|
|||
| fflas-ffpack = callPackage ../development/libraries/fflas-ffpack {}; | |||
| fflas-ffpack = callPackage ../development/libraries/fflas-ffpack { | |||
| blas = if stdenv.isDarwin then { | |||
There was a problem hiding this comment.
I think this is a bit confusing since blas is supposed to be a package. Adding a linkName as a passthru attribute to the packages and using blas.linkName or "blas" instead seems much nicer.
There was a problem hiding this comment.
Oh, a passthru attribute is exactly what we want. Didn't know about that!
There was a problem hiding this comment.
As always, please add a comment about the purpose of the linkName attribute. Otherwise these things tend to stay around forever even after nobody uses them anymore because nobody knows what it was good for.
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
2 times, most recently
from
35c2860
to
fbf012f
Compare
pkgs/top-level/all-packages.nix
Outdated
| # see https://github.com/NixOS/nixpkgs/pull/45013. | ||
| # We need to pass a different name to --with-blas-libs | ||
| # based on which one we use. | ||
| blas = let addLinkName = { pkg, linkName }: |
There was a problem hiding this comment.
You can just add it directly to the openblas and blas packages. passthru won't cause a rebuild.
There was a problem hiding this comment.
Ohh you're saying I should modify them to add the passthru attribute in all-packages.nix.
| configureFlags = [ | ||
| "--with-blas-libs=-lopenblas" | ||
| "--with-lapack-libs=-lopenblas" | ||
| "--with-blas-libs=-l${blas.passthru.linkName}" |
There was a problem hiding this comment.
Not 100% sure on this but you should be able to just do blas.linkName.
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
2 times, most recently
from
f9a15b2
to
55c4572
Compare
pkgs/top-level/all-packages.nix
Outdated
| # elsewhere. | ||
| # See see https://github.com/NixOS/nixpkgs/pull/45013. | ||
| blas = (callPackage ../development/libraries/science/math/blas { }) | ||
| .overrideAttrs(oldAttrs: { passthru.linkName = "blas"; }); |
There was a problem hiding this comment.
I think I didn't explain this properly, there's no need for overrides in all-packages.nix. Add the overrides eg. here, similar to meta this won't cause a rebuild when changed. https://github.com/NixOS/nixpkgs/blob/55c4572dd1fae7043b1697116007d0f35edbcb64/pkgs/development/libraries/science/math/blas/default.nix#L55
There was a problem hiding this comment.
So you can directly add this to pkgs/development/libraries/sciene/math/{open,}blas/default.nix.
There was a problem hiding this comment.
Yeah sorry, makes sense now.
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
3 times, most recently
from
57acd20
to
5d958cc
Compare
jbaum98
force-pushed
the
fflas-ffpack_darwin
branch
from
5d958cc
to
8374400
Compare
| "--with-blas-libs=-lopenblas" | ||
| "--with-lapack-libs=-lopenblas" | ||
| "--with-blas-libs=-l${blas.linkName}" | ||
| "--with-lapack-libs=-l${blas.linkName}" | ||
| ] ++ stdenv.lib.optionals (!optimize) [ | ||
| # disable SIMD instructions (which are enabled *when available* by default) | ||
| "--disable-sse" |
There was a problem hiding this comment.
Never mind, I must've mis-read the github diff. I thought you removed a bunch of --disable-X.
|
As soon as you think this is ready, please remove the wip tag. |
jbaum98
changed the title
|
@GrahamcOfBorg build fflas-ffpack |
|
Success on x86_64-linux (full log) Attempted: fflas-ffpack Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: fflas-ffpack Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: fflas-ffpack Partial log (click to expand)
|
|
Thank you for your persistence! |
|
I've reported this issue at sage upstream, maybe somebody there can help: https://trac.sagemath.org/ticket/26130 |
Motivation for this change
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)