Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake Fix split command flags to be correctly populated #2108

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

islas
Copy link
Collaborator

@islas islas commented Sep 12, 2024

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)

@islas islas requested review from a team as code owners September 12, 2024 23:16
@amstokely amstokely self-requested a review September 23, 2024 21:01
Copy link
Collaborator

@amstokely amstokely left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@amstokely amstokely self-requested a review September 23, 2024 21:03
amstokely
amstokely previously approved these changes Sep 23, 2024
@islas islas changed the base branch from release-v4.6.1 to develop December 9, 2024 23:37
@islas islas dismissed amstokely’s stale review December 9, 2024 23:37

The base branch was changed.

@amstokely amstokely self-requested a review December 16, 2024 17:17
amstokely
amstokely previously approved these changes Dec 16, 2024
mgduda
mgduda previously approved these changes Dec 20, 2024
@islas islas dismissed stale reviews from mgduda and amstokely via 33e751b December 21, 2024 00:04
@amstokely amstokely self-requested a review January 7, 2025 19:32
@mgduda mgduda self-requested a review January 10, 2025 19:43
@islas islas merged commit 5b09725 into wrf-model:develop Jan 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants