Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new Spack package recipe for eic-opticks, which provides GPU-accelerated optical photon simulation using NVIDIA OptiX. The package currently tracks the main branch until tagged releases become available upstream.
Changes:
- Added a new Spack package recipe for eic-opticks with CMake and CUDA build system support
- Defined dependencies including CUDA, Geant4, OptiX, and various graphics libraries
- Set up package metadata including license, maintainer, and homepage information
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from spack_repo.builtin.build_systems.cmake import CMakePackage | ||
| from spack_repo.builtin.build_systems.cuda import CudaPackage | ||
|
|
There was a problem hiding this comment.
The import statements for CMakePackage and CudaPackage are using an unusual pattern. In this repository, packages that are not extending builtin packages should import these classes via the wildcard import from spack.package import * rather than explicitly importing from spack_repo.builtin.build_systems.
The explicit imports from spack_repo.builtin are only used when extending existing builtin packages (as seen in packages like geant4, dd4hep, acts, etc.). Since eic-opticks is a new package that doesn't extend a builtin, it should follow the standard pattern used by other new packages in this repository.
Remove lines 5-6 and rely on the wildcard import on line 8 to provide CMakePackage and CudaPackage, as demonstrated in packages/jana2/package.py which also uses CudaPackage.
| from spack_repo.builtin.build_systems.cmake import CMakePackage | |
| from spack_repo.builtin.build_systems.cuda import CudaPackage |
wdconinc
left a comment
wdconinc
left a comment
There was a problem hiding this comment.
Some other comments from spack/spack#49990 still apply as well.
| """GPU-Accelerated Optical Photon Simulation using NVIDIA OptiX""" | ||
|
|
||
| homepage = "https://github.com/bnlnpps/eic-opticks" | ||
| git = "https://github.com/bnlnpps/eic-opticks.git" |
There was a problem hiding this comment.
Needs to be on the EIC organization on GitHub to allow/encourage co-development by EIC community.
This is also problematic since it is a fork of another project with changes applied in the original source tree. That's not a sustainable situation. Changes should be upstreamed and we should package opticks, or we should be able to add the EIC specific aspects without needing to pin a specific upstream commit.
|
|
||
| maintainers("plexoos") | ||
|
|
||
| version("main", branch="main") |
There was a problem hiding this comment.
This needs released versions. No moving targets as the only version we can install.
3 participants
Briefly, what does this PR introduce?
This PR adds a new Spack package recipe for eic-opticks, enabling it to be built and installed via Spack.
The package currently tracks the main branch, as no tagged releases are available yet. Once a stable version is tagged
upstream, the recipe can be updated to use versioned releases.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No
Does this PR change default behavior?
No