Build: enable -Wdocumentation via isystem

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

fanquake added the label Build system on Dec 11, 2018

fanquake requested review from theuni on Dec 11, 2018

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.

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

Concept ACK

Thanks for working on this!

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?

MarcoFalke added the label Needs gitian build on Dec 11, 2018

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/

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.

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.

DrahtBot removed the label Needs gitian build on Dec 12, 2018

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.

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.

Empact force-pushed on Dec 14, 2018

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.

Empact force-pushed on Dec 15, 2018

Empact force-pushed on Dec 16, 2018

Empact force-pushed on Dec 18, 2018

Empact force-pushed on Dec 31, 2018

Empact force-pushed on Jan 2, 2019

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.

Empact force-pushed on Jan 4, 2019

Empact force-pushed on Jan 5, 2019

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

Empact force-pushed on Jan 5, 2019

practicalswift commented at 10:14 am on January 5, 2019: contributor

utACK 72878aac48e87e17646fa1889c554e229d76c7bb

MarcoFalke deleted a comment on Jan 5, 2019

Empact force-pushed on Jan 7, 2019

Empact commented at 7:03 pm on January 7, 2019: member

Added Qt Dbus and Qt Test isystem treatment.

practicalswift commented at 9:07 pm on January 7, 2019: contributor

utACK d4baebfc06bf375b64f30298b685b4d66f32cae8

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?

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.

Empact force-pushed on Jan 9, 2019

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.

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. 👍

Empact force-pushed on Jan 9, 2019

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.

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.

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.

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

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.

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.

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

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 :)

DrahtBot added the label Needs rebase on Jan 13, 2019

Empact force-pushed on Jan 14, 2019

DrahtBot removed the label Needs rebase on Jan 14, 2019

Empact force-pushed on Jan 14, 2019

Empact force-pushed on Jan 16, 2019

DrahtBot added the label Needs rebase on Jan 19, 2019

Empact force-pushed on Jan 20, 2019

Empact commented at 4:57 am on January 20, 2019: member

Rebased for #15175

fanquake removed the label Needs rebase on Jan 20, 2019

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"

Empact force-pushed on Jan 20, 2019

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

Empact commented at 5:55 pm on January 22, 2019: member

@theuni would you take another look? I believe I’ve addressed your feedback.

practicalswift commented at 9:25 pm on January 22, 2019: contributor

utACK 535bf5a4f4a9a5a99e188a2f38e9d129e61e5b94

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

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.

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.

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.

Empact force-pushed on Jan 23, 2019

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.

practicalswift commented at 8:30 am on January 24, 2019: contributor

utACK c0ee524b77eacd3e7a2c31be9a722c467d876293

MarcoFalke added the label Needs gitian build on Jan 25, 2019

DrahtBot commented at 6:54 pm on January 26, 2019: member

Gitian builds for commit ab46fe6ec1b37e88c5a06ee7a06ab31cbd18304f (master):

fbdd9c4b4d378ae2718ec2f4ef8260aa... bitcoin-0.17.99-aarch64-linux-gnu-debug.tar.gz
7f9a76f58b7cb8580cc642ad0322967c... bitcoin-0.17.99-aarch64-linux-gnu.tar.gz
1a502ba687d3b294561a53adb0e441a5... bitcoin-0.17.99-arm-linux-gnueabihf-debug.tar.gz
f3c8e673f9b3317640cd23693a26b8dd... bitcoin-0.17.99-arm-linux-gnueabihf.tar.gz
f0fadf1c41cd9a8e106eb01b7e7dcf8f... bitcoin-0.17.99-i686-pc-linux-gnu-debug.tar.gz
3e4a7258898b168c24f6153f2c2b5522... bitcoin-0.17.99-i686-pc-linux-gnu.tar.gz
966bba74494a523856f6ebafeb82a2db... bitcoin-0.17.99-osx-unsigned.dmg
492c6748825d414dc8ad79c36f755f9e... bitcoin-0.17.99-osx64.tar.gz
9a517f6873f88546c0b9746d3e22bfa7... bitcoin-0.17.99-riscv64-linux-gnu-debug.tar.gz
aa91da37fe568f4f93cb370e61f5267b... bitcoin-0.17.99-riscv64-linux-gnu.tar.gz
b2b154a4f16d2dbee951ee5d1fdaf6e0... bitcoin-0.17.99-win32-debug.zip
165e8dd87de75c4d99ccdc44f23cf869... bitcoin-0.17.99-win32-setup-unsigned.exe
22fd2b30bd51031b71d25ba316b073cd... bitcoin-0.17.99-win32.zip
f8752f4a172ad7a9a5313f0f05d9e4a8... bitcoin-0.17.99-win64-debug.zip
5b3482091b0839717f90bcdb2f8a78d0... bitcoin-0.17.99-win64-setup-unsigned.exe
34a5338ae1ac0037c1bce05361d9dde3... bitcoin-0.17.99-win64.zip
90c3917880f1c301c18b5115b7400aff... bitcoin-0.17.99-x86_64-linux-gnu-debug.tar.gz
2903be9dc6198c4d7a3ff525b4c84871... bitcoin-0.17.99-x86_64-linux-gnu.tar.gz
4159d94c0a52df25bf1b8f26bc96ca55... bitcoin-0.17.99.tar.gz
c8aa9cceb2e38b2f7c652c9e496aea02... bitcoin-linux-0.18-res.yml
72ad96793debb2699c52245447200512... bitcoin-linux-build.log
a4d4f322edd43d0206be3d40b29034a0... bitcoin-osx-0.18-res.yml
88a1a732368c84ee469ded587ee5043f... bitcoin-osx-build.log
ee1819f70b4655b769c789491cc3c29d... bitcoin-win-0.18-res.yml
58dc03a703337e1c521bc8996af77c9a... bitcoin-win-build.log

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

4475fb4b3ada5e737f7c1a065e890220... bitcoin-0.17.99-aarch64-linux-gnu-debug.tar.gz
11f7e2e7e927808e8b81ed6afcb45bf9... bitcoin-0.17.99-aarch64-linux-gnu.tar.gz
6ebf5479ed478dfc85b7d0143d6ddceb... bitcoin-0.17.99-arm-linux-gnueabihf-debug.tar.gz
dc65299e4a833a4e21728b73624b30cf... bitcoin-0.17.99-arm-linux-gnueabihf.tar.gz
57d6c61a8dbbcbb73873c9a79cd680fe... bitcoin-0.17.99-i686-pc-linux-gnu-debug.tar.gz
28f684c67b6f4ad490ac6e833cc2c83d... bitcoin-0.17.99-i686-pc-linux-gnu.tar.gz
94bae6cca45c25c0f4173413e9f32fcb... bitcoin-0.17.99-osx-unsigned.dmg
de6ea64a98b75242ee2b6b323fa89570... bitcoin-0.17.99-osx64.tar.gz
07dae9b5de7e0021c9457d8e10e2912c... bitcoin-0.17.99-riscv64-linux-gnu-debug.tar.gz
d711437bb1ea38b34f104540f3ae7343... bitcoin-0.17.99-riscv64-linux-gnu.tar.gz
95601a827148d720659e1ca4e7c2ad4b... bitcoin-0.17.99-win32-debug.zip
8090ffbedc7af759a9d854a8c3ee841b... bitcoin-0.17.99-win32-setup-unsigned.exe
26c579b0d4078ba0a117e7b199886daf... bitcoin-0.17.99-win32.zip
f525a0e00f51c2093fc71bb51870d379... bitcoin-0.17.99-win64-debug.zip
e28a2fc8106ff75a33e3124ba4c42be8... bitcoin-0.17.99-win64-setup-unsigned.exe
3aacbd4599809bca4c5e2ce341dbf6f9... bitcoin-0.17.99-win64.zip
aeb2d0d7112c2c1a3e2f7ef600e894e0... bitcoin-0.17.99-x86_64-linux-gnu-debug.tar.gz
2b8d73d039a5396938a248fb3e83522a... bitcoin-0.17.99-x86_64-linux-gnu.tar.gz
1fc1d1e0f0f104081e053a7eadbc0a2f... bitcoin-0.17.99.tar.gz
baae8b0bbf3469a9e0d723d01ba26640... bitcoin-linux-0.18-res.yml
a397cf6d104b9b041c2d8be306c8a1e2... bitcoin-linux-build.log
e379fb6a8b33b9082f6880e7165f524a... bitcoin-osx-0.18-res.yml
81d0db199063948034b965c4d25aea13... bitcoin-osx-build.log
5456006e08d866782da929f2346b099f... bitcoin-win-0.18-res.yml
dfda72040686def107a6861ae2e5eb38... bitcoin-win-build.log

DrahtBot removed the label Needs gitian build on Jan 26, 2019

Empact force-pushed on Feb 9, 2019

Empact commented at 11:48 pm on February 9, 2019: member

Rebased due to conflicts in #15377

Empact force-pushed on Mar 5, 2019

Empact commented at 5:37 am on March 5, 2019: member

Rebased due to conflicts in #15112

Empact force-pushed on Mar 7, 2019

Empact commented at 3:22 pm on March 7, 2019: member

Fix documentation errors added in #14978 bd0dbe8763fc3029cf96531c9ccaba280b939445

Empact force-pushed on Mar 7, 2019

Empact commented at 4:23 pm on March 7, 2019: member

Fix more documentation errors, including some added in #14021.

practicalswift commented at 8:12 pm on March 7, 2019: contributor

utACK b51f4e14137e45d8d52fbf0b925287dde45ee97b

Let’s move forward with this one :-)

Empact commented at 1:42 pm on June 5, 2019: member

@theuni could you take another look at this?

Empact force-pushed on Jun 5, 2019

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

Empact commented at 2:10 pm on June 5, 2019: member

@dongcarl could you take a look at this, given your recent build investigations?

DrahtBot added the label Needs rebase on Aug 2, 2019

Sjors commented at 11:38 am on November 20, 2019: member

Concept ACK

Needs rebase now that openssl and protobuf are gone.

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)

Empact force-pushed on Jan 17, 2020

DrahtBot removed the label Needs rebase on Jan 17, 2020

laanwj referenced this in commit 5d2ff75e20 on Jan 20, 2020

sidhujag referenced this in commit fef8e0ea0b on Jan 24, 2020

DrahtBot added the label Needs rebase on Feb 16, 2020

Empact force-pushed on Feb 20, 2020

DrahtBot removed the label Needs rebase on Feb 20, 2020

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?

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=...).

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.

DrahtBot added the label Needs rebase on Mar 6, 2020

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

build: Enable -Wdocumentation if isystem is enabled

Or -Werror=documentation if isystem & werror are enabled.

c8f4620f88

Empact force-pushed on Mar 9, 2020

DrahtBot removed the label Needs rebase on Mar 9, 2020

DrahtBot commented at 10:24 am on March 26, 2020: member

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

DrahtBot added the label Needs rebase on Mar 26, 2020

sidhujag referenced this in commit 5b583d501a on Nov 10, 2020

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.

fanquake closed this on Apr 6, 2021

MarcoFalke commented at 7:07 am on April 6, 2021: member

#21613

Munkybooty referenced this in commit b4d65497eb on Jun 9, 2022

Munkybooty referenced this in commit 199a46df11 on Jun 21, 2022

Munkybooty referenced this in commit 91e92eb508 on Jun 25, 2022

Munkybooty referenced this in commit 64c7ce2db5 on Jun 28, 2022

Munkybooty referenced this in commit 50aa088657 on Jul 6, 2022

Munkybooty referenced this in commit f45d9128ee on Aug 3, 2022

Munkybooty referenced this in commit 5095c6ee0b on Aug 16, 2022

DrahtBot locked this on Aug 18, 2022

Empact deleted the branch on Oct 10, 2022

Build: enable -Wdocumentation via isystem #14920

Conflicts