-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
fflas-ffpack: Add darwin support #45013
Conversation
2fc87cc
to
d07a7e1
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, a passthru attribute is exactly what we want. Didn't know about that!
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure on this but you should be able to just do blas.linkName
.
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.
Indeed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you can directly add this to pkgs/development/libraries/sciene/math/{open,}blas/default.nix
.
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.
Yeah sorry, makes sense now.
57acd20
to
5d958cc
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove those?
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.
What do you mean?
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.
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. |
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 now.
@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
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)