Skip to content

Do not allow code with warnings to pass CI#2544

Merged
npaun merged 2 commits intomainfrom
npaun/werror
Aug 19, 2024
Merged

Do not allow code with warnings to pass CI#2544
npaun merged 2 commits intomainfrom
npaun/werror

Conversation

@npaun
Copy link
Copy Markdown
Member

@npaun npaun commented Aug 16, 2024

This PR enables -Werror, when building in the CI. As a result, warnings are treated as a fatal error. This ensures that warnings will be addressed, and won't pile up over time in our codebase. Here are some specific details about the configuration.

  • Warnings aren't fatal locally. This is for convenience while working on code.
  • Warnings aren't fatal in external/. This is because these warnings come from our dependencies, and need to be fixed there.
  • Deprecation warnings and #warning are not fatal. They're just here to give us a heads up that something will need to be fixed eventually.
  • Warnings are only fatal on Linux builds. I didn't want to make this a maddening quest of fixing macOS, Windows AND Linux compiler warnings every time we do a PR.

@npaun npaun force-pushed the npaun/werror branch 2 times, most recently from f55b763 to a671f25 Compare August 16, 2024 17:19
@npaun npaun marked this pull request as ready for review August 16, 2024 19:12
@npaun npaun requested review from a team as code owners August 16, 2024 19:12
Copy link
Copy Markdown
Contributor

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you also update compile_flags.txt?

@danlapid
Copy link
Copy Markdown
Collaborator

danlapid commented Aug 16, 2024

Can you also update compile_flags.txt?

I think that would break "Warnings aren't fatal locally. This is for convenience while working on code."
As it will cause clangd to show error

@fhanau
Copy link
Copy Markdown
Contributor

fhanau commented Aug 16, 2024

  • Warnings aren't fatal locally. This is for convenience while working on code.
  • Warnings are only fatal on Linux builds. I didn't want to make this a maddening quest of fixing macOS, Windows AND Linux compiler warnings every time we do a PR.

I think these two aspects are key, thank you for covering them. 1) is important for development velocity, 2) means we won't have to fix the many unhelpful Windows warnings we have. Including macOS via build:unix could be a good option as both systems share the vast majority of warnings when they happen but it conceivably could make things harder for non-mac developers and is not needed at this stage.

Copy link
Copy Markdown
Contributor

@fhanau fhanau left a comment

Choose a reason for hiding this comment

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

@npaun npaun merged commit c1f8e1c into main Aug 19, 2024
@npaun npaun deleted the npaun/werror branch August 19, 2024 15:18
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.

4 participants