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

Support to use external arpack libraries #554

Merged
merged 2 commits into from
Sep 2, 2024
Merged

Conversation

ealbiter
Copy link
Contributor

Porpouse

With this pull request, you can add support for using external arpack libraries provided by the system. Nowadays, several platforms provide precompiled packages of the arpack library based on the arpack-ng project, which can be used to compile elmerfem.
Currently, the compiled arpack libraries are installed in ELMER_INSTALL_LIB_DIR, which prevents the installation of the system-provided arpack package if this path is set to the system lib path. Consequently, other software that depends on this package can not be installed (e.g., octave, igraph).

Proposed changes

A new cmake variable (EXTERNAL_ARPACK::BOOL) can be used to employ the internal arpack library or the system-provided one. By default, the internal arpack libraries will be used. The user optionally can set variables to define the path to the libraries' location and include directory (ARPACK_LIBRARIES, ARPACK_INCLUDE_DIR, and if MPI is used PARPACK_LIBRARIES, PARPACK_INCLUDE_DIR). If the user does not specify these variables, cmake will try to find the libraries using the FindARPACK.cmake script.

@ealbiter ealbiter marked this pull request as ready for review August 31, 2024 03:04
Comment on lines +9 to +13
IF(NOT EXTERNAL_ARPACK)
ADD_SUBDIRECTORY(arpack)
IF(MPI_FOUND)
ADD_SUBDIRECTORY(parpack)
ENDIF()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be possible to use an external ARPACK in combination with the PARPACK that is bundled with Elmer?
What do you think about this change?

Suggested change
IF(NOT EXTERNAL_ARPACK)
ADD_SUBDIRECTORY(arpack)
IF(MPI_FOUND)
ADD_SUBDIRECTORY(parpack)
ENDIF()
IF(NOT ARPACK_FOUND)
ADD_SUBDIRECTORY(arpack)
ENDIF()
IF(MPI_FOUND AND NOT PARPACK_FOUND)
ADD_SUBDIRECTORY(parpack)
ENDIF()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, depending on the components' search results, you can have a mixed configuration (arpack: external and parpack: internal, or vice-versa). Your proposed change would act as a failsafe mechanism if something went wrong. However, I think it could defeat the original idea of explicitly using external libraries.
Plus, some platforms provide separate packages for arpack and parpack. So if the user forgets to install both, this omission could go unnoticed if they ignore the cmake's output.

Perhaps the solution to have a mixed configuration is to split the logic of FindARPACK.cmake into two scripts (FindARPACK.cmake and FindPARPACK.cmake) and add a new variable (EXTERNAL_PARPACK::BOOL).
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separating the Find* modules for the two libraries and having dedicated EXTERNAL_* options sounds like a good idea to me. 👍

@mmuetzel
Copy link
Contributor

I haven't tested this yet. But for the case that the external ARPACK doesn't provide a CMake module that imports the respective targets, shouldn't it be necessary to create the CMake import targets arpack and parpack with the information about the external libraries (e.g., their include path, ...)?
I can't find that in the new FindARPACK.cmake file.

Also, use the provided information by the config script to setup some
variables.
@juharu
Copy link
Contributor

juharu commented Sep 2, 2024 via email

@juharu
Copy link
Contributor

juharu commented Sep 2, 2024 via email

@ealbiter
Copy link
Contributor Author

ealbiter commented Sep 2, 2024

I haven't tested this yet. But for the case that the external ARPACK doesn't provide a CMake module that imports the respective targets, shouldn't it be necessary to create the CMake import targets arpack and parpack with the information about the external libraries (e.g., their include path, ...)? I can't find that in the new FindARPACK.cmake file.

Yep, you are right. I have added a new commit taking into consideration your comments.

@raback raback merged commit 0747a2b into ElmerCSC:devel Sep 2, 2024
10 checks passed
@mmuetzel
Copy link
Contributor

mmuetzel commented Sep 2, 2024

It looks like this was merged before the CMake module for PARPACK has been split from the CMake module for ARPACK.

@ealbiter: Could you please suggest the necessary changes in a follow-up PR?

@raback
Copy link
Contributor

raback commented Sep 2, 2024

Ok, sorry I was a little hasty! As the ARPACK and PARPACK have so far always been married, I guess this does not break anything that has been?

@mmuetzel
Copy link
Contributor

mmuetzel commented Sep 2, 2024

No, the default is still to use the bundled implementations.

The ARPACK package in MSYS2 doesn't contain a parallel version of ARPACK. So, it won't be possible to use the ARPACK package from MSYS2 but still use the bundled PARPACK.
But that doesn't break anything with respect to what was possible before this PR was merged.

@ealbiter
Copy link
Contributor Author

ealbiter commented Sep 2, 2024

@mmuetzel

@ealbiter: Could you please suggest the necessary changes in a follow-up PR?

I am currently working on this. Once I finish some tests, I will make the new PR.

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.

4 participants