build: Disable warnings for leveldb subtree by default #16722

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20190825-leveldb-warnings changing 2 files +17 −1

hebasto commented at 4:22 pm on August 25, 2019: member
This PR adds --enable-wleveldb option to the configure script.

Here are some examples of usefulness:
- #15539
- #15822
- #16387
- #16710
Related to #14920.

Inspired by practicalswift and promag.
hebasto commented at 4:31 pm on August 25, 2019: member

ping @theuni @practicalswift @promag @Empact
DrahtBot added the label Build system on Aug 25, 2019

in configure.ac:1684 in 8e64235f9a outdated

1680@@ -1667,6 +1681,7 @@ echo "  sanitizers    = $use_sanitizers"
1681 echo "  debug enabled = $enable_debug"
1682 echo "  gprof enabled = $enable_gprof"
1683 echo "  werror        = $enable_werror"
1684+echo "  wleveldb      = $enable_wleveldb"

practicalswift commented at 8:22 pm on August 25, 2019:

I’m not sure if wleveldb is significant enough to be part of the default configure output.

hebasto commented at 7:32 pm on October 5, 2019:

Fixed.

practicalswift commented at 8:22 pm on August 25, 2019: contributor

Concept ACK
fanquake commented at 3:27 am on August 26, 2019: member

-0 on this.

Can you give examples of what’s actually being hidden?

Have the changes been made upstream to “fix” the warnings, rather than just hiding them here?

It seems a bit overkill that we’d add an on-by-default flag to configure to hide a couple warnings from leveldb.

As linked in other PRs, @theuni’s comment from a while back:

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?
hebasto commented at 5:49 am on August 26, 2019: member

@fanquake Thank you for your review.

As linked in other PRs, @theuni’s comment from a while back

IIUC, @theuni concerns about -isystem approach. This PR is an alternative.

It seems a bit overkill that we’d add an on-by-default flag to configure to hide a couple warnings from leveldb.

There are much more warnings in #15822, #16387, #16710 ;) This PR is a step to implement building with -Werror=suggest-override.

Have the changes been made upstream to “fix” the warnings, rather than just hiding them here?

IMHO, it is not safe to update leveldb subtree just to “fix” warnings. This PR suggests a more conservative approach to removing noise from a build log.
DrahtBot commented at 0:02 am on August 30, 2019: 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:
- #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
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.
hebasto force-pushed on Oct 5, 2019
hebasto commented at 7:40 pm on October 5, 2019: member

@practicalswift

Thank you for your review. Your comment has been addressed. @laanwj #16710 (comment)

and if you really want to have a different warning level for built-in dependencies, maybe make it general and cover secp256k1 too?

I thought about it. Configuring of leveldb and secp256k1 subtrees are different. The latter uses AC_CONFIG_SUBDIRS macro. So, let this PR be limited with leveldb subtree scope.
laanwj commented at 9:10 am on October 6, 2019: member

I thought about it. Configuring of leveldb and secp256k1 subtrees are different.

That’s fine, but again, I just don’t like adding a configure argument that only covers disable warnings for leveldb. It is so small in scope it seems silly to have a configure argument.
laanwj commented at 7:07 am on October 15, 2019: member

I think this discussion is stuck. @theuni @dongcarl what do you think about the leveldb warning situation? (as well as #16710)

build: Disable warnings for leveldb by default

This commit adds "--enable-wleveldb" option to the configure script.

d18e50800f

hebasto force-pushed on Nov 2, 2019
hebasto commented at 1:39 pm on November 2, 2019: member

Rebased together with #16710.
practicalswift commented at 2:39 pm on November 2, 2019: contributor

ACK d18e50800fee0c3c0b36491e6af5314f82b4f2b1 – diff looks correct
laanwj commented at 10:00 am on November 6, 2019: member

I think it would be better to only disable the warnings that are spammy and won’t be solved upstream, with a good rationale documented in configure.ac, instead of a blanket ‘disable warnings for leveldb’ opton. It seems like either flying blind or being overwhelmed with spammy warnings.
hebasto commented at 1:01 pm on February 10, 2020: member

Since leveldb has been updated (#17398), this PR is no longer needed.

Even #16710 fires only 3 warnings, which are by no means spammy.

So, closing.
hebasto closed this on Feb 10, 2020
fanquake referenced this in commit 219c55da75 on May 13, 2020
sidhujag referenced this in commit 4f62a6d3b7 on May 14, 2020
hebasto deleted the branch on Jun 13, 2020
DrahtBot locked this on Feb 15, 2022

Contributors
hebasto practicalswift fanquake DrahtBot laanwj

Labels
Build system

build: Disable warnings for leveldb subtree by default #16722

Conflicts