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

CMake: Use LAPACK imported target #542

Merged
merged 26 commits into from
Sep 16, 2024
Merged

CMake: Use LAPACK imported target #542

merged 26 commits into from
Sep 16, 2024

Conversation

gardner48
Copy link
Member

  1. Use LAPACK::LAPACK rather than LAPACK_LIBRARIES
  2. Simplify try_compile test
  3. Don't overwrite LAPACK_WORKS (test every configuration pass)
  4. Use CHECK_START, CHECK_PASSED, and CHECK_FAILED messages in compile test
  5. Replace manual include guard with include_guard(GLOBAL)
  6. Update SundialsTPL.cmake.template with above changes

@gardner48
Copy link
Member Author

This may help address #350.

@gardner48
Copy link
Member Author

gardner48 commented Jul 15, 2024

@balos1 If you're good with the updates to SundialsTPL.cmake.template and LAPACK_WORKS, I'll see about propagating these changes to the other TPL files (in follow on PRs).

With LAPACK_WORKS this is now only used to bypass the compile test and not to indicate if the compile test passed (since failing is a fatal error). So _WORKS might not be the best name, maybe LAPACK_TEST or SUNDIALS_LAPACK_TEST or SUN_LAPACK_TEST (for better namespacing).

@balos1
Copy link
Member

balos1 commented Jul 16, 2024

This looks great!

balos1
balos1 previously approved these changes Jul 16, 2024
@gardner48
Copy link
Member Author

Looks like there is a problem with the compile test that might be specific to CMake 3.18, I'm looking into it.

Copy link
Collaborator

@Steven-Roberts Steven-Roberts left a comment

Choose a reason for hiding this comment

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

This fixes the missing linking flags issue I had. When I have two lapack versions (system and spack) it finds both in cmake and only uses the system version for compilation. It's unclear how I would use the spack version for compilation.

cmake/tpl/SundialsLapack.cmake Outdated Show resolved Hide resolved
cmake/tpl/SundialsLapack.cmake Outdated Show resolved Hide resolved
src/sunlinsol/lapackdense/CMakeLists.txt Outdated Show resolved Hide resolved
src/sunlinsol/lapackband/CMakeLists.txt Outdated Show resolved Hide resolved
@gardner48
Copy link
Member Author

It's unclear how I would use the spack version for compilation.

Unfortunately FindLAPACK does not seem to support the usual _ROOT or _DIR options to help the set search path so you'll probably have to set LAPACK_LIBRARIES and LAPACK_LINKER_FLAGS manually. I've not tried it but setting BLA_VENDOR might also work e.g., -DBLA_VENDOR="OpenBLAS"

@Steven-Roberts
Copy link
Collaborator

Got it working with David's help. For posterity, -DBLA_VENDOR="OpenBLAS" did work as well as manually setting LAPACK_LIBRARIES and LAPACK_LINKER_FLAGS. Just had to make sure the compiler version was consistent with the OpenBLAS build from spack.

@balos1 balos1 added this to the SUNDIALS Next milestone Sep 10, 2024
@Steven-Roberts
Copy link
Collaborator

@gardner48 The latest version still works for me, both with a system LAPACK and spack one.

@gardner48 gardner48 merged commit 83a4c3c into develop Sep 16, 2024
24 of 25 checks passed
@gardner48 gardner48 deleted the cmake/lapack-target branch September 16, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants