HDF5 Build Hack Resolved

[Malmahrouqi3]

User description

Description

Concerning #621, contingent upon LLNL/Silo improving the Silo's HDF5 search.
I removed FindHDF5.cmake intended for CCE machines to now fully rely on SiloFindHDF5.cmake.

---

PR Type

Enhancement, Bug fix

---

Description

• Removed custom CCE-specific FindHDF5.cmake module

• Eliminated CCE toolchain conditional from CMakeLists.txt

• Removed pinned Silo Git commit tag for flexibility

• Relies on Silo's native HDF5 discovery mechanism

---

Diagram Walkthrough

flowchart LR
  A["CCE-specific FindHDF5.cmake"] -->|removed| B["Use Silo's HDF5 discovery"]
  C["CMakeLists.txt CCE conditional"] -->|removed| B
  D["Pinned Silo commit"] -->|removed| E["Flexible Silo version"]

Loading

File Walkthrough

	Relevant files
Enhancement	FindHDF5.cmake Removed custom CCE HDF5 finder module                                        toolchain/cmake/cce/FindHDF5.cmake Deleted entire custom HDF5 finder module (39 lines) Module provided HDF5 library and include directory detection Functionality now delegated to Silo's native HDF5 search +0/-39    CMakeLists.txt Removed CCE toolchain conditional logic                                    CMakeLists.txt Removed conditional CMAKE_MODULE_PATH prepend for Cray compiler Eliminated CCE-specific toolchain directory from module search path Simplifies build configuration by removing compiler-specific logic +0/-4
Bug fix	CMakeLists.txt Removed Silo commit pin for flexibility                                    toolchain/dependencies/CMakeLists.txt Removed pinned GIT_TAG for Silo external project Allows Silo to use latest available version Enables automatic HDF5 discovery via Silo's native mechanism +0/-1

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 621 - Partially compliant Compliant requirements: Stop relying on custom HDF5 CMake find-module hacks. Remove the project’s custom FindHDF5.cmake and associated special-casing (e.g., Cray CCE paths). Eliminate Silo patching that injects our custom FindHDF5; rely on Silo’s own HDF5 discovery. Non-compliant requirements: Use upstream mechanisms so that system HDF5 can be used and we no longer need to force-build HDF5. Keep builds working across compilers/platforms (including historically-problematic CCE) without regressions. Requires further human verification: Confirm Silo’s current CMake properly finds HDF5 across toolchains (including Cray CCE) without our module. Validate that system HDF5 configurations are detected and used when available in CI and target environments. Ensure no downstream packages break due to removal of our custom module and CCE path injection.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Regression Removing the CCE-specific module path may break HDF5 detection on Cray environments; verify builds on CCE toolchains. list(PREPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/toolchain/cmake/regular") # Compiler Flags: Here, we specify our own compiler flags for both release and # debug builds. These include optimization and debug flags, as well as some that Version Drift Dropping the pinned Silo commit introduces variability; ensure Silo’s HEAD remains compatible and its HDF5 finder is stable. ExternalProject_Add(silo GIT_REPOSITORY "https://github.com/LLNL/Silo" GIT_PROGRESS ON PATCH_COMMAND "${GIT_EXECUTABLE}" stash && "${GIT_EXECUTABLE}" apply "${CMAKE_SOURCE_DIR}/Silo.patch"

[qodo-merge-pro]

✅ Suggestion: Pin the silo external project to a specific Git tag or commit to ensure reproducible builds. [possible issue, importance: 10]

Suggested change

	ExternalProject_Add(silo
	GIT_REPOSITORY "https://github.com/LLNL/Silo"
	GIT_TAG 9af504ef4fb79153e1fbf3bdb75421b6b65f6dc4
	GIT_PROGRESS ON
	PATCH_COMMAND "${GIT_EXECUTABLE}" stash
	&& "${GIT_EXECUTABLE}" apply "${CMAKE_SOURCE_DIR}/Silo.patch"
	ExternalProject_Add(silo
	GIT_REPOSITORY "https://github.com/LLNL/Silo"
	GIT_TAG "v4.11.1" # Or another specific, known-good commit/tag
	GIT_PROGRESS ON
	PATCH_COMMAND "${GIT_EXECUTABLE}" stash
	&& "${GIT_EXECUTABLE}" apply "${CMAKE_SOURCE_DIR}/Silo.patch"

[Copilot]

Pull Request Overview

This PR aims to remove a workaround for HDF5 detection on Cray systems by relying on upstream improvements in Silo's SiloFindHDF5.cmake. The changes remove the custom FindHDF5.cmake module and associated Cray-specific CMake module path configuration.

Key changes:

• Removes custom FindHDF5.cmake from toolchain/cmake/cce/ directory
• Removes Cray-specific CMake module path from main build configuration
• Updates Silo external project to track latest upstream (removes pinned commit)

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
CMakeLists.txt	Removes conditional CCE module path prepending for Cray compilers
toolchain/cmake/cce/FindHDF5.cmake	Deletes custom HDF5 finder module for Cray systems
toolchain/dependencies/CMakeLists.txt	Removes pinned Git commit for Silo to track latest upstream

[Copilot]

Critical Issue - Incomplete Refactoring:

This PR deletes toolchain/cmake/cce/FindHDF5.cmake and removes CCE module path references from the main CMakeLists.txt, but this file still contains references to the now-deleted cmake/cce directory:

• Lines 9-11: if (CMAKE_Fortran_COMPILER_ID STREQUAL "Cray") block adds the cce path
• Line 96: Passes -DCMAKE_MODULE_PATH=${CMAKE_SOURCE_DIR}/../cmake/cce to Silo's build

These will cause build failures on Cray systems. Both references must be removed.

Additional Note: Removing GIT_TAG means Silo will track the default branch instead of the previously pinned commit. Consider adding a comment explaining this is intentional (e.g., "Track latest Silo for improved HDF5 detection via SiloFindHDF5.cmake").