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

Do not allow code with warnings to pass CI #2544

Merged
merged 2 commits into from
Aug 19, 2024
Merged

Do not allow code with warnings to pass CI #2544

merged 2 commits into from
Aug 19, 2024

Conversation

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

@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?

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
@danlapid
Copy link
Contributor

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

.bazelrc Outdated Show resolved Hide resolved
@fhanau
Copy link
Collaborator

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
Collaborator

@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
9 checks passed
@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