Skip to content

Fixing issues related to linking parallel hdf5 on cmake - #391

Open
dutta-alankar wants to merge 6 commits into
idefix-code:developfrom
dutta-alankar:hdf5_link_compile_fix
Open

Fixing issues related to linking parallel hdf5 on cmake#391
dutta-alankar wants to merge 6 commits into
idefix-code:developfrom
dutta-alankar:hdf5_link_compile_fix

Conversation

@dutta-alankar

@dutta-alankardutta-alankar commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

On our Freya cluster at MPCDF, I tried compiling Idefix using the following after setting IDEFIX_DIR:

module purge
module load gcc/15 cuda/13.0 cmake/4.0
OMPI=/freya/ptmp/mpa/adutt/openmpi_gpu-5.0
HDF5=/freya/ptmp/mpa/adutt/hdf5-2.0.0-openmpi_gpu-5.0/parallel
export PATH=$OMPI/bin:$HDF5/bin:$PATH
export LD_LIBRARY_PATH=$OMPI/lib:$HDF5/lib:${LD_LIBRARY_PATH:-}
CC=mpicc CXX=mpic++ cmake \
-DIdefix_MPI=ON -DIdefix_DEBUG=OFF \
-DIdefix_HDF5=ON -DKokkos_ENABLE_CUDA=ON \
-DKokkos_ARCH_AMPERE80=ON \
-DHDF5_ROOT=/freya/ptmp/mpa/adutt/hdf5-2.0.0-openmpi_gpu-5.0/parallel \
-DIdefix_PROBLEM_DIR=$PWD \
-GNinja -B ./build -S $IDEFIX_DIR cmake --build ./build/

This resulted in an error in linking hdf5 library. Following is the error message:

-- Found HDF5: hdf5-shared (found version "2.0.0")
-- Found HDF5 include directories: /freya/ptmp/mpa/adutt/hdf5-2.0.0-openmpi_gpu-5.0/parallel/include
CMake Error at CMakeLists.txt:122 (message):
Parallel HDF5 required for Idefix_MPI but the found HDF5 library does not
support it

This comes up despite having parallel support enabled hdf5 library. This PR addresses the issue and introduces a robust hdf5 linking section in CMakeLists.txt.
Apart from this, there are two minor edits to suppress unused variable warnings.

find_package(HDF5) can resolve through the HDF5-provided CMake config package (config mode), which does not set HDF5_IS_PARALLEL. The previous 'if(NOT HDF5_IS_PARALLEL)' check then wrongly failed for a genuinely parallel HDF5 build.
Detect parallel support robustly: prefer imported targets (hdf5::hdf5 / HDF5::HDF5), fall back to the FindHDF5 module, and determine parallelism from HDF5_IS_PARALLEL/HDF5_C_IS_PARALLEL, 'h5cc -showconfig', or an H5Pset_fapl_mpio compile+link probe.
Mark variables that are unused in some build configurations as [[maybe_unused]]: the MPI buffer index locals in Axis::ExchangeMPI and tot_dim in the DIMENSIONS==2 branch of Xdmf::Write.

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts Idefix’s CMake HDF5 discovery/linking to better support parallel HDF5 builds (especially when HDF5 is found via a CMake config package), and it also suppresses a couple of unused-variable warnings in C++ sources.

Changes:

  • Update CMakeLists.txt to prefer HDF5 imported targets when available and add a robust parallel-HDF5 detection probe for MPI builds.
  • Suppress unused-variable warnings in src/output/xdmf.cpp and src/fluid/boundary/axis.cpp using [[maybe_unused]].

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

FileDescription
CMakeLists.txtImproves HDF5 linking target selection and adds a more reliable parallel-HDF5 detection mechanism when Idefix_MPI=ON.
src/output/xdmf.cppAdds [[maybe_unused]] for tot_dim in the DIMENSIONS==2 branch to silence warnings.
src/fluid/boundary/axis.cppMarks local variables as [[maybe_unused]] in Axis::ExchangeMPI to suppress unused-variable warnings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadCMakeLists.txt Outdated
Comment threadCMakeLists.txt Outdated
@dutta-alankar

Copy link
Copy Markdown
ContributorAuthor

This is first of few PRs (as discussed in #349) that I am going to put in the next few weeks, final being the introduction of the Tabulated radiative cooling module once all these other changes get merged.

dutta-alankarand others added 2 commits July 15, 2026 11:50
Minor Co-pilot suggested fixes
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@dutta-alankar

Copy link
Copy Markdown
ContributorAuthor

@svalat I plan to put in the Cooling module on top of these commits. So, it would be nice if you could review this and this PR gets merged.

@svalatsvalat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A minor change request and it looks ok to me, thanks for the proposal.

Comment threadCMakeLists.txt Outdated
@dutta-alankar

dutta-alankar commented Aug 18, 2026

Copy link
Copy Markdown
ContributorAuthor

@svalat Let me know if you're happy with the changes introduced.

@svalat

Copy link
Copy Markdown
Collaborator

Hi, the patch is OK to me, thanks for it.
Waiting the status of the test suite to merge.
We just merged a fix about the test suite which might collide with your fixes about "maybe unused", I solved the conflict, if you can accept it.

@dutta-alankar

dutta-alankar commented Aug 18, 2026

Copy link
Copy Markdown
ContributorAuthor

Hi, the patch is OK to me, thanks for it. Waiting the status of the test suite to merge. We just merged a fix about the test suite which might collide with your fixes about "maybe unused", I solved the conflict, if you can accept it.

@svalat I do not seem to find the option to accept your change.

Sign up for freeto 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

@dutta-alankar@svalat