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

Remove -Werror in tests #1268

Closed
wants to merge 1 commit into from
Closed

Remove -Werror in tests #1268

wants to merge 1 commit into from

Conversation

AMDmi3
Copy link

@AMDmi3 AMDmi3 commented Mar 1, 2024

You should not define -Werror in the build system (though it may be defined in CI). This one prevents building glm with tests enabled and requires patching when packaging.

@christophe-lunarg
Copy link

It doesn't seem reasonable to just remove -Werror for the unit tests as new warnings in PR would not cause C.I. to fail. Instead I am creating a PR that disable the unit tests build by default.

@AMDmi3
Copy link
Author

AMDmi3 commented Mar 5, 2024

Please reopen.

It doesn't seem reasonable to just remove -Werror for the unit tests as new warnings in PR would not cause C.I. to fail

That is not correct, as you may and should add -Werror (only) in CI.

Instead I am creating a PR that disable the unit tests build by default.

That would not solve the problem, as when packaging tests are usually built and run regardless of default setting, and with -Werror the build will fail.

@christophe-lunarg
Copy link

But if that fail with -Werror, it's because there is an issue that should be fix right?

Application developers would use -Werror in their application as numerous reports shown in the past so that needs to be checked.

@AMDmi3
Copy link
Author

AMDmi3 commented Mar 5, 2024

But if that fail with -Werror, it's because there is an issue that should be fix right?

Obviously. But do you want for force downstream to report warnings with -Werror? Well it will not work - downstream will just patch it away and you'll be known as a bad actor, so no gain at the price of extra pain. Which is particularly bad about -Werror is that even if it doesn't fire right now, it may fire after compiler or dependency update, resulting in spurious build failure. Nobody likes that.

Application developers would use -Werror in their application as numerous reports shown in the past so that needs to be checked.

Yes it's common mistake and there are tons of articles explaining why this is a bad idea.

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.

2 participants