build: add --enable-isystem and change --enable-werror to enable -Werror#18149
build: add --enable-isystem and change --enable-werror to enable -Werror#18149vasild wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
vasild
force-pushed
the
werror
branch
5 times, most recently
from
4bdc0a6 to
f1f8e84
Compare
configure.ac
Outdated
There was a problem hiding this comment.
Why not isystem? It would more closely match the behavior of the -I it replaces.
https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html
There was a problem hiding this comment.
I just rebased on top of 572db3c and ditched the above piece.
There was a problem hiding this comment.
Here's a much pared-down version of 572db3c that looks to be effective (applied to Boost only):
master...Empact:2020-02-werror
There was a problem hiding this comment.
master...Empact:2020-02-werror looks good. I confirm it works as expected on FreeBSD 12 with clang 9 - it makes it possible to compile Bitcoin Core with full -Werror! \o/
Are you going to create PR from it? If yes, then I will ditch this PR.
nit: the new option's description enable isystem includes for dependencies and stricter warning checks (default is no) warrants s/ and stricter warning checks//
Thanks!
There was a problem hiding this comment.
Feel free to cherry-pick and modify the subject. Deferring to you as the original author unless you want to punt.
There was a problem hiding this comment.
Done. I also picked up the QT changes to get this working also with Clang 10.
vasild
changed the title
|
@hebasto this PR went through some significant changes, I edited my comments accordingly and changed its status from "Draft" to "Open". Your re-review would be welcome. |
When configured with --enable-isystem.
Was necessary to split QT_INCLUDES into QT_INCLUDES and
QT_MOC_INCLUDES because moc does not understand -isystem, e.g.:
Unknown options: isystem/usr/local/Cellar/qt/5.10.0_1/include/QtNetwork[...]
This patch treats just Boost and QT headers. The other external headers could be
added as well, should it be necessary.
Note -isystem should not be applied to /usr/include, see BITCOIN_SYSTEM_INCLUDE
for a helper to convert -I to -isystem with /usr/include excepted.
Before this patch `./configure --enable-werror` would selectively turn only some warnings into errors, not all, by using `-Werror=foo -Werror=bar` instead of a plain `-Werror` which turns all warnings into errors. To accommodate the new stricter build config, use --enable-isystem to suppress warnings from external headers (e.g. boost, qt) on systems that have them installed outside of /usr/include. On Linux the external headers are typically installed in /usr/include and so warnings from them are suppressed by default.
|
Closing this as it contains two not directly related changes - optional suppression of warnings from external headers and -Werror expansion. I will submit them separately. |
|
Took the idea of the first commit and re-implemented it in a (hopefully) better/shorter way in #18750 |
4 participants
Before this patch
./configure --enable-werrorwould selectivelyturn only some warnings into errors, not all, by using
-Werror=foo -Werror=barinstead of a plain-Werrorwhich turns allwarnings into errors.
To accommodate the new stricter build config, introduce
--enable-isystemto optionally suppress warnings from external headers(e.g. boost, qt). That may be useful on systems that have them installed
outside of
/usr/include. On Linux the external headers are typicallyinstalled in
/usr/includeand so warnings from them are always suppressed.