From b1ec9641b732615828d1e61189162bd768464de1 Mon Sep 17 00:00:00 2001 From: Anthony Islas <128631809+islas@users.noreply.github.com> Date: Fri, 11 Oct 2024 17:05:57 -0700 Subject: [PATCH] CMake build no longer uses generator expressions in defines (#2056) TYPE: bug fix KEYWORDS: cmake, compilation SOURCE: internal DESCRIPTION OF CHANGES: Problem: The use of generator expressions in the defines compacts the logic neatly but removes the ability to evaluate these conditionals at configuration time. As such, assumptions must either be made or defines wholly dropped when adding configure-time commands like C preprocessing, both of which are wrong. Solution: Switch the logic to a more verbose `if()`-style that guarantees defines that can be known at configure time are resolved. LIST OF MODIFIED FILES: M CMakeLists.txt M cmake/c_preproc.cmake RELEASE NOTE: CMake build no longer uses generator expressions in defines --- CMakeLists.txt | 246 +++++++++++++++++++++++++++++------------- cmake/c_preproc.cmake | 9 +- 2 files changed, 175 insertions(+), 80 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d1a1297f2f..e719518246 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -619,55 +619,8 @@ add_compile_definitions( DWORDSIZE=${DWORDSIZE} LWORDSIZE=${LWORDSIZE} RWORDSIZE=${RWORDSIZE} - # Only define if set, this is to use #ifdef/#ifndef preprocessors - # in code since cmake cannot handle basically any others :( - # https://gitlab.kitware.com/cmake/cmake/-/issues/17398 - $<$:WRF_CHEM=$> - $<$:BUILD_CHEM=$> - $<$:WRF_CMAQ=$> - $<$,$>:WRF_KPP=$> - $<$:WRF_DFI_RADAR=$> - $<$:WRF_TITAN=$> - $<$:WRF_MARS=$> - $<$:WRF_VENUS=$> - $<$:WRF_HYDRO=$> - - # Because once again we need two defines to control one thing - $<$:WRF_USE_CTSM=$> - $<$>:WRF_USE_CLM> - - # If force classic or no nc-4 support enable classic - $<$,$>>:NETCDF_classic=1> - $<$,$>>:WRFIO_NCD_NO_LARGE_FILE_SUPPORT=1> - # May need a check for WRFIO_ncdpar_LARGE_FILE_SUPPORT - - # Now set the opposite in different defines, because why not :) - $<$>,$>:USE_NETCDF4_FEATURES=1> - $<$>,$>:WRFIO_NCD_LARGE_FILE_SUPPORT=1> - - # Could simplify logic to just check if RPC is available but to be explicit - # Does this actually need to check for EM_CORE (Config.pl:443) - # not enable terran or not rpc_found do - # not ( enable terrain and rpc_found ) - $<$,$>>:LANDREAD_STUB> - $<$:TERRAIN_AND_LANDUSE> - - - $<$:USE_ALLOCATABLES> - $<$:wrfmodel> - $<$:GRIB1> - $<$:INTIO> - $<$:KEEP_INT_AROUND> - $<$:LIMIT_ARGS> - - #!TODO Always defined - fix the ambiguous english in these BUILD_*_FAST defines - BUILD_RRTMG_FAST=$ - BUILD_RRTMK=$ - BUILD_SBM_FAST=$ - SHOW_ALL_VARS_USED=$ - - # Alwasys set - NMM_CORE=$ + + NMM_MAX_DIM=2600 NETCDF @@ -675,33 +628,180 @@ add_compile_definitions( NONSTANDARD_SYSTEM_SUBR EM_CORE=${EM_CORE} - WRFPLUS=$> - DA_CORE=$,$>> - # DFI_RADAR=$ - - # Nesting options - $<$:MOVE_NESTS> - $<$>:VORTEX_CENTER> - - # Configuration checks - $<$>:NO_IEEE_MODULE> - $<$>:NO_ISO_C_SUPPORT> - # If flush fails, check if we can fall back to fflush, and if not no support - $<$>:$,USE_FFLUSH,NO_FLUSH_SUPPORT>> - $<$>:NO_GAMMA_SUPPORT> - - #!TODO Leaving as is in WRF for now but investigate why we don't do this - # https://stackoverflow.com/a/1035713 - # If fseeko64 succeeds, use that, else check if we can fall back to fseeko, and if not just use fseek - $,FSEEKO64_OK,$,FSEEKO_OK,FSEEK_OK>> - - # I don't believe these are used anymore... - # $<$:MPI2_SUPPORT=$> - # $<$:MPI2_THREAD_SUPPORT=$> - ) +# Only define if set, this is to use #ifdef/#ifndef preprocessors +# in code since cmake cannot handle basically any others :( +# https://gitlab.kitware.com/cmake/cmake/-/issues/17398 +if ( ${ENABLE_CHEM} ) + add_compile_definitions( WRF_CHEM=1 ) + if ( ${ENABLE_KPP} ) + add_compile_definitions( WRF_KPP=1 ) + endif() +endif() +if ( ${ENABLE_CHEM} ) + add_compile_definitions( BUILD_CHEM=1 ) +endif() +if ( ${ENABLE_CMAQ} ) + add_compile_definitions( WRF_CMAQ=1 ) +endif() +if ( ${ENABLE_DFI_RADAR} ) + add_compile_definitions( WRF_DFI_RADAR=1 ) +endif() +if ( ${ENABLE_TITAN} ) + add_compile_definitions( WRF_TITAN=1 ) +endif() +if ( ${ENABLE_MARS} ) + add_compile_definitions( WRF_MARS=1 ) +endif() +if ( ${ENABLE_VENUS} ) + add_compile_definitions( WRF_VENUS=1 ) +endif() +if ( ${ENABLE_HYDRO} ) + add_compile_definitions( WRF_HYDRO=1 ) +endif() + +# Because once again we need two defines to control one thing +if ( ${ENABLE_CTSM} ) + add_compile_definitions( WRF_USE_CTSM ) +else() + #!TODO there are some files that rely on this being 1, but that is never set by the legacy make system + add_compile_definitions( WRF_USE_CLM ) +endif() + +# If force classic or no nc-4 support enable classic +if ( ${FORCE_NETCDF_CLASSIC} OR ( NOT ${netCDF_NC4} ) ) + add_compile_definitions( NETCDF_classic=1 ) +endif() +if ( ${WRFIO_NCD_NO_LARGE_FILE_SUPPORT} OR ( NOT ${netCDF_LARGE_FILE_SUPPORT} ) ) + add_compile_definitions( WRFIO_NCD_NO_LARGE_FILE_SUPPORT=1 ) +endif() +# May need a check for WRFIO_ncdpar_LARGE_FILE_SUPPORT + +# Now set the opposite in different defines, because why not :) +if ( ( NOT ${FORCE_NETCDF_CLASSIC} ) AND ${netCDF_NC4} ) + add_compile_definitions( USE_NETCDF4_FEATURES=1 ) +endif() +if ( ( NOT ${WRFIO_NCD_NO_LARGE_FILE_SUPPORT} ) AND ${netCDF_LARGE_FILE_SUPPORT} ) + add_compile_definitions( WRFIO_NCD_LARGE_FILE_SUPPORT=1 ) +endif() + +# Could simplify logic to just check if RPC is available but to be explicit +# Does this actually need to check for EM_CORE (Config.pl:443) +# not enable terran or not rpc_found do +# not ( enable terrain and rpc_found ) +# if ( NOT ( ${ENABLE_TERRAIN} AND ${RPC_FOUND} ) ) # this is wrong, needs fixing +# add_compile_definitions( LANDREAD_STUB ) +# endif() +if ( ${ENABLE_TERRAIN} AND ${MOVE_NESTS} ) + add_compile_definitions( TERRAIN_AND_LANDUSE ) +else () + add_compile_definitions( LANDREAD_STUB ) +endif() + +if ( ${USE_ALLOCATABLES} ) + add_compile_definitions( USE_ALLOCATABLES ) +endif() +if ( ${wrfmodel} ) + add_compile_definitions( wrfmodel ) +endif() +if ( ${GRIB1} ) + add_compile_definitions( GRIB1 ) +endif() +if ( ${INTIO} ) + add_compile_definitions( INTIO ) +endif() +if ( ${KEEP_INT_AROUND} ) + add_compile_definitions( KEEP_INT_AROUND ) +endif() +if ( ${LIMIT_ARGS} ) + add_compile_definitions( LIMIT_ARGS ) +endif() + + +if ( ${BUILD_RRTMG_FAST} ) + add_compile_definitions( BUILD_RRTMG_FAST=1 ) +else() + add_compile_definitions( BUILD_RRTMG_FAST=0 ) +endif() +if ( ${BUILD_RRTMK} ) + add_compile_definitions( BUILD_RRTMK=1 ) +else() + add_compile_definitions( BUILD_RRTMK=0 ) +endif() +if ( ${BUILD_SBM_FAST} ) + add_compile_definitions( BUILD_SBM_FAST=1 ) +else() + add_compile_definitions( BUILD_SBM_FAST=0 ) +endif() +if ( ${SHOW_ALL_VARS_USED} ) + add_compile_definitions( SHOW_ALL_VARS_USED=1 ) +else() + add_compile_definitions( SHOW_ALL_VARS_USED=0 ) +endif() +if ( ${NMM_CORE} ) + add_compile_definitions( NMM_CORE=1 ) +else() + add_compile_definitions( NMM_CORE=0 ) +endif() + +if ( "${WRF_CORE}" STREQUAL "PLUS" ) + add_compile_definitions( WRFPLUS=1 ) +else() + add_compile_definitions( WRFPLUS=0 ) +endif() + +if ( "${WRF_CORE}" STREQUAL "DA_CORE" OR "${WRF_CORE}" STREQUAL "DA_4D_VAR" ) + add_compile_definitions( DA_CORE=1 ) +else() + add_compile_definitions( DA_CORE=0 ) +endif() +# DFI_RADAR=$ + +# Nesting options +if ( ${MOVE_NESTS} ) + add_compile_definitions( MOVE_NESTS ) +endif() +if ( "${WRF_NESTING}" STREQUAL "VORTEX" ) + add_compile_definitions( VORTEX_CENTER ) +endif() + +# Configuration checks +if ( NOT ${Fortran_2003_IEEE} ) + add_compile_definitions( NO_IEEE_MODULE ) +endif() +if ( NOT ${Fortran_2003_ISO_C} ) + add_compile_definitions( NO_ISO_C_SUPPORT ) +endif() +# If flush fails, check if we can fall back to fflush, and if not no support +if ( NOT ${Fortran_2003_FLUSH} ) + if ( "${Fortran_2003_FFLUSH}" ) + add_compile_definitions( USE_FFLUSH ) + else() + add_compile_definitions( NO_FLUSH_SUPPORT ) + endif() +endif() +if ( NOT ${Fortran_2003_GAMMA} ) + add_compile_definitions( NO_GAMMA_SUPPORT ) +endif() + +#!TODO Leaving as is in WRF for now but investigate why we don't do this +# https://stackoverflow.com/a/1035713 +# If fseeko64 succeeds, use that, else check if we can fall back to fseeko, and if not just use fseek +if ( NOT ${FSEEKO64} ) + add_compile_definitions( FSEEKO64_OK ) +elseif( "${FSEEKO}" ) + add_compile_definitions( FSEEKO_OK ) +else() + add_compile_definitions( FSEEK_OK ) +endif() + +# I don't believe these are used anymore... +# $<$:MPI2_SUPPORT=$> +# $<$:MPI2_THREAD_SUPPORT=$> + + # Make core target add_library( ${PROJECT_NAME}_Core diff --git a/cmake/c_preproc.cmake b/cmake/c_preproc.cmake index 14f7fe9295..4de0a1e7a1 100644 --- a/cmake/c_preproc.cmake +++ b/cmake/c_preproc.cmake @@ -111,14 +111,9 @@ macro( wrf_expand_definitions ) set( WRF_EXP_DEFS ) foreach( WRF_EXP_DEF ${WRF_EXP_DEFINITIONS} ) if ( NOT ${WRF_EXP_DEF} MATCHES ".*-D.*" ) - # We have a generator expression, inject the -D correctly - # THIS SHOULD ONLY BE USED FOR CONDITIONALLY APPLIED DEFINITIONS + # We have a generator expression, error! no way we can evaluate this correctly if ( ${WRF_EXP_DEF} MATCHES "^[$]<" ) - # Take advantage of the fact that a define is most likely not an expanded variable (i.e. starts with a-zA-Z, adjust if not) - # preceeded by the defining generator expression syntax $<>:var or ,var - # Yes this is fragile but is probably more robust than the current code if you're relying on this macro :D - string( REGEX REPLACE "(>:|,)([a-zA-Z])" "\\1-D\\2" WRF_EXP_DEF_SANITIZED ${WRF_EXP_DEF} ) - list( APPEND WRF_EXP_DEFS ${WRF_EXP_DEF_SANITIZED} ) + message( FATAL_ERROR "Generator expressions not allowed in preprocessing defines" ) else() list( APPEND WRF_EXP_DEFS -D${WRF_EXP_DEF} ) endif()