Fix BLAS++ and LAPACK++ build #903

weslleyspereira · 2023-08-30T23:43:59Z

In the current LAPACK master, building with BLAS++ flag ON causes the following error in CMake:

[build] [ 47%] Building CXX object test/CMakeFiles/tester.dir/test.cc.o
[build] In file included from /home/weslleyp/storage/lapack/build_RelWithDebInfo/blaspp-prefix/src/blaspp/test/test.cc:12:
[build] /home/weslleyp/storage/lapack/build_RelWithDebInfo/blaspp-prefix/src/blaspp/test/test.hh:9:10: fatal error: testsweeper.hh: No such file or directory
[build]     9 | #include "testsweeper.hh"
[build]       |          ^~~~~~~~~~~~~~~~
[build] compilation terminated.
[build] make[2]: *** [test/CMakeFiles/tester.dir/build.make:76: test/CMakeFiles/tester.dir/test.cc.o] Error 1
[build] make[1]: *** [CMakeFiles/Makefile2:154: test/CMakeFiles/tester.dir/all] Error 2
[build] make: *** [Makefile:136: all] Error 2
[build] ninja: build stopped: subcommand failed.
[proc] The command: /usr/local/bin/cmake --build /home/weslleyp/storage/lapack/build_RelWithDebInfo --config Release --target all -- exited with code: 1

There is some issue in BLAS++ (https://bitbucket.org/icl/lapackpp/downloads/lapackpp-2020.10.02.tar.gz) so that it can't find testsweeper.hh. I can give more details about my system configuration.

The point is: we actually don't need to build the BLAS++ tests to obtain libblaspp. Thus, this PR solves the issue by adding -Dbuild_tests=OFF to the configuration step of BLAS++ and LAPACK++ in LAPACK.

Closes #905.

weslleyspereira · 2023-08-30T23:46:23Z

I have two other concerns related to my last message:

We are using an old version of BLAS++ and LAPACK++ (2020.10.02). Is that intentional?
We have no tests in cmake.yml with BLAS++=ON and LAPACK++=ON.

codecov · 2023-08-30T23:51:14Z

Codecov Report

Attention: 176 lines in your changes are missing coverage. Please review.

Comparison is base (90398ae) 0.00% compared to head (ef44890) 0.00%.
Report is 24 commits behind head on master.

❗ Current head ef44890 differs from pull request most recent head 7f2c622. Consider uploading reports for the commit 7f2c622 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #903    +/-   ##
========================================
  Coverage    0.00%    0.00%            
========================================
  Files        1918     1918            
  Lines      188653   188757   +104     
========================================
- Misses     188653   188757   +104

Files	Coverage Δ
SRC/cgedmd.f90	`0.00% <ø> (ø)`
SRC/cgedmdq.f90	`0.00% <ø> (ø)`
SRC/dgedmdq.f90	`0.00% <ø> (ø)`
SRC/sgedmdq.f90	`0.00% <ø> (ø)`
SRC/zgedmdq.f90	`0.00% <ø> (ø)`
SRC/clarfgp.f	`0.00% <0.00%> (ø)`
SRC/dlarfgp.f	`0.00% <0.00%> (ø)`
SRC/slarfgp.f	`0.00% <0.00%> (ø)`
SRC/zlarfgp.f	`0.00% <0.00%> (ø)`
SRC/dgedmd.f90	`0.00% <0.00%> (ø)`
... and 16 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mgates3 · 2023-08-31T15:19:03Z

1. We are using an old version of BLAS++ and LAPACK++ (2010.10.02). Is that intentional?

2020.10.02. You had me worried for a minute.

I think that was just what was available at the time. Perhaps we could use a release branch or tag to refer to the latest release. Is there a standard convention for that in Git? I just added the release branch, but we can do whatever seems best.

We can update the release version, I'm just not excited about updating LAPACK every time that BLAS++ and LAPACK++ are updated.

weslleyspereira · 2023-08-31T15:48:38Z

Sorry for the "2010", @mgates3. Not intentional. I fixed my message above.

So, I have an idea on how we could make it easy to

I think that was just what was available at the time. Perhaps we could use a release branch or tag to refer to the latest release. Is there a standard convention for that in Git? I just added the release branch, but we can do whatever seems best.

We can update the release version, I'm just not excited about updating LAPACK every time that BLAS++ and LAPACK++ are updated.

Ok. Thanks. We could at least try to update these dependencies whenever we release LAPACK, or whenever BLAS++ and LAPACK++ have a major update regarding the wrappers. I am also not too excited to update LAPACK every time that BLAS++ and LAPACK++ are updated. I was just surprised it was using a release from almost 3 years ago.

weslleyspereira · 2023-08-31T15:52:38Z

Now, about my second bullet: We have no tests in cmake.yml with BLAS++=ON and LAPACK++=ON. This is more worrisome to me. Yesterday, I was trying to use those flags and end up finding out that sometimes BLAS++ links with a BLAS library outside the repository, and I think that was not the plan when the flags were added. I will create an issue with more details.

weslleyspereira · 2023-09-08T19:58:08Z

I updated this PR so that it solves #905. It should be ready for review.

Note that the changes do not add tests for the flag LAPACK++ because of icl-utk-edu/lapackpp#44.

mgates3

I coded up some changes in this commit. These work on my Mac with this configuration:

cmake -D BLAS++=true -D LAPACK++=true \
      -D USE_OPTIMIZED_BLAS=true -D BLA_VENDOR=Apple \
      -D CMAKE_INSTALL_PREFIX=${HOME}/install-lapack -B build
cd build
make
make install

Haven't tried other configurations or elsewhere yet.

mgates3 · 2023-10-10T15:41:59Z

CMakeLists.txt

+  lapackpp
+  GIT_REPOSITORY https://github.com/icl-utk-edu/lapackpp
+  GIT_TAG        62680a16a9aba2a426e3d089dd13e18bfd140c74 # v2023.08.25
+)


May want blaspp 91dd418fa910498cc03dee397826099914cc3185
and lapackpp 88088c33cd9467475e8f139f42d158620f11e64d
which have the fixes for Fortran strlen.

We will likely do a SLATE / BLAS++ / LAPACK++ bug fix release this month, but perhaps for that just update LAPACK right before the SC23 release.

mgates3 · 2023-10-11T05:33:26Z

CMakeLists.txt

+      # TODO: This is incomplete. Fill in the other cases.
+      set(BLAS_Fortran_LIB "")
+    endif()
+  endif()


Is this when using Reference BLAS, is that why it checks NOT BLAS_FOUND?

When I tried to compile, linking with liblapack.a also needed -lgfortran, but since I used optimized BLAS, BLAS_FOUND was true and BLAS_Fortran_LIB was empty.

BTW, setting it to empty in the else case is redundant with set above if.

mgates3 · 2023-10-11T06:20:34Z

CMakeLists.txt

+    -D CMAKE_INSTALL_PREFIX="${blaspp_BINARY_DIR}"
+    -D CMAKE_INSTALL_LIBDIR="${PROJECT_BINARY_DIR}/lib"
+    -D blas_libraries_cached=""
+    -D lapack_libraries_cached=""


Why was it needed to set blas_libraries_cached and lapack_libraries_cached? These are internal variables to the BLAS++ and LAPACK++ CMake scripts and subject to change.

Is this from having 2 COMMAND cmake above in the add_custom_command? Better to have only 1 COMMAND cmake so it searches correctly the first time.

(The additional COMMAND cmake --build below seems fine.)

I don't think I need to set blas_libraries_cached and lapack_libraries_cached even if we configure cmake 2x. I needed it at some point of writing the PR, but I can't reproduce it anymore.

mgates3 · 2023-10-11T06:48:15Z

CMakeLists.txt

+      COMMAND ${CMAKE_COMMAND}
+      -B "${lapackpp_BINARY_DIR}"
+      -D LAPACK_LIBRARIES="${LAPACK_LIBRARIES}" )
+  endif()


As above, this can be simplified.

mgates3 · 2023-10-11T06:48:30Z

CMakeLists.txt

+    -B "${lapackpp_BINARY_DIR}"
+    -D CMAKE_INSTALL_PREFIX="${lapackpp_BINARY_DIR}"
+    -D CMAKE_INSTALL_LIBDIR="${PROJECT_BINARY_DIR}/lib"
+    -D lapack_libraries_cached=""


As above, shouldn't set internal variables.

mgates3 · 2023-10-11T06:58:28Z

CMakeLists.txt

+  # Setup remaining configuration options and installation
+  add_custom_command( OUTPUT lapackpp-cmd APPEND
+    COMMAND ${CMAKE_COMMAND}
+    -B "${lapackpp_BINARY_DIR}"


For me, LAPACK++ couldn't find BLAS++. I found it easier to install them in the same directory. Maybe setting CMAKE_PREFIX_PATH would also work.

mgates3 · 2023-10-11T07:44:34Z

CMakeLists.txt

-  FILES_MATCHING REGEX "liblapackpp.(a|so)$"
+    DIRECTORY "${LAPACK_BINARY_DIR}/lib/"
+    DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    FILES_MATCHING REGEX "liblapackpp.(a|so)$"


There could be other lapackpp files. Matching on just lapackpp would capture everything. For instance:

-- Installing: /.../lapack/build4/lib/cmake/lapackpp/lapackppTargets.cmake -- Installing: /.../lapack/build4/lib/cmake/lapackpp/lapackppTargets-noconfig.cmake -- Installing: /.../lapack/build4/lib/cmake/lapackpp/lapackppConfig.cmake -- Installing: /.../lapack/build4/lib/cmake/lapackpp/lapackppConfigVersion.cmake

mgates3 · 2023-10-11T07:46:37Z

CMakeLists.txt

-    FILES "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfig.cmake"
-          "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfigVersion.cmake"
+    FILES "${CMAKE_CURRENT_BINARY_DIR}/lib/cmake/lapackpp/lapackppConfig.cmake"
+          "${CMAKE_CURRENT_BINARY_DIR}/lib/cmake/lapackpp/lapackppConfigVersion.cmake"
    DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/"


There are a couple other files; see above for easier fix.

mgates3 · 2023-10-11T08:01:51Z

CMakeLists.txt

+        -D BLAS_LIBRARIES="${BLAS_LIBRARIES}"
+        -D LAPACK_LIBRARIES="${LAPACK_LIBRARIES}" )
+    endif()
+  endif()


These 4 add_custom_command blocks are quite redundant. May I suggest setting some variables to have one 1 instance? See my top-level review comment for suggestion.

The redundancy is because we cannot use CMake generator expressions (e.g., $<TARGET_FILE:${LAPACKLIB}>) to set CMake variables at configuration time. See https://stackoverflow.com/questions/51353110/how-do-i-output-the-result-of-a-generator-expression-in-cmake. There is certainly a better way to do this, but I couldn't figure it out.

mgates3 · 2023-10-11T08:03:30Z

CMakeLists.txt

-    FILES "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfig.cmake"
-          "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfigVersion.cmake"
+    FILES "${CMAKE_CURRENT_BINARY_DIR}/lib/cmake/lapackpp/lapackppConfig.cmake"
+          "${CMAKE_CURRENT_BINARY_DIR}/lib/cmake/lapackpp/lapackppConfigVersion.cmake"
    DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/"
  )

 endif()
 if (BLAS++)


LAPACK++ requires BLAS++, so as above, I think this should be if (BLAS++ OR LAPACK++).

With cmake -D LAPACK++=on .., this will build BLAS++ and LAPACK++, but doesn't install libblaspp.a. Changing to if (BLAS++ or LAPACK++) fixes the issue. Can be merged with if block below (line 650).

weslleyspereira · 2023-10-13T22:02:39Z

Even with the changes from this commit. I am still having trouble to use the flag LAPACK++. See icl-utk-edu/lapackpp#50

weslleyspereira · 2023-10-31T16:54:01Z

The AppVeyor script needs some change. I will open an issue for that.

This PR is ready to be merged.

weslleyspereira · 2023-10-31T16:58:40Z

@mgates3, could you review the current stage of this PR. I think this is ready to merge. Thanks!

See mgates3@ecf6f41

mgates3

Here's the patch that worked for me:

sh leconte lapack> git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 17609c05d..89db6a866 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -633,21 +633,19 @@ install(FILES
 
 if (LAPACK++)
   install(
-    DIRECTORY "${LAPACK_BINARY_DIR}/lib/"
+    DIRECTORY "${LAPACK_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}/"
     DESTINATION ${CMAKE_INSTALL_LIBDIR}
     FILES_MATCHING REGEX "lapackpp"
   )
 endif()
 
-if (BLAS++)
+if (BLAS++ OR LAPACK++)
   install(
-    DIRECTORY "${LAPACK_BINARY_DIR}/lib/"
+    DIRECTORY "${LAPACK_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}/"
     DESTINATION ${CMAKE_INSTALL_LIBDIR}
     FILES_MATCHING REGEX "blaspp"
   )
-endif()
 
-if (BLAS++ OR LAPACK++)
   install(
     DIRECTORY "${LAPACK_BINARY_DIR}/include/"
     DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"

Otherwise, looks good. I tested on macOS with GNU compilers and OpenBLAS, and on Linux (Redhat) with GNU compilers, Intel MKL, and CUDA.

One minor issue, on Linux it said

-- Build files have been written to: /home/mgates/repos/lapack/build2/_deps/lapackpp-build
gmake[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.

and then run make in serial instead of parallel, so it was rather slow. Not a big deal.

mgates3 · 2023-11-11T23:00:35Z

CMakeLists.txt

-  DIRECTORY "${LAPACK_BINARY_DIR}/lib/"
-  DESTINATION ${CMAKE_INSTALL_LIBDIR}
-  FILES_MATCHING REGEX "libblaspp.(a|so)$"
+    DIRECTORY "${LAPACK_BINARY_DIR}/lib/"


On Linux, it put them in ${LAPACK_BINARY_DIR}/lib64, so it find libblaspp.a and liblapackpp.a to install:

sh leconte build2> pwd /home/mgates/repos/lapack/build2 sh leconte build2> ls lib lib64 lib: liblapack.a lib64: cmake libblaspp.a liblapackpp.a sh leconte build2> ls ~/install-test/lib64/ cmake liblapack.a pkgconfig

On macOS, it put them in ${LAPACK_BINARY_DIR}/lib, and everything worked (with previously mentioned fix about if (BLAS++ or LAPACK++)):

pangolin lapack/build3> pwd /Users/mgates/Documents/lapack/build3 pangolin lapack/build3> ls lib cmake/ libblaspp.a liblapack.a liblapackpp.a pangolin lapack/build3> ls ~/install-test/lib/ cmake/ libblaspp.a liblapack.a liblapackpp.a pkgconfig/

mgates3 · 2023-11-12T00:06:56Z

CMakeLists.txt

-    FILES "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfig.cmake"
-          "${CMAKE_CURRENT_BINARY_DIR}/lib/lapackpp/lapackppConfigVersion.cmake"
-    DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/"
+    DIRECTORY "${LAPACK_BINARY_DIR}/lib/"


Same issue as below; use ${CMAKE_INSTALL_LIBDIR}.

Reference-LAPACK#903 (review)

weslleyspereira mentioned this pull request Aug 31, 2023

Flag BLAS++ seems not to be working properly #905

Open

weslleyspereira force-pushed the fix-blaspp-build branch 2 times, most recently from 95bd970 to 35c78b7 Compare September 7, 2023 18:07

weslleyspereira force-pushed the fix-blaspp-build branch 4 times, most recently from 66905e9 to 680cfd8 Compare September 8, 2023 19:42

weslleyspereira mentioned this pull request Oct 10, 2023

Try testBLAS with LAPACK #913

Draft

mgates3 suggested changes Oct 11, 2023

View reviewed changes

weslleyspereira force-pushed the fix-blaspp-build branch from 00f51ca to fc569f4 Compare October 16, 2023 14:54

weslleyspereira mentioned this pull request Oct 31, 2023

LAPACKFinder issue error - LAPACK++ requires a LAPACK library and none was found. #828

Open

weslleyspereira added 6 commits November 8, 2023 15:09

Fix BLAS++ and LAPACK++ build

e7f4bd3

Add test to the CI for BLAS++ and LAPACK++ flags

0b9193a

Try to fix BLAS++ and LAPACK++ installation so that the tests pass

0c31c63

More changes to improve flags BLAS++ and LAPACK++

ca6673b

Apply @mgates3's patch and add BUILD_TYPE to BLAS++ and LAPACK++

38e5b08

See mgates3@ecf6f41

Updates BLAS++ and LAPACK++ to v2023.11.05

b50109c

weslleyspereira force-pushed the fix-blaspp-build branch from 9ded2af to b50109c Compare November 8, 2023 22:13

mgates3 suggested changes Nov 12, 2023

View reviewed changes

Applies @mgates3 patch to fix BLAS++ and LAPACK++ installation

7f2c622

Reference-LAPACK#903 (review)

mgates3 approved these changes Nov 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BLAS++ and LAPACK++ build #903

Fix BLAS++ and LAPACK++ build #903

weslleyspereira commented Aug 30, 2023 •

edited

Loading

weslleyspereira commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

mgates3 commented Aug 31, 2023

weslleyspereira commented Aug 31, 2023

weslleyspereira commented Aug 31, 2023

weslleyspereira commented Sep 8, 2023

mgates3 left a comment

mgates3 Oct 10, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

weslleyspereira Oct 13, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

mgates3 Oct 11, 2023

weslleyspereira Oct 13, 2023

mgates3 Oct 11, 2023

mgates3 Nov 11, 2023

weslleyspereira commented Oct 13, 2023

weslleyspereira commented Oct 31, 2023

weslleyspereira commented Oct 31, 2023

mgates3 left a comment

mgates3 Nov 11, 2023

mgates3 Nov 12, 2023

Fix BLAS++ and LAPACK++ build #903

Are you sure you want to change the base?

Fix BLAS++ and LAPACK++ build #903

Conversation

weslleyspereira commented Aug 30, 2023 • edited Loading

weslleyspereira commented Aug 30, 2023 • edited Loading

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

mgates3 commented Aug 31, 2023

weslleyspereira commented Aug 31, 2023

weslleyspereira commented Aug 31, 2023

weslleyspereira commented Sep 8, 2023

mgates3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weslleyspereira commented Oct 13, 2023

weslleyspereira commented Oct 31, 2023

weslleyspereira commented Oct 31, 2023

mgates3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weslleyspereira commented Aug 30, 2023 •

edited

Loading

weslleyspereira commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading