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

-WError abuse #512

Closed
adevress opened this issue Oct 11, 2016 · 7 comments
Closed

-WError abuse #512

adevress opened this issue Oct 11, 2016 · 7 comments

Comments

@adevress
Copy link

The file CommonCompiler.cmake force the usage of the flag -WError.

His usage is completely understandable and justified for build in "Debug" mode.
It is however a bad practice to enable it by default for release build due to its tendency to break the build each time a new compiler implement new warning reporting.

Could you please, enable this flag only when the build is configured in "Debug" mode ?

Thank you in advance,
Adev

@hernando
Copy link

hernando commented Oct 11, 2016

Some warnings, mostly related to optimizations, only occur in release builds. If we turn -Werror off in release builds chances are that they will go unnoticed for long time, for example until they cause a bug.

If I may ask, why is it such a big problem for you? I recently compiled most of your software with gcc 6.1 and there were only a couple or minor error, but no warning as fas as I can remember.

@adevress
Copy link
Author

adevress commented Oct 11, 2016

Warnings comes and go with compiler versions.

GCC 4.9.3 throws some warnings on some of your softwares for instance, even if your builds with GCC 5.2 does not.

Warnings can also pop up if you use a slightly different version of one dependency that has a change in its header.

Warnings are warnings, Error are errors and there is a reason for that. I am reduce to hack the build by enforcing XCODE_VERSION as a workaround to make this work, and this is a pity.

I maintain it is a bad practice to enable -WError by default. That's the guarantee that anyone packaging your software will run into troubles at each compiler upgrade. Nor Fedora nor Debian would accept a package that do that without patching it.

Please, if you do not want to disable it, at least provide a cmake option to disable this behavior... using XCODE_VERSION for that is very ugly.

@eile
Copy link
Member

eile commented Oct 11, 2016

We had magic to disable Werror on "released software" for this reason. The magic caused other issues, such as deformations of the lower limbs, which is why it was removed.

Imo we should have a COMMON_DISABLE_WERROR option.

Opinions?

@eile
Copy link
Member

eile commented Oct 11, 2016

@tribal-tec @rdumusc FYI

@tribal-tec
Copy link
Member

Support for different compilers other than the supported ones aka gcc 4.8 and 5.4 should be added then. Having a warning is one thing, but you might get real errors as well which have to be addressed similarly.

Warnings from out-of-our-control dependencies are a non-issue with -isystem, so I'm still wondering if it's crucial to have the option, because on a generalization one disables all warnings and hell breaks loose.

I'm not super-positive with adding the option, but it seems to be the shortest path to success.

@adevress
Copy link
Author

adevress commented Oct 11, 2016

Warnings from out-of-our-control dependencies are a non-issue with -isystem, so I'm still wondering if it's crucial to have the option, because on a generalization one disables all warnings and hell breaks loose.

-isystem unfortunatly does not solve all issues.

If you are using any component that do code generation and that your code generator is in a different version where you deploy your software, the build will explode to your face if you use -WError . That's the case for mod2c of neuron, bluron and coreneuron, ISPC of Brayns, protobuf, FLATBuffer, etc....

An option like COMMON_DISABLE_WERROR seems a good compromise to me

@hernando
Copy link

For code created by code generators a project may opt to use a specific set of compilation options for the generated source files.

I'm happy with the option as long as it's used only for packaging on unsupported platforms and assuming any derived problem is the packager responsibility.

eile pushed a commit to eile/CMake that referenced this issue Oct 11, 2016
eile pushed a commit to eile/CMake that referenced this issue Oct 11, 2016
@eile eile closed this as completed in 16da796 Oct 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants