Skip to content

Commit

Permalink
CMake Fix split command flags to be correctly populated (#2108)
Browse files Browse the repository at this point in the history
TYPE: bug fix

KEYWORDS: cmake, mpi, compilation

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
The `arch/configure_reader.py` does the job of parsing, organizing, and
sanitizing input from configuration stanzas into a CMake toolchain file
which can then be used to inform the build about which compilers, flags,
and options to use.

Occasionally, stanzas fields inject flags into a compiler or other
command field (like `DM_FC`) so that the actual command is a command +
flags. Part of the `arch/configure_reader.py` organization is breaking
these up into separable sections automatically. With the example of
`DM_FC = mpif90 -f90=gfortran` (`$(SFC)` already expanded) this _should_
be broken into `DM_FC = mpif90` and `DM_FC_FLAGS = -f90=gfortran`.
Currently, the `*_FLAGS` field when split out for certain keys in a
stanza is not populated due to using the wrong index from the Python
`str.partition()` call.

Secondly, when these fields are actually provided to CMake compilation
breaks for MPI specifically. Since the MPI "compilers" are wrappers,
they are then interrogated for the underlying flags and options meaning
further adding the flags back into compilation results in things like
`gfortran <all the other flags> -f90=gfortran`. This is incorrect, and
instead the flags should be provided to the MPI flags used during
wrapper interrogation on a per-language basis. Furthermore, for certain
MPI implementations supplying any flags renders the query command (e.g.
`-show`, `-showme`, or `-compileinfo`) useless. For instance, OpenMPI
`mpif90 -f90=gfortran -show` only outputs `gfortran -f90=gfortran` which
is also wrong.

Solution:
1. The Python call to split the fields should take the actual values
2. MPI flags per language should be supplied to the interrogation flags
`MPI_<LANG>_COMPILER_FLAGS` if needed
3. The `FindMPI` module already feeds in the underlying compiler
specification for wrappers that support it so flags like `-f90=$(SFC)`
should be filtered out from `DM_*_FLAGS` before being written to the
`wrf_config.cmake` toolchain file

TESTS CONDUCTED: 
1. Flags in specific command fields are now split and appearing in the
`wrf_config.cmake` file
2. MPI compilation works with compiler specification filtered out but
correct underlying compiler still selected even when multiple compilers
are in the same environment (note: this was the original behavior but
now is deliberate)
  • Loading branch information
islas authored Jan 10, 2025
1 parent 7195dc2 commit 5b09725
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 12 deletions.
19 changes: 8 additions & 11 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -405,23 +405,20 @@ if ( ${USE_MPI} )
# Which may or may not get polluted by the environment
# It still technically finds MPI but the output is nonintuitive
# saying things like hdf5 or pthread
find_package( MPI REQUIRED COMPONENTS Fortran C )
list( APPEND PROJECT_COMPILE_DEFINITIONS_OPTIONS
USE_MPI=1
DM_PARALLEL
)

# Supply any language-specific flags for interrogation
if ( DEFINED WRF_MPI_Fortran_FLAGS AND NOT "${WRF_MPI_Fortran_FLAGS}" STREQUAL "" )
list( APPEND PROJECT_COMPILE_OPTIONS_OPTIONS
$<$<COMPILE_LANGUAGE:Fortran>:${WRF_MPI_Fortran_FLAGS}>
)
set( MPI_Fortran_COMPILER_FLAGS ${WRF_MPI_Fortran_FLAGS} )
endif()

if ( DEFINED WRF_MPI_C_FLAGS AND NOT "${WRF_MPI_C_FLAGS}" STREQUAL "" )
list( APPEND PROJECT_COMPILE_OPTIONS_OPTIONS
$<$<COMPILE_LANGUAGE:C>:${WRF_MPI_C_FLAGS}>
)
set( MPI_C_COMPILER_FLAGS ${WRF_MPI_C_FLAGS} )
endif()
find_package( MPI REQUIRED COMPONENTS Fortran C )
list( APPEND PROJECT_COMPILE_DEFINITIONS_OPTIONS
USE_MPI=1
DM_PARALLEL
)

# Check if MPI in all its glory has forced IPO down our throats due to hard-coding the wrapper flags
# https://www.open-mpi.org/faq/?category=mpi-apps#why-no-rpath LOL!
Expand Down
22 changes: 21 additions & 1 deletion arch/configure_reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ def splitIntoFieldAndFlags( self, field ) :
fieldValue = self.kvPairs_[ field ]

self.kvPairs_[field] = fieldValue.partition(" ")[0]
self.kvPairs_[field + "_FLAGS"] = fieldValue.partition(" ")[1]
self.kvPairs_[field + "_FLAGS"] = fieldValue.partition(" ")[2]

######################################################################################################################
##
Expand Down Expand Up @@ -182,6 +182,26 @@ def sanitize( self ) :
# And for final measure strip
self.kvPairs_[ key ] = self.kvPairs_[ key ].strip()

# Finally, further sanitize MPI compilers, we don't need to specify underlying
# compiler since CMake already does that
filters = [
self.kvPairs_[ "SFC" ],
self.kvPairs_[ "SCC" ],
"-compiler"
]
keysToSanitize = [ "DM_FC_FLAGS", "DM_CC_FLAGS" ]

for keyToSan in keysToSanitize :
if self.kvPairs_[ keyToSan ] :
allFlags = self.kvPairs_[ keyToSan ].split( " " )
newFlags = []
for flag in allFlags :
if not any( [ f in flag for f in filters ] ) :
newFlags.append( flag )

# We always need this field updated
self.kvPairs_[ keyToSan ] = " ".join( newFlags )

def serialCompilersAvailable( self ) :
return which( self.kvPairs_["SFC"] ) is not None and which( self.kvPairs_["SCC"] ) is not None

Expand Down

0 comments on commit 5b09725

Please sign in to comment.