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

Improve Detection of libMesh Installation via LIBMESH_ROOT and CMake's PkgConfig #3149

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

ahnaf-tahmid-chowdhury
Copy link
Contributor

Overview:

This PR enhances the current CMake configuration for finding the libMesh installation when building OpenMC with libMesh support. The existing implementation relies on the system's pkg-config path, which can be problematic in environments where libMesh is installed in non-standard locations. The proposed changes address this issue by introducing the ability to use a LIBMESH_ROOT environment variable to locate the libMesh installation.

Changes:

  • LIBMESH_ROOT Support: If LIBMESH_ROOT is set, the CMake script will now search for the libmesh.pc file in several common pkgconfig subdirectories within the provided root path. If found, the PKG_CONFIG_PATH is updated to include the directory containing the .pc file.
  • Improved Error Handling: The script now outputs more informative messages when the libmesh.pc file is found or when it fails to locate the file.
  • Backward Compatibility: The original behavior remains intact for systems where LIBMESH_ROOT is not set, defaulting to using the system's pkg-config path and CMake’s prefix path mechanism.

Example:

  • Set LIBMESH_ROOT environment variable:

    export LIBMESH_ROOT=/path/to/custom/libmesh/install
  • CMake now finds libmesh.pc automatically in $LIBMESH_ROOT/lib/pkgconfig or other common pkgconfig directories within the installation.

Additional Information:

The updated CMake script can handle installations in various locations, such as:

  • ${LIBMESH_ROOT}/lib/pkgconfig
  • ${LIBMESH_ROOT}/lib64/pkgconfig
  • ${LIBMESH_ROOT}/lib/x86_64-linux-gnu/pkgconfig
  • ${LIBMESH_ROOT}/share/pkgconfig

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

The existing implementation relies on the system's pkg-config path, which can be problematic in environments where libMesh is installed in non-standard locations.

Can you provide an example where the root directory of the libMesh installation is the cause of the problem? We currently rely on a libMesh installation installed in a custom location in CI and this should be captured as part of the CMAKE_PREFIX_PATH variable in the current implementation. I'd probably rather dig into why that isn't working, if that's the case, than add a new variable to our CMake files.

The search suffixes I can understand may be a problem, but, similar to the root directory, I'd rather avoid deviating from CMake's built-in capabilities if at all possible.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I encountered an issue when building everything locally within a Docker container. The installation works fine when OpenMC is installed in the system directory. However, we can only configure CMAKE_INSTALL_PREFIX once, and we've configured it multiple times to identify different root directories for various packages like DAGMC and NCrystal.

While enabling all of them, I faced problems where the setup for scikit-build-core even CMake couldn't locate DAGMC, NCrystal, or libMesh at a same time as one replaced another. Ideally, we should only specify CMAKE_INSTALL_PREFIX for the OpenMC installation path. This approach would allow us to install it in different locations without conflicts.

@pshriwise
Copy link
Contributor

pshriwise commented Sep 30, 2024

I encountered an issue when building everything locally within a Docker container. The installation works fine when OpenMC is installed in the system directory. However, we can only configure CMAKE_INSTALL_PREFIX once,

Just to be clear, you're saying that the CMAKE_INSTALL_PREFIX value for OpenMC's configuration is influencing it's ability to find optional dependencies that have already been installed?

and we've configured it multiple times to identify different root directories for various packages like DAGMC and NCrystal.
While enabling all of them, I faced problems where the setup for scikit-build-core even CMake couldn't locate DAGMC, NCrystal, or libMesh at a same time as one replaced another. Ideally, we should only specify CMAKE_INSTALL_PREFIX for the OpenMC installation path. This approach would allow us to install it in different locations without conflicts.

can you specify where we're configuring it multiple times. I'm wondering if we're miscommunicating about CMAKE_PREFIX_PATH vs. CMAKE_INSTALL_PREEFIX?

I do see in our CI files that the CMAKE_PREFIX_PATH is set multiple times. However, if we need to append other paths to this value, then I think we should learn to do so when constructing the CMake command rather than modify our CMake files.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

Thanks for introducing CMAKE_PREFIX_PATH. It looks like the workflow is functioning with the previous config file, which makes me wonder why my Docker setup was consistently failing. I’ll dig deeper into the Docker environment to identify what might be causing the issue there.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

Added some hints, and have removed LIBMESH_PC variable.

@pshriwise
Copy link
Contributor

@ahnaf-tahmid-chowdhury were you able to accomplish what's needed by updating the CMAKE_PREFIX_PATH differently?

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

Yes, we can rely on CMAKE_PREFIX_PATH. However, since everything is working, I have just added some hints for getting the configuration file by setting the root dir only.

@pshriwise
Copy link
Contributor

Yes, we can rely on CMAKE_PREFIX_PATH. However, since everything is working, I have just added some hints for getting the configuration file by setting the root dir only.

It appears that a find_path has been added here as well. Is that still necessary?

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Thanks @ahnaf-tahmid-chowdhury for these improvements.

@pshriwise pshriwise merged commit fb3aaa4 into openmc-dev:develop Oct 10, 2024
16 checks passed
@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury deleted the add-libmesh-root branch October 10, 2024 17:19
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