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.

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

Distinguish openblas variants on windows; remove blis workarounds #130

Merged
merged 24 commits into from
Jan 28, 2025

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Dec 19, 2024

This combines two PRs, where one doesn't work (or make much sense) without the other.

Mainly I wanted to do #129, which is a follow-up to #126, which however runs into a bunch of conda-build bugs (conda/conda-build#5571, conda/conda-build#5572).

The other (#115) is for removing a very long-standing workaround for an unspecified conda-build bug that goes back forever (f9b54a0), but which doesn't seem to trigger anymore with 24.11.

The hope is that #129 + #115 passes, where #129 alone is currently stuck

Closes #129
Closes #115

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Dec 19, 2024

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13009183272. Examine the logs at this URL for more detail.

@h-vetinari h-vetinari force-pushed the bug_hunt branch 2 times, most recently from 49259c1 to 4075fe2 Compare January 8, 2025 14:40
@jaimergp
Copy link
Member

@h-vetinari, as per my findings in conda/conda-build#5571, ff0294e (#130) should work.

@h-vetinari
Copy link
Member Author

@h-vetinari, as per my findings in conda/conda-build#5571, ff0294e (#130) should work.

Amazing work! 🤩 Thank you so much! 🙏

@h-vetinari h-vetinari force-pushed the bug_hunt branch 2 times, most recently from 39d8013 to 0a93b4f Compare January 25, 2025 15:02
@h-vetinari
Copy link
Member Author

@conda-forge/blas, thanks to the magic touch of @jaimergp this is now ready, PTAL! :)

It does a few long-planned things:

  • enable an openmp variant for openblas on windows (had been descoped from Rebuild for flang 19 #126)
  • un-special-case blis after it's not necessary anymore (workarounds for this go back ~forever: f9b54a0)
  • turn blas into a pure compatibility wrapper around blas-devel (there's an old comment above its name that it's "for compatibility", but this wasn't really the case).
  • un-break the build

@h-vetinari
Copy link
Member Author

@isuruf, I'd appreciate your review on this change

recipe/meta.yaml Outdated
Comment on lines 272 to 276
- test -f $PREFIX/lib/liblapacke.so # [linux]
- test -f $PREFIX/lib/liblapacke.so.{{ version_major }} # [linux]
- test -f $PREFIX/lib/liblapacke.dylib # [osx]
- test -f $PREFIX/lib/liblapacke.{{ version_major }}.dylib # [osx]
- if not exist %LIBRARY_BIN%/liblapacke.dll exit 1 # [win]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I've kept these, but in blas-devel (which blasnow wraps)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, but it's better to have the tests here in addition to the ones in blas-devel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll reinstate, but I don't understand... If the files are actually contained in blas-devel, and blas is a pure wrapper-package with no content, tests on the wrapper are redundant with the tests for the package being wrapped.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment

recipe/meta.yaml Outdated
Comment on lines 272 to 276
- test -f $PREFIX/lib/liblapacke.so # [linux]
- test -f $PREFIX/lib/liblapacke.so.{{ version_major }} # [linux]
- test -f $PREFIX/lib/liblapacke.dylib # [osx]
- test -f $PREFIX/lib/liblapacke.{{ version_major }}.dylib # [osx]
- if not exist %LIBRARY_BIN%/liblapacke.dll exit 1 # [win]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I've kept these, but in blas-devel (which blasnow wraps)

recipe/meta.yaml Show resolved Hide resolved
@h-vetinari h-vetinari merged commit 7784e3d into conda-forge:main Jan 28, 2025
15 of 17 checks passed
@h-vetinari h-vetinari deleted the bug_hunt branch January 28, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants