-
Notifications
You must be signed in to change notification settings - Fork 58
update cmake build #2019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
update cmake build #2019
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the CMake build system by updating the minimum required CMake version from 3.5.2 to 3.10 and adopting newer CMake conventions. The update removes obsolete functions and replaces them with modern alternatives, while also improving dependency management for NetCDF and PnetCDF libraries.
Changes:
- Updated minimum CMake version requirement from 3.5.2 to 3.10 across all CMakeLists.txt files
- Replaced uppercase CMake commands (e.g.,
ADD_DEFINITIONS,SET) with lowercase equivalents (e.g.,add_definitions,set) - Replaced deprecated
find_packagemodules for NetCDF and PnetCDF with pkg-config and direct configuration script detection - Added explicit library directory linking via
link_directories()for NetCDF and PnetCDF dependencies
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Updated minimum version, modernized commands, replaced FindNetCDF/FindPnetCDF with pkg-config and pnetcdf-config detection |
| src/clib/CMakeLists.txt | Updated minimum version, fixed PnetCDF library variable reference |
| src/flib/CMakeLists.txt | Updated minimum version, added NetCDF variable propagation from cache |
| src/gptl/CMakeLists.txt | Updated minimum version, modernized formatting with consistent spacing |
| src/CMakeLists.txt | Added blank lines for improved readability |
| tests/*/CMakeLists.txt | Added link_directories for NetCDF and PnetCDF library paths |
| examples/*/CMakeLists.txt | Modernized commands to lowercase, added NetCDF library directory linking |
| cmake/FindNetCDF.cmake | Removed obsolete custom find module |
| cmake/FindPnetCDF.cmake | Removed obsolete custom find module |
| cmake/LibFind.cmake | Fixed find_package_handle_standard_args to use main package name |
| doc/CMakeLists.txt | Modernized commands to lowercase |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CMakeLists.txt
Outdated
| # Always detect NetCDF C and PnetCDF for C library | ||
| find_package(NetCDF QUIET COMPONENTS C) | ||
| if(NOT NetCDF_C_FOUND) | ||
| find_package(PkgConfig REQUIRED) | ||
| pkg_check_modules(NetCDF_C REQUIRED IMPORTED_TARGET "netcdf") | ||
| if(NetCDF_C_FOUND OR NetCDF_C_INCLUDE_DIRS) | ||
| set(NetCDF_C_FOUND TRUE CACHE BOOL "NetCDF C found via pkg-config" FORCE) | ||
| endif() | ||
| endif() |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NetCDF C detection code is duplicated (lines 67-75 and 333-341). This duplicate check should be removed and the first detection block should be used consistently throughout the file.
CMakeLists.txt
Outdated
| endif() | ||
| endif() | ||
|
|
||
| find_program(PNETCDF_CONFIG pnetcdf-config REQUIRED) |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REQUIRED keyword for find_program causes a CMake fatal error if pnetcdf-config is not found, but there's no informative error message guiding users on how to resolve this. Consider adding a custom error message or making it conditional based on WITH_PNETCDF option.
| find_program(PNETCDF_CONFIG pnetcdf-config REQUIRED) | |
| find_program(PNETCDF_CONFIG pnetcdf-config) | |
| if(NOT PNETCDF_CONFIG) | |
| message(FATAL_ERROR | |
| "Could not find 'pnetcdf-config' in your PATH.\n" | |
| "Please install PnetCDF (which provides the 'pnetcdf-config' tool) " | |
| "or disable PnetCDF support by configuring with -DWITH_PNETCDF=OFF." | |
| ) | |
| endif() |
| # Set include and library variables from top-level CACHE | ||
| set(PnetCDF_C_INCLUDE_DIRS "${PNETCDF_INCLUDEDIR}") | ||
| set(PnetCDF_C_LIBRARIES "${PNETCDF_LIBS}") |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable naming inconsistency: PnetCDF_C_INCLUDE_DIRS and PnetCDF_C_LIBRARIES use component-style naming (with C), but are being set from PNETCDF_INCLUDEDIR and PNETCDF_LIBS which don't include the C component. This creates confusion about which variables represent what. Consider using consistent naming throughout.
| choke me | ||
| #endif | ||
| int main() {return 0;}" PIO_HAS_PAR_FILTERS) | ||
| int main() {return 0;}" HAVE_PAR_FILTERS) |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable naming inconsistency: line 491 sets HAVE_PAR_FILTERS but line 493 also sets HAVE_PAR_FILTERS, while line 590 checks HAVE_PAR_FILTERS and sets PIO_HAS_PAR_FILTERS. This dual naming convention (HAVE_PAR_FILTERS vs PIO_HAS_PAR_FILTERS) should be unified for clarity.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Brings minimum cmake version to 3.10 and removes some obsolete functions.