Flag existing and new compiler warnings with GitHub Action

[NickSzapiro-NOAA]

Commit Queue Requirements:

• This PR addresses a relevant WM issue (if not, create an issue).
• All subcomponent pull requests (if any) have been reviewed by their code managers.
• Run the full Intel+GNU RT suite (compared to current baselines), preferably on Ursa (Derecho or Hercules are acceptable alternatives). Exceptions: documentation-only PRs, CI-only PRs, etc.
  • Commit log file w/full results from RT suite run (if applicable).
  • Verify that test_changes.list indicates which tests, if any, are changed by this PR. Commit test_changes.list, even if it is empty.
• Fill out all sections of this template.

---

Description:

We're looking for a friendlier way of iterating with developers to reduce compiler warnings, towards better code quality and EE2 requirements. Here is a go at this via custom GitHub Action to flag existing and new compiler warnings for s2swa application in GNU debug mode. These complement the tracking of the number of warnings on supported platforms via the regression test platform logs.

This PR introduces a lightweight Python parser that reads Spack CI build logs, checks the Git diff, and leverages built-in GitHub Action commands to print warnings and annotate the PR.

Some examples for what a developer may see are:

• CICE: https://github.com/NickSzapiro-NOAA/ufs-weather-model/actions/runs/24874708020/job/72829670380
• ccpp-physics: https://github.com/NickSzapiro-NOAA/ufs-weather-model/actions/runs/25090070520/job/73514880958

Commit Message:

* UFSWM - Flag existing and new compiler warnings via GitHubAction

Priority:

• Normal

Git Tracking

UFSWM:

• Closes Compile warnings in UFS and subcomponents #2703

Sub component Pull Requests:

• None

UFSWM Blocking Dependencies:

• None

Documentation:

• Documentation update required.
  • Relevant updates are included with this PR.
  • A WM issue has been opened to track the need for a documentation update; a person responsible for submitting the update has been assigned to the issue (link issue).
• Documentation update NOT required.
  • Explanation:

---

Changes

Regression Test Changes (Please commit test_changes.list):

• No Baseline Changes.

Input data Changes:

• None.

Library Changes/Upgrades:

• No Updates

---

Testing Log:

• RDHPCS
  • Orion
  • Hercules
  • GaeaC6
  • Derecho
  • Ursa
• WCOSS2
  • Dogwood/Cactus
  • Acorn
• CI
• opnReqTest (complete task if unnecessary)

[gspetro-NOAA]

Is this line needed at all? It seems like if success or failure covers most (all?) circumstances.

[NickSzapiro-NOAA]

So the job waits for Spack to finish (whether Spack succeeds or fails)

[gspetro-NOAA]

Ah, ok. Thanks!

[gspetro-NOAA]

In the past, I've used needs: for that purpose, which you already have there. 🤔 Does the conditional bring something additional? Like, would the warning check not run if the Spack step failed, and so you're trying to also capture that? Could you use if: always() or something similar (I think always() would have it run even if the Spack step were cancelled, which doesn't make sense, but there may be another option)?

[NickSzapiro-NOAA]

Yes, we can still check the compile logs for warnings even if the Spack build fails
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-not-requiring-successful-dependent-jobs

I think if: always() is similar but doesn't care if job is cancel vs. success vs. fail
https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#status-check-functions

Thanks for reviewing @gspetro-NOAA , especially as I'm learning as I go here

[NickSzapiro-NOAA]

fyi, I tested this with changing the CICE branch to CICE-Consortium/main and the warnings were incorrectly lumped into the existing "legacy" category

[NickSzapiro-NOAA]

This now seems to work ok @gspetro-NOAA ... flagged test warnings in CICE and ccpp-physics as new (by recursing down the .gitmodules for changed lines).

I didn't run full RTs as changes are all in CI, except for adding -Wall -Wimplicit-interface flags to gnu.cmake that should not change compiled machine code. Could combine this PR into non-baseline changing PR to be cautious

[DeniseWorthen]

@NickSzapiro-NOAA In the description, you point to a closed PR "for what a developer might see" but I don't see anything there?

The CI compile job is just the S2SWA, which does not use PDLIB for WW3, so we're missing any compiler warnings that might be triggered there. Is that right?

[NickSzapiro-NOAA]

Thanks @DeniseWorthen , sorry about that. I've updated the PR description with two more recent examples

The CI compile job is just the S2SWA, which does not use PDLIB for WW3, so we're missing any compiler warnings that might be triggered there. Is that right

That's right. You're also right that it only covers a few ccpp-physics suites too.
This action depends on having compiler logs (like from a Spack build job). I'm not sure about adding more Spack application builds, but this should work with any of them.

I'm thinking the first use case is as a tool that once a PR's RT logs show increased number of warnings for any test(s), developer or CMs can then identify them by changing application, if needed. It's at least better than in #2810 (comment)

We still need the more enforceable solution of turning warnings into errors and then add exceptions for some in the mean time

[DeniseWorthen]

Thanks for posting what a developer would see.

Do you know whether "unused variable" is problematic for NCO?

And also, we're going on the assumption that GNU (CI) compiles are "stricter" than non-GNU, right? So we'd hopefully be ahead of the game when it comes to a non-GNU compile.

[NickSzapiro-NOAA]

Do you know whether "unused variable" is problematic for NCO?

I don't see such details in the standards. The current -Wall "enables all the warnings about constructions that some users consider questionable, and that are easy to avoid (or modify to prevent the warning)" https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

And also, we're going on the assumption that GNU (CI) compiles are "stricter" than non-GNU, right? So we'd hopefully be ahead of the game when it comes to a non-GNU compile.

That's what I found ... Intel warnings were a subset of the GNU ones.
I would treat operational requirements as the minimum standard, especially as compilers change and may get pickier. UFS code quality could be better.

[DeniseWorthen]

@gspetro-NOAA Nick and I discussed that the current CI is using a compile (S2SWA) that does not track any opn configuration; and it is the opn standards that require no warnings. So it makes sense to us that the CI compile something that exercises one or more opn confgs. The s2sw_32bit_pdlib would cover the components and SDF used for both v17 and SFS. We should also add the hafs_mom6w compile.

Is that doable?

[gspetro-NOAA]

@gspetro-NOAA Nick and I discussed that the current CI is using a compile (S2SWA) that does not track any opn configuration; and it is the opn standards that require no warnings. So it makes sense to us that the CI compile something that exercises one or more opn confgs. The s2sw_32bit_pdlib would cover the components and SDF used for both v17 and SFS. We should also add the hafs_mom6w compile.

Is that doable?

I love the idea! @AlexanderRichert-NOAA is the one who put together the original CI with S2SWA, so he may be better able to comment on whether that needs to stay, how doable it is to switch it or to add another task that does track operational configs, and whether he could do it or whether I/EPIC should look into it.

[AlexanderRichert-NOAA]

My two cents is that I chose the S2SWA configuration just as a way to exercise the various dependencies etc., but I don't think there's any special reason to stay with that compared with testing more operationally relevant setups.

[DeniseWorthen]

It was pretty straightforward to figure out how to add the HAFS build but I give up on the gfsv17 case; that requires PDLIB, which requires SCOTCH and I ended up down a rabbity hole of AI misdirection.

I also don't really understand why we have to build the entire stack all the time and why the common-build-cache isn't getting used.

This is what I added for hafs:

        - {
            os: "ubuntu-24.04",
            app: "HAFS-MOM6W",
            ccpp_suites: "FV3_HAFS_v1_gfdlmp_tedmf,FV3_HAFS_v1_gfdlmp_tedmf_nonsst,FV3_HAFS_v1_thompson,FV3_HAFS_v1_thompson_nonsst",
            other-variants: ' +32bit ^esmf@8.8.0',
          }

[AlexanderRichert-NOAA]

It looks like most packages are being pulled from the build cache, but ESMF and MAPL were rebuilt and I'm not sure why. There may be some possible workarounds, but hopefully soon we can default to running with a container image with everything preinstalled.

[NickSzapiro-NOAA]

After some rounds in ci/package.py with AI, GFS s2sw+pdlib builds in this branch:
https://github.com/NickSzapiro-NOAA/ufs-weather-model/tree/test_warnings_ghaction_gfs

I don't know if you want to take a look @AlexanderRichert-NOAA but it looks hacky to me

[AlexanderRichert-NOAA]

Yeah I see what you mean... Ideally the flag settings would be taken care of by scotch's own cmake configuration, and if not that then by WW3's FindSCOTCH.cmake. If you don't add the new logic, does the build fail or is it not a viable executable?

Also, https://github.com/NickSzapiro-NOAA/ufs-weather-model/blob/test_warnings_ghaction_gfs/ci/package.py restricts the scotch version. Is that necessary?

[NickSzapiro-NOAA]

I was iterating for the compile not to fail with errors @AlexanderRichert-NOAA . I have not tested the validity of the executable ... fwiw, for compiler warnings I don't know that we actually need the different options to be able to run together "scientifically".

But, forcing static depends_on("scotch@:7.0.4 +mpi+metis~shared", when="+pdlib") was one of the last "fixes" so maybe the hackiness can be cleaned up.

As for scotch version, I think Spack concretized and built Scotch 7.0.6. While wavewatch added the #ifdef SCOTCH_707 logic, apparently the SCOTCHF_ API change was actually in 7.0.5 so there's a mismatch in that exact version.

It may be more worth sorting out scotch-related issues for wavewatch pdlib and eventually mpas-a options

[NickSzapiro-NOAA]

Tagging @mingchen-NOAA @JessicaMeixner-NOAA ... could it be that the scotch API change (needed for pdlib) is in v7.0.5 (not 7.0.7)?

[AlexanderRichert-NOAA]

I think WW3 can/should be modified to use CMake-built scotch instead of FindSCOTCH.cmake. Here are my tweaks to ci/package.py based on the head of develop, let me know if you'd like this as a branch/PR:

diff --git a/repos/spack_repo/builtin/packages/ufs_weather_model/package.py b/repos/spack_repo/builtin/packages/ufs_weather_model/package.py
index 37f5420f..380a92f6 100644
--- a/repos/spack_repo/builtin/packages/ufs_weather_model/package.py
+++ b/repos/spack_repo/builtin/packages/ufs_weather_model/package.py
@@ -146,7 +146,7 @@ class UfsWeatherModel(CMakePackage):
     with when("@develop app=ATMAERO"):
         depends_on("mapl")
         depends_on("gftl-shared")
-    depends_on("scotch", when="+pdlib")
+    depends_on("scotch +metis build_system=cmake", when="+pdlib")
 
     depends_on("w3nco", when="@:2.0.0")
     depends_on("python", type="build", when="@:2.0.0")
@@ -208,12 +208,15 @@ class UfsWeatherModel(CMakePackage):
 
         return args
 
-    # This patch can be removed once https://github.com/NOAA-EMC/WW3/issues/1021
-    # is resolved.
-    @when("+pdlib ^scotch+shared")
+    @when("+pdlib")
     def patch(self):
-        filter_file(r"(lib[^ ]+)\.a", r"\1.so", "WW3/cmake/FindSCOTCH.cmake")
-        filter_file("STATIC", "SHARED", "WW3/cmake/FindSCOTCH.cmake")
+        filter_file(
+            "PTSCOTCHparmetis::PTSCOTCHparmetis",
+            "SCOTCH::ptscotchparmetisv3 SCOTCH::scotcherr SCOTCH::scotcherrexit",
+            "WW3/model/src/CMakeLists.txt"
+        )
+        import pathlib
+        pathlib.Path.unlink("WW3/cmake/FindSCOTCH.cmake",missing_ok=True)
 
     @run_after("install")
     def install_additional_files(self):

[JessicaMeixner-NOAA]

@AlexanderRichert-NOAA - I'm not a cmake expert, so I will trust your expertise if you think we should be updating our CMake build for scotch. If you submit a PR to WW3, we'll test and make sure it works for our non-UFS use cases as well.

We can also update the 7.0.6 -> 7.0.5 for the API changes to make that clearer.

[NickSzapiro-NOAA]

@AlexanderRichert-NOAA Your changes look cleaner and this builds
NickSzapiro-NOAA/ufs-weather-model@helper_warnings_script...NickSzapiro-NOAA:ufs-weather-model:test_warnings_ghaction_gfs
but still need static ~shared or else link fails

Is the best solution to fix this in wavewatch?

Instead of https://github.com/NOAA-EMC/WW3/blob/dev/ufs-weather-model/model/src/CMakeLists.txt#L179-L189
something like delete FindSCOTCH.cmake (which drops the scotcherr dependency) and :

if("SCOTCH" IN_LIST switches)
    # Force CMake to use native SCOTCHConfig.cmake
    find_package(SCOTCH CONFIG REQUIRED)
    target_sources(ww3_lib PRIVATE ${pdlib_src})

    # Explicitly link native SCOTCH components in order from INSTALL.txt manual.
    # https://gitlab.inria.fr/scotch/scotch/-/blob/master/INSTALL.txt#L901-922
    target_link_libraries(ww3_lib PUBLIC 
        SCOTCH::ptscotchparmetisv3 
        SCOTCH::ptscotch 
        SCOTCH::scotch 
        SCOTCH::scotcherr
    )
    
    # The Fortran API changed in 7.0.5, so apply the macro for 7.0.5 and above
    if(SCOTCH_VERSION VERSION_GREATER_EQUAL "7.0.5")
      list(APPEND compile_flags -DSCOTCH_707)
      list(APPEND compile_flags_release -DSCOTCH_707)
      list(APPEND compile_flags_debug -DSCOTCH_707)
    else()
      message(STATUS "SCOTCH version 7.0.4 or lower")
    endif()

I guess change the "DSCOTCH_707" too

[AlexanderRichert-NOAA]

Glad it worked, and yes, that makes a lot of sense as far as how to update WW3. I think having the "CONFIG REQUIRED" piece makes sense if the desire is to also keep FindSCOTCH.cmake in order to use non-CMake based versions of Scotch. On the other hand, if WW3 will only ever use CMake-based versions (7.x onward, I believe), then I think it makes sense to remove FindSCOTCH.cmake from WW3 altogether and update the target names as you've done there.

@JessicaMeixner-NOAA what are your thoughts on whether there's value in keeping FindSCOTCH.cmake as a fallback vs. scrapping it and just supporting Scotch 7.x+?

[JessicaMeixner-NOAA]

I'm concerned about supporting WW3 users that are not UFS or NOAA users. I'd like to see a PR and ask a few others to test if we're going to make a major change.

In terms of versions of SCOTCH, I had no issues before just moving to the latest version but we had issues between WCOSS2 versions and other machines - so it was actually best to support the multiple versions. SCOTCH 7.0.0 is from 2021, so I think moving to 7.0 or higher is fine -but I think at least for now we'd want to have the switch for the 7.0.5 version update.

[NickSzapiro-NOAA]

It seems better to have the changes upstream.

If it's helpful for community, here is also an untested updated FindSCOTCH.cmake in WW3:
NOAA-EMC/WW3@dev/ufs-weather-model...NickSzapiro-NOAA:WW3:ufs_findScotch

[JessicaMeixner-NOAA]

@NickSzapiro-NOAA - I've tested your WW3 changes and have no issues with them on our machines. The next step is to make a PR to develop so that we can ensure we are not breaking things for WW3 community members that are not "UFS". I'm hopeful to have something by the end of the week and feedback next week.