-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[openblas] Fix build errors on osx #36072
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
Conversation
dg0yt
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.
Unrelated to this PR: The port doesn't correctly acquire git (if needed at all) and sed.
| if(VCPKG_TARGET_IS_OSX) | ||
| list(APPEND COMMON_OPTIONS -DONLY_CBLAS=1) |
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.
Does this change also make it work for android?
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.
Added option separately for Android.
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.
Added option separately for Android.
Did you test it? Android is skipped in ci.baseline.
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.
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 were adding untested changes...
I prefer to start with logs from vcpkg CI before spending local build time.
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.
The conclusion is wrong. Only arm64 and arm are detected. For others, you are asked to define TARGET.
And I only ask that the android shall be tested before merged. If possible, by removing the ci.baseline entries.
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.
Possible targets are found here:
https://github.com/OpenMathLib/OpenBLAS/blob/d6a5174e9c50b9f68db96d7d7818f92cdfb4e7ec/TargetList.txt
The (Android x64) task is to make a reasonable choice in the portfile. And the user may even override in the triplet file.
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 queried the CPU model of the device, but it was not in the support list provided by xxx and therefore could not be tested.
16 AMD EPYC 7452 32-Core Processor
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.
@jimwang118 this PR works for me, thanks
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.
Thanks for testing.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Any updates on this? |
Passing on https://dev.azure.com/vcpkg/public/_build/results?buildId=99959&view=results. Added `openblas` to ci.baseline.txt by #29406, which has been fixed by #36072: ``` PASSING, REMOVE FROM FAIL LIST: openblas:arm-neon-android PASSING, REMOVE FROM FAIL LIST: openblas:arm64-android ``` Add the following ports to the ci.baseline.txt because they are direct/indirect downstream of `openblas`: ``` REGRESSION: blas:arm-neon-android failed with BUILD_FAILED. If expected, add blas:arm-neon-android=fail to /vcpkg/scripts/azure-pipelines/../ci.baseline.txt. REGRESSION: lapack-reference:arm64-android failed with BUILD_FAILED. If expected, add lapack-reference:arm64-android=fail to /vcpkg/scripts/azure-pipelines/../ci.baseline.txt. REGRESSION: clapack:arm64-android failed with BUILD_FAILED. If expected, add clapack:arm64-android=fail to /vcpkg/scripts/azure-pipelines/../ci.baseline.txt. REGRESSION: shogun:arm64-android failed with BUILD_FAILED. If expected, add shogun:arm64-android=fail to /vcpkg/scripts/azure-pipelines/../ci.baseline.txt. ``` - [x] Changes comply with the [maintainer guide](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md). - [ ] ~SHA512s are updated for each updated download.~ - [x] The "supports" clause reflects platforms that may be fixed by this new version. - [ ] ~Any fixed [CI baseline](https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt) entries are removed from that file.~ - [ ] ~Any patches that are no longer applied are deleted from the port's directory.~ - [x] The version database is fixed by rerunning `./vcpkg x-add-version --all` and committing the result. - [x] Only one version is added to each modified port's versions file. --------- Co-authored-by: Monica <[email protected]>
Fixes #36031
SHA512s are updated for each updated downloadThe "supports" clause reflects platforms that may be fixed by this new versionAny fixed CI baseline entries are removed from that file.Any patches that are no longer applied are deleted from the port's directory../vcpkg x-add-version --alland committing the result.Usage test pass with following triplets: