Fixes #111 and other mixed clang/gfortran issues

[platipodium]

gfortran cannot handle Clang's --preprocess flag, thus we need to
remove it (Clang preprocesses by default, even without the flag)
and enable Fortran preprocessing for .f90 and F90 files.

This fixes #111

[Copilot]

Pull Request Overview

This PR addresses mixed clang/gfortran issues by removing the unnecessary "--preprocess" flag and enabling Fortran preprocessing for .f90 and F90 files. The key changes include:

• Removing the explicit setting of the "--preprocess" flag when using Clang.
• Enabling Fortran preprocessing for source files in SCHISMCompile.cmake.
• Cleaning up redundant configuration in the SCHISM.local.conda file.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
cmake/SCHISMCompile.cmake	Adjusts flag handling and adds source file property for preprocessing.
cmake/SCHISM.local.conda	Removes legacy warning and duplicated preprocessing settings.

Comments suppressed due to low confidence (1)

cmake/SCHISM.local.conda:5

• [nitpick] The removal of the legacy warning and redundant preprocessing property setup streamlines the configuration; ensure that Fortran builds in all environments have been adequately validated after this change.

message(WARNING "If you get an error 'Error copying Fortran module' ...

[PhilMiller]

I'd strongly recommend just using CMake's built-in knowledge of how to preprocess Fortran files across various compilers, as applied to the BMI implementation:
https://github.com/schism-dev/schism/pull/161/files#diff-cea5df5756b0fd4a4d084b753903c8a5bc7b6d07b62c138d289d2071ea3f5f0d
I've run into a bunch of different issues with how SCHISM's CMake files try to add these and other flags to compilation command line arguments.

I see that you're setting the property for relevant source files with this PR. Given that, could we just delete all of the C_PREPROCESS_FLAG logic and usage and move the property setting up to cover all compilers?

[platipodium]

There is already this comment in CMAkeList.txt

########### C Preprocessor #####
# The thought is to enable this globally, since it is needed for all
# of SCHISM and several Utilities
# @todo this might be more elegantly handled (from cmake 3.18) with FORTRAN_PREPROCESS boolean flag
if (NOT DEFINED C_PREPROCESS_FLAG)
  message(FATAL_ERROR "Preprocessor flag (e.g. -cpp for intel, -MPreprocess for PGI0 is not set")
endif()
add_compile_options(${C_PREPROCESS_FLAG})

So it seems we could do this if we require cmake 3.18

[PhilMiller]

For reference on CMake versions, Ubuntu no longer ships any CMake older than 3.22, in the 22.04 LTS release:
https://packages.ubuntu.com/search?keywords=cmake&searchon=names&exact=1&suite=all&section=all

[PhilMiller]

Debian has 3.18 or later from its oldstable release 'Bullseye' (August 2021) onward:
https://packages.debian.org/search?keywords=cmake&searchon=names&exact=1&suite=all&section=all

[PhilMiller]

Doing a broader scan, https://pkgs.org/search/?q=cmake

Alpine's oldest listed release 3.18 has CMake version 3.26. That's a substantially relevant target because it's popular to use in building container images.

Alma and Rocky 8, representing Redhat and derivatives, also have CMake version 3.26.

Fedora 40, a few releases back, has CMake 3.28

So, I think we can count on CMake later than 3.18 being available.

[platipodium]

Thanks for all this research @PhilMiller. I have recently played with the new LLVM Fortran compiler flang, I'll be updating the CMake for that one once I've tested it, but outside this PR

[platipodium]

Makes sense to move it up

[platipodium]

I addressed the remarks by greatly simplifying (C ID is not asked anymore) and moving everything up to top-level cmake

[PhilMiller]

Did you miss committing a changed file? As of now, it looks like the fortran preprocessing flag variable setting is being removed, but nothing has replaced it.

[platipodium]

Did you miss committing a changed file? As of now, it looks like the fortran preprocessing flag variable setting is being removed, but nothing has replaced it.

Thanks @PhilMiller. The logic was moved from the conda specific file to the main file. There C_PREPROCESS_FLAG is set to empty (there is no fortran-specific one) as required for llvm and gnu compilers, and preprocessing is enabled for all .F90 files for all compilers. C logic was removed and only Fortran preserved (and added for Flang).

[PhilMiller]

I'm a bit confused, then. I see this removed from the conda config file:

set_source_files_properties(
  *.F90
  PROPERTIES Fortran_PREPROCESS ON
)

But I don't see it being added anywhere else.

Am I missing the implication that for gfortran and flang, preprocessing is enabled by default, and does not need to be specified? Checking the GFortran docs, it seems I am - the source files outside of src/Utility are all named .F90, which enables pre-processing implicitly.

I still think it would be preferable to add the above snippet to make the desired behavior explicit. On the other hand, if there's nothing said, and we're just taking the default compiler behavior that Fortran developers expect from what's already there, then I think this is good and fine.

[PhilMiller]

Or, perhaps you could go the other direction, rename the few files in src/BMI/*.f90 to .F90, and remove the snippet from src/BMI/CMakeLists.txt as well.

[platipodium]

@PhilMiller thanks for your thoughts.

   *.F90
   PROPERTIES Fortran_PREPROCESS ON
 )

should be the equivalent of set(CMAKE_Fortran_PREPROCESS ON) from BMI/CMakeLists.txt for cmake < 3.18. I removed this from the conda environment files, as with the open source compilers gnu/llvm preprocessing is always on, and the respective options -E or --preprocess tell the compiler to only preprocess, other than -fpp or -Mpreprocess options for other compilers that explicitly turn preprocessing on.

Indeed, we could consider

• moving these statements to the SCHISMcompile.cmake top file, but I have no way of testing the effects on my systems.
• consistently renaming .f90 to .F90 to indicate preprocessing demand. (downside: there is lots of third-party content that we should rather keep in its original form)

I would like to go ahead with this PR without major modifications and potential side effects to other modules, however.

[josephzhang8]

Thx @platipodium. I don't see any problem with the proposed changes for gfort and flang. Other should review, especially wrt SCHISMCompile.cmake.

[platipodium]

Recently some problems have resurfaced (with updated versions of clang and gfortran) ... let me investigate a little more, I'll update on this issue.