Achieve and enforce warning-free builds with GNU -Wall -Werror in CI

[edwardhartnett]

We can turn on full warning checks in GNU gfortran and then clear all warnings.

Then we can turn on -Werror and ensure that no code with warnings gets committed to the repo.

[edwardhartnett]

I am still getting warnings when I build like this:

cmake -DTEST_FILE_DIR=/home/ed/bufr -DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=/usr/local/NCEPLIBS-sp.2.4.0 -DENABLE_DOCS=ON .. && make VERBOSE=1 && make test

For example:

cd /home/ed/b2/b/src && /usr/bin/gfortran -DLITTLE_ENDIAN -I/home/ed/b2/b/src -I/home/ed/b2/src -g -fbacktrace -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/b2/src/copymg.f -o CMakeFiles/bufr_4_f.dir/copymg.f.o
/home/ed/b2/src/copymg.f:85:72:

85 | SUBSET = TAG(INODE(LIN))
| 1
Warning: CHARACTER expression will be truncated in assignment (8/10) at (1) [-Wcharacter-truncation]

and

cd /home/ed/b2/b/src && /usr/bin/gfortran -DLITTLE_ENDIAN -I/home/ed/b2/b/src -I/home/ed/b2/src -g -fbacktrace -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/b2/src/cpymem.f -o CMakeFiles/bufr_4_f.dir/cpymem.f.o
/home/ed/b2/src/cpymem.f:83:72:

83 | SUBSET = TAG(INODE(LIN))
| 1
Warning: CHARACTER expression will be truncated in assignment (8/10) at (1) [-Wcharacter-truncation]

OK, the problem is that in the developer workflow, we are not using -Wall:

    cmake -DTEST_FILE_DIR=/home/runner/data -DCMAKE_INSTALL_PREFIX=./install -DCMAKE_Fortran_FLAGS="-Werror -g -fprofile-arcs -ftest-coverage -fprofile-abs-path -O0 -fsanitize=address" -DCMAKE_C_FLAGS="-Werror -g -fprofile-arcs -ftest-coverage -fprofile-abs-path -O0 -fsanitize=address" -DCMAKE_BUILD_TYPE=Debug -DENABLE_DOCS=On ..

[edwardhartnett]

OK, one problem was that I was using gcc-9 and gfortran-9. When I switch to 11 or 12 I get no warnings.

[edwardhartnett]

But one thing I notice is that we don't need this: -fallow-invalid-boz

[edwardhartnett]

The -Wall was already being included by the -DCMAKE_BUILD_TYPE=Debug.

I have upgraded the developer build to gcc-11/gfortran-11. Using 12 causes gcovr to break!

I have removed the unneeded invalid boz flag.

We are still covering many warnings with the -fallow-argument-mismatch flag. Can we remove it?

[jbathegit]

We are still covering many warnings with the -fallow-argument-mismatch flag. Can we remove it?

I honestly have no idea. I wasn't the one who put it there in the first place, so I'm not sure what it may or may not still be covering. I guess the only way to find out would be to try removing it and see what happens(?)

[edwardhartnett]

Yes, I am working on these flags now, and keeping notes in this issue. ;-)

Kyle and I were the ones who put this flag in everywhere. It was required because gfortran-10 and later started complaining about mismatched arguments in code, and that was a common practice in our code.

But it should not be, so we should strive to match all our arguments and remove this flag.

However, what is even more interesting is we are using the '-w' flag, and I don't know what that is. When I remove it, we get a lot of warnings that I also get when using gfortran-9. In other words, -w is turning off some warnings. These look easier than the mismatched arguments, so lets work on them first.

[edwardhartnett]

Here's the first set of warnings encountered after I remove the -w flag.

/home/runner/work/NCEPLIBS-bufr/NCEPLIBS-bufr/bufr/src/gettbh.f:62:14:

   62 |         IMT = VALX ( TAGS(2) )
      |              1
Error: Possible change of value in conversion from REAL(4) to INTEGER(4) at (1) [-Werror=conversion]
/home/runner/work/NCEPLIBS-bufr/NCEPLIBS-bufr/bufr/src/gettbh.f:63:15:

   63 |         IMTV = VALX ( TAGS(3) )
      |               1
Error: Possible change of value in conversion from REAL(4) to INTEGER(4) at (1) [-Werror=conversion]
/home/runner/work/NCEPLIBS-bufr/NCEPLIBS-bufr/bufr/src/gettbh.f:72:15:

   72 |         IMT2 = VALX ( TAGS(2) )
      |               1
Error: Possible change of value in conversion from REAL(4) to INTEGER(4) at (1) [-Werror=conversion]
/home/runner/work/NCEPLIBS-bufr/NCEPLIBS-bufr/bufr/src/gettbh.f:73:16:

   73 |         IOGCE = VALX ( TAGS(3) )
      |                1
Error: Possible change of value in conversion from REAL(4) to INTEGER(4) at (1) [-Werror=conversion]
/home/runner/work/NCEPLIBS-bufr/NCEPLIBS-bufr/bufr/src/gettbh.f:74:15:

   74 |         ILTV = VALX ( TAGS(4) )
      |               1
Error: Possible change of value in conversion from REAL(4) to INTEGER(4) at (1) [-Werror=conversion]
f951: all warnings being treated as errors

These look like the kind of warnings we want to find and want to fix. In the above case, these warnings can be fixed by explicitly using the int() function to covert to int, instead of relying on automatic conversion.

[jbathegit]

OK, this is now fixed in #406. The VALX routine was only ever being used to decode integers anyway, so I upgraded STRNUM to handle all of the cases that VALX had been doing, and I've now completely removed VALX from the library.

[edwardhartnett]

OK, if you remove -w from the GNU fortran flags in the top-level makefile, you will see some warnings.

Here's some:

/home/ed/NCEPLIBS-bufr/src/modules_arrs.F90:213:33:

  213 |   integer :: IDCACH(MXCNEM,MAXNC)
      |                                 1
Warning: Array ‘idcach’ at (1) is larger than limit set by ‘-fmax-stack-var-size=’, moved from stack to static storage. This makes the procedure unsafe when called recursively, or concurrently from multiple threads. Consider using ‘-frecursive’, or increase the ‘-fmax-stack-var-size=’ limit, or change the code to use an ALLOCATABLE array. [-Wsurprising]

and

cd /home/ed/NCEPLIBS-bufr/b/src && /usr/bin/gfortran-11 -DLITTLE_ENDIAN -I/home/ed/NCEPLIBS-bufr/b/src -I/home/ed/NCEPLIBS-bufr/src -Wall -g -fbacktrace -fallow-argument-mismatch -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/NCEPLIBS-bufr/src/copymg.f -o CMakeFiles/bufr_4_f.dir/copymg.f.o
/home/ed/NCEPLIBS-bufr/src/copymg.f:85:72:

   85 |       SUBSET = TAG(INODE(LIN))
      |                                                                        1
Warning: CHARACTER expression will be truncated in assignment (8/10) at (1) [-Wcharacter-truncation]

and

cd /home/ed/NCEPLIBS-bufr/b/src && /usr/bin/gfortran-11 -DLITTLE_ENDIAN -I/home/ed/NCEPLIBS-bufr/b/src -I/home/ed/NCEPLIBS-bufr/src -Wall -g -fbacktrace -fallow-argument-mismatch -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/NCEPLIBS-bufr/src/cpymem.f -o CMakeFiles/bufr_4_f.dir/cpymem.f.o
/home/ed/NCEPLIBS-bufr/src/cpymem.f:83:72:

   83 |       SUBSET = TAG(INODE(LIN))
      |                                                                        1
Warning: CHARACTER expression will be truncated in assignment (8/10) at (1) [-Wcharacter-truncation]

and

ude/bufr_4 -fPIC -c /home/ed/NCEPLIBS-bufr/src/drfini.f -o CMakeFiles/bufr_4_f.dir/drfini.f.o
/home/ed/NCEPLIBS-bufr/src/drfini.f:75:31:

   73 |          CALL X84(LUNIT,MY_LUNIT,1)
      |                                 2
   74 |          CALL X84(NDRF,MY_NDRF,1)
   75 |          CALL X84(MDRF,MY_MDRF,MY_NDRF)
      |                               1
Warning: Rank mismatch between actual argument at (1) and actual argument at (2) (scalar and rank-1)

and

cd /home/ed/NCEPLIBS-bufr/b/src && /usr/bin/gfortran-11 -DLITTLE_ENDIAN -I/home/ed/NCEPLIBS-bufr/b/src -I/home/ed/NCEPLIBS-bufr/src -Wall -g -fbacktrace -fallow-argument-mismatch -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/NCEPLIBS-bufr/src/dxinit.f -o CMakeFiles/bufr_4_f.dir/dxinit.f.o
/home/ed/NCEPLIBS-bufr/src/dxinit.f:77:72:

   77 |       TABB(I,LUN)(  1:  6) = INIB(1,I)
      |                                                                        1
Warning: CHARACTER expression will be truncated in assignment (6/8) at (1) [-Wcharacter-truncation]
/home/ed/NCEPLIBS-bufr/src/dxinit.f:80:72:

   80 |       TABB(I,LUN)( 95: 98) = INIB(4,I)
      |                                                                        1
Warning: CHARACTER expression will be truncated in assignment (4/8) at (1) [-Wcharacter-truncation]
/home/ed/NCEPLIBS-bufr/src/dxinit.f:82:72:

   82 |       TABB(I,LUN)(110:112) = INIB(6,I)
      |                                                                        1
Warning: CHARACTER expression will be truncated in assignment (3/8) at (1) [-Wcharacter-truncation]
[  8%] Building Fortran object src/CMakeFiles/bufr_4_f.dir/dxmini.f.o
cd /home/ed/NCEPLIBS-bufr/b/src && /usr/bin/gfortran-11 -DLITTLE_ENDIAN -I/home/ed/NCEPLIBS-bufr/b/src -I/home/ed/NCEPLIBS-bufr/src -Wall -g -fbacktrace -fallow-argument-mismatch -ggdb -Wall -O0 -Jinclude/bufr_4 -fPIC -c /home/ed/NCEPLIBS-bufr/src/dxmini.f -o CMakeFiles/bufr_4_f.dir/dxmini.f.o
/home/ed/NCEPLIBS-bufr/src/dxmini.f:22:27:

   22 |       SUBROUTINE DXMINI(LUN,MBAY,MBYT,MB4,MBA,MBB,MBD)
      |                           1
Warning: Unused dummy argument ‘lun’ at (1) [-Wunused-dummy-argument]

[jbathegit]

I've got everything done now so that we can remove the -w flag, and I'll push up a PR within the next hour. The only thing that's still warning is the following line in outtest8.F90, and for the life of me I can't figure out why:

   72 |   nbyt = int(lenbmg,4) * 8
      |         1
Warning: Possible change of value in conversion from INTEGER(8) to INTEGER(4) at (1) [-Wconversion]

This sort of conversion works fine inside of the library (in the src directory) to get rid of the same warning in numerous places, but for whatever reason it doesn't work here in this one test code. I've tried a number of hacks and variations to try to get around this, including declaring a separate integer*4 variable nwrd, and then calling x84 with lenbmg as input and nwrd as output and then just setting nbyt=nwrd*8. But this still leads to the same warning:

   72 |   nbyt = nwrd * 8
      |         1
Warning: Possible change of value in conversion from INTEGER(8) to INTEGER(4) at (1) [-Wconversion]

which makes no sense to me b/c I honestly have no idea why the compiler would think that either nbyt or nwrd was an integer*8 value in this situation.

For now, I'm just going to hardcode a workaround into outtest8.F90 with a note referring to this issue, and hopefully we can get to the bottom of it at some point. But this way we can at least move forward with turning off the -w flag in CMakeLists.txt.

[edwardhartnett]

Can we close this issue now?

[jbathegit]

I had left this open b/c of the lingering issue in outtest8.F90 that's still unresolved. But as I noted, there is a workaround in place, and the issue is documented inside of the test code, so I guess we can go ahead and close this now.

[edwardhartnett]

Is there a CI run with GNU compilers using -Werror -Wall?

If so, I don't see it. We need to have a warning check in place.

[jbathegit]

Rest assured this is already being done, albeit indirectly so admittedly it can be a bit confusing :-)

Specifically, the developer.yaml CI run has the following in its build step:

cmake -DTEST_FILE_DIR=/home/runner/data -DCMAKE_INSTALL_PREFIX=./install -DCMAKE_Fortran_FLAGS="-Werror -g -fprofile-arcs -ftest-coverage -fprofile-abs-path -O0 -fsanitize=address -fno-omit-frame-pointer" -DCMAKE_C_FLAGS="-Werror -g -fprofile-arcs -ftest-coverage -fprofile-abs-path -O0 -fsanitize=address -fno-omit-frame-pointer" -DCMAKE_BUILD_TYPE=Debug -DENABLE_DOCS=OFF ..

There are two relevant points here. First off, you can see where the -Werror flag is already directly included in the CMAKE_C_FLAGS and CMAKE_Fortran_FLAGS of this cmake command. Secondly, and for the indirect part, note that we also have CMAKE_BUILD_TYPE=Debug, which in turn triggers the following in the main CMakelists.txt file:

for C:

elseif(CMAKE_C_COMPILER_ID MATCHES "^(GNU)$")
  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -g")
  set(CMAKE_C_FLAGS_DEBUG "-ggdb -Wall -O0")
  set(CMAKE_C_FLAGS_RELEASE "-O3")

for Fortran:

elseif(CMAKE_Fortran_COMPILER_ID MATCHES "^(GNU)$")
  set(fortran_8_flags "-fdefault-real-8 -fdefault-integer-8")
  set(fortran_d_flags "-fdefault-real-8")
  set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -g -fbacktrace")
  set(CMAKE_Fortran_FLAGS_DEBUG "-ggdb -Wall -O0")
  set(CMAKE_Fortran_FLAGS_RELEASE "-O3")

So bottom line, both the -Werror and -Wall flags are indeed being included in the developer.yaml build.