-
Notifications
You must be signed in to change notification settings - Fork 51
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
Update Github actions #298
Conversation
11bf5ba
to
f7ac3a2
Compare
f7ac3a2
to
b888d6d
Compare
12e97ce
to
f5a8756
Compare
…uild. Also fix printing message for BLAS discovery
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.
Everything looks good. Couple of minor suggestions but these are optional.
else() | ||
message(STATUS "Using BLAS/LAPACK located by CMake") | ||
endif() | ||
|
||
list(APPEND CMAKE_PREFIX_PATH ${OPENBLAS_DIR}) |
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 line (new 175) might be better in the if clause above, although it does nothing if empty.
cmake/ExternalBLASLAPACK.cmake
Outdated
set(BLA_VENDOR "OpenBLAS") | ||
message(STATUS "Using BLAS/LAPACK from OpenBLAS") | ||
else() | ||
message(STATUS "Using BLAS/LAPACK located by CMake") |
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 looks ok — for this to trigger we need to not have defined the root paths (ARMPL|AOCL|MKL|OPENBLAS)(_DIR|ROOT|_ROOT)
. Then it will just call the default find lapack which will look for BLA_VENDOR
.
The one question I had — do we want to treat the case specially where BLA_VENDOR
is already set at the beginning of the script? Right now BLA_VENDOR
is ignored or overwritten without warning/error, if even one of the root paths above is set. (e.g. if ARMPL_DIR
is set in the environment and I set BLA_VENDOR=OpenBLAS
I will not get 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.
I think this probably worth handling, but I'll do in a follow up PR (#299), as doing it neatly is going to involve a modest amount of refactoring in this file (in particular handling the AOCL section) and then a significant amount of testing for the edges.
@@ -84,7 +88,7 @@ jobs: | |||
export FC=gfortran-12 | |||
fi | |||
if [[ "${{ matrix.math-libs }}" == 'openblas' ]]; then | |||
export OPENBLAS_DIR=/usr/local/opt/openblas | |||
export OPENBLAS_DIR=/opt/homebrew/opt/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.
Ok, but might suggest export OPENBLAS_DIR=$(brew --prefix openblas)
for consistency with llvm on line 78.
@@ -53,6 +53,10 @@ jobs: | |||
run: | | |||
brew install pkg-config | |||
|
|||
- uses: julia-actions/setup-julia@v2 | |||
with: | |||
version: '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.
Is there a recommendation from julia whether to also pin subversion (1.10 / 1.11)? I know these can have some breaking changes, although our usage might be light enough not to worry.
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'd rather let it run along at 1 until there's a bug to prevent us otherwise. That way we'll naturally incorporate any updates. Our usage is very light anyway, so should be fine I think.
In some macos github action builds:
Looks like the github action runners
macos-latest-xl
might've been moved tomacos-latest-large
. Should probably add an m1 build to the CI.