-
Notifications
You must be signed in to change notification settings - Fork 218
fix $BLAS_LIB_MT for OpenBLAS, ensure -lpthread is included #3584
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
fix $BLAS_LIB_MT for OpenBLAS, ensure -lpthread is included #3584
Conversation
This comment has been minimized.
This comment has been minimized.
| """ | ||
| BLAS_MODULE_NAME = ['OpenBLAS'] | ||
| BLAS_LIB = ['openblas'] | ||
| BLAS_LIB_MT = ['openblas'] |
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.
This doesn't actually add -lpthread. And the libopenblas.{a,so} we build actually depends on -lpthread even for the non-MT version since there is only one lib and it's built with OpenMP enabled.
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.
This doesn't actually add -lpthread.
yes it does. did you actually check it? it’s because BLAS_LIB_MT was not defined that -lpthread was not added to LIBBLAS_MT. defining it here fixes it (for me at least).
And the libopenblas.{a,so} we build actually depends on -lpthread even for the non-MT version since there is only one lib and it’s built with OpenMP enabled.
that’s true for the OpenBLAS versions in the official EB repo but people might still want to build it without OpenMP (for example on macOS), and it does not hurt to add it, right?
anyway I don't mind closing this if you think it's not worth it.
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, I didn't check the actual output (got sidetracked as usual), was making the comment so you would correct me if needed :-)
akesandgren
left a comment
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.
LGTM
|
Going in, thanks @smoors! |
(created using
eb --new-pr)fixes #3579