-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
build: use newer source for libnatpmp #21209
Conversation
Concept ACK. Are there steps to reproduce the bug, that was fixed in miniupnp/libnatpmp#13, on a Windows 10 machine? |
On macOS bitcoin/depends/packages/libnatpmp.mk Line 12 in 6d3d7ac
Does it related to some recent changes in the build system? |
6d3d7ac
to
bc0e862
Compare
Oops. Thanks. We just now have to tell it how to find |
Out of curiosity, what does cause this need now? |
Looks like it's the changes: miniupnp/libnatpmp@73968c9. Prior to this commit, Thus you would normally end up with: libtool static -o libnatpmp.a natpmp.o getgateway.o or for the case of our depends build, it's currently: depends/x86_64-apple-darwin18/native/bin/x86_64-apple-darwin18-libtool -static -o libnatpmp.a natpmp.o getgateway.o |
Many thanks for explanation. Sorry for re-iterating:
|
No worries. I don't currently have any steps to reproduce the issue. |
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. |
Just for the reference, Homebrew suggests the |
Currently, our cross-compile of libnatpmp for macOS doesn't work at all. The wrong archiver is used, which produces an archive the linker doesn't like. This becomes clear when configuring: ```bash configure:25722: checking for initnatpmp in -lnatpmp configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim> -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp >&5 ld: archive has no table of contents for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` Fix this by using the right `ar` (we do the same for upnp). While we're at it, we fixe the build so that we are using our c/ppflags. This means building with `-O2` rather than `-Os`. Note that this fixes an issue that is also fixed by bitcoin#21209. However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
bd49ac4 build: fix libnatpmp macos cross compile (fanquake) Pull request description: Currently, our cross-compile of libnatpmp for macOS doesn't work at all. The wrong archiver is used, which produces an archive the linker doesn't like. This becomes clear when configuring: ```bash configure:25722: checking for initnatpmp in -lnatpmp configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim> -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp >&5 ld: archive has no table of contents for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` Fix this by using the right `ar` (we do the same for upnp). While we're at it, fix the build so that we are using our c/ppflags. In practice this basically means building with `-O2` rather than `-Os`. Note that this fixes an issue that is also fixed by #21209. However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now. ACKs for top commit: hebasto: ACK bd49ac4, tested: Tree-SHA512: 2efc2c788ef3ebebfbf564ef07b6cf63a72d8a0bccc22b0ba36537216aa575436b7e87088477e85f6d9191ad34f0b13f1c22cf88c90e1cb81641bfee5dc3058a
bc0e862
to
a164c02
Compare
bd49ac4 build: fix libnatpmp macos cross compile (fanquake) Pull request description: Currently, our cross-compile of libnatpmp for macOS doesn't work at all. The wrong archiver is used, which produces an archive the linker doesn't like. This becomes clear when configuring: ```bash configure:25722: checking for initnatpmp in -lnatpmp configure:25747: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/bitcoin/depends/x86_64-apple-darwin18/native/bin/clang++ --target=x86_64-apple-darwin18 <trim> -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-dead_strip_dylibs conftest.cpp -lnatpmp >&5 ld: archive has no table of contents for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` Fix this by using the right `ar` (we do the same for upnp). While we're at it, fix the build so that we are using our c/ppflags. In practice this basically means building with `-O2` rather than `-Os`. Note that this fixes an issue that is also fixed by bitcoin#21209. However, given there are reservations about updating to use a newer libnatpmp source, we should just fix this for now. ACKs for top commit: hebasto: ACK bd49ac4, tested: Tree-SHA512: 2efc2c788ef3ebebfbf564ef07b6cf63a72d8a0bccc22b0ba36537216aa575436b7e87088477e85f6d9191ad34f0b13f1c22cf88c90e1cb81641bfee5dc3058a
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: miniupnp/libnatpmp#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... ```
This fixes linking issues and mirrors what we do with miniupnpc.
a164c02
to
7af2502
Compare
I've updated this PR to also fix the issue described in #21350. We define and use A couple additional points: Even though our current source tarball is named This means the source is missing even more fixes than first though, including a some related to the windows cross compile, i.e: miniupnp/libnatpmp@cf7f452. 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. |
Guix builds at 7af2502: bash-5.1# find output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
1ff0a38e72aba62097e0dc1232104542a9d363e577c25b710b250db78cd876a2 output/bitcoin-7af25024e956-aarch64-linux-gnu-debug.tar.gz
c53e3b36343053fd99794232147a01c4b6742a8fd62005804e6b47c8b6b05933 output/bitcoin-7af25024e956-aarch64-linux-gnu.tar.gz
a8343f9ba8a48787f9a752591bb34e216ce81807efcfcaecd649bfc36976e1af output/bitcoin-7af25024e956-arm-linux-gnueabihf-debug.tar.gz
50b92e5a3835fe3d5169baa4e0d300700e0cc9c665eb23c2fc1d94e8fa2ad3b7 output/bitcoin-7af25024e956-arm-linux-gnueabihf.tar.gz
5bff7037bd2354a232fa49870c4445a260b47e565118ad690808f6d4e86bab3e output/bitcoin-7af25024e956-osx-unsigned.dmg
ad390dcef15db2e27d2a9ff645201a71defc3a468bc28bd8c633ccdbfdddc927 output/bitcoin-7af25024e956-osx-unsigned.tar.gz
7cfd3d90b32da70858f7e5bbd01fcc1bd11043e0f553e5fa51c3e7be500b9876 output/bitcoin-7af25024e956-osx64.tar.gz
ff641a1efac1ef3135926c1256b79a1a1836b2642517eefd87f1b98aeaca6f4b output/bitcoin-7af25024e956-powerpc64-linux-gnu-debug.tar.gz
5ca45d13dea11d248c46196b07a98854716a108d9fb9039d168c7a4442ef5881 output/bitcoin-7af25024e956-powerpc64-linux-gnu.tar.gz
452659a9981b784fb3613278a20c5773945b87f9f2b3a0d588fc00762ffc3f35 output/bitcoin-7af25024e956-powerpc64le-linux-gnu-debug.tar.gz
3ef3a063dfe0af25b32c35d86e2ee986b8c866d3cd6baad4213bc636aea09c0d output/bitcoin-7af25024e956-powerpc64le-linux-gnu.tar.gz
c16b092ac98e419c49ca80cefb9faa743ee8d55161ffce30672bbaf280c047d8 output/bitcoin-7af25024e956-riscv64-linux-gnu-debug.tar.gz
9afdf1d981d0ee7494e86633fe563c092ea4ef8e314665c3a3319dadbdb70832 output/bitcoin-7af25024e956-riscv64-linux-gnu.tar.gz
08edaef880ae35257140fc53a8752e7c0619b2d2890d7ee867548f3da61c36d4 output/bitcoin-7af25024e956-win-unsigned.tar.gz
22c5990c60511f794a222d821c51bddcd06ccc8f1c280c91779bdc4e72729bd8 output/bitcoin-7af25024e956-win64-debug.zip
486347fc37cf51a2c68154d16febda59563d6f85def81a49604c10d6e70cf888 output/bitcoin-7af25024e956-win64-setup-unsigned.exe
92e6591faf775b772a764b0ade11ad43e7be10efbfcc21193417abec9c626585 output/bitcoin-7af25024e956-win64.zip
b87bb8aa05b47ba976fd007d899c14aacc23cd5315340d52231885e5963fe5fa output/bitcoin-7af25024e956-x86_64-linux-gnu-debug.tar.gz
87bb163a401386e34a649945534fbab44d396ed876afa318abd9418f7404972b output/bitcoin-7af25024e956-x86_64-linux-gnu.tar.gz
d05f6c0a4255e2255f3a9e1d808b511dee5783c422c2ea262a6a5df7135177e0 output/src/bitcoin-7af25024e956.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK 7af2502
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.
@@ -1669,6 +1669,9 @@ else | |||
fi | |||
AC_MSG_RESULT($use_natpmp_default) | |||
AC_DEFINE_UNQUOTED([USE_NATPMP], [$natpmp_setting], [NAT-PMP support not compiled if undefined, otherwise value (0 or 1) determines default state]) | |||
if test x$TARGET_OS = xwindows; then | |||
NATPMP_CPPFLAGS="-DSTATICLIB -DNATPMP_STATICLIB" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STATICLIB
is also included, because both of these defines used to beSTATICLIB
, 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?
Concept/Approach ACK, agree with @fanquake's reasoning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7af2502
Guix build:
$ find output -type f -name *$(git rev-parse --short HEAD)*.* -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
1ff0a38e72aba62097e0dc1232104542a9d363e577c25b710b250db78cd876a2 output/bitcoin-7af25024e956-aarch64-linux-gnu-debug.tar.gz
c53e3b36343053fd99794232147a01c4b6742a8fd62005804e6b47c8b6b05933 output/bitcoin-7af25024e956-aarch64-linux-gnu.tar.gz
a8343f9ba8a48787f9a752591bb34e216ce81807efcfcaecd649bfc36976e1af output/bitcoin-7af25024e956-arm-linux-gnueabihf-debug.tar.gz
50b92e5a3835fe3d5169baa4e0d300700e0cc9c665eb23c2fc1d94e8fa2ad3b7 output/bitcoin-7af25024e956-arm-linux-gnueabihf.tar.gz
5bff7037bd2354a232fa49870c4445a260b47e565118ad690808f6d4e86bab3e output/bitcoin-7af25024e956-osx-unsigned.dmg
ad390dcef15db2e27d2a9ff645201a71defc3a468bc28bd8c633ccdbfdddc927 output/bitcoin-7af25024e956-osx-unsigned.tar.gz
7cfd3d90b32da70858f7e5bbd01fcc1bd11043e0f553e5fa51c3e7be500b9876 output/bitcoin-7af25024e956-osx64.tar.gz
ff641a1efac1ef3135926c1256b79a1a1836b2642517eefd87f1b98aeaca6f4b output/bitcoin-7af25024e956-powerpc64-linux-gnu-debug.tar.gz
5ca45d13dea11d248c46196b07a98854716a108d9fb9039d168c7a4442ef5881 output/bitcoin-7af25024e956-powerpc64-linux-gnu.tar.gz
452659a9981b784fb3613278a20c5773945b87f9f2b3a0d588fc00762ffc3f35 output/bitcoin-7af25024e956-powerpc64le-linux-gnu-debug.tar.gz
3ef3a063dfe0af25b32c35d86e2ee986b8c866d3cd6baad4213bc636aea09c0d output/bitcoin-7af25024e956-powerpc64le-linux-gnu.tar.gz
c16b092ac98e419c49ca80cefb9faa743ee8d55161ffce30672bbaf280c047d8 output/bitcoin-7af25024e956-riscv64-linux-gnu-debug.tar.gz
9afdf1d981d0ee7494e86633fe563c092ea4ef8e314665c3a3319dadbdb70832 output/bitcoin-7af25024e956-riscv64-linux-gnu.tar.gz
08edaef880ae35257140fc53a8752e7c0619b2d2890d7ee867548f3da61c36d4 output/bitcoin-7af25024e956-win-unsigned.tar.gz
22c5990c60511f794a222d821c51bddcd06ccc8f1c280c91779bdc4e72729bd8 output/bitcoin-7af25024e956-win64-debug.zip
486347fc37cf51a2c68154d16febda59563d6f85def81a49604c10d6e70cf888 output/bitcoin-7af25024e956-win64-setup-unsigned.exe
92e6591faf775b772a764b0ade11ad43e7be10efbfcc21193417abec9c626585 output/bitcoin-7af25024e956-win64.zip
b87bb8aa05b47ba976fd007d899c14aacc23cd5315340d52231885e5963fe5fa output/bitcoin-7af25024e956-x86_64-linux-gnu-debug.tar.gz
87bb163a401386e34a649945534fbab44d396ed876afa318abd9418f7404972b output/bitcoin-7af25024e956-x86_64-linux-gnu.tar.gz
d05f6c0a4255e2255f3a9e1d808b511dee5783c422c2ea262a6a5df7135177e0 output/src/bitcoin-7af25024e956.tar.gz
I get the same Guix output as @hebasto. |
bb3f79f doc: Update libnatpmp info in dependencies.md (Hennadii Stepanov) 1a01a5d doc: Update zlib info in dependencies.md (Hennadii Stepanov) Pull request description: Update docs according to the recent changes in the code: - #21209 (zlib) - #21376 (libnatpmp) ACKs for top commit: fanquake: ACK bb3f79f - thanks for keeping this updated. Tree-SHA512: 48350ad07700aa071ad6c34e4c161aaadc050488fc068cf478e9781d632828187962a4384c1b67c2344145a2c00c3e16cddd09259130af8e9e86cd76cd32900d
bb3f79f doc: Update libnatpmp info in dependencies.md (Hennadii Stepanov) 1a01a5d doc: Update zlib info in dependencies.md (Hennadii Stepanov) Pull request description: Update docs according to the recent changes in the code: - bitcoin#21209 (zlib) - bitcoin#21376 (libnatpmp) ACKs for top commit: fanquake: ACK bb3f79f - thanks for keeping this updated. Tree-SHA512: 48350ad07700aa071ad6c34e4c161aaadc050488fc068cf478e9781d632828187962a4384c1b67c2344145a2c00c3e16cddd09259130af8e9e86cd76cd32900d
c198797 build: compile libnatpmp with -DNATPMP_STATICLIB on Windows (Fuzzbawls) f2b62a3 build: use newer source for libnatpmp (fanquake) 8143139 GUI: Re-work port mapping saving (Fuzzbawls) c4300c0 Clang-Tidy up mapport.cpp (Fuzzbawls) f1951b5 Add libnatpmp to nightly snapcraft builds (Fuzzbawls) d48ef00 doc: Add release notes (Fuzzbawls) 7886374 doc: Add libnatpmp stuff (Fuzzbawls) cf992d0 ci: Add libnatpmp-dev package to some builds (Fuzzbawls) 5a5e3cf gui: Add NAT-PMP network option (Fuzzbawls) 9a4ba48 net: Add -natpmp command line option (Fuzzbawls) 9927296 net: Add NAT-PMP to port mapping loop (Hennadii Stepanov) 84cd118 net: Add initial libnatpmp support (Fuzzbawls) faed148 gui: Apply port mapping changes on save (Fuzzbawls) 266d322 scripted-diff: Rename UPnP stuff (Fuzzbawls) 06dc0dd net: Add flags for port mapping protocols (Fuzzbawls) 2abea67 net: Keep trying to use UPnP when -upnp=1 (Hennadii Stepanov) d2fa5c4 Make ThreadMapPort static (Fuzzbawls) 7532090 refactor: Replace magic number with named constant (Hennadii Stepanov) c481f62 refactor: Move port mapping code to its own module (Fuzzbawls) Pull request description: Built on top of #2330, this backports bitcoin#18077 and bitcoin#21209 to add NAT-PMP port forwarding support, which can function along side of or instead of UPnP. Similar to how UPnP is treated, NAT-PMP support will be compiled in if the library is found, but the functionality is disabled by default unless passing `--enable-natpmp-default` to `configure`, or by using the `-natpmp` command line option at startup. A checkbox has also been added to the settings area of the GUI that allows for on-the-fly enabling/disabling of the port mapping features when saving the settings. - [x] #2330 to be merged first. ACKs for top commit: furszy: upnp and code check ACK c198797. random-zebra: utACK c198797 after rebase, and merging... Tree-SHA512: e4b47dcd9589a1fd7a1bf3254c319d80af92c878e1657a17a169e49aa888aea33921a9428da3e8d6ee023d971143cf4a495e5a32957370174cff57e6cc6f2cc0
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: miniupnp/libnatpmp#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: