build: use newer source for libnatpmp

fanquake commented at 7:20 am on February 17, 2021: member

The source we are currently using is from 2015. The upstream repo has received a small number of bug fixes and improvements since then. Including one that fixes an issue for Windows users: https://github.com/miniupnp/libnatpmp/pull/13.

The source we are currently using is the most recent “official” release, however I don’t think it’s worth waiting for a new one. The maintainer was prompted to do so in Oct 2020, then again in Jan of this year, and no release has eventuated. Given libnatpmp is a new inclusion into our repository, I think we should be using this newer source.

This also cleans up a few warnings we currently see in Windows depends builds:

 0Extracting libnatpmp...
 1/home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK
 2Preprocessing libnatpmp...
 3Configuring libnatpmp...
 4Building libnatpmp...
 5make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
 6x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o natpmp.o natpmp.c
 7x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o getgateway.o getgateway.c
 8natpmp.c:42: warning: "EWOULDBLOCK" redefined
 9   42 | #define EWOULDBLOCK WSAEWOULDBLOCK
10      |
11In file included from natpmp.c:38:
12/usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition
13  166 | #define EWOULDBLOCK 140
14      |
15natpmp.c:43: warning: "ECONNREFUSED" redefined
16   43 | #define ECONNREFUSED WSAECONNREFUSED
17      |
18In file included from natpmp.c:38:
19/usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition
20  110 | #define ECONNREFUSED 107
21      |
22natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
23  271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response)
24      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
25ar crs libnatpmp.a natpmp.o getgateway.o
26make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
27Staging libnatpmp...
28Postprocessing libnatpmp...
29Caching libnatpmp...

fanquake added the label Build system on Feb 17, 2021

fanquake added the label Needs gitian build on Feb 17, 2021

fanquake added the label Needs Guix build on Feb 17, 2021

fanquake requested review from hebasto on Feb 17, 2021

hebasto commented at 7:31 am on February 17, 2021: member

Concept ACK.

Are there steps to reproduce the bug, that was fixed in https://github.com/miniupnp/libnatpmp/pull/13, on a Windows 10 machine?

hebasto commented at 8:36 am on February 17, 2021: member

On macOS https://github.com/bitcoin/bitcoin/blob/6d3d7ac8d0db758b422fab811a3cafd93eee3a6a/depends/packages/libnatpmp.mk#L12 becomes (https://cirrus-ci.com/task/4969862553403392?command=ci#L1115)

0static -o libnatpmp.a natpmp.o getgateway.o

Does it related to some recent changes in the build system?

fanquake force-pushed on Feb 17, 2021

fanquake commented at 9:02 am on February 17, 2021: member

Does it related to some recent changes in the build system?

Oops. Thanks. We just now have to tell it how to find libtool. The macOS cross build should be ok now.

hebasto commented at 12:57 pm on February 17, 2021: member

@fanquake

We just now have to tell it how to find libtool.

Out of curiosity, what does cause this need now?

fanquake commented at 1:28 pm on February 17, 2021: member

Out of curiosity, what does cause this need now?

Looks like it’s the changes: https://github.com/miniupnp/libnatpmp/commit/73968c934dc9d90226b18fb596c5e611c947c461.

Prior to this commit, ar crs was being used unconditionally for building the static library. After this change, the Makefile uses libtool for macOS builds, with libtool being set via LIBTOOL ?= $(shell which libtool). However, in our macOS depends environment, which libtool wont return anything, which is why you saw static -o libnatpmp.a natpmp.o getgateway.o in the build logs, and why we now need to set LIBTOOL explicitly (note we do the same for miniupnpc).

Thus you would normally end up with:

0libtool static -o libnatpmp.a natpmp.o getgateway.o

or for the case of our depends build, it’s currently:

0depends/x86_64-apple-darwin18/native/bin/x86_64-apple-darwin18-libtool -static -o libnatpmp.a natpmp.o getgateway.o

hebasto commented at 1:31 pm on February 17, 2021: member

Out of curiosity, what does cause this need now?

Looks like it’s the changes: miniupnp/libnatpmp@73968c9.

Many thanks for explanation.

Sorry for re-iterating:

Are there steps to reproduce the bug, that was fixed in miniupnp/libnatpmp#13, on a Windows 10 machine?

fanquake commented at 2:16 pm on February 17, 2021: member

Sorry for re-iterating:

No worries. I don’t currently have any steps to reproduce the issue.

hebasto commented at 7:03 pm on February 17, 2021: member

The patch is correct, but I’m a bit reluctant to ACK update to a branch that:

is not a proved bugfix
has no long history of wide testing

DrahtBot removed the label Needs gitian build on Feb 18, 2021

DrahtBot removed the label Needs Guix build on Feb 20, 2021

MarcoFalke deleted a comment on Feb 20, 2021

laanwj commented at 10:03 am on February 26, 2021: member

The patch is correct, but I’m a bit reluctant to ACK update to a branch that:

You have a point, though if the maintainer has stopped tagging releases but otherwise the project is still making fixes, we have to switch to tracking commits at some point. Still it is good to be careful and see if the gain outweighs the risk.

hebasto commented at 7:24 pm on February 26, 2021: member

Just for the reference, Homebrew suggests the 20150609 version (which is our current version).

fanquake referenced this in commit bd49ac4168 on Mar 1, 2021

DrahtBot commented at 1:29 pm on March 1, 2021: member

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

Conflicts

No conflicts as of last run.

MarcoFalke deleted a comment on Mar 3, 2021

MarcoFalke added the label Needs gitian build on Mar 3, 2021

MarcoFalke added the label Needs Guix build on Mar 3, 2021

fanquake referenced this in commit 97a35f3ae5 on Mar 3, 2021

fanquake force-pushed on Mar 3, 2021

sidhujag referenced this in commit 9c7d3e6eb6 on Mar 3, 2021

build: use newer source for libnatpmp

The source we are currently using is from 2015. The upstream repo has
received a small number of bug fixes and improvements since then.
Including one that fixes an issue for Windows users:
https://github.com/miniupnp/libnatpmp/pull/13.

The source we are currently using is the most recent "official" release,
however I don't think it's worth waiting for a new one. The maintainer
was prompted to do so in Oct 2020, then again in Jan of this year, and
no release has eventuated. Given libnatpmp is a new inclusion into our
repository, I think we should be using this newer source.

This also cleans up a few warnings we currently see in depends builds:
```bash
Extracting libnatpmp...
/home/ubuntu/bitcoin/depends/sources/libnatpmp-20150609.tar.gz: OK
Preprocessing libnatpmp...
Configuring libnatpmp...
Building libnatpmp...
make[1]: Entering directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o natpmp.o natpmp.c
x86_64-w64-mingw32-gcc -Os -fPIC -Wall -DENABLE_STRNATPMPERR   -c -o getgateway.o getgateway.c
natpmp.c:42: warning: "EWOULDBLOCK" redefined
   42 | #define EWOULDBLOCK WSAEWOULDBLOCK
      |
In file included from natpmp.c:38:
/usr/share/mingw-w64/include/errno.h:166: note: this is the location of the previous definition
  166 | #define EWOULDBLOCK 140
      |
natpmp.c:43: warning: "ECONNREFUSED" redefined
   43 | #define ECONNREFUSED WSAECONNREFUSED
      |
In file included from natpmp.c:38:
/usr/share/mingw-w64/include/errno.h:110: note: this is the location of the previous definition
  110 | #define ECONNREFUSED 107
      |
natpmp.c:271:5: warning: ‘readnatpmpresponseorretry’ redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
  271 | int readnatpmpresponseorretry(natpmp_t * p, natpmpresp_t * response)
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
ar crs libnatpmp.a natpmp.o getgateway.o
make[1]: Leaving directory '/home/ubuntu/bitcoin/depends/work/build/x86_64-w64-mingw32/libnatpmp/20150609-13efa1beb87'
Staging libnatpmp...
Postprocessing libnatpmp...
Caching libnatpmp...
```

ee35745754

build: compile libnatpmp with -DNATPMP_STATICLIB on Windows

This fixes linking issues and mirrors what we do with miniupnpc.

7af25024e9

fanquake force-pushed on Mar 4, 2021

fanquake commented at 4:45 am on March 4, 2021: member

I’ve updated this PR to also fix the issue described in #21350. We define and use NATPMP_STATICLIB, similar to how we use MINIUPNP_STATICLIB. STATICLIB is also included, because both of these defines used to be STATICLIB, so if someone wass using an older source (actually the commit after what is in our current tarball, https://github.com/miniupnp/libnatpmp/commit/d5d2d6d74a25f019c367c4a1942e98906b97cb89) , they would need that defined instead.

A couple additional points:

Even though our current source tarball is named 20150609, that is misleading, as the source inside is actually this commit: https://github.com/miniupnp/libnatpmp/commit/f376ab72bc4de0b45f4e10c9b287b7ffe2a1fcd1, from April 2014..

This means the source is missing even more fixes than first though, including a some related to the windows cross compile, i.e: https://github.com/miniupnp/libnatpmp/commit/cf7f452d667658592e354c6ae6a0d539e367285c.

I think this is even more argument to move to the latest source. Otherwise we are just going to have to start applying patches to our build anyway. I’m planning on upstreaming some changes to https://github.com/miniupnp/libnatpmp/. Maybe that will spur the author into tagging a new release at some point soon.

fanquake commented at 6:34 am on March 4, 2021: member

Guix builds at 7af25024e9563241c72797d4eeabdf660e548f53:

 0bash-5.1# find output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 11ff0a38e72aba62097e0dc1232104542a9d363e577c25b710b250db78cd876a2  output/bitcoin-7af25024e956-aarch64-linux-gnu-debug.tar.gz
 2c53e3b36343053fd99794232147a01c4b6742a8fd62005804e6b47c8b6b05933  output/bitcoin-7af25024e956-aarch64-linux-gnu.tar.gz
 3a8343f9ba8a48787f9a752591bb34e216ce81807efcfcaecd649bfc36976e1af  output/bitcoin-7af25024e956-arm-linux-gnueabihf-debug.tar.gz
 450b92e5a3835fe3d5169baa4e0d300700e0cc9c665eb23c2fc1d94e8fa2ad3b7  output/bitcoin-7af25024e956-arm-linux-gnueabihf.tar.gz
 55bff7037bd2354a232fa49870c4445a260b47e565118ad690808f6d4e86bab3e  output/bitcoin-7af25024e956-osx-unsigned.dmg
 6ad390dcef15db2e27d2a9ff645201a71defc3a468bc28bd8c633ccdbfdddc927  output/bitcoin-7af25024e956-osx-unsigned.tar.gz
 77cfd3d90b32da70858f7e5bbd01fcc1bd11043e0f553e5fa51c3e7be500b9876  output/bitcoin-7af25024e956-osx64.tar.gz
 8ff641a1efac1ef3135926c1256b79a1a1836b2642517eefd87f1b98aeaca6f4b  output/bitcoin-7af25024e956-powerpc64-linux-gnu-debug.tar.gz
 95ca45d13dea11d248c46196b07a98854716a108d9fb9039d168c7a4442ef5881  output/bitcoin-7af25024e956-powerpc64-linux-gnu.tar.gz
10452659a9981b784fb3613278a20c5773945b87f9f2b3a0d588fc00762ffc3f35  output/bitcoin-7af25024e956-powerpc64le-linux-gnu-debug.tar.gz
113ef3a063dfe0af25b32c35d86e2ee986b8c866d3cd6baad4213bc636aea09c0d  output/bitcoin-7af25024e956-powerpc64le-linux-gnu.tar.gz
12c16b092ac98e419c49ca80cefb9faa743ee8d55161ffce30672bbaf280c047d8  output/bitcoin-7af25024e956-riscv64-linux-gnu-debug.tar.gz
139afdf1d981d0ee7494e86633fe563c092ea4ef8e314665c3a3319dadbdb70832  output/bitcoin-7af25024e956-riscv64-linux-gnu.tar.gz
1408edaef880ae35257140fc53a8752e7c0619b2d2890d7ee867548f3da61c36d4  output/bitcoin-7af25024e956-win-unsigned.tar.gz
1522c5990c60511f794a222d821c51bddcd06ccc8f1c280c91779bdc4e72729bd8  output/bitcoin-7af25024e956-win64-debug.zip
16486347fc37cf51a2c68154d16febda59563d6f85def81a49604c10d6e70cf888  output/bitcoin-7af25024e956-win64-setup-unsigned.exe
1792e6591faf775b772a764b0ade11ad43e7be10efbfcc21193417abec9c626585  output/bitcoin-7af25024e956-win64.zip
18b87bb8aa05b47ba976fd007d899c14aacc23cd5315340d52231885e5963fe5fa  output/bitcoin-7af25024e956-x86_64-linux-gnu-debug.tar.gz
1987bb163a401386e34a649945534fbab44d396ed876afa318abd9418f7404972b  output/bitcoin-7af25024e956-x86_64-linux-gnu.tar.gz
20d05f6c0a4255e2255f3a9e1d808b511dee5783c422c2ea262a6a5df7135177e0  output/src/bitcoin-7af25024e956.tar.gz

in configure.ac:1673 in 7af25024e9

1668@@ -1669,6 +1669,9 @@ else
1669     fi
1670     AC_MSG_RESULT($use_natpmp_default)
1671     AC_DEFINE_UNQUOTED([USE_NATPMP], [$natpmp_setting], [NAT-PMP support not compiled if undefined, otherwise value (0 or 1) determines default state])
1672+    if test x$TARGET_OS = xwindows; then
1673+      NATPMP_CPPFLAGS="-DSTATICLIB -DNATPMP_STATICLIB"

hebasto commented at 7:16 am on March 4, 2021:

STATICLIB is also included, because both of these defines used to be STATICLIB, so if someone wass using an older source (actually the commit after what is in our current tarball, miniupnp/libnatpmp@d5d2d6d) , they would need that defined instead.

~~I’m not follow why do we need -DSTATICLIB now?~~

hebasto commented at 7:17 am on March 4, 2021: member

Approach ACK 7af25024e9563241c72797d4eeabdf660e548f53

Even though our current source tarball is named 20150609, that is misleading, as the source inside is actually this commit: miniupnp/libnatpmp@f376ab7, from April 2014..

Agree.

laanwj commented at 9:56 am on March 4, 2021: member

Concept/Approach ACK, agree with @fanquake’s reasoning

hebasto approved

hebasto commented at 10:13 am on March 4, 2021: member

ACK 7af25024e9563241c72797d4eeabdf660e548f53

Guix build:

 0$ find output -type f -name *$(git rev-parse --short HEAD)*.* -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 11ff0a38e72aba62097e0dc1232104542a9d363e577c25b710b250db78cd876a2  output/bitcoin-7af25024e956-aarch64-linux-gnu-debug.tar.gz
 2c53e3b36343053fd99794232147a01c4b6742a8fd62005804e6b47c8b6b05933  output/bitcoin-7af25024e956-aarch64-linux-gnu.tar.gz
 3a8343f9ba8a48787f9a752591bb34e216ce81807efcfcaecd649bfc36976e1af  output/bitcoin-7af25024e956-arm-linux-gnueabihf-debug.tar.gz
 450b92e5a3835fe3d5169baa4e0d300700e0cc9c665eb23c2fc1d94e8fa2ad3b7  output/bitcoin-7af25024e956-arm-linux-gnueabihf.tar.gz
 55bff7037bd2354a232fa49870c4445a260b47e565118ad690808f6d4e86bab3e  output/bitcoin-7af25024e956-osx-unsigned.dmg
 6ad390dcef15db2e27d2a9ff645201a71defc3a468bc28bd8c633ccdbfdddc927  output/bitcoin-7af25024e956-osx-unsigned.tar.gz
 77cfd3d90b32da70858f7e5bbd01fcc1bd11043e0f553e5fa51c3e7be500b9876  output/bitcoin-7af25024e956-osx64.tar.gz
 8ff641a1efac1ef3135926c1256b79a1a1836b2642517eefd87f1b98aeaca6f4b  output/bitcoin-7af25024e956-powerpc64-linux-gnu-debug.tar.gz
 95ca45d13dea11d248c46196b07a98854716a108d9fb9039d168c7a4442ef5881  output/bitcoin-7af25024e956-powerpc64-linux-gnu.tar.gz
10452659a9981b784fb3613278a20c5773945b87f9f2b3a0d588fc00762ffc3f35  output/bitcoin-7af25024e956-powerpc64le-linux-gnu-debug.tar.gz
113ef3a063dfe0af25b32c35d86e2ee986b8c866d3cd6baad4213bc636aea09c0d  output/bitcoin-7af25024e956-powerpc64le-linux-gnu.tar.gz
12c16b092ac98e419c49ca80cefb9faa743ee8d55161ffce30672bbaf280c047d8  output/bitcoin-7af25024e956-riscv64-linux-gnu-debug.tar.gz
139afdf1d981d0ee7494e86633fe563c092ea4ef8e314665c3a3319dadbdb70832  output/bitcoin-7af25024e956-riscv64-linux-gnu.tar.gz
1408edaef880ae35257140fc53a8752e7c0619b2d2890d7ee867548f3da61c36d4  output/bitcoin-7af25024e956-win-unsigned.tar.gz
1522c5990c60511f794a222d821c51bddcd06ccc8f1c280c91779bdc4e72729bd8  output/bitcoin-7af25024e956-win64-debug.zip
16486347fc37cf51a2c68154d16febda59563d6f85def81a49604c10d6e70cf888  output/bitcoin-7af25024e956-win64-setup-unsigned.exe
1792e6591faf775b772a764b0ade11ad43e7be10efbfcc21193417abec9c626585  output/bitcoin-7af25024e956-win64.zip
18b87bb8aa05b47ba976fd007d899c14aacc23cd5315340d52231885e5963fe5fa  output/bitcoin-7af25024e956-x86_64-linux-gnu-debug.tar.gz
1987bb163a401386e34a649945534fbab44d396ed876afa318abd9418f7404972b  output/bitcoin-7af25024e956-x86_64-linux-gnu.tar.gz
20d05f6c0a4255e2255f3a9e1d808b511dee5783c422c2ea262a6a5df7135177e0  output/src/bitcoin-7af25024e956.tar.gz

laanwj commented at 3:24 pm on March 5, 2021: member

I get the same Guix output as @hebasto.

fanquake merged this on Mar 6, 2021

fanquake closed this on Mar 6, 2021

fanquake deleted the branch on Mar 6, 2021

sidhujag referenced this in commit d430fa1468 on Mar 6, 2021

MarcoFalke removed the label Needs Guix build on Mar 6, 2021

MarcoFalke removed the label Needs gitian build on Mar 6, 2021

DrahtBot commented at 4:03 am on March 7, 2021: member

Guix builds

File	commit b4d22654fe9e90093e643cb7beb896c48a274d47(master)	commit 765be8dc13020897eb8a15b7e4aea93109906062(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`6a823a1f747b82d7...`	`f1d3a283d0442c59...`
*-aarch64-linux-gnu.tar.gz	`854ba7d7fbe90d6d...`	`a0bc041d99ca7462...`
*-arm-linux-gnueabihf-debug.tar.gz	`0e2c46d1d20c94aa...`	`ec916fcc5f86cb0d...`
*-arm-linux-gnueabihf.tar.gz	`b110fa9b673a555f...`	`f05f7c20a5c8fc4c...`
*-osx-unsigned.dmg	`51f57af29f3b65dd...`	`5b1c220728a860a3...`
*-osx-unsigned.tar.gz	`21d22cb49f9067cd...`	`81f2ba2452d1cfb6...`
*-osx64.tar.gz	`22ceafcd3f1e1227...`	`e51f493674749ffc...`
*-powerpc64-linux-gnu-debug.tar.gz	`c377e33096fb8f32...`	`9d6b19bd55c7afcc...`
*-powerpc64-linux-gnu.tar.gz	`ef46ae658557c13e...`	`40edeadc018ccea2...`
*-powerpc64le-linux-gnu-debug.tar.gz	`ae896cd55f89a354...`	`bd4c25241f9a994d...`
*-powerpc64le-linux-gnu.tar.gz	`24153b562f2e75a2...`	`732f185f7536dddb...`
*-riscv64-linux-gnu-debug.tar.gz	`93701e039ba98f44...`	`8ff0dfdbe3743ad1...`
*-riscv64-linux-gnu.tar.gz	`9b0730144529e23c...`	`56952a02b6b9935d...`
*-win-unsigned.tar.gz	`5b68e0ff68de904c...`	`12b00663b3405d8f...`
*-win64-debug.zip	`67386013c6cf3b31...`	`9845a487ff232b16...`
*-win64-setup-unsigned.exe	`16aa436df456790c...`	`6df8b327b3d7eca7...`
*-win64.zip	`f4e6938fd0cfbf94...`	`413140738fe80dac...`
*-x86_64-linux-gnu-debug.tar.gz	`2be7496341e0cd11...`	`545ffefc2d4e11c4...`
*-x86_64-linux-gnu.tar.gz	`d51283654f44df76...`	`306d960bdcd10872...`
*.tar.gz	`09b983d63bc7a32a...`	`2b1ccfd0ebc0ccf3...`
guix_build.log	`c7da8d1daae8c5bb...`	`84dcb0e74fa3e608...`
guix_build.log.diff		`b93956f75d5753a3...`

fanquake referenced this in commit bf7c22f7ff on Mar 18, 2021

sidhujag referenced this in commit b3e4229108 on Mar 18, 2021

random-zebra referenced this in commit fce12cf6c3 on Jun 24, 2021

kittywhiskers referenced this in commit c382730033 on Feb 26, 2022

DrahtBot locked this on Aug 16, 2022

build: use newer source for libnatpmp #21209

Conflicts

Guix builds