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

Check BLAS/LAPACK libraries at configure time #795

Merged
merged 4 commits into from
Aug 24, 2024

Conversation

halleysfifthinc
Copy link
Contributor

@halleysfifthinc halleysfifthinc commented Jul 22, 2024

There are 4 ways simbody finds BLAS/LAPACK libraries:

  • Windows
    • Default (WINDOWS_USE_EXTERNAL_LIBS=OFF AND BUILD_USING_OTHER_LAPACK="") is a set of simbody vendored BLAS/LAPACK libraries
    • If WINDOWS_USE_EXTERNAL_LIBS=ON, then the system BLAS/LAPACK is used (whatever is found by CMake FindBlas and FindLapack)
  • Unix default (BUILD_USING_OTHER_LAPACK="") is the system BLAS/LAPACK (whatever is found by CMake FindBlas and FindLapack)
  • Set manually using BUILD_USING_OTHER_LAPACK

This could fail any number of ways (especially when using BUILD_USING_OTHER_LAPACK), so it
would be helpful to fail as early as possible if there is a problem. Currently, a build with
a poorly specified BUILD_USING_OTHER_LAPACK won't fail until linking the SimTKcommon
library, after building approximately 1/3 of Simbody.

This PR adds a try_compile check that try's to link to the chosen BLAS/LAPACK libraries.
If it fails, a helpful error message is printed by CMake:

CMake Error at CMakeLists.txt:537 (message):
  Failed to compile using the BLAS/LAPACK libraries -lnotablas.

          If BUILD_USING_OTHER_LAPACK was given; check that it was set correctly.

You can locally test this with either of these (valid) configurations (on Linux):
cmake -B build -S . --fresh
cmake -B build -S . --fresh -DBUILD_USING_OTHER_LAPACK="-lblas;-llapack"

This should fail:
cmake -B build -S . --fresh -DBUILD_USING_OTHER_LAPACK="-lnotablas"

Note: This does not change any generated CMakefiles, and it won't break anything that isn't/wouldn't already be broken.


This change is Reviewable

@halleysfifthinc
Copy link
Contributor Author

Bump

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks again, Allen. This looks great and works as you said. I did notice on Linux that I get the message No Package 'glut' found. That's not due to your changes however; just a minor annoyance since it actually does find glut and use it successfully in the visualizer. In case you're in the mood for more CMake cleanup, it would likely avoid confusion if it didn't complain about something it actually found!

:lgtm:

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved

@sherm1 sherm1 merged commit 34b0ac4 into simbody:master Aug 24, 2024
6 of 7 checks passed
@sherm1
Copy link
Member

sherm1 commented Aug 25, 2024

Mac CI fails now -- I think it's related to this change. Can you tell?
https://github.com/simbody/simbody/actions/runs/10542062601/job/29208417524

@halleysfifthinc
Copy link
Contributor Author

Hmm looking at the CI logs for after the merge and the most recent run from this branch, it seems like there is an interaction between this branch and the CMake presets because the "found" BLAS/LAPACK are now different. I will investigate when I get back from vacation at the end of this week.

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.

2 participants