Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat compiler warnings as errors #781

Merged
merged 7 commits into from
Apr 3, 2024
Merged

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Mar 29, 2024

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

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

Target Environment support

  • Ubuntu Linux 20.04 / gcc on Github CI
  • macOS 12 with AppleClang 14 on Github CI

@PhilMiller
Copy link
Contributor Author

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

@PhilMiller
Copy link
Contributor Author

PET update in PR #785

@PhilMiller
Copy link
Contributor Author

Topmodel update in that PR too

@PhilMiller
Copy link
Contributor Author

PhilMiller commented Apr 1, 2024

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

@PhilMiller
Copy link
Contributor Author

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.

@PhilMiller PhilMiller marked this pull request as ready for review April 2, 2024 21:59
Copy link
Contributor

@program-- program-- left a comment

Choose a reason for hiding this comment

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

May want to wait for other reviewers before merging so everyone is fully aware of the change, but looks good to me!

Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

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

This looks good to me too!

@hellkite500
Copy link
Member

hellkite500 commented Apr 3, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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.

@donaldwj donaldwj merged commit 932a862 into NOAA-OWP:master Apr 3, 2024
19 checks passed
@PhilMiller PhilMiller deleted the Phil/werror branch April 3, 2024 16:07
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