Skip to content
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.

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

depends: disable Werror when building zmq #13689

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

greenaddress
Copy link
Contributor

@greenaddress greenaddress commented Jul 17, 2018

This PR backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version. passes --disable-Werror when we build zmq.

For reference see #11844 (comment)

This patch A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to have

It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only #11986 as non-necessary but otherwise unreleased.

In the likely case #13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

@DrahtBot
Copy link
Contributor

Note to reviewers: 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.

@theuni
Copy link
Member

theuni commented Jul 17, 2018

@greenaddress Is this not enough to solve the problem?

--- a/depends/packages/zeromq.mk
+++ b/depends/packages/zeromq.mk
@@ -6,7 +6,7 @@ $(package)_sha256_hash=8f1e2b2aade4dbfde98d82366d61baef2f62e812530160d2e6d0a5bb2
 $(package)_patches=0001-fix-build-with-older-mingw64.patch 0002-disable-pthread_set_name_np.patch

 define $(package)_set_vars
-  $(package)_config_opts=--without-docs --disable-shared --without-libsodium --disable-curve --disable-curve-keygen --disable-perf
+  $(package)_config_opts=--without-docs --disable-shared --without-libsodium --disable-curve --disable-curve-keygen --disable-perf --disable-Werror
   $(package)_config_opts_linux=--with-pic
   $(package)_cxxflags=-std=c++11
 endef

I'm assuming the problem is that newer clang versions emit a new warning which turns to error due to -Werror being on by default. It's considered bad practice to enable Werror in releases for exactly this reason, so disabling it is for sure a reasonable thing to do.

@greenaddress
Copy link
Contributor Author

@theuni I think it should solve it and it would be preferable indeed. I'll give it a try and get back to you

@greenaddress
Copy link
Contributor Author

greenaddress commented Jul 17, 2018

@theuni doesn't seem to work, at least on 64 bit systems it breaks. see https://travis-ci.org/greenaddress/bitcoin_ndk/builds/405007976 when run with greenaddress/bitcoin_ndk@8a72679#diff-c8562f9b2c76d2f7f4c4d15dd3e9f474R74

configure: WARNING: unrecognized options: --disable-Werror

Ok it didn't work cause it only became available in 4.2.3 and not in 4.2.2 (used by 0.16.2) https://github.com/zeromq/libzmq/blob/v4.2.2/configure.ac

I'll test it against master and get back...

@greenaddress
Copy link
Contributor Author

Kicked a test against master with the same patch you suggested running against ndk

https://travis-ci.org/greenaddress/bitcoin_ndk/builds/405019232

@greenaddress
Copy link
Contributor Author

@theuni your suggestions seems to work fine at least in master so it is a better fix, I updated this PR.

@greenaddress greenaddress changed the title depends: backport fix zmq build with clang 6 on crossbuild depends: disable Werror for zmqlib release, fixes some issues with clang 6+ Jul 17, 2018
@theuni
Copy link
Member

theuni commented Jul 17, 2018

@greenaddress Thanks, looks good now.

Are we confident, though, that this isn't masking a legitimate bug?

@greenaddress
Copy link
Contributor Author

@theuni even if it isn't now it could in the future

@fanquake fanquake changed the title depends: disable Werror for zmqlib release, fixes some issues with clang 6+ depends: disable Werror when building zmq Jul 18, 2018
@laanwj
Copy link
Member

laanwj commented Jul 18, 2018

I'm assuming the problem is that newer clang versions emit a new warning which turns to error due to -Werror being on by default. It's considered bad practice to enable Werror in releases for exactly this reason, so disabling it is for sure a reasonable thing to do.

agree, Werror is for development, not for releases, it makes builds fragile.

utACK a4ba238

@fanquake
Copy link
Member

utACK a4ba238

@theuni Could you give a last ACK here as well.

@theuni
Copy link
Member

theuni commented Jul 19, 2018

utACK a4ba238

@maflcko maflcko merged commit a4ba238 into bitcoin:master Jul 19, 2018
maflcko pushed a commit that referenced this pull request Jul 19, 2018
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see #11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only #11986 as non-necessary but otherwise unreleased.~~

  In the likely case #13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
@greenaddress greenaddress deleted the zmq_clang_depends branch July 20, 2018 17:22
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 9, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 9, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 11, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 11, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 12, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 14, 2020
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 24, 2021
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Jul 2, 2021
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see bitcoin#11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](bitcoin#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only bitcoin#11986 as non-necessary but otherwise unreleased.~~

  In the likely case bitcoin#13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants