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

Link to LAPACK before BLAS #384

Merged
merged 1 commit into from
Jan 15, 2023
Merged

Conversation

FabienPean
Copy link
Contributor

This PR swaps the order of linking to have LAPACK before BLAS.

Why ? When experimenting packaging the repo through vcpkg, I faced a peculiar problem when using static libraries (through WSL on Ubuntu 20.04 with GCC 9.4).

The first generated build command failed (with undefined references)

[1/2] /usr/bin/c++  -isystem /home/fabien/vcpkg/installed/x64-linux/include/arpack-ng -O2 -g -DNDEBUG -std=gnu++17 -MD -MT CMakeFiles/main.dir/main.cpp.o -MF CMakeFiles/main.dir/main.cpp.o.d -o CMakeFiles/main.dir/main.cpp.o -c ../../../main.cpp
[2/2] : && /usr/bin/c++ -O2 -g -DNDEBUG  CMakeFiles/main.dir/main.cpp.o -o main  /home/fabien/vcpkg/installed/x64-linux/lib/libarpack.a  /home/fabien/vcpkg/installed/x64-linux/lib/libopenblas.a  /home/fabien/vcpkg/installed/x64-linux/lib/liblapack.a  -lm  /usr/lib/x86_64-linux-gnu/libgfortran.so.5  -lgfortran  -lquadmath && :
FAILED: main 

By some chance it worked when fiddling around by manually adding targets in the CMakeLists of the test case

[1/2] /usr/bin/c++  -isystem /home/fabien/vcpkg/installed/x64-linux/include/arpack-ng -O2 -g -DNDEBUG -std=gnu++17 -MD -MT CMakeFiles/main.dir/main.cpp.o -MF CMakeFiles/main.dir/main.cpp.o.d -o CMakeFiles/main.dir/main.cpp.o -c ../../../main.cpp
[2/2] : && /usr/bin/c++ -O2 -g -DNDEBUG  CMakeFiles/main.dir/main.cpp.o -o main  /home/fabien/vcpkg/installed/x64-linux/lib/libarpack.a  /home/fabien/vcpkg/installed/x64-linux/lib/liblapack.a  -lm  /usr/lib/x86_64-linux-gnu/libgfortran.so.5  /home/fabien/vcpkg/installed/x64-linux/lib/libopenblas.a  -lgfortran  -lquadmath && :

The main difference was that liblapack.a was put before libopenblas.a. I tried the change given in this PR and it built successfully.

It might not be the optimal solution, but the correct CMake alternative seems to be relatively recent (v3.24), see https://gitlab.kitware.com/cmake/cmake/-/issues/20078 and https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:LINK_LIBRARY

@fghoussen
Copy link
Collaborator

@FabienPean: I get your point but this breaks CI so it seems something's wrong.
@juanjosegarciaripoll: can you help? Likely in connection with work previously done before merging #379.

@FabienPean
Copy link
Contributor Author

FabienPean commented Dec 11, 2022

All the CI tests reporting failure are the MacOS ones and they failed on the step Update OS. This looks to me like something Github actions runners and/or MacOS specific, since it happened also for a documentation modification here: #383.

It might be related to the transitioning macOS-latest to macOS 12 (actions/runner-images#6384).

EDIT: observing the log of the CI for the other PR, one of the 3 macOS job succeeds while other fails and they all are on macOS 12. This seems random, so re-running the failed jobs might end up working. This still seems like a github actions related issue though.

@fghoussen
Copy link
Collaborator

This seems random, so re-running the failed jobs might end up working.

Didn't check logs! I'll re-run jobs, we'll see... If CI is broken, we will to fix this before merging anything else.

@juanjosegarciaripoll
Copy link
Contributor

The change looks correct to me. In my patches simply copied the order that was in the original CMakeLists.txt but this one does definitely makes more sense. In many systems, such as the ones I use, it makes no difference if openblas already includes lapack (which is often the case), which is why I did not notice a difference -- and probably why CI also did not.

@FabienPean
Copy link
Contributor Author

FabienPean commented Dec 12, 2022

Here is the reason for macOS job failing actions/runner-images#2247

Jobs using any of the macOS virtual environments may experience longer than normal queue times. In some cases it's possible workflows will time out, be cancelled, and need to be restarted.

I made a PR in my fork to be able to run the CI at will, it worked there for all without retry: FabienPean#1

@fghoussen
Copy link
Collaborator

OK no hurry we can wait for github-actions to be back working!

@fghoussen
Copy link
Collaborator

Good to merge if CI OK

@fghoussen fghoussen merged commit 6f58bb1 into opencollab:master Jan 15, 2023
@FabienPean FabienPean deleted the patch-1 branch January 15, 2023 19:32
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.

3 participants