Feature/vcpkg - #7
Open
ericwait wants to merge 23 commits into
Open
Conversation
Reviewer's GuideThis PR introduces vcpkg packaging support alongside CI enhancements, integrates GitVersion-based version and install/export in CMake, restructures CUDA, MEX and Python CMake modules for consistency, and standardizes exception handling in CUDA kernels. Class diagram for updated CMake build targets and interfacesclassDiagram
class HydraCudaStatic {
+STATIC library
+CUDA kernels
+OpenMP support
+Install rules
}
class HydraMex {
+MODULE library
+Matlab MEX interface
+Depends on HydraCudaStatic
+Custom output name
+Platform-specific suffix
}
class HydraPy {
+MODULE library
+Python interface
+Depends on HydraCudaStatic
+Custom output name
+Platform-specific suffix
}
HydraMex --|> HydraCudaStatic : links to
HydraPy --|> HydraCudaStatic : links to
Class diagram for standardized exception handling in CUDA kernelsclassDiagram
class ImageChunk {
+sendROI(...)
+throws std::runtime_error
}
class Kernel {
+getOffsetCopy(...)
+throws std::runtime_error
}
class CudaKernels {
+cAddTwoImages(...)
+cElementWiseDifference(...)
+cClosure(...)
+cEntropyFilter(...)
+cGaussian(...)
+cHighPassFilter(...)
+cIdentityFilter(...)
+cMaxFilter(...)
+cMeanAndVariance(...)
+cMeanFilter(...)
+cMedianFilter(...)
+cMinFilter(...)
+cMinMax(...)
+cMultiplySum(...)
+cNLMeans(...)
+cOpener(...)
+cStdFilter(...)
+cSum(...)
+cVarFilter(...)
+cWienerFilter(...)
+All: throw std::runtime_error on ROI failure
}
Flow diagram for dynamic vcpkg version update in CIflowchart TD
A[Start CI Job]
B[Run GitVersion to get version info]
C[Run update-vcpkg-files.py]
D[Update vcpkg.json version]
E[Update portfile.cmake REF and SHA512]
F[Continue build]
A --> B --> C --> D --> E --> F
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive vcpkg packaging support for the Hydra Image Processor library. It sets up a complete build and packaging infrastructure with modern CMake practices, automated CI workflows, and vcpkg integration for distribution.
- Adds vcpkg packaging files with dependency metadata and CMake configuration
- Modernizes the CMake build system with improved structure and proper installation targets
- Implements GitHub Actions CI workflow with GitVersion integration for automated builds
Reviewed Changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
vcpkg.json / portfile.cmake |
vcpkg package definition and build configuration |
CMakeLists.txt |
Modernized root CMake with versioning, dependencies, and package config |
src/c/Cuda/CMakeLists.txt |
Restructured CUDA library build with proper OpenMP and installation setup |
.github/workflows/hydra-ci.yml |
Complete CI pipeline with GitVersion, multi-platform builds, and artifact uploads |
scripts/update-vcpkg-files.py |
Python utility for dynamic version updates during CI |
Multiple .cuh files |
Bug fixes for missing throw statements in error handling |
Comments suppressed due to low confidence (1)
CMakeLists.txt:36
- CUDA architecture '103' and '121' do not exist. Valid CUDA compute capabilities include values like 52, 61, 70, 75, 80, 86, 89, 90. These invalid architectures will cause compilation failures.
set(CMAKE_CUDA_ARCHITECTURES "89;90;103;121"
| vcpkg_from_github( | ||
| OUT_SOURCE_PATH SOURCE_PATH | ||
| REPO ericwait/hydra-image-processor | ||
| REF v${GITVERSION_SEMVER} # you can pass this in via `cmake -D` |
There was a problem hiding this comment.
The REF should use a fixed version tag or commit SHA for vcpkg stability. Using a variable like ${GITVERSION_SEMVER} will cause build failures as this variable is not available in the vcpkg context.
Suggested change
| REF v${GITVERSION_SEMVER} # you can pass this in via `cmake -D` | |
| REF v1.2.3 # Replace with the actual fixed version tag or commit SHA |
| OUT_SOURCE_PATH SOURCE_PATH | ||
| REPO ericwait/hydra-image-processor | ||
| REF v${GITVERSION_SEMVER} # you can pass this in via `cmake -D` | ||
| SHA512 0 # This must be updated |
There was a problem hiding this comment.
The SHA512 value is set to 0, which is invalid. This must be updated with the actual SHA512 hash of the source archive for vcpkg to verify the download integrity.
Suggested change
| SHA512 0 # This must be updated | |
| SHA512 <calculated_sha512_hash> # Replace with the actual hash |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ericwait - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Redundant 'throw' keyword in exception statement. (link)
General comments:
- Consider consolidating the repetitive binding build patterns in the MATLAB and Python CMakeLists into a shared function or macro to reduce duplication and maintenance overhead.
- The update-vcpkg-files.py script downloads the entire source tarball to compute the SHA512; you could improve CI performance and reliability by leveraging vcpkg’s built-in hash fetching or the GitHub API instead of re-downloading the full archive.
- The new portfile.cmake pins HEAD_REF to main and uses a fixed REF pattern—consider parameterizing or documenting the expected tag format so future version bumps won’t break the vcpkg port build.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the repetitive binding build patterns in the MATLAB and Python CMakeLists into a shared function or macro to reduce duplication and maintenance overhead.
- The update-vcpkg-files.py script downloads the entire source tarball to compute the SHA512; you could improve CI performance and reliability by leveraging vcpkg’s built-in hash fetching or the GitHub API instead of re-downloading the full archive.
- The new portfile.cmake pins HEAD_REF to main and uses a fixed REF pattern—consider parameterizing or documenting the expected tag format so future version bumps won’t break the vcpkg port build.
## Individual Comments
### Comment 1
<location> `src/c/Cuda/_TemplateKernel.cuh:75` </location>
<code_context>
{
if (!chunks[i].sendROI(imageIn, deviceImages.getCurBuffer()))
- std::runtime_error("Error sending ROI to device!");
+ throw throw std::runtime_error("Error sending ROI to device!");
deviceImages.setAllDims(chunks[i].getFullChunkSize());
</code_context>
<issue_to_address>
Redundant 'throw' keyword in exception statement.
'throw throw std::runtime_error(...)' will cause a compilation error. Use only 'throw std::runtime_error(...)'.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…r Windows and Linux
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Owner
Author
|
Retargeted to Heads-up: since this branch was cut, v4.0.0 moved the build to scikit-build-core (single |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✅ Add vcpkg Packaging Support for Hydra
Summary
This PR introduces initial support for building and packaging the Hydra Image Processor library via vcpkg. It includes:
vcpkg.jsonwith dependencies and metadataportfile.cmakethat fetches the GitHub repo usingvcpkg_from_githubvcpkg.jsonduring CIhydra-ci.yml) workflow:workflow_dispatchand push support forfeature/vcpkgTesting
vcpkg install hydra --overlay-ports=portscompletes successfullyfeature/vcpkg(both Windows and Linux)vcpkg.jsonNext Steps
Once merged:
portfile.cmakex64-windows(non-static),x64-linux, etc.Summary by Sourcery
Enable vcpkg support and improve build/packaging infrastructure by enhancing CMake scripts, integrating GitVersion, standardizing CUDA error handling, and adding a comprehensive GitHub Actions CI workflow for building, packaging, and releasing Hydra.
New Features:
Enhancements:
CI:
Chores: