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

Feedback: should we use -Werror & /WX only in CI? #140

Closed
zeux opened this issue Nov 5, 2021 · 2 comments · Fixed by #201
Closed

Feedback: should we use -Werror & /WX only in CI? #140

zeux opened this issue Nov 5, 2021 · 2 comments · Fixed by #201
Labels
build Build system or platform compatibility issue

Comments

@zeux
Copy link
Collaborator

zeux commented Nov 5, 2021

Right now both Makefile and CMakeLists.txt treat warnings as errors unconditionally.

In general we'd like to be warning-free if possible. But due to the multitude of compilers and compiler versions this isn't a guarantee that is going to be easy for us to provide. We'll keep hitting random warnings from random compilers on random architectures.

Right now when a warning like that is encountered, it blocks progress for the users or distributions. Which doesn't seem ideal.
Of course if we simply remove these flags, we'll just start accumulating warnings on our primary supported systems which is not healthy.

Considering adding flags to both build infras to enable warnings-as-errors and keep the off by default, but enable in CI. The downside is that developers may ignore these when developing locally, but the CI will catch it so it's probably fine. Some warnings due to build misconfiguration may be more important than others, so it's not always safe to ignore all of them either...

Looking for feedback. I know different projects have different sensibilities around this.

@zeux zeux added the build Build system or platform compatibility issue label Nov 5, 2021
@MathematicalDessert
Copy link
Contributor

MathematicalDessert commented Nov 5, 2021

I think that direction makes sense. But perhaps WAS (warnings-as-errors) should also be enabled by default for all builds, but make it easy for developers to disable via flag as you mentioned.

Following this direction assumes that developers using different compilers will eventually resolve warnings as they come (and hopefully this is the case). It's not ideal but I think it's important to have them as errors to enforce good code standards.

@vegorov-rbx
Copy link
Collaborator

I believe we should disable warnings as errors by default.
We will always be behind the new warnings that are developed in compiler updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system or platform compatibility issue
Development

Successfully merging a pull request may close this issue.

3 participants