[BUG/ISSUE] CMake's Fortran dependency scanner's preprocessing limitation

LiamBindle commented 5 years ago

Hi everyone,

Sorry for the confusing name...I'm not really know how to title this because this is a bit obscure.

After yesterdays update to feature/MAPL_v1.0.0 (compatibility with dev/12.4.0), I tried running GCHP's CMake build (which is based on feature/MAPL_v1.0.0) and I got an error I hadn't seen before. I've tracked down the problem—it's a limitation of CMake—and the solution is easy, but I think this is something the GCHP devs should know about.

tl;dr: When CMake generates Makefiles, a limitation of its dependency scanner is that it can't handle #if defined( xxxx ) statements in module declarations. Could we change the #if defined( MODEL_GEOS ) statement in Chem_GridCompMod.F90 to #ifdef MODEL_GEOS, and recommend developers only use #ifdef statements in module declarations in the future?

@lizziel: this relates to the preprocessor conditionals that facilitate GEOS and GEOS-Chem shared files (specifically, 122faa1ec3fc52a27685d3f23335182fa340af3c)

The problem (explained)

When I tried to build GCHP I got the following error

Scanning dependencies of target GIGC 
[ 98%] Building Fortran object GCHP/CMakeFiles/GIGC.dir/GEOS_ctmEnvGridComp.F90.o 
[ 98%] Building Fortran object GCHP/CMakeFiles/GIGC.dir/gigc_historyexports_mod.F90.o 
[ 98%] Building Fortran object GCHP/CMakeFiles/GIGC.dir/gigc_providerservices_mod.F90.o
[ 98%] Building Fortran object GCHP/CMakeFiles/GIGC.dir/gigc_chunk_mod.F90.o 
[ 98%] Building Fortran object GCHP/CMakeFiles/GIGC.dir/Chem_GridCompMod.F90.o 
Error copying Fortran module "mod/geoschemchem_gridcompmod".  Tried "mod/GEOSCHEMCHEM_GRIDCOMPMOD.mod" and "mod/geoschemchem_gridcompmod.mod". 
GCHP/CMakeFiles/GIGC.dir/depend.make:58: recipe for target 'GCHP/CMakeFiles/GIGC.dir/Chem_GridCompMod.F90.o.provides.build' failed 
make[3]: *** [GCHP/CMakeFiles/GIGC.dir/Chem_GridCompMod.F90.o.provides.build] Error 1
GCHP/CMakeFiles/GIGC.dir/build.make:78: recipe for target 'GCHP/CMakeFiles/GIGC.dir/Chem_GridCompMod.F90.o.provides' failed
make[2]: *** [GCHP/CMakeFiles/GIGC.dir/Chem_GridCompMod.F90.o.provides] Error 2
CMakeFiles/Makefile2:121: recipe for target 'GCHP/CMakeFiles/GIGC.dir/all' failed
make[1]: *** [GCHP/CMakeFiles/GIGC.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

I got this for CMake version 3.5 (GEOS-Chem's minimum requirement) and version 3.15 (latest release).

This error looked weird because Chem_GridCompMod.F90 declares the GEOSCHEMchem_GridCompMod module iff MODEL_GEOS is defined which it isn't (I checked the verbose compile log).

https://github.com/geoschem/gchp/blob/71543ab21f7420f013fa62a4fa40108e5201e976/Chem_GridCompMod.F90#L44-L48

So, I dug around online and found a number of issues related to how CMake's Makefile generator engine does preprocessing for dependency scanning for Fortran files. IIUC when CMake is generating Makefiles, it does an "approximate" preprocessing [1,2] before scanning USE statements and MODULE declarations to create a dependency tree.

It looks like the problem is that this "approximate preprocessor" can't handle ()-enclosed conditions [1], and it looks like this is how Chem_GridCompMod.F90 is shared between GEOS and GEOS-Chem (see 122faa1ec3fc52a27685d3f23335182fa340af3c).

What are supported by the "approximate preprocessor" though (and tested in CMake's unit tests) are #ifdef statements.

Note: #ifdef xxxx and #if defined( xxxx ) are equivalent as long as there is only one definition.

References:

[1] https://gitlab.kitware.com/cmake/cmake/issues/17398 (very similar)
[2] https://cmake.org/pipermail/cmake-developers/2015-July/025558.html

A solution

Unfortunately, because this is a limitation of CMake, it seems like a small change to Chem_GridCompMod.F90 is the easiest solution.

This change would be replacing the #if defined( MODEL_GEOS) statement in the module declaration in Chem_GridCompMod.F90 with #ifdef MODEL_GEOS , and in the future, recommending that developers only use #ifdef conditionals in module declarations.

In Chem_GridCompMod.F90:

-   44  #if defined( MODEL_GEOS )
+   44  #ifdef MODEL_GEOS
    45  MODULE GEOSCHEMchem_GridCompMod
    46  #else
    47  MODULE Chem_GridCompMod
    48  #endif

- 7604  #if defined( MODEL_GEOS )
+ 7604  #ifdef MODEL_GEOS
  7605  END MODULE GEOSCHEMchem_GridCompMod
  7606  #else
  7607  END MODULE Chem_GridCompMod
  7608  #endif

This fixes building GCHP with CMake. I believe #ifdef xxxx and if defined( xxxx ) are equivalent as long as there's only one definition that's being checked.

Alternatives (if changing the code isn't an option)

Because this is shared code, I realize there could be reasons that changing the code isn't possible. If that's the case, here are a two alternative alternatives

Use a different build system. The Ninja build system does full preprocessing before generating the dependency tree. GCHP builds fine with Ninja (rather than Make) without any modifications (I tried building with Ninja this morning).
I could probably come up with a CMake-based workaround using regex's but I'm very hesitant about this.

lizziel commented 5 years ago

Hi Liam, I think perhaps the best solution to this problem would be to eliminate the need for that C-preprocessor block entirely. This could be done by using the same module name in both GCHP and GEOS. Unless I get an outcry about this I will plan on doing this in 12.5. For now make whatever update is easiest for you to work with.

LiamBindle commented 5 years ago

Hi Lizzie. Sounds good. Yeah, it's no rush. I decided to fix it in my branch so that people that are demoing it don't have to apply a patch. The conflict will be easy to resolve, and I'll deal with that later if it comes up.

lizziel commented 4 years ago

This fix is applied in GEOS-Chem 12.7.0.

geoschem / gchp_legacy