Skip to content

build: Disable warnings for leveldb subtree by default #16722

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

build: Disable warnings for leveldb subtree by default #16722

wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 25, 2019

This PR adds --enable-wleveldb option to the configure script.

Here are some examples of usefulness:

Related to #14920.

Inspired by practicalswift and promag.

@hebasto
Copy link
Member Author

hebasto commented Aug 25, 2019

ping @theuni @practicalswift @promag @Empact

practicalswift
practicalswift reviewed Aug 25, 2019
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

configure.ac Outdated
@@ -1667,6 +1681,7 @@ echo " sanitizers = $use_sanitizers"
echo " debug enabled = $enable_debug"
echo " gprof enabled = $enable_gprof"
echo " werror = $enable_werror"
echo " wleveldb = $enable_wleveldb"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@fanquake
Copy link
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
Copy link
Member Author

hebasto commented Aug 26, 2019

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

@hebasto hebasto mentioned this pull request Aug 26, 2019
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 30, 2019 •
edited
Loading

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 hebasto force-pushed the 20190825-leveldb-warnings branch from 8e64235 to 8ae6c43 Compare October 5, 2019 19:31
@hebasto
Copy link
Member Author

hebasto commented Oct 5, 2019

@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
Copy link
Member

laanwj commented Oct 6, 2019

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
Copy link
Member

laanwj commented Oct 15, 2019 •
edited
Loading

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

@hebasto
build: Disable warnings for leveldb by default
d18e508 
This commit adds "--enable-wleveldb" option to the configure script.
@hebasto hebasto force-pushed the 20190825-leveldb-warnings branch from 8ae6c43 to d18e508 Compare November 2, 2019 13:37
@hebasto
Copy link
Member Author

hebasto commented Nov 2, 2019

Rebased together with #16710.

@practicalswift
Copy link
Contributor

ACK d18e508 -- diff looks correct

@hebasto hebasto mentioned this pull request Nov 3, 2019
Closed
@laanwj
Copy link
Member

laanwj commented Nov 6, 2019 •
edited
Loading

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
Copy link
Member Author

hebasto commented Feb 10, 2020

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 hebasto closed this Feb 10, 2020
@hebasto hebasto mentioned this pull request Feb 15, 2020
Closed
@hebasto hebasto mentioned this pull request May 3, 2020
Merged
fanquake added a commit that referenced this pull request May 13, 2020
@fanquake
Merge #16710: build: Enable -Wsuggest-override if available
219c55d 
839add1 build: Enable -Wsuggest-override (Hennadii Stepanov)
de5e91c refactor: Add BerkeleyDatabaseVersion() function (Hennadii Stepanov)

Pull request description:

  From GCC [docs](https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Warning-Options.html):
  > `-Wsuggest-override`
  > Warn about overriding virtual functions that are not marked with the override keyword.

  ~This PR is based on #16722 (the first commit).~ See: #16722 (comment)

ACKs for top commit:
  fanquake:
    ACK 839add1
  vasild:
    ACK 839add1
  practicalswift:
    ACK 839add1 assuming Travis is happy: patch looks correct

Tree-SHA512: 1e8cc085da30d41536deff9b181962c1882314ab252c2ad958294087ae1e5a0dfa4886bdbe36f21cf6ae71df776a8420f349f007d4b5b49fd79ba98ce308965a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 14, 2020
@fanquake
Merge bitcoin#16710: build: Enable -Wsuggest-override if available
4f62a6d 
839add1 build: Enable -Wsuggest-override (Hennadii Stepanov)
de5e91c refactor: Add BerkeleyDatabaseVersion() function (Hennadii Stepanov)

Pull request description:

  From GCC [docs](https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Warning-Options.html):
  > `-Wsuggest-override`
  > Warn about overriding virtual functions that are not marked with the override keyword.

  ~This PR is based on bitcoin#16722 (the first commit).~ See: bitcoin#16722 (comment)

ACKs for top commit:
  fanquake:
    ACK 839add1
  vasild:
    ACK 839add1
  practicalswift:
    ACK 839add1 assuming Travis is happy: patch looks correct

Tree-SHA512: 1e8cc085da30d41536deff9b181962c1882314ab252c2ad958294087ae1e5a0dfa4886bdbe36f21cf6ae71df776a8420f349f007d4b5b49fd79ba98ce308965a
@hebasto hebasto deleted the 20190825-leveldb-warnings branch June 13, 2020 12:48
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @fanquake @DrahtBot @laanwj @practicalswift