Fix CMake configuration issues: hard-coded paths, deprecated syntax, and best practices violations#5
Fix CMake configuration issues: hard-coded paths, deprecated syntax, and best practices violations#5
Conversation
|
@WardF 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
…est practices violations Co-authored-by: WardF <4324946+WardF@users.noreply.github.com>
…ted syntax Co-authored-by: WardF <4324946+WardF@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes CMake configuration files to address portability issues and bring the build system up to current standards. The changes replace hard-coded Windows paths with template placeholders, update deprecated CMake syntax to modern conventions, and improve target encapsulation.
Key changes include:
- Converting ConfigUser.cmake to a template format with commented-out examples instead of hard-coded paths
- Replacing global
include_directories()with target-specifictarget_include_directories() - Modernizing CMake syntax from uppercase 2.x style to lowercase 3.x conventions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmake/ConfigUser.cmake | Converted hard-coded Windows paths to commented template examples |
| cmake/netcdf_functions_macros.cmake | Replaced global include_directories with target-specific approach |
| cmake/modules/FindBlosc.cmake | Updated from uppercase CMake 2.x syntax to modern lowercase conventions |
| cmake/modules/FindBz2.cmake | Updated from uppercase CMake 2.x syntax to modern lowercase conventions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if(${filter}_FOUND) | ||
| include_directories(${${filter}_INCLUDE_DIRS}) | ||
| # Use target-specific include directories instead of global ones | ||
| # Note: This assumes the netcdf target exists and is defined |
There was a problem hiding this comment.
The comment should clarify what happens if the netcdf target doesn't exist, since the include directories won't be set in that case, which could lead to build failures.
| # Note: This assumes the netcdf target exists and is defined | |
| # Note: This assumes the netcdf target exists and is defined. |
| set (ENV{HDF4_ROOT} "c:/Program Files/HDF Group/HDF4/4.2.7") | ||
| set (ENV{HDF5_ROOT} "c:/Program Files/HDF Group/HDF5/1.8.8") | ||
| set (ENV{ZLIB_ROOT} "$ENV{HDF5_ROOT}") | ||
| # Location of HDF4, HDF5 and zlib (example Windows paths) |
There was a problem hiding this comment.
There's a typo in the comment: 'HFD4' should be 'HDF4'.
|
@K20shores tag |
This PR addresses multiple critical issues found during the CMake configuration review that were causing portability problems and compatibility issues with modern CMake versions.
Issues Fixed
1. Hard-coded Windows Paths in ConfigUser.cmake
The
ConfigUser.cmakefile contained hard-coded Windows-specific paths that would break builds on systems without those exact directory structures:2. Deprecated CMake Practices in netcdf_functions_macros.cmake
Fixed use of deprecated
include_directories()which affects all targets globally, replacing it with modern target-specific approach:3. Outdated CMake Syntax in Find Modules
Modernized multiple Find*.cmake modules that were using deprecated uppercase syntax:
Impact
Files Modified
cmake/ConfigUser.cmake- Converted to template formatcmake/netcdf_functions_macros.cmake- Fixed target property usagecmake/modules/FindBlosc.cmake- Modernized syntaxcmake/modules/FindBz2.cmake- Modernized syntaxThe changes maintain backward compatibility while bringing the CMake configuration up to modern standards.
Fixes #4.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.