Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the nGITool project to use Conan for dependency management, replacing custom Find*.cmake modules and manual dependency handling. The changes include adding Conan profiles for multiple platforms (Windows MSVC, macOS x64/ARM, Linux GCC), a comprehensive conanfile.py, and extensive CMakeLists.txt refactoring. The PR also fixes several critical bugs including incorrect loop variables, missing return statements, and improper exception handler ordering.
- Introduces Conan-based dependency management with conanfile.py and platform-specific profiles
- Fixes critical bugs: loop variable error in nGIProcessor.cpp, missing return in ProjectionReader.cpp, and LoadLibrary/LoadLibraryW mismatch
- Refactors CMakeLists.txt files to unify cross-platform builds and remove platform-specific conditionals
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| profiles/windows_msvc_17_release | Adds Windows MSVC 17 Conan profile with C++20 standard |
| profiles/macos_x64_clang_14_release | Adds macOS x64 Conan profile for Apple Clang 14 |
| profiles/macos_arm_clang_15_release | Adds macOS ARM64 Conan profile for Apple Clang 15 |
| profiles/linux_gcc_11_testing | Adds Linux GCC 11 testing profile with sanitizers |
| profiles/linux_gcc_11_release | Adds Linux GCC 11 release Conan profile |
| pixi.toml | Adds Pixi configuration with Python dependencies and tools |
| frameworks/ngi/nGIPreprocessing/src/nGISpotClean.cpp | Marks unused parameters with comment syntax |
| frameworks/ngi/nGIPreprocessing/src/nGIPreprocessing.cpp | Reorders exception handlers from specific to general |
| frameworks/ngi/nGIPreprocessing/src/nGILogNorm.cpp | Marks unused configuration parameters |
| frameworks/ngi/nGIPreprocessing/src/nGIISSfilter.cpp | Aligns parameter assignments and marks unused parameters |
| frameworks/ngi/nGIPreprocessing/CMakeLists.txt | Removes platform-specific include paths, unifies linking |
| frameworks/ngi/nGIFramework/src/ngigenerator.cpp | Marks unused parameters in GeneratePhaseSteps |
| frameworks/ngi/nGIFramework/src/nGIProcessor.cpp | Fixes critical loop bug, adds static_cast for size comparisons |
| frameworks/ngi/nGIFramework/src/nGIEngine.cpp | Marks unused ROI parameter |
| frameworks/ngi/nGIFramework/src/nGIConfig.cpp | Adds second parameter "estimators" to ModuleConfig constructor |
| frameworks/ngi/nGIFramework/src/ProjectionReader.cpp | Fixes missing return statement and reorders exception handlers |
| frameworks/ngi/nGIFramework/src/ModuleItem.cpp | Fixes LoadLibrary to LoadLibraryW for wide string compatibility |
| frameworks/ngi/nGIFramework/src/EstimatorBase.cpp | Changes loop variables from int to auto, comments out unused variables |
| frameworks/ngi/nGIFramework/include/nGIEngine.h | Marks unused Progress function parameters |
| frameworks/ngi/nGIFramework/include/PreprocModuleBase.h | Marks unused parameters in virtual functions |
| frameworks/ngi/nGIFramework/CMakeLists.txt | Unifies target_link_libraries across platforms, removes NEXUS |
| frameworks/ngi/nGIEstimators/src/nGIStandardEstimator.cpp | Marks unused parameters parameter |
| frameworks/ngi/nGIEstimators/src/nGIPenalizedMLEstimator.cpp | Marks unused parameters and bCompletePeriod |
| frameworks/ngi/nGIEstimators/src/nGILSEstimator.cpp | Marks unused parameters in Configure and ProcessCore |
| frameworks/ngi/nGIEstimators/src/nGIEstimators.cpp | Reorders exception handlers to catch specific before general |
| frameworks/ngi/nGIEstimators/src/nGIBLUEEstimator.cpp | Marks unused parameters and comments out unused variable M1 |
| frameworks/ngi/nGIEstimators/CMakeLists.txt | Unifies linking, adds OpenBLAS and armadillo dependencies |
| conanfile.py | Adds comprehensive Conan recipe with dependencies and build configuration |
| cmake/FindNexus.cmake | Removes custom Nexus finder (now using Conan) |
| cmake/FindFFTW.cmake | Removes custom FFTW finder (now using Conan) |
| cmake/FindCFITSIO.cmake | Removes custom CFITSIO finder (now using Conan) |
| applications/ngitool/src/ngimainwindow.cpp | Reorders exception handlers, comments out unused variables |
| applications/ngitool/CMakeLists.txt | Fixes MuhRec references, adds RPATH configuration |
| CMakeLists.txt | Major refactoring: bumps minimum CMake to 3.30, adds Conan package finding, configures cross-platform paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@anderskaestner I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@anderskaestner I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@anderskaestner I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@anderskaestner I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.