Create an arpack-ng-config.cmake that propagates arpack's dependencies

[juanjosegarciaripoll]

Pull request purpose

The CMake files that are installed by arpack are non-standard and do not enumerate the library dependencies. This is critical where arpack is not built as a shared library (e.g. Windows), but more generally it is not considered good practice. This way, users can just use

target_add_libraries(myproject PRIVATE arpack)

instead of having themselves to build something like

find_package(BLAS REQUIRED)
[... code for older versions of cmake's findBlas ...]
find_package(LAPACK REQUIRED)
[... code for older versions of cmake's findBlas ...]
target_add_libraries(myproject PRIVATE arpack BLAS::BLAS LAPACK::LAPACK)

Detailed changes proposed in this pull request

• Create an arpack-ng-targets.cmake file that relies on CMake to create proper targets
• Declare BLAS::BLAS and LAPACK::LAPACK and MPI::Fortran as public dependencies of arpack and parpack,
• Rewrite arpack-ng-config.cmake to find its dependencies using standard tools (find_dependency)

[fghoussen]

@juanjosegarciaripoll : can you describe in detail what kind of problem you encounter on Window? Don't get it

[juanjosegarciaripoll]

@juanjosegarciaripoll : can you describe in detail what kind of problem you encounter on Window? Don't get it

It's not exclusive of Windows but of any platform where the library is built statically and CMake is used to link to Arpack. In those situations the user must explicitly list not only Arpack but also all the blas, lapack and pthreads libraries used by Arpack, because the currently CMake exported files (before my fix) do not record this information (they only recorded the CMake equivalent of -larpack, which is not enough for statically linked libraries)

Moreover, the way that CMake files must be written is more or less how I have done it: the project must create a config file that reproduces the steps to find the libraries that Arpack depends on, not just record the libraries as the pkgconfig files do. This is good practice and helps for the project to be easily relocatable.

So summing up, the CMake project needs this fix for the library to be used with CMake without any hacks. This fix is also required for the library to be easy and robustly integrated in other non-Unix, CMake-basef distribution such as vcpkg or Conan.

[fghoussen]

@juanjosegarciaripoll: can you rebase?

[juanjosegarciaripoll]

I have rebased this other branch as well.

[fghoussen]

@juanjosegarciaripoll : can you rebase on master? Looks like you are merging into master which makes history a mess to understand/track!... I'd like to keep a clean history (otherwise no way you can use it)!

[juanjosegarciaripoll]

As in the complex numbers branch, I have rebased and squashed all changes into a single commit. I am not a git expert, but it seems to have worked.

[fghoussen]

@juanjosegarciaripoll: can you report your changes into arpack-ng/cmake/arpackng-config.cmake.in and rebase on top of (new) master?

arpack-ng/cmake/arpack-ng-config.cmake.in had to be moved into arpack-ng/cmake/arpackng-config.cmake.in in 3ba4dbfbe908340512a2d0d731598d843462e531: this was needed to be able to test arpack-ng install, your rebase missed that move.

I am not a git expert

You are becoming one! :D

[fghoussen]

@juanjosegarciaripoll: can you report your changes into arpack-ng/cmake/arpackng-config.cmake.in and rebase?

[juanjosegarciaripoll]

Apologies, I am away for holidays and conferences and have not been able to address this. I need to find a weekend to do the rebating.

El lun., 15 ago. 2022 11:25, Franck HOUSSEN @.***> escribió:

@juanjosegarciaripoll https://github.com/juanjosegarciaripoll: can you report your changes into arpack-ng/cmake/arpackng-config.cmake.in and rebase?

— Reply to this email directly, view it on GitHub https://github.com/opencollab/arpack-ng/pull/335#issuecomment-1214811987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQWH7R2AZTSRXXLRGQLEZDVZIEJZANCNFSM5M6DO4XA . You are receiving this because you were mentioned.Message ID: @.***>

[fghoussen]

@juanjosegarciaripoll: that's OK! No hurry :)

[fghoussen]

Can you report your changes into arpack-ng/cmake/arpackng-config.cmake.in and rebase? (no merge). Strange message in PR at my side.

[juanjosegarciaripoll]

I haven't submitted a PR recently. I am working on my GitHub fork merging upstream changes and trying to fix what is left and makes current CMake build non-functional.

For instance, just discovered that there are multiple naming inconsistencies between the directories used for installation, the name of the CMake package and the name of the config files.

What is the desired name of the project? arpack, arpack-ng, arpackng...

El lun., 17 oct. 2022 17:55, Franck HOUSSEN @.***> escribió:

Can you report your changes into arpack-ng/cmake/arpackng-config.cmake.in and rebase? (no merge). Strange message in PR at my side.

[image: image] https://user-images.githubusercontent.com/20612589/196225015-327dde23-030d-49b1-87e5-e14c6ec82c81.png

— Reply to this email directly, view it on GitHub https://github.com/opencollab/arpack-ng/pull/335#issuecomment-1281095113, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQWH7RTQ3LX5HP2YYUKG5TWDVZGHANCNFSM5M6DO4XA . You are receiving this because you were mentioned.Message ID: @.***>

[fghoussen]

What is the desired name of the project? arpack, arpack-ng, arpackng...

The project is named arpack-ng (arpack is the original reference codebase - not maintained).

But sometimes one need to use arpackng (without dash): cmake hints (find_package and *.cmake files mechanism rely on the default XXX_DIR env variable), but, AFAIR, bash doesn't allow variable names like arpack-ng_DIR (dash not allowed in bash variable names) so I had to rename the cmake file from the initial arpack-ng-config.cmake.in.sh into arpackng-config.cmake.in.sh (without the dash in the project name) to get a way to run the test tstCMakeInstall OK: https://github.com/opencollab/arpack-ng/blob/8ecf1be71347f6b6eff5fc4f8c48e509cf3f4add/cmake/tstCMakeInstall.sh.in#L31

Please, do the minimal impact, I don't want to break everything: do as less as possible! :)

[juanjosegarciaripoll]

I have made various tests with Debian bullseye and CMake 3.18. The problem is that the current CMakeLists.txt installs the configuration files under the arpack-ng directory. In that situation cmake does not seem able to find the package. However, in the current rebased branch I changed CMakeLists.txt to install files under lib/cmake/arpackng and now CMake finds the configuration file. I am using the tests in this repository to verify the CMake configuration is properly installed and usable for C sources https://github.com/juanjosegarciaripoll/arpack-ng-cmake-test

[juanjosegarciaripoll]

I think I have finished the patch. The previous errors also exist in the main trunk because the CMake code for pyarpack defines the installation directories before loading GNUInstallDirs, the CMake module that defines the environment variables used. With these three fixes, I can build C/C++ sources and pull in the dependencies from arpack. I should now test this with VCPkg, but VCPkg's license prevents me from contributing a module for arpack unless these changes are already upstream in opencollab/arpack-ng.

[fghoussen]

@juanjosegarciaripoll: thanks! I had no time to have a close look (maybe this week-end), but, 2 on-the-fly remarks:

• there should be only one .cmake file, but, seems there are 2: arpack-ng-config.cmake.in (arpack-ng with dash) and ./cmake/arpackng-config.cmake.in (arpackng without dash). My understanding is that the one that should stay is ./cmake/arpackng-config.cmake.in (for reasons explained above) and that modifications done in arpack-ng-config.cmake.in should have been done in ./cmake/arpackng-config.cmake.in
• autotools should do the same as cmake: can you change the installation directory (libraries and include) from arpack-ng to arpackng for both autools (Makefile.am, ...) and cmake? (to get consistent install whatever the build system used)

[juanjosegarciaripoll]

You were right. I have also noticed that the current configuration did not propagate dependencies when the libraries were built in statically linked mode. I am working on further changes, as well as some cleanup of the code that builds the tests and examples.

[fghoussen]

This patch is becoming huge which is no good: mixing-up things is the best way to reg! :( I thought at first it was only about using find_dependency in ./cmake/arpackng-config.cmake.in (arpackng without dash): can you just stick to and push that please? Others things could come after: one minimal step at a time.

Note: seems your patch creates ./arpack-ng-config.cmake.in (arpack-ng with dash) which should not be used (or I missed something in the process - this PR has a blurry history!...).

[juanjosegarciaripoll]

I am afraid this cannot be a small patch. There are many errors in the actual CMake file because of the lack of order in the construction:

• find_dependency is not used, but also...
• targets do not set the include paths properly so it does not suffice to include it
• the configuration file did not use CMake targets export file, which is required to carry on dependencies on BLAS, LAPACK, MPI
• GNUInstallPaths was included late, leading to variables being wrongly defined
• tests and examples are defined before all targets they depend on are completely constructed
• the library only worked with shared library linking, not statically linked projects
• configure_file was used where a file copy was required, placing various tests at risk
• tests use hardcoded locations for the executables which CMake does not follow on some platforms
• at various places the script redefined the global variable that defines the location of binary files, an effect that propagates in a risky way throughout the script; the new version avoids this
• moreover, those hardcoded locations do not work in Windows, where the DLL is not available in the executable path; for that reason none of the examples work in mingw64 and only some of the tests worked (none of the ICBEXMM) as confirmed by your GitHub workflow which does not include those options
• the arpack .GitHub/workflow tests for CMake are themselves wrong because they supply the build script with the location of the CMake file which does not test the discovery of those files ...

The history may seem complicated but the fact is that the CMake file is not functioning and requires a lot of fixes, many of which are coupled among themselves. It has various intermediate fixes because I do not have access to all test platforms (OSX) and these reveal further inconsistencies in the original code.

Also, taking the tests and examples outside of the main source is good practice and helped me debugging the actual main targets. Your autoconf version also does this. This also avoids having adhoc rules that are broken by having common standard function for all tests, and solves the problem of execution order I mentioned before.

However, my intention is to clean up a single patch with a cleaner description after rebasing my branch to a single commit.

Regarding your last remark the patch is not creating an arpack-ng file but arpackng prefixed ones. I have tested it on Linux and Windows. Currently the only platform that does not run all tests is OSX because I forgot a $<TARGET_FILE> macro.

El lun., 24 oct. 2022 21:11, Franck HOUSSEN @.***> escribió:

This patch is becoming huge which is no good: mixing-up things is the best way to reg! :( I thought at first it was only about using find_dependency in ./cmake/ arpackng-config.cmake.in (arpackng without dash): can you just stick to and push that please? Others things could come after: one minimal step at a time.

Note: seems your patch creates ./arpack-ng-config.cmake.in (arpack-ng with dash) which should not be used (or I missed something in the process - this PR has a blurry history!...).

— Reply to this email directly, view it on GitHub https://github.com/opencollab/arpack-ng/pull/335#issuecomment-1289477028, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQWH7TOW56HQCAGXYXVYY3WE3NMRANCNFSM5M6DO4XA . You are receiving this because you were mentioned.Message ID: @.***>

[juanjosegarciaripoll]

I am sorry if the patch is large, but I think once cleaned up it is not difficult to read, and solves important issues.

For instance, the Windows build now runs with all tests and examples, even in Mingw64. The CMake files no longer uses non-standard practices, such as setting the global directory for executable files.

If the history of this pull request is too messy, I apologize for it. If required, I can take this patch into a fresh new branch.

[juanjosegarciaripoll]

It seems that there is something wrong I am doing with the OS X platform. I have been able to refactor the dependencies tracking to a smaller patch which I am moving to a different pull request https://github.com/opencollab/arpack-ng/pull/377

[fghoussen]

The history may seem complicated but the fact is that the CMake file is not functioning and requires a lot of fixes

I agree. Would be good if you could propose one (minimal) PR one-step-at-a-time for all bugs you listed above (if you have time for it?). We would be glad to merge them :) arpack-ng is made of volunteers (no pay...), mostly unix-based and shared lib I guess, trying to use cmake in the best possible way: not perfect but we are opened to improvements, so do not hesitate to PR fixes (one step-at-a-time is easier for maintainers to follow - I only stick around one or twice max a week... For free...)

taking the tests and examples outside of the main source is good practice and helped me debugging the actual main targets. Your autoconf version also does this.

I'am OK with that. @sylvestre: are you? (I would say we have one only CMakeLists.txt for historical reasons). Yes, autotools does this too. Doing the same with CMake would imply lots of files (CMakeLists.txt) in each subfolder.

solves important issues.

I agree. Do not hesitate to PR next after #377 is merged

those hardcoded locations do not work in Windows ... Windows build now runs with all tests and examples, even in Mingw64

Would be great if we could have all running OK on Windows too! :) AFAIR, we only have partial build and/or tests in the CI.

only platform that does not run all tests is OSX... something wrong I am doing with the OS X platform

A step-by-step PR approach will certainly help to understand what/why/when things break on OSX

[fghoussen]

I am afraid this cannot be a small patch. There are many errors in the actual CMake file because of the lack of order in the construction:

• find_dependency is not used, but also...
• targets do not set the include paths properly so it does not suffice to include it
• the configuration file did not use CMake targets export file, which is required to carry on dependencies on BLAS, LAPACK, MPI
• GNUInstallPaths was included late, leading to variables being wrongly defined
• tests and examples are defined before all targets they depend on are completely constructed
• the library only worked with shared library linking, not statically linked projects
• configure_file was used where a file copy was required, placing various tests at risk
• tests use hardcoded locations for the executables which CMake does not follow on some platforms
• at various places the script redefined the global variable that defines the location of binary files, an effect that propagates in a risky way throughout the script; the new version avoids this
• moreover, those hardcoded locations do not work in Windows, where the DLL is not available in the executable path; for that reason none of the examples work in mingw64 and only some of the tests worked (none of the ICBEXMM) as confirmed by your GitHub workflow which does not include those options
• the arpack .GitHub/workflow tests for CMake are themselves wrong because they supply the build script with the location of the CMake file which does not test the discovery of those files ...

@juanjosegarciaripoll: do not hesitate to PR other fix. Would be good to have full tests running on W10 in CI