Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Aug 12, 2025

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.cmake file contained hard-coded Windows-specific paths that would break builds on systems without those exact directory structures:

# Before - breaks on most systems
set (CMAKE_INSTALL_PREFIX z:/software)
set (CMAKE_FIND_ROOT_PATH z:/software)
set (ENV{HDF4_ROOT} "c:/Program Files/HDF Group/HDF4/4.2.7")

# After - proper template format
# set (CMAKE_INSTALL_PREFIX /path/to/install/directory)
# set (CMAKE_FIND_ROOT_PATH /path/to/common/prefix)
# set (ENV{HDF4_ROOT} "c:/Program Files/HDF Group/HDF4/4.2.7")

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:

# Before - affects all targets globally (bad practice)
include_directories(${${filter}_INCLUDE_DIRS})

# After - target-specific and safer
if(TARGET netcdf)
  target_include_directories(netcdf PRIVATE ${${filter}_INCLUDE_DIRS})
endif()

3. Outdated CMake Syntax in Find Modules

Modernized multiple Find*.cmake modules that were using deprecated uppercase syntax:

# Before - old CMake 2.x style
SET(Blosc_FOUND FALSE)
IF(Blosc_INCLUDE_DIRS)
  FIND_LIBRARY(...)
ENDIF(Blosc_INCLUDE_DIRS)

# After - modern CMake 3.x style  
set(Blosc_FOUND FALSE)
if(Blosc_INCLUDE_DIRS)
  find_library(...)
endif()

Impact

  • Improved Portability: Builds no longer fail due to hard-coded Windows paths
  • Modern CMake Compatibility: Updated syntax works with current CMake versions
  • Better Encapsulation: Target-specific properties prevent unintended side effects
  • Maintainability: Cleaner, more readable CMake code following current best practices

Files Modified

  • cmake/ConfigUser.cmake - Converted to template format
  • cmake/netcdf_functions_macros.cmake - Fixed target property usage
  • cmake/modules/FindBlosc.cmake - Modernized syntax
  • cmake/modules/FindBz2.cmake - Modernized syntax

The 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.

@Copilot Copilot AI assigned Copilot and WardF Aug 12, 2025
Copy link
Author

Copilot AI commented Aug 12, 2025

@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.

@Copilot Copilot AI changed the title [WIP] CMake Review Fix CMake configuration issues: hard-coded paths, deprecated syntax, and best practices violations Aug 12, 2025
@Copilot Copilot AI requested a review from WardF August 12, 2025 20:26
Copilot finished work on behalf of WardF August 12, 2025 20:26
@WardF WardF requested a review from Copilot August 12, 2025 20:35
Copy link

@Copilot Copilot AI left a 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 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-specific target_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.

@@ -9,7 +9,11 @@ function(set_std_filter filter)
if(NETCDF_ENABLE_FILTER_${upfilter})
# Define a test flag for filter
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
Copy link
Preview

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# Note: This assumes the netcdf target exists and is defined
# Note: This assumes the netcdf target exists and is defined.

Copilot uses AI. Check for mistakes.

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)
Copy link
Preview

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in the comment: 'HFD4' should be 'HDF4'.

Copilot uses AI. Check for mistakes.

@WardF
Copy link
Owner

WardF commented Aug 12, 2025

@K20shores tag

Copy link

@K20shores K20shores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake Review
3 participants