Treat compiler warnings as errors

PhilMiller commented 7 months ago

We should guard against integrating code that introduces new warnings. There are few enough existing warnings that it's worth just fixing them in the process.

Changes

Build ngen C++ with -Werror
Build ngen C with -Werror
Fix the few warnings that appear

Notes

It's rather important that we're using fixed versions of the underlying toolchain, so that reported warnings aren't a moving target except at update time.

Checklist

[x] PR has an informative and human-readable title
[x] Changes are limited to a single goal (no scope creep)
[x] Code can be automatically merged (no conflicts)
[x] Code follows project standards (link if applicable)
[x] Passes all existing automated tests
[x] Any change in functionality is tested
[x] New functions are documented (with a description, list of inputs, and expected output)
[x] Placeholder code is flagged / future todos are captured in comments
[x] Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
[x] Reviewers requested with the Reviewers tool :arrow_right:

Target Environment support

[x] Ubuntu Linux 20.04 / gcc on Github CI
[x] macOS 12 with AppleClang 14 on Github CI

PhilMiller commented 7 months ago

This'll need updates from NOAA-OWP/evapotranspiration#35 and NOAA-OWP/topmodel#47

PhilMiller commented 7 months ago

PET update in PR #785

PhilMiller commented 7 months ago

Topmodel update in that PR too

PhilMiller commented 7 months ago

Looks like there's another round of changes to Topmodel and CFE that'll be necessary to get this through.

PhilMiller commented 7 months ago

My mistake in finding it earlier was apparently in not building with -O1 which does a bit more control and data flow analysis while compiling.

hellkite500 commented 7 months ago

I'm inclined to pick a core set of tests which we would like to run on latest, perhaps without -Werror, just to help keep an eye on upgrade paths and what is/isn't working on newer tool chains. We can setup an explicit workflow for these. I don't think it is worth running all the various tests, but maybe just the core unit tests as early indicators.

PhilMiller commented 7 months ago

I'm inclined to pick a core set of tests which we would like to run on latest, perhaps without -Werror, just to help keep an eye on upgrade paths and what is/isn't working on newer tool chains. We can setup an explicit workflow for these. I don't think it is worth running all the various tests, but maybe just the core unit tests as early indicators.

I'd be open to adding something like that. I was actually planning to open a follow-on PR trial the switch to Ubuntu 22.04, knowing that it will fail everything right now.

hellkite500 commented 7 months ago

Let's open an issue for tracking a latest build CI workflow for a subset of tests, and we can work off that and get this merged. We should probably also check our documentation on supported tool chains and make sure it is consistent. Another issue we may want to open is a plan for CI upgrades.

NOAA-OWP / ngen