Build: enable -Wdocumentation via isystem #14920

pull Empact wants to merge 2 commits into bitcoin:master from Empact:isystem changing 7 files +55 −12
  1. Empact commented at 6:00 am on December 11, 2018: member

    This converts some dependency includes to use -isystem, so that we can enable additional compiler checks and warnings independent of the conformance of those dependencies.

    The included new check is -Wdocumentation, which was previously proposed under #13914. See #14103 for examples of issues this has identified.

    Here’s a significant list of other clang checks we could consider given this. Some additional checks would require additional -isystem conversions, e.g. LevelDB is not converted here: -Wreserved-id-macro -Wthread-safety-attributes #15556 -Wthread-safety-negative -Wfloat-equal -Wfloat-conversion -Wsign-conversion -Wstring-conversion -Wcast-qual -Wcast-align -Wshorten-64-to-32 -Wconversion -Wfloat-conversion -Wdouble-promotion -Wshadow #15377 -Wshadow-field-in-constructor -Wunused-exception-parameter -Wunreachable-code-break -Wdocumentation-unknown-command -Wgnu-zero-variadic-macro-arguments -Wgnu-anonymous-struct -Wglobal-constructors -Wexit-time-destructors -Wnon-virtual-dtor -Wweak-vtables -Wcomma -Wmissing-variable-declarations -Wundefined-func-template -Wzero-as-null-pointer-constant #15112 -Winconsistent-missing-destructor-override -Wswitch-enum -Wcovered-switch-default -Wmissing-noreturn -Wextra-semi -Wc++17-extensions -Wpadded -Wold-style-cast -Wmissing-prototypes

  2. fanquake added the label Build system on Dec 11, 2018
  3. fanquake requested review from theuni on Dec 11, 2018
  4. DrahtBot commented at 6:19 am on December 11, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18397 (build: Fix libevent linking for bench_bitcoin binary by hebasto)
    • #18361 (build: Make the help string for –with-gui configure option unambiguous by hebasto)
    • #18307 (build: Require pkg-config for all of the hosts by hebasto)
    • #18298 (build: Fix Qt processing of configure script for depends with DEBUG=1 by hebasto)
    • #18297 (build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts including Windows by hebasto)
    • #18216 (test, build: Enable -Werror=sign-compare by Empact)
    • #18149 (build: add –enable-isystem and change –enable-werror to enable -Werror by vasild)
    • #16883 (WIP: Qt: add QML based mobile GUI by icota)

    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.

  5. practicalswift commented at 7:18 am on December 11, 2018: contributor

    Concept ACK

    Thanks for working on this!

  6. laanwj commented at 11:28 am on December 11, 2018: member

    I think this can be useful for warnings.

    Which compilers/versions is -isystem compatible with? does it need to be optional/tested?

  7. MarcoFalke added the label Needs gitian build on Dec 11, 2018
  8. Empact commented at 11:54 pm on December 11, 2018: member

    For GCC, it has been available since 1994, the egcs days. https://github.com/gcc-mirror/gcc/commit/0330c077aec77a65faee5ba84eb84c721f92c223

    For Clang, has been in the codebase since at least 2009, coinciding with 2.7, though it wasn’t mentioned in the release notes I checked. https://github.com/llvm-mirror/clang/commit/33a33d8abd7a3d49eacc05e40c00b00634bf1ee9

    MSVC has had a version of this since 2017, but AFAIK this should not affect those builds. https://blogs.msdn.microsoft.com/vcblog/2017/12/13/broken-warnings-theory/

  9. Empact commented at 1:01 am on December 12, 2018: member
    Another option is -cxx-isystem, but I’m not sure there’s a good reason to prefer it. Does build with all -isystem replaced with -cxx-isystem under current clang.
  10. fanquake commented at 4:23 pm on December 12, 2018: member
    Concept ACK Compiled on macOS, this doesn’t have the problem with warnings that #13914 did.
  11. DrahtBot removed the label Needs gitian build on Dec 12, 2018
  12. laanwj commented at 12:41 pm on December 13, 2018: member

    For GCC, it has been available since 1994, the egcs days. gcc-mirror/gcc@0330c07 For Clang, has been in the codebase since at least 2009, coinciding with 2.7, though it wasn’t mentioned in the release notes I checked. llvm-mirror/clang@33a33d8

    TIL! Thanks for checking. That’s far back enough, given that the minimum releases supported according to dependencies.md are gcc 4.8 and clang 3.3.

    MSVC has had a version of this since 2017, but AFAIK this should not affect those builds. https://blogs.msdn.microsoft.com/vcblog/2017/12/13/broken-warnings-theory/

    Right—MSVC is a different beast.

  13. laanwj commented at 1:02 pm on December 13, 2018: member
    utACK—would be nice if @theuni could take a look at the build system changes.
  14. Empact force-pushed on Dec 14, 2018
  15. Empact force-pushed on Dec 14, 2018
  16. Empact force-pushed on Dec 14, 2018
  17. Empact commented at 11:39 am on December 14, 2018: member

    In Makefile.leveldb.include, I split LEVELDB_CPPFLAGS_INT from LEVELDB_CPPFLAGS, and only gave the latter isystem treatment. The former is used for building leveldb as a lib, in which case the includes are not system includes.

    I dropped the leveldb changes as leveldb is built with the warning options via AM_CXXFLAGS. Can be addressed separately as needed.

  18. Empact force-pushed on Dec 15, 2018
  19. Empact force-pushed on Dec 16, 2018
  20. Empact force-pushed on Dec 18, 2018
  21. Empact force-pushed on Dec 31, 2018
  22. Empact force-pushed on Jan 2, 2019
  23. fanquake commented at 11:21 am on January 4, 2019: member

    Looks like this has issues on two of the Travis linux builds (re-ran to make sure).

    build:

    0checking miniupnpc/upnpcommands.h usability... yes
1checking miniupnpc/upnpcommands.h presence... yes
2checking for miniupnpc/upnpcommands.h... yes
3checking for upnpDiscover in -lminiupnpc... (cached) yes
4checking miniupnpc/upnperrors.h usability... yes
5checking miniupnpc/upnperrors.h presence... yes
6checking for miniupnpc/upnperrors.h... yes
7checking for upnpDiscover in -lminiupnpc... (cached) yes
8../configure: 25050: ./configure.lineno: Syntax error: redirection unexpected

    build:

    0checking whether the Boost::System library is available... no
1checking whether the Boost::Filesystem library is available... no
2checking whether the Boost::Thread library is available... no
3checking whether the Boost::Chrono library is available... no
4checking whether the Boost::Unit_Test_Framework library is available... no
5checking for dynamic linked boost test... no
6checking for mismatched boost c++11 scoped enums... ok
7configure: error: No working boost sleep implementation found.
  24. Empact force-pushed on Jan 4, 2019
  25. Empact force-pushed on Jan 4, 2019
  26. Empact force-pushed on Jan 5, 2019
  27. Empact force-pushed on Jan 5, 2019
  28. Empact force-pushed on Jan 5, 2019
  29. Empact commented at 7:10 am on January 5, 2019: member

    Thanks, the redirection issue was a dash incompatibility, and the boost issue was a case of -isystem/usr/include (see below). I updated any include that could resolve to /usr/include to run through the newBITCOIN_SYSTEM_INCLUDE function.

    This is ready for review.

     0configure:29481: checking whether the Boost::System library is available
 1configure:29506: clang++ -std=c++11 -c  -DDEBUG_LOCKORDER -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -isystem/usr/include conftest.cpp >&5
 2In file included from conftest.cpp:76:
 3In file included from /usr/include/boost/system/error_code.hpp:14:
 4In file included from /usr/include/boost/system/config.hpp:13:
 5In file included from /usr/include/boost/config.hpp:57:
 6In file included from /usr/include/boost/config/platform/linux.hpp:15:
 7/usr/bin/../lib/gcc/x86_64-linux-gnu/7.3.0/../../../../include/c++/7.3.0/cstdlib:75:15: fatal error: 'stdlib.h' file not found
 8#include_next <stdlib.h>
 9              ^~~~~~~~~~
101 error generated.

    https://travis-ci.org/bitcoin/bitcoin/jobs/474239063#L6175

  30. Empact force-pushed on Jan 5, 2019
  31. Empact force-pushed on Jan 5, 2019
  32. practicalswift commented at 10:14 am on January 5, 2019: contributor
    utACK 72878aac48e87e17646fa1889c554e229d76c7bb
  33. MarcoFalke deleted a comment on Jan 5, 2019
  34. Empact force-pushed on Jan 7, 2019
  35. Empact commented at 7:03 pm on January 7, 2019: member
    Added Qt Dbus and Qt Test isystem treatment.
  36. practicalswift commented at 9:07 pm on January 7, 2019: contributor
    utACK d4baebfc06bf375b64f30298b685b4d66f32cae8
  37. theuni commented at 11:52 pm on January 8, 2019: member

    I’m really not a fan of disabling warnings for 3rd party headers, but I’m not completely opposed. I’m more afraid that this will be one of those changes that causes lots of subtle future grief due to edge-cases in libtool, automake, ccache, non-glibc builds, dependency generation (gcc’s “-M*” options), etc.

    Clang has the “system-header-prefix” option, which could be hooked up to depends with just a few lines. It doesn’t seem to exist in GCC yet, but I’d be happy to add and upstream that feature.

    Thoughts on that approach?

  38. Empact commented at 0:20 am on January 9, 2019: member
    Can you elaborate on what sorts of issues you expect? One option is to apply isystem as a build option (by running everything through BITCOIN_SYSTEM_INCLUDE). Then we could enable the stricter checks only if isystem is enabled, but keep the existing arrangement otherwise.
  39. Empact force-pushed on Jan 9, 2019
  40. Empact force-pushed on Jan 9, 2019
  41. Empact commented at 3:14 am on January 9, 2019: member

    Updated to put these changes behind --enable-isystem. Also added -Werror=documentation for when --enable-werror is also specified.

    The idea is to eventually error on all warnings when both are specified, so we can have a travis run which strictly applies all warnings.

  42. in configure.ac:517 in eddb7098e4 outdated 
    522@@ -507,10 +523,12 @@ case $host in
523        AC_CHECK_PROG([PORT],port, port)
524        if test x$PORT = xport; then
525          dnl add default macports paths
526-         CPPFLAGS="$CPPFLAGS -isystem /opt/local/include"
    Empact commented at 3:16 am on January 9, 2019:
    Note I changed this for consistency with the option.
    theuni commented at 11:57 pm on January 10, 2019:
    The entire port section can be removed. I’m not sure if macports is even maintained anymore, and we for sure no longer make any attempt to keep it working.
    Empact commented at 9:10 am on January 16, 2019:
    Opened #15175 for separate consideration.
    Empact commented at 4:57 am on January 20, 2019:
    Now merged. 👍
  43. Empact force-pushed on Jan 9, 2019
  44. Empact force-pushed on Jan 9, 2019
  45. in build-aux/m4/bitcoin_qt.m4:433 in d88dc48a66 outdated 
    429@@ -430,17 +430,17 @@ AC_DEFUN([_BITCOIN_QT_FIND_LIBS_WITH_PKGCONFIG],[
430     QT_LIB_PREFIX=Qt5
431     qt5_modules="Qt5Core Qt5Gui Qt5Network Qt5Widgets"
432     BITCOIN_QT_CHECK([
433-      PKG_CHECK_MODULES([QT5], [$qt5_modules], [QT_INCLUDES="$QT5_CFLAGS"; QT_LIBS="$QT5_LIBS" have_qt=yes],[have_qt=no])
434+      PKG_CHECK_MODULES([QT5], [$qt5_modules], [BITCOIN_SYSTEM_INCLUDE([QT_INCLUDES], [$QT5_CFLAGS]) QT_MOC_INCLUDES="$QT5_CFLAGS" QT_LIBS="$QT5_LIBS" have_qt=yes],[have_qt=no])
    theuni commented at 11:25 pm on January 10, 2019:

    Missing ;s between assignments. I’m actually not sure why have_qt works.

    (These are missing in a few other places too, but I won’t bother pointing out each one)

    Empact commented at 9:23 am on January 16, 2019:
    Added ;, except after BITCOIN_SYSTEM_INCLUDE, which evaluates to an if statement.
  46. in build-aux/m4/bitcoin_subdir_to_include.m4:15 in d88dc48a66 outdated 
    11@@ -12,7 +12,8 @@ AC_DEFUN([BITCOIN_SUBDIR_TO_INCLUDE],[
12     newinclpath=`${CXXCPP} ${CPPFLAGS} -M conftest.cpp 2>/dev/null | [ tr -d '\\n\\r\\\\' | sed -e 's/^.*[[:space:]:]\(\/[^[:space:]]*\)]$3[\.h[[:space:]].*$/\1/' -e t -e d`]
13     AC_MSG_RESULT([${newinclpath}])
14     if test "x${newinclpath}" != "x"; then
15-      eval "$1=\"\$$1\"' -I${newinclpath}'"
16+      BITCOIN_SYSTEM_INCLUDE([newincl], ["-I${newinclpath}"])
    theuni commented at 11:29 pm on January 10, 2019:
    To be safe, I believe you need to AC_INCLUDE or AC_REQUIRE BITCOIN_SYSTEM_INCLUDE first.
    Empact commented at 6:43 am on January 16, 2019:
    I’m skeptical, I don’t find docs or use of AC_INCLUDE, and AC_REQUIRE is about requiring that a function be called. I think this is good thanks to AC_CONFIG_MACRO_DIR. https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Prerequisite-Macros.html https://www.gnu.org/software/automake/manual/html_node/Local-Macros.html
    theuni commented at 11:22 pm on January 22, 2019:
    Yes, you’re right.
  47. in build-aux/m4/bitcoin_system_include.m4:15 in d88dc48a66 outdated 
    10+AC_DEFUN([BITCOIN_SYSTEM_INCLUDE],[
11+  if test "x$use_isystem" = "xyes" && test "x$2" != "x"; then
12+    dnl Including /usr/include with -isystem is known to cause problems, e.g.
13+    dnl breaking include_next due to, the directive changing inclusion order.
14+    dnl https://bugreports.qt.io/browse/QTBUG-53367
15+    $1=$(echo $2 | sed -e 's|-I|-isystem|g' -e 's|-isystem/usr/include|-I/usr/include|g')
    theuni commented at 11:43 pm on January 10, 2019:
    /usr/include is not sufficient, that won’t work for cross toolchains. Instead, the compiler’s build-in system paths will need to be dumped using something like ${CXX} -x c++ -Wp,-v < /dev/null, and added iteratively.
    theuni commented at 11:50 pm on January 10, 2019:
    This would also break if any include dir ever includes an “-I”, like: /home/Foo-I/
    Empact commented at 9:20 am on January 16, 2019:
    Fixed the -I issue, and I’d prefer to leave the cross toolchain issue aside. The goal being CI runs, initially, then perhaps broad support in the future.
  48. in configure.ac:377 in d88dc48a66 outdated 
    330@@ -325,6 +331,16 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
331   AX_CHECK_COMPILE_FLAG([-Wunused-local-typedef],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-local-typedef"],,[[$CXXFLAG_WERROR]])
332   AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
333   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
334+
335+  ## Some warnings alert on violations in our dependencies. Only enable them when
    theuni commented at 11:52 pm on January 10, 2019:
    Could you move this inside of the enable_werror check above to keep related things in the same place?
    Empact commented at 9:03 am on January 16, 2019:
    Fixed
  49. in configure.ac:348 in d88dc48a66 outdated 
    336+  ## isystem is in use to keep the build clean
337+  if test "x$use_isystem" = "xyes"; then
338+    if test "x$enable_werror" = "xyes" && test "x$CXXFLAG_WERROR" != "x"; then
339+      AX_CHECK_COMPILE_FLAG([-Werror=documentation],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=documentation"],,[[$CXXFLAG_WERROR]])
340+    else
341+      AX_CHECK_COMPILE_FLAG([-Wdocumentation],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"],,[[$CXXFLAG_WERROR]])
    theuni commented at 11:55 pm on January 10, 2019:
    This needs to check against -Wno-documentation instead. See the note above.
    Empact commented at 9:22 am on January 16, 2019:
    I believe this is correct - it’s the non-werror, with-isystem case. So it activates -Wdocumentation as a non-error.
  50. theuni commented at 0:16 am on January 11, 2019: member

    @Empact I gave a few examples above about possible weird edge cases. Qt demonstrates another with include_next. Non-system toolchains and sysroots containing -I are two others.

    I’m really uneasy about this due to the diversity of compilation environments. But other than the things I pointed out, it looks ok to me on paper.

    -0.

  51. in build-aux/m4/ax_boost_base.m4:150 in d88dc48a66 outdated 
    146@@ -147,6 +147,7 @@ if test "x$want_boost" = "xyes"; then
147     if test "$ac_boost_lib_path" != ""; then
148        BOOST_LDFLAGS="-L$ac_boost_lib_path"
149     fi
150+    BITCOIN_SYSTEM_INCLUDE([BOOST_CPPFLAGS], [$BOOST_CPPFLAGS])
    theuni commented at 0:30 am on January 11, 2019:
    These changes should be done outside of ax_boost_base.m4. This is an upstream file.
    Empact commented at 9:03 am on January 16, 2019:
    Fixed
  52. theuni commented at 0:32 am on January 11, 2019: member
    On second thought, as long as it’s off by default, concept ACK. Thanks for the compromise :)
  53. DrahtBot added the label Needs rebase on Jan 13, 2019
  54. Empact force-pushed on Jan 14, 2019
  55. DrahtBot removed the label Needs rebase on Jan 14, 2019
  56. Empact force-pushed on Jan 14, 2019
  57. Empact force-pushed on Jan 16, 2019
  58. Empact force-pushed on Jan 16, 2019
  59. DrahtBot added the label Needs rebase on Jan 19, 2019
  60. Empact force-pushed on Jan 20, 2019
  61. Empact commented at 4:57 am on January 20, 2019: member
    Rebased for #15175
  62. fanquake removed the label Needs rebase on Jan 20, 2019
  63. practicalswift commented at 8:32 am on January 20, 2019: contributor

    utACK 1433ed22dad8940e1339d741039d5882196e37b0

    What about enabling isystem for the macOS 10.10 Travis job? That way we’ll guard against broken Doxygen comments getting merged.

     0diff --git a/.travis.yml b/.travis.yml
 1index 6181726fb..bdde7bbf7 100644
 2--- a/.travis.yml
 3+++ b/.travis.yml
 4@@ -132,9 +132,9 @@ jobs:
 5       name: 'macOS 10.10  [GOAL: deploy]'
 6       env: >-
 7         HOST=x86_64-apple-darwin14
 8         PACKAGES="cmake imagemagick libcap-dev librsvg2-bin libz-dev libbz2-dev libtiff-tools python-dev python3-setuptools-git"
 9         OSX_SDK=10.11
10         RUN_UNIT_TESTS=false
11         RUN_FUNCTIONAL_TESTS=false
12         GOAL="deploy"
13-        BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror"
14+        BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror -enable-isystem"
  64. Empact force-pushed on Jan 20, 2019
  65. laanwj commented at 12:27 pm on January 22, 2019: member

    I think this caused the MacOSX job to fail (restarted it just in case it’s a transient error)

    Edit: apparently that was the case it’s passing now

  66. Empact commented at 5:55 pm on January 22, 2019: member
    @theuni would you take another look? I believe I’ve addressed your feedback.
  67. practicalswift commented at 9:25 pm on January 22, 2019: contributor

    utACK 535bf5a4f4a9a5a99e188a2f38e9d129e61e5b94

    Very nice! Let’s get this merged! :-)

  68. in configure.ac:351 in 535bf5a4f4 outdated 
    344@@ -332,6 +345,12 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
345   AX_CHECK_COMPILE_FLAG([-Wunused-local-typedef],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-local-typedef"],,[[$CXXFLAG_WERROR]])
346   AX_CHECK_COMPILE_FLAG([-Wdeprecated-register],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-register"],,[[$CXXFLAG_WERROR]])
347   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
348+
349+  ## Some warnings alert on violations in our dependencies. Only enable them when
350+  ## isystem is in use to keep the build clean
351+  if test "x$use_isystem" = "xyes" && test "x$werror_enabled" == "x"; then
    theuni commented at 11:16 pm on January 22, 2019:
    You can just use test "x$enable_werror" = xyes here, no need for the new variable.
    Empact commented at 9:42 am on January 23, 2019:
    Ah good point - I dropped werror_enabled, as it was extraneous. I do think testing that isystem is active & werror is inactive is appropriate here, as this is the warn-but-don’t-error isystem case. Added some comments to further clarify and connect the cases.
  69. theuni commented at 11:56 pm on January 22, 2019: member

    I believe you’ll need to remove these two lines before this works correctly with depends builds: https://github.com/bitcoin/bitcoin/blob/master/depends/config.site.in#L61

    That should be OK, because all depends should be found anyway without requiring those paths.

  70. theuni commented at 11:57 pm on January 22, 2019: member

    Ok, thinking about it more, the cross-compile issue probably isn’t that big of a deal, because those cross-compiler include paths are very unlikely to be tacked on by any part of the build process.

    utACK after addressing the last round of comments.

  71. Empact force-pushed on Jan 23, 2019
  72. Empact force-pushed on Jan 23, 2019
  73. theuni commented at 7:31 pm on January 23, 2019: member

    I believe you’ll need to remove these two lines before this works correctly with depends builds: https://github.com/bitcoin/bitcoin/blob/master/depends/config.site.in#L61

    That should be OK, because all depends should be found anyway without requiring those paths.

    Nevermind on this. The -isystem that inevitably gets added later will override the “-I” added here.

    I’ll remove these lines as part of another cleanup.

  74. practicalswift commented at 8:30 am on January 24, 2019: contributor
    utACK c0ee524b77eacd3e7a2c31be9a722c467d876293
  75. MarcoFalke added the label Needs gitian build on Jan 25, 2019
  76. DrahtBot commented at 6:54 pm on January 26, 2019: member

    Gitian builds for commit ab46fe6ec1b37e88c5a06ee7a06ab31cbd18304f (master):

    Gitian builds for commit 99ea0384775ef7fde133f0dabfd7814ac8ec6078 (master and this pull):

  77. DrahtBot removed the label Needs gitian build on Jan 26, 2019
  78. Empact force-pushed on Feb 9, 2019
  79. Empact commented at 11:48 pm on February 9, 2019: member
    Rebased due to conflicts in #15377
  80. Empact force-pushed on Mar 5, 2019
  81. Empact commented at 5:37 am on March 5, 2019: member
    Rebased due to conflicts in #15112
  82. Empact force-pushed on Mar 7, 2019
  83. Empact commented at 3:22 pm on March 7, 2019: member
    Fix documentation errors added in #14978 bd0dbe8763fc3029cf96531c9ccaba280b939445
  84. Empact force-pushed on Mar 7, 2019
  85. Empact commented at 4:23 pm on March 7, 2019: member
    Fix more documentation errors, including some added in #14021.
  86. practicalswift commented at 8:12 pm on March 7, 2019: contributor

    utACK b51f4e14137e45d8d52fbf0b925287dde45ee97b

    Let’s move forward with this one :-)

  87. Empact commented at 1:42 pm on June 5, 2019: member
    @theuni could you take another look at this?
  88. Empact force-pushed on Jun 5, 2019
  89. Empact force-pushed on Jun 5, 2019
  90. Empact commented at 2:08 pm on June 5, 2019: member
    Rebased, squashed the latter two commits, fixed a net_processing.cpp doc error introduced in #15141
  91. Empact commented at 2:10 pm on June 5, 2019: member
    @dongcarl could you take a look at this, given your recent build investigations?
  92. DrahtBot added the label Needs rebase on Aug 2, 2019
  93. Sjors commented at 11:38 am on November 20, 2019: member

    Concept ACK

    Needs rebase now that openssl and protobuf are gone.

  94. Sjors commented at 11:50 am on November 20, 2019: member
    Consider cherry-picking the reverse of f327e82b0266b9ac455c8b82434528d670eee6a8 (but moved under the use_isystem" = "xyes" check)
  95. Empact force-pushed on Jan 17, 2020
  96. DrahtBot removed the label Needs rebase on Jan 17, 2020
  97. laanwj referenced this in commit 5d2ff75e20 on Jan 20, 2020
  98. sidhujag referenced this in commit fef8e0ea0b on Jan 24, 2020
  99. DrahtBot added the label Needs rebase on Feb 16, 2020
  100. Empact force-pushed on Feb 20, 2020
  101. DrahtBot removed the label Needs rebase on Feb 20, 2020
  102. in build-aux/m4/bitcoin_system_include.m4:12 in 875f8e2c24 outdated 
     6+dnl Populates the variable DESTINATION with the value from INCLUDES but
 7+dnl with -I includes switched to -isystem, with the exception of -I/usr/include,
 8+dnl which is left unmodified to avoid order issues with /usr/include. See:
 9+dnl https://stackoverflow.com/questions/37218953/isystem-on-a-system-include-directory-causes-errors
10+AC_DEFUN([BITCOIN_SYSTEM_INCLUDE],[
11+  if test "x$use_isystem" = "xyes" && test "x$2" != "x"; then
    vasild commented at 5:15 pm on February 21, 2020:
    Nit: test "x$2" != "x" seems unnecessary? If $2 is empty, then the below echo | sed ... would still assign an empty string to $1?
  103. vasild commented at 5:39 pm on February 21, 2020: member

    ACK 572db3ce8 I tested this on FreeBSD 12 with clang 9 and it works as expected: -I is replaced with -isystem, excellent!

    This PR does 2 things - add a new ./configure option (first commit 572db3ce8) and add a new warning flag (second commit 875f8e2c2).

    NACK 875f8e2c2 I do not like that the new warning flag is enabled only if this and that configure options are turned on or off. 875f8e2c2 turns --enable-isystem into a multi-tool that replaces -I with -isystem and also enables some compiler flags, maybe, depending on another configure option (--enable-werror).

    --enable-isystem should do one thing and do it well, and that is to just replace -I with -isystem, nothing more (as in the first commit 572db3ce8). There are a few very good reasons to have such an option:

    • Warnings from external headers (boost, qt, etc) are already suppressed if they are installed in /usr/include. I guess this is the common case with Linux. So, we just add an option to achieve the same if external headers are installed in another location.
    • The new option is off by default.
    • It would allow us to reliably build our code with full -Werror (not a partial set of -Werror=...).
  104. Empact commented at 6:14 pm on February 21, 2020: member

    I do not like that the new warning flag is enabled only if this and that configure options are turned on or off.

    Here --enable-isystem here has two modes: if --enable-werror is active, it will enable -Werror=*, if not, it will enable -W*. The difference being whether or not to error on a related failure.

  105. DrahtBot added the label Needs rebase on Mar 6, 2020
  106. build: Optionally include dependency headers with -isystem 
    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 does not convert all uses, but focuses on libraries which have triggered
warnings/errors when applying initial additional build checks: QT, Univalue, and Berkeley DB.
LevelDb requires additional measures as its code is compiled with the project warnings
via AM_CXXFLAGS.

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.
    422dcd5b4e
  107. build: Enable -Wdocumentation if isystem is enabled 
    Or -Werror=documentation if isystem & werror are enabled.
    c8f4620f88
  108. Empact force-pushed on Mar 9, 2020
  109. DrahtBot removed the label Needs rebase on Mar 9, 2020
  110. DrahtBot commented at 10:24 am on March 26, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

  111. DrahtBot added the label Needs rebase on Mar 26, 2020
  112. sidhujag referenced this in commit 5b583d501a on Nov 10, 2020
  113. fanquake commented at 6:48 am on April 6, 2021: member
    Thanks for the contributions here, however I’m going to enable -Wdocumentation using a different approach.
  114. fanquake closed this on Apr 6, 2021
  115. MarcoFalke commented at 7:07 am on April 6, 2021: member
    #21613
  116. Munkybooty referenced this in commit b4d65497eb on Jun 9, 2022
  117. Munkybooty referenced this in commit 199a46df11 on Jun 21, 2022
  118. Munkybooty referenced this in commit 91e92eb508 on Jun 25, 2022
  119. Munkybooty referenced this in commit 64c7ce2db5 on Jun 28, 2022
  120. Munkybooty referenced this in commit 50aa088657 on Jul 6, 2022
  121. Munkybooty referenced this in commit f45d9128ee on Aug 3, 2022
  122. Munkybooty referenced this in commit 5095c6ee0b on Aug 16, 2022
  123. DrahtBot locked this on Aug 18, 2022
  124. Empact deleted the branch on Oct 10, 2022


Empact DrahtBot practicalswift laanwj fanquake theuni Sjors vasild MarcoFalke

Labels
Build system Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me