Skip to content

Conversation

Copy link

Copilot AI commented Oct 8, 2025

Problem

The CI build failed when configuring lxelsim via Spack because CMake could not find or populate HDF5 include directories (HDF5_INCLUDE_DIRS) even though HDF5 1.14.6 was present. The existing packages/lxelsim/package.py did not explicitly expose the Spack-installed HDF5 prefix or CMake config directory to the CMake configure step, which allowed FindHDF5 to pick up wrong/system HDF5 installations or miss include/library paths entirely.

Solution

This PR fixes the HDF5 configuration issue through two complementary approaches:

  1. Remove system HDF5 installations from Docker image: Added a build step in Docker/Dockerfile that explicitly removes all system HDF5 packages and files to prevent CMake from finding them
  2. Set HDF5_ROOT in lxelsim package: Updated packages/lxelsim/package.py to set HDF5_ROOT to point to the Spack-installed HDF5 prefix

Changes

Docker/Dockerfile

  • Added a new RUN step that removes system HDF5 installations using apt-get remove and manual file deletion
  • Covers common system locations for both x86_64 and aarch64 architectures:
    • /usr/lib/x86_64-linux-gnu/
    • /usr/lib/aarch64-linux-gnu/
    • /usr/lib/
    • /usr/include/
    • /usr/share/
    • /usr/local/lib/
    • /usr/local/include/

packages/lxelsim/package.py

  • Updated maintainers list to include madbaron
  • Enhanced cmake_args() method to set HDF5_ROOT to the Spack-installed HDF5 prefix
  • Preserved existing CMAKE_CXX_STANDARD=20 setting

Implementation Details

The solution eliminates the root cause by removing system HDF5 installations entirely from the Docker build environment. This approach:

  • Prevents CMake from finding conflicting system HDF5 installations
  • Does not interfere with Spack's automatic handling of CMAKE_PREFIX_PATH
  • Does not override CMake's exported configuration from the HDF5 package
  • Avoids potential conflicts from explicitly setting HDF5_INCLUDE_DIRS or HDF5_LIBRARIES
  • Ensures only the Spack-managed HDF5 is available during the build

This comprehensive solution addresses the original issue by removing ambiguity at the source while following Spack best practices.

Original prompt

Problem:
The CI build failed when configuring lxelsim via Spack because CMake could not find or populate HDF5 include dirs (HDF5_INCLUDE_DIRS) even though HDF5 1.14.6 was present. The existing packages/lxelsim/package.py does not explicitly expose the Spack-installed HDF5 prefix or CMake config dir to the CMake configure step, which allows FindHDF5 to pick up wrong/system HDF5 or miss include/library paths.

Goal:
Update packages/lxelsim/package.py to be robust when locating HDF5 by:

  • Explicitly setting HDF5_ROOT and HDF5_DIR (when the HDF5 CMake config dir exists) to the Spack-installed HDF5 prefix.
  • Adding the HDF5 prefix to CMAKE_PREFIX_PATH so CMake's find_package uses the Spack installation first.
  • Providing fallback explicit HDF5_INCLUDE_DIRS and HDF5_LIBRARIES (full paths when available) to avoid missing include directories.
  • Keep existing CMAKE_CXX_STANDARD setting and existing dependencies.

Changes to make (file: packages/lxelsim/package.py):

  • Add imports for os.
  • Add a maintainer ('madbaron').
  • Implement a robust cmake_args method that reads the hdf5 prefix from the Spack spec and appends the cmake args described above.

Updated file contents:

from spack.package import *
from spack.pkg.k4.key4hep_stack import *

import os

class Lxelsim(CMakePackage, Key4hepPackage):
    """LUXE simulation framework"""

    homepage = "https://github.com/LUXEsoftware/lxelsim"
    git      = "https://github.com/LUXEsoftware/lxelsim.git"
    url      = "https://github.com/LUXEsoftware/lxelsim/archive/refs/tags/v00-01.tar.gz"

    maintainers = ['madbaron']

    version('master',  branch='master')

    depends_on('geant4')
    depends_on('doxygen')
    depends_on('glib')
    depends_on('hdf5')

    def cmake_args(self):
        args = []
        args.append(self.define('CMAKE_CXX_STANDARD', '20'))

        # Ensure CMake/Find modules pick up Spack-installed HDF5
        h5 = self.spec['hdf5']
        h5_prefix = str(h5.prefix)

        # If the HDF5 CMake package config directory exists, point CMake to it
        h5_cmake_dir = os.path.join(h5_prefix, 'lib', 'cmake', 'HDF5')
        if os.path.isdir(h5_cmake_dir):
            args.append(self.define('HDF5_DIR', h5_cmake_dir))

        # Always provide HDF5 root and add it to CMAKE_PREFIX_PATH as a robust fallback
        args.append(self.define('HDF5_ROOT', h5_prefix))

        # Extend CMAKE_PREFIX_PATH so find_package searches Spack prefixes first
        # Use semicolon-separated list which CMake expects
        cmake_prefix_path = ';'.join(filter(None, [h5_prefix, str(self.spec.prefix)]))
        args.append(self.define('CMAKE_PREFIX_PATH', cmake_prefix_path))

        # Provide explicit include directory if present (fallback)
        h5_include = os.path.join(h5_prefix, 'include')
        if os.path.isdir(h5_include):
            args.append(self.define('HDF5_INCLUDE_DIRS', h5_include))

        # Provide explicit library paths for common HDF5 libs as a fallback
        lib_dir = os.path.join(h5_prefix, 'lib')
        if os.path.isdir(lib_dir):
            libs = []
            for libname in ('libhdf5.so', 'libhdf5.a', 'libhdf5_hl.so', 'libhdf5_hl.a'):
                lp = os.path.join(lib_dir, libname)
                if os.path.exists(lp):
                    libs.append(lp)
            if libs:
                args.append(self.define('HDF5_LIBRARIES', ';'.join(libs)))

        return args

Rationale:

  • Setting HDF5_DIR/HDF5_ROOT and CMAKE_PREFIX_PATH forces CMake's FindHDF5 or the imported HDF5 targets to reference the Spack-installed HDF5. If the project's CMake uses HDF5::HDF5 targets, the include dirs and link libraries will be provided automatically. The explicit fallbacks (HDF5_INCLUDE_DIRS / HDF5_LIBRARIES) ensure older or non-standard Find modules still get correct paths.

Please open the PR against the repository default branch. If you prefer a specific target branch, tell me which branch to use.

*This pull request was created as a result of the following prompt from Copilot chat.* > Problem: > The CI build failed when configuring lxelsim via Spack because CMake could not find or populate HDF5 include dirs (HDF5_INCLUDE_DIRS) even though HDF5 1.14.6 was present. The existing packages/lxelsim/package.py does not explicitly expose the Spack-installed HDF5 prefix or CMake config dir to the CMake configure step, which allows FindHDF5 to pick up wrong/system HDF5 or miss include/library paths. > > Goal: > Update packages/lxelsim/package.py to be robust when locating HDF5 by: > - Explicitly setting HDF5_ROOT and HDF5_DIR (when the HDF5 CMake config dir exists) to the Spack-installed HDF5 prefix. > - Adding the HDF5 prefix to CMAKE_PREFIX_PATH so CMake's find_package uses the Spack installation first. > - Providing fallback explicit HDF5_INCLUDE_DIRS and HDF5_LIBRARIES (full paths when available) to avoid missing include directories. > - Keep existing CMAKE_CXX_STANDARD setting and existing dependencies. > > Changes to make (file: packages/lxelsim/package.py): > - Add imports for os. > - Add a maintainer ('madbaron'). > - Implement a robust cmake_args method that reads the hdf5 prefix from the Spack spec and appends the cmake args described above. > > Updated file contents: > > ```python > from spack.package import * > from spack.pkg.k4.key4hep_stack import * > > import os > > class Lxelsim(CMakePackage, Key4hepPackage): > """LUXE simulation framework""" > > homepage = "https://github.com/LUXEsoftware/lxelsim" > git = "https://github.com/LUXEsoftware/lxelsim.git" > url = "https://github.com/LUXEsoftware/lxelsim/archive/refs/tags/v00-01.tar.gz" > > maintainers = ['madbaron'] > > version('master', branch='master') > > depends_on('geant4') > depends_on('doxygen') > depends_on('glib') > depends_on('hdf5') > > def cmake_args(self): > args = [] > args.append(self.define('CMAKE_CXX_STANDARD', '20')) > > # Ensure CMake/Find modules pick up Spack-installed HDF5 > h5 = self.spec['hdf5'] > h5_prefix = str(h5.prefix) > > # If the HDF5 CMake package config directory exists, point CMake to it > h5_cmake_dir = os.path.join(h5_prefix, 'lib', 'cmake', 'HDF5') > if os.path.isdir(h5_cmake_dir): > args.append(self.define('HDF5_DIR', h5_cmake_dir)) > > # Always provide HDF5 root and add it to CMAKE_PREFIX_PATH as a robust fallback > args.append(self.define('HDF5_ROOT', h5_prefix)) > > # Extend CMAKE_PREFIX_PATH so find_package searches Spack prefixes first > # Use semicolon-separated list which CMake expects > cmake_prefix_path = ';'.join(filter(None, [h5_prefix, str(self.spec.prefix)])) > args.append(self.define('CMAKE_PREFIX_PATH', cmake_prefix_path)) > > # Provide explicit include directory if present (fallback) > h5_include = os.path.join(h5_prefix, 'include') > if os.path.isdir(h5_include): > args.append(self.define('HDF5_INCLUDE_DIRS', h5_include)) > > # Provide explicit library paths for common HDF5 libs as a fallback > lib_dir = os.path.join(h5_prefix, 'lib') > if os.path.isdir(lib_dir): > libs = [] > for libname in ('libhdf5.so', 'libhdf5.a', 'libhdf5_hl.so', 'libhdf5_hl.a'): > lp = os.path.join(lib_dir, libname) > if os.path.exists(lp): > libs.append(lp) > if libs: > args.append(self.define('HDF5_LIBRARIES', ';'.join(libs))) > > return args > ``` > > Rationale: > - Setting HDF5_DIR/HDF5_ROOT and CMAKE_PREFIX_PATH forces CMake's FindHDF5 or the imported HDF5 targets to reference the Spack-installed HDF5. If the project's CMake uses HDF5::HDF5 targets, the include dirs and link libraries will be provided automatically. The explicit fallbacks (HDF5_INCLUDE_DIRS / HDF5_LIBRARIES) ensure older or non-standard Find modules still get correct paths. > > Please open the PR against the repository default branch. If you prefer a specific target branch, tell me which branch to use.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: madbaron <7315158+madbaron@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix HDF5 configuration issue in lxelsim package Fix HDF5 configuration in lxelsim package to prevent CMake find errors Oct 8, 2025
Copilot AI requested a review from madbaron October 8, 2025 18:40
@madbaron madbaron requested a review from tmadlener October 8, 2025 18:47
@madbaron madbaron marked this pull request as ready for review October 8, 2025 19:18
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

If the underlying reason truly is an hdf5 installation on the system, I think we should try and figure out where that came from and then try to purge that.

Co-authored-by: madbaron <7315158+madbaron@users.noreply.github.com>
@madbaron
Copy link
Member

madbaron commented Oct 8, 2025

@copilot the suggested changes don't work (see for example https://github.com/LUXEsoftware/luxe-spack/actions/runs/18356130853/job/52288696064?pr=2)

Can you focus your changes on removing any system installation of the hdf5 library from the docker image?

Co-authored-by: madbaron <7315158+madbaron@users.noreply.github.com>
Copy link
Author

Copilot AI commented Oct 8, 2025

@copilot the suggested changes don't work (see for example https://github.com/LUXEsoftware/luxe-spack/actions/runs/18356130853/job/52288696064?pr=2)

Can you focus your changes on removing any syste...

Added a Docker build step to explicitly remove system HDF5 installations. The change removes HDF5 packages via apt-get and manually deletes HDF5 files from common system locations (/usr/lib, /usr/include, /usr/local, etc.) for both x86_64 and aarch64 architectures. Commit: 2411b23

Copilot AI changed the title Fix HDF5 configuration in lxelsim package to prevent CMake find errors Fix HDF5 configuration by removing system installations and setting HDF5_ROOT Oct 8, 2025
@madbaron
Copy link
Member

madbaron commented Oct 8, 2025

This is fun, but it's not going well

@madbaron
Copy link
Member

Closing, as @tmadlener fixed this in #3.

Humans 1 - Machines 0

@madbaron madbaron closed this Oct 10, 2025
@madbaron madbaron deleted the copilot/fix-hdf5-configuration-issue branch October 10, 2025 15:38
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.

3 participants