Skip to content

Clean up compiler flag specification#297

Draft
fmahebert wants to merge 7 commits into
developfrom
feature/cleanup_compiler_flags_part1
Draft

Clean up compiler flag specification#297
fmahebert wants to merge 7 commits into
developfrom
feature/cleanup_compiler_flags_part1

Conversation

@fmahebert

@fmahebert fmahebert commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Description

As a first step towards cleaning JEDI's specification of compilers flags, this PR reorganizes and changes flags as follows:

  • a new file cmake/build_type_compiler_flags.cmake provides default build-type compiler flags for compilers not in ecbuild; this could eventually be contributed upstream. This file should be identical in all JEDI repos.
  • a new file cmake/jedi_common_compiler_flags.cmake provides JEDI specific compiler flags; for example warning levels, floating-point behavior, etc. This file should be identical in all JEDI repos.
  • repo-specific compiler flags

This has been tested with gcc13, IntelLLVM 2025.3, Intel mixing icx with legacy ifort; and also with Cray by David Davies at UKMO. In general, for these compilers, the cleaned up flags have fewer runtime floating-point issues, and more compile-time warnings.

Issue(s) addressed

This is work towards https://github.com/JCSDA-internal/CI/issues/173

Dependencies

Part of a large family of PRs, see the CI issue for a complete list. These do not strictly depend on each other.

Impact

Impact on users:

  • expect change in warnings emitted
  • when using Intel LLVM, possible small changes to floating-point values as a result of changing floating-point handling
  • this has not been tested with all compilers, so users of less-common compilers may need to iterate on the flags

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have run the unit tests before creating the PR

jedi-ci-test-select=intel

@ytremolet

Copy link
Copy Markdown

Approving but we need to check the failures here.

@svahl991 svahl991 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are the test failures here understood?

@fmahebert

fmahebert commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure why this change in CRTM could cause so many failures in IODA. The CDash output is missing, which makes it hard to diagnose... and I haven't seen it in local testing.

@eap

eap commented Mar 31, 2026

Copy link
Copy Markdown
Contributor
  1. I don't know what's going on with the test failures.
  2. I do think that the test failures failed to push to cdash for reasons described here; the build started before UTC date flip and finished after, combined with some ctest flags that are causing ctest to ignore the first tag. Luckily this can be fixed in the short term by restarting the test.
  3. When this was originally tested it used the old test flags which had not been submitted (jedi-ci fixes for "inconsistent compiler flags" coordinated merge. JCSDA-internal/jedi-ci#62).

I've restarted the tests which should hopefully resolve issues 2 and 3. With luck, the third issue fixes the ioda test failures, but at least we should get cdash uploads this time.

@BenjaminTJohnson

BenjaminTJohnson commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

Just getting back from PTO -- just so I'm clear, the aforementioned IODA test failures occur with this proposed PR, but not with the current develop branch?

@BenjaminTJohnson

Copy link
Copy Markdown
Contributor

@fmahebert is this still something that needs to be done (i.e., is it working correctly for IODA, etc.)?

@eap

eap commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Figured out the problem! The fix here isn't applying and the test run starting with the config from develop

Inside the start-jedi-ci action here's the logs. Note both test_dependencies and test_strategy are incorrect. I'll need to fix this with a separate pull request, then we can merge it here and the tests should pass.

    container_version: latest
    target_project_name: crtm
    test_dependencies: ioda oops gsw ufo-data ufo
    test_strategy: INTEGRATION
    test_script: run_tests.sh

eap added a commit that referenced this pull request Apr 8, 2026
Necessary for #297 to pass tests.
@eap eap mentioned this pull request Apr 8, 2026
eap added a commit that referenced this pull request Apr 8, 2026
Necessary for #297 to pass tests. Changes to this config must be submitted in order for CI actions to use them. This is because we use the pull_request_target event type which is necessary to maintain safety in a public repository.
@fmahebert

Copy link
Copy Markdown
Contributor Author

Something still amiss here @eap, I wonder if the just-updated CI strategy is not correctly identifying CRTM's tests as tests to run during the "unit tests" check?

@ytremolet

Copy link
Copy Markdown

I don't think we updated crtm to match what we did for all the other repos during the sprint, did we?

@fmahebert

fmahebert commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Well this PR keeps delivering!

New error building the bundle for integration CI:

[ 19%] Building Fortran object mpas/src/framework/CMakeFiles/framework.dir/mpas_dmpar.F.o
fpp: warning: keyword redefined: USE_PIO2
mpas_dmpar.F(5547): #error: number of arguments doesn't match.

And note that the log includes

-- Cloning mpas from https://github.com/MPAS-Dev/MPAS-Model.git into /workdir/bundle/mpas...
-- /workdir/bundle/mpas retrieved.
-- Updating mpas to TAG v8.2.1...

So the jedi-bundle PR 140, merged two days ago, that updated the MPAS-Model tag is not being picked up?

@BenjaminTJohnson

Copy link
Copy Markdown
Contributor

@fmahebert @eap I'm not sure what exactly is trying to be accomplished here. CRTM is not a "JEDI-only" component -- the requested changes appear to be overly specific toward supporting JEDI while (possibly) removing support for other stakeholders. I'm happy to make flag / compilers support modifications to resolve compatibility issues, but I also have to support other users / stakeholders across multiple compilers & versions / platforms -- most of whom operate outside of JEDI/UFO.

Please set up a meeting with me at your convenience to discuss what the needs are.

@fmahebert

fmahebert commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

@BenjaminTJohnson — your position makes sense, and if this PR is too jedi-specific I'm ok with considering a different solution.

The issues with develop compiler flags are,

  • some build types are missing e.g., RELWITHDEBINFO flags are missing for gfortran
  • some build types compiler flags are wrong, e.g. IntelLLVM RELWITHDEBINFO is -O0 instead of the standard -O2
  • IntelLLVM builds producing a numerous warnings about the flags
  • the pattern of setting CMAKE_Fortran_FLAGS_<BUILDTYPE> prevents environment-level flag configurations from taking effect in CRTM — CRTM's internal flags will overrule anything passed in by the user, which is I think harmful because it prevents users from specifying flags appropriate to their machine/compiler/use-case. Ideally CRTM's build system would add any CRTM-specific required flags to whatever gets passed in from the broader build, but would't control the whole flag set.

Note nearly all JEDI repos had these issues as well, because CRTM develop is already following JEDI patterns for setting compiler flags.

Making progress on these issues (across all of JEDI) was the goal of this set of PRs. However I don't believe anything is JEDI-specific, except perhaps the use of "jedi" in the name of one file in my PR. If that name or some other specific aspect of the PR is problematic for CRTM, I'm happy to iterate.

Can you clarify why you say "the requested changes appear to be overly specific toward supporting JEDI"?

I'll email you to set up a meeting to keep discussing this.

@fmahebert fmahebert left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can't change-request on myself, but this PR needs changes:

message( STATUS "Fortran compiler with ID ${CMAKE_Fortran_COMPILER_ID} will be used with CMake default options")
# Set CRTM-specific compiler flags
if(CMAKE_Fortran_COMPILER_ID STREQUAL GNU)
ecbuild_add_fortran_flags("-ffree-line-length-none")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spoke with @BenjaminTJohnson — CRTM has the requirement of being buildable outside of the JEDI environment, in particular of being buildable as a standalone repo with a CMake (not ecbuild) build system.

Thus, these ecbuild functions shouldn't be used to specify flags.

I'll go back and rethink this PR to see if I can improve the consistency of CRTM flags with the rest of JEDI, while still falling within CRTM's constraints.

@fmahebert fmahebert marked this pull request as draft April 22, 2026 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants