search

g-truc / glm

OpenGL Mathematics (GLM)
https://glm.g-truc.net
Other
9.05k stars 2.1k forks source link

Remove -Werror in tests #1268

Closed AMDmi3 closed 5 months ago

AMDmi3 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.