-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add support for BLAS and LAPACK dependencies (continued) #14773
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
base: master
Are you sure you want to change the base?
Conversation
c2a41f4
to
08bec81
Compare
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.
Didn't take too close of a look, but just a few modernization things and type annotation things.
Thanks for your comments. To be honest, I haven't gotten to cleaning up / modernizing the code yet — so far my main focus was getting it to work correctly :-). But pushed the suggested fixes now. |
de104e9
to
13b93a8
Compare
bfa9c78
to
c4f4671
Compare
Okay, I think this is ready for review. FWIU the "linux" test failures are due to the test images not being updated — and the Ubuntu image failure is flakiness of the zig version check. |
keyword, with possible values: | ||
|
||
- `'interface: lp64'` (default) or `'interface: ilp64'`: to select the default | ||
integer size |
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 understand why you did this, but it's conceptually not really correct. An "interface" is not really a module. Could we have some other way of expressing this information that is more explicit about what it does?
This a rebase / continuation of #10921.
So far just gotten it to work again, will look through the previous review next.