Skip to content

Modernize CMake to use target-scoped constructs (minimum 3.22)#141

Closed
Copilot wants to merge 2 commits intomasterfrom
copilot/update-cmake-implementation
Closed

Modernize CMake to use target-scoped constructs (minimum 3.22)#141
Copilot wants to merge 2 commits intomasterfrom
copilot/update-cmake-implementation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

Replaces deprecated directory-scoped CMake commands with modern target-scoped equivalents across the entire build system.

Minimum version & project

  • Bump cmake_minimum_required from 3.10 to 3.22
  • Add LANGUAGES C CXX to project()

Target-scoped build properties

  • add_definitions()target_compile_definitions() on library targets
  • include_directories()target_include_directories() with PUBLIC visibility for transitive propagation
  • link_libraries() → per-target target_link_libraries() in testdir, ezminc/tests, ezminc/examples
# Before (directory-scoped, affects all targets)
add_definitions(-DHAVE_CONFIG_H)
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/libcommon)
link_libraries(${LIBMINC_LIBRARIES})

# After (target-scoped, propagates via PUBLIC)
target_compile_definitions(${LIBMINC_LIBRARY} PUBLIC HAVE_CONFIG_H)
target_include_directories(${LIBMINC_LIBRARY} PUBLIC ${MINC_INCLUDE_DIRS})
target_link_libraries(volume_test ${LIBMINC_LIBRARIES})

Other modernizations

  • Manual -fPIC flag injection → CMAKE_POSITION_INDEPENDENT_CODE
  • set(CMAKE_MODULE_PATH ...)list(APPEND CMAKE_MODULE_PATH ...)
  • FindNETCDF.cmake / FindNIFTI.cmake: replace manual found-logic with find_package_handle_standard_args, drop hardcoded search paths
  • UseLIBMINC.cmake.in: remove deprecated include_directories() and link_directories()

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

- Update cmake_minimum_required from 3.10 to 3.22
- Add LANGUAGES to project() command
- Use list(APPEND ...) for CMAKE_MODULE_PATH
- Replace -fPIC flags with CMAKE_POSITION_INDEPENDENT_CODE
- Replace add_definitions() with target_compile_definitions()
- Replace include_directories() with target_include_directories()
- Replace link_libraries() with per-target target_link_libraries()
- Modernize FindNETCDF.cmake and FindNIFTI.cmake with find_package_handle_standard_args
- Remove deprecated include_directories/link_directories from UseLIBMINC.cmake.in

Co-authored-by: gdevenyi <3001850+gdevenyi@users.noreply.github.com>
Copilot AI changed the title [WIP] Update cmake implementation to modern constructs for version 3.22 Modernize CMake to use target-scoped constructs (minimum 3.22) Mar 6, 2026
@gdevenyi gdevenyi marked this pull request as ready for review March 6, 2026 21:40
Copilot AI review requested due to automatic review settings March 6, 2026 21:40
Copy link
Copy Markdown
Contributor

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 the libminc build system by replacing directory-scoped CMake settings with target-scoped properties and updating several custom find modules, aligned with a new minimum CMake version (3.22).

Changes:

  • Replaces directory-scoped link_libraries() usage in tests/examples with per-target target_link_libraries().
  • Introduces target-scoped include paths and compile definitions for core targets (e.g., minc2, minc_io).
  • Modernizes FindNETCDF.cmake / FindNIFTI.cmake to use find_package_handle_standard_args() and simplifies CMAKE_MODULE_PATH handling.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
CMakeLists.txt Bumps CMake minimum, switches to target-scoped include dirs/compile defs, enables PIC via CMAKE_POSITION_INDEPENDENT_CODE.
testdir/CMakeLists.txt Replaces directory-wide linking with explicit per-test target linking.
ezminc/CMakeLists.txt Makes minc_io include paths/compile defs target-scoped.
ezminc/tests/CMakeLists.txt Switches to per-target linking for test executables.
ezminc/examples/CMakeLists.txt Switches to per-target linking for example executables.
cmake-modules/FindNETCDF.cmake Uses FindPackageHandleStandardArgs for standardized discovery reporting.
cmake-modules/FindNIFTI.cmake Uses FindPackageHandleStandardArgs for standardized discovery reporting.
UseLIBMINC.cmake.in Removes legacy include/link directory side effects and adds guidance comments.
Comments suppressed due to low confidence (2)

CMakeLists.txt:490

  • When building the static library variant (LIBMINC_BUILD_SHARED_LIBS), MINC1 support currently only adds NETCDF include/link settings to the shared target. The static target (${LIBMINC_LIBRARY_STATIC}) may fail to compile (missing netcdf headers) and/or link (missing netcdf lib) when LIBMINC_MINC1_SUPPORT is ON. Apply NETCDF include dirs and NETCDF_LIBRARY to the static target as well, and ensure the NETCDF link is attached to the static target (not the shared one) inside the UNIX/LIBMINC_BUILD_SHARED_LIBS block.
  if(LIBMINC_BUILD_SHARED_LIBS)
    add_library(${LIBMINC_LIBRARY_STATIC} STATIC ${minc_LIB_SRCS} ${minc_HEADERS} ${volume_io_LIB_SRCS} ${volume_io_HEADERS} )
    target_include_directories(${LIBMINC_LIBRARY_STATIC} PUBLIC ${MINC_INCLUDE_DIRS} ${HDF5_INCLUDE_DIRS})
    target_compile_definitions(${LIBMINC_LIBRARY_STATIC} PUBLIC ${MINC_COMPILE_DEFINITIONS})
    target_link_libraries(${LIBMINC_LIBRARY_STATIC} ${HDF5_LIBRARY} ${NIFTI_LIBRARIES} ${ZLIB_LIBRARY} ${RT_LIBRARY} m ${CMAKE_DL_LIBS} )
    if(LIBMINC_MINC1_SUPPORT)
      target_link_libraries(${LIBMINC_LIBRARY} ${NETCDF_LIBRARY})
    endif()

UseLIBMINC.cmake.in:17

  • UseLIBMINC.cmake.in no longer applies LIBMINC_INCLUDE_DIRS/LIBMINC_LIBRARY_DIRS to consumers (include_directories/link_directories were removed), but the installed package does not appear to install an EXPORT file / imported targets either. This makes the advertised “backward-compatible variable-based configuration” effectively non-functional for downstream projects that still do include(${LIBMINC_USE_FILE}) and link via ${LIBMINC_LIBRARIES}/${EZMINC_LIBRARIES}. Either restore the legacy include/link directory setup here, or complete the migration by installing/exporting targets and updating the config/use-file to rely on them.
# Usage: Downstream projects should prefer using target_link_libraries()
# with the imported LIBMINC targets. This file provides backward-compatible
# variable-based configuration.

# Legacy include/link directory variables for projects not using
# target-based dependencies directly:
# LIBMINC_INCLUDE_DIRS, LIBMINC_LIBRARY_DIRS, LIBMINC_LIBRARIES

if(HAVE_MINC1)
  add_definitions( -DHAVE_MINC1=1)
endif()

if(HAVE_MINC2)
  set(MINC2 "1")
  add_definitions( -DMINC2=1 -DHAVE_MINC2=1)
endif()


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vfonov
Copy link
Copy Markdown
Member

vfonov commented Mar 7, 2026

@gdevenyi what is this?

@vfonov
Copy link
Copy Markdown
Member

vfonov commented Mar 7, 2026

i was working on my own changes in https://github.com/BIC-MNI/libminc/tree/modernize_cmake

@gdevenyi
Copy link
Copy Markdown
Contributor

gdevenyi commented Mar 7, 2026

i was working on my own changes in modernize_cmake

Sorry I didn't see that. I am indeed also trying to modernize the cmake because it won't build on the newest versions of cmake. I'll leave this here for you to cherry-pick any changes, but I will otherwise defer to your branch.

@gdevenyi
Copy link
Copy Markdown
Contributor

Already fixed.

@gdevenyi gdevenyi closed this Mar 24, 2026
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.

4 participants