Skip to content
Snippets Groups Projects

M300433/cmake improvements and define cleanups

Merged M300433/cmake improvements and define cleanups
m300433/cmake_improvements_and_define_cleanups into develop
4 unresolved threads
Merged Oliver Heidmann requested to merge m300433/cmake_improvements_and_define_cleanups into develop
4 unresolved threads

Merge request reports

Merge request pipeline #97218 passed

Stage: basic
Stage: levante

Merge request pipeline passed for c8067751

Approval is optional

Merged by Oliver HeidmannOliver Heidmann 1 month ago (Feb 17, 2025 4:50pm UTC)

Merge details

  • Changes merged into develop with c8067751.
  • Did not delete the source branch.

Pipeline #97317 passed

Stage: basic
Stage: levante

Pipeline passed for c8067751 on develop

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
app/CMakeLists.txt
2 2 cdi.c printinfo.c
3 3 )
4 4
5 target_include_directories(cdi PUBLIC
5 target_include_directories(cdi PRIVATE
6 6 $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>
7 7 $<INSTALL_INTERFACE:include
8 8 )
9 target_link_libraries(cdi PUBLIC cdilib netCDF::netcdf pthread)
9
10 target_link_libraries(cdi PRIVATE cdilib netCDF::netcdf pthread)
11 target_include_directories(cdi PRIVATE "${PROJECT_BINARY_DIR}/src/config.h")
  • Mathieu Westphal
    Mathieu Westphal @b383306 started a thread on the diff
    • CMakeLists.txt
    31 message(VERBOSE "added compile definition HAVE_NETCDF4=${netCDF_FOUND}")
    32 add_compile_definitions(HAVE_LIBNETCDF=${netCDF_FOUND})
    33 add_compile_definitions(HAVE_NETCDF4=${netCDF_FOUND})
    34 else()
    35 message(WARNING "netcdf not found, compiling without netcdf")
    36 endif ()
    62 option(enable_netcdf "Use the netcdf library [default=ON]" ON)
    63 if(${enable_netcdf} OR netCDF_ROOT )
    64 find_package(netCDF COMPONENTS C REQUIRED)
    65 if (TARGET netCDF::netcdf)
    66 list(APPEND cdi_compile_defs
    67 HAVE_LIBNETCDF=${netCDF_FOUND}
    68 HAVE_LIBNC_DAP=${netCDF_FOUND}
    69 HAVE_NETCDF4=${netCDF_FOUND}
    70 HAVE_LIBGRIB_API=${netCDF_FOUND}
    71 HAVE_LIBGRIB=${netCDF_FOUND}
  • Mathieu Westphal
    Mathieu Westphal @b383306 started a thread on the diff
    • CMakeLists.txt
    34 else()
    35 message(WARNING "netcdf not found, compiling without netcdf")
    36 endif ()
    62 option(enable_netcdf "Use the netcdf library [default=ON]" ON)
    63 if(${enable_netcdf} OR netCDF_ROOT )
    64 find_package(netCDF COMPONENTS C REQUIRED)
    65 if (TARGET netCDF::netcdf)
    66 list(APPEND cdi_compile_defs
    67 HAVE_LIBNETCDF=${netCDF_FOUND}
    68 HAVE_LIBNC_DAP=${netCDF_FOUND}
    69 HAVE_NETCDF4=${netCDF_FOUND}
    70 HAVE_LIBGRIB_API=${netCDF_FOUND}
    71 HAVE_LIBGRIB=${netCDF_FOUND}
    72 )
    73 else()
    74 message(WARNING "netcdf target not found, compiling without netcdf")
    • Mathieu Westphal
      Mathieu Westphal @b383306 ·
      Contributor

      I think this code is not reachable, as the find_package is REQUIRED.

    • M
      Oliver Heidmann @m300433 ·
      Author Maintainer

      Why is (enable_netcdf OR netCDF_ROOT) not enough to be able to reach the find_package? I have trouble following you in some cases. Please explain in short why something does not work.

    • Mathieu Westphal
      Mathieu Westphal @b383306 ·
      Contributor

      find_package(netCDF COMPONENTS C REQUIRED) will either find netCDF::netcdf or fail with an error right away because of the REQUIRED keyword, which means if (TARGET netCDF::netcdf) is always true, at least according to FindNetCDF.cmake documentation and logic. I just quickly read it to make sure.

      By the way it is considered a good practice to use REQUIRED, so I'm not advocating to remove it, however I do think the else() and warning below cannot be reached and can be removed.

    • M
      Oliver Heidmann @m300433 ·
      Author Maintainer

      Oh you mean the else. Ok that makes sense. Thanks for clarifying.

    • Please register or sign in to reply
    Please register or sign in to reply

    Imprint | Privacy Policy