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

Adjust #defines to accommodate future CMake build #1910

Merged
merged 5 commits into from
Jan 11, 2024

Conversation

islas
Copy link
Collaborator

@islas islas commented Aug 15, 2023

TYPE: bug fix

KEYWORDS: defines, cmake

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
Certain #define constructs prove to be challenging for CMake to process automatically into a dependency graph.

Solution:
Reduce complexity for #defines that can be rewritten. For those that cannot, create stub modules.

LIST OF MODIFIED FILES:
M dyn_em/module_advect_em.F
M external/atm_ocn/cmpcomm.F
M external/io_adios2/wrf_io.F90
M external/io_netcdf/wrf_io.F90
M external/io_netcdfpar/wrf_io.F90
M external/io_pnetcdf/wrf_io.F90
M frame/module_configure.F
M phys/module_mp_SBM_polar_radar.F
M phys/module_mp_fast_sbm.F

TESTS CONDUCTED:

  1. Do mods fix problem? Local tests demonstrate fixes satisfy new upcoming cmake build
  2. Are the Jenkins tests all passing? Yes

RELEASE NOTE:
Adjust #defines to accommodate future CMake build

@islas islas requested review from a team as code owners August 15, 2023 22:24
@islas
Copy link
Collaborator Author

islas commented Aug 15, 2023

@dudhia @weiwangncar
At the suggesting of @mgduda I've split #1899 into more digestible PRs addressing individual issues

@@ -15,7 +15,8 @@ SUBROUTINE init_module_scalar_tables
END SUBROUTINE init_module_scalar_tables
END MODULE module_scalar_tables

#if( WRF_CHEM == 1 && WRF_KPP == 1 )
#ifdef WRF_CHEM
#ifdef WRF_KPP

Choose a reason for hiding this comment

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

Perhaps hypothetically, but without duplicating the enclosed code, how would one transform the original directive here if it involved an or instead of and relationship? Inability to process logical expressions in #if directives might be an issue to raise with the CMake developers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be impossible, and the current guidance (from CMake developers) is to (1) switch to Ninja or (2) rewrite the code - both of which at times are nonstarters.

This is a huge annoyance that has been noted to the CMake developers in https://gitlab.kitware.com/cmake/cmake/-/issues/17398 (which coincidentally happens to be a derivative issue spawned from https://github.com/WRF-CMake/wrf that helped me find out this limitation).

There are a few instances in WRF already that run into this scenario, and the solution there is to do neither (1) or (2) but instead do traditional C preprocessing and generate an intermediate file. The makefile system already does this for everything; moving away from this to do native Fortran preprocessing through the respective compiler would be ideal.

As a middle ground compromise limiting two-step intermediate file C preprocessing to only those files which absolutely need it has been an okay solution.

@weiwangncar
Copy link
Collaborator

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@mgduda
Copy link
Collaborator

mgduda commented Jan 4, 2024

Would it be worth splitting the changes for the png_voidp type cast as well as the additions to the .gitignore file into their own PRs?

I'll give it some thought, but perhaps there's a way to describe the changes in this PR differently. Looking through the diffs, my take-away is that (1) CMake doesn't support compound preprocessor conditionals, and (2) to enable CMake to generate a correct dependency graph we need to define stub modules in cases where those modules wouldn't otherwise be defined (because they had been pre-processed out).

@islas islas force-pushed the definesAndSyntaxAlignCMake branch from 4908aac to fc27eaf Compare January 5, 2024 23:44
@islas islas changed the title Adjust #defines and erroneous syntax to accommodate future CMake build Adjust #defines to accommodate future CMake build Jan 5, 2024
@islas
Copy link
Collaborator Author

islas commented Jan 6, 2024

@mgduda I took the approach of splitting off the syntax error, png fix, and .gitignore changes to #1976, #1977, and #1978 respectively. As this PR has decent discussion I didn't want to close it to rework the now split off commits, hence the force-push.

Copy link
Collaborator

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

I've visually verified that the revised preprocessing directives are logically equivalent to those they replace.

@mgduda
Copy link
Collaborator

mgduda commented Jan 11, 2024

@weiwangncar or @dudhia would you give this a second review?

@dudhia
Copy link
Collaborator

dudhia commented Jan 11, 2024

Can you explain why some physics have stub modules?

@islas
Copy link
Collaborator Author

islas commented Jan 11, 2024

@dudhia Since sometimes multiple modules exist within one file, they are considered "compiled" regardless of preprocessor directive conditional compilation to CMake. This is due to how CMake generates interdependencies for files and when conditionally compiled out, if CMake cannot process the #<conditional> (e.g. #if( BUILD_SBM_FAST != 1)) then it expects a .mod file or equivalent to exist.

@dudhia
Copy link
Collaborator

dudhia commented Jan 11, 2024

@islas Thanks

@mgduda
Copy link
Collaborator

mgduda commented Jan 11, 2024

@dudhia Since sometimes multiple modules exist within one file, they are considered "compiled" regardless of preprocessor directive conditional compilation to CMake. This is due to how CMake generates interdependencies for files and when conditionally compiled out, if CMake cannot process the #<conditional> (e.g. #if( BUILD_SBM_FAST != 1)) then it expects a .mod file or equivalent to exist.

To use a specific example, the module_mp_SBM_polar_radar.F file already contains a "stub" module of sorts:

#if( BUILD_SBM_FAST != 1)
      MODULE module_mp_SBM_polar_radar
      CONTAINS
      SUBROUTINE SBM_polar_radar
         REAL :: dummy
         dummy = 1
      END SUBROUTINE SBM_polar_radar
      END MODULE module_mp_SBM_polar_radar
#else
! ... other code ...
! +----------------------------------------------------------------------------+
! +----------------------------------------------------------------------------+
MODULE module_mp_SBM_polar_radar

! Kind paramater
INTEGER, PARAMETER, PRIVATE:: R8SIZE = 8
INTEGER, PARAMETER, PRIVATE:: R4SIZE = 4

private
public polar_hucm
! ... code ...
END MODULE module_mp_SBM_polar_radar
!**** **************************************************************** &
#endif

The first module_mp_SBM_polar_radar "stub" is provided in case the code is compiled with BUILD_SBM_FAST not equal to 1 so that (presumably) any Fortran USE statements of the module_mp_SBM_polar_radar module would still be valid. (Actually, my suspicion is that this is not needed anymore after looking through other parts of the WRF code.)

If I understand correctly, the additional stub modules are there to ensure that the same set of Fortran .mod files are created during the build process regardless of preprocessing directives so that CMake's dependencies will always be satisfied. @islas is this correct?

@dudhia
Copy link
Collaborator

dudhia commented Jan 11, 2024

@mgduda Even more clear now. Thanks.

@islas
Copy link
Collaborator Author

islas commented Jan 11, 2024

@mgduda That is correct. This was only done for modules where rewriting the defines+directives for CMake to understand was not possible without greater modification to the code. In the above example BUILD_SBM_FAST is always defined and other parts of the code rely on whether it is 0 or 1, thus to keep the logic the same would not be able to be reduced to CMake's limited vocabulary of #ifdef & #ifndef

@islas islas merged commit f81f982 into wrf-model:develop Jan 11, 2024
1 of 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.

6 participants