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: build miniupnpc with CMake #29707

Merged
merged 4 commits into from May 2, 2024
Merged

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 22, 2024

This picks up one of the changes from #29232, which is a switch to building miniupnpc with CMake. It includes an update to the most recent version of miniupnpc (2.2.7), which means we can drop one patch from that commit, and includes a new patch for a change I've upstreamed miniupnp/miniupnp#721, as well as some suggestions from the previous PR.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 22, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theuni, TheCharlatan
Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Nice. Concept ACK.

@hebasto
Copy link
Member

hebasto commented Mar 22, 2024

Concept ACK.

@fanquake
Copy link
Member Author

Pushed for builds now that the mirrors are back up.

@hebasto
Copy link
Member

hebasto commented Apr 20, 2024

Tested e79c544.

I've verified compiler flags for CMake vs Autotools without providing the HOST variable:

  1. A new -DMINIUPNP_STATICLIB macro, which basically is Windows-specific and has no affect on non-Windows targets.
  2. The -fno-common flag is being missed, however, it the default for both GCC and Clang.
  3. Warning flags with Autotools are -Wall -W -Wstrict-prototypes, which effectively is equivalent to -Wstrict-prototypes. CMake provides just -Wall. However, no new warnings are being emitted.

So far so good.


As miniupnp/miniupnp#721 has been merged, does it make sense to bump the source to the top commit and drop the related patch?

@hebasto
Copy link
Member

hebasto commented Apr 20, 2024

Continued testing e79c544.

Compiler flags for HOST=x86_64-w64-mingw32 are OK, including correct -D_WIN32_WINNT=0x0601.

However, the commit e5a114a is broken, which is not good for the commit history:

$ make -C depends miniupnpc HOST=x86_64-w64-mingw32
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Extracting miniupnpc...
/home/hebasto/git/bitcoin/depends/sources/miniupnpc-2.2.7.tar.gz: OK
Preprocessing miniupnpc...
patching file src/minisoap.c
patching file src/miniwget.c
patching file Makefile
Hunk #1 succeeded at 303 with fuzz 1 (offset 5 lines).
Configuring miniupnpc...
Building miniupnpc...
make[1]: Entering directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/miniupnpc/2.2.7-40ffab43d67'
make[1]: *** No rule to make target 'build/libminiupnpc.a'.  Stop.
make[1]: Leaving directory '/home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/miniupnpc/2.2.7-40ffab43d67'
make: *** [funcs.mk:297: /home/hebasto/git/bitcoin/depends/work/build/x86_64-w64-mingw32/miniupnpc/2.2.7-40ffab43d67/./.stamp_built] Error 2
make: Leaving directory '/home/hebasto/git/bitcoin/depends'

Even after adjusting paths, it still remains broken:

x86_64-w64-mingw32-gcc -pipe -std=c11 -O2 -D_WIN32_WINNT=0x0601 -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -DMINIUPNP_STATICLIB -c -o miniwget.o src/miniwget.c
<command-line>: warning: "_WIN32_WINNT" redefined

@hebasto
Copy link
Member

hebasto commented Apr 20, 2024

FWIW, I found another Windows-specific bug in the upstream: miniupnp/miniupnp#727.

@fanquake
Copy link
Member Author

FWIW, I found another Windows-specific bug in the upstream: miniupnp/miniupnp#727.

Rebased again, but can pull in this change + fixups on next push

fanquake and others added 3 commits April 30, 2024 10:07
Includes a temporary patch to fix the Windows Autotools build.

See
https://miniupnp.tuxfamily.org/files/changelog.php?file=miniupnpc-2.2.7.tar.gz.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK 5195baa.

Compile-tested for Linux, output looks good for debug and release builds.

@DrahtBot DrahtBot requested a review from hebasto May 1, 2024 14:42
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

utACK 5195baa

Guix builds (aarch64)

fb77af7f4d684e28e9eecc7680af6aec9f6789921d87c49673c51c917b1fe2ed  guix-build-5195baa60087/output/aarch64-linux-gnu/SHA256SUMS.part
8962d6eb216e19c95e9f90ec1e879e309d3bbed62a1cb91b67b2155956580a50  guix-build-5195baa60087/output/aarch64-linux-gnu/bitcoin-5195baa60087-aarch64-linux-gnu-debug.tar.gz
f2dd00f3502e32dd50f232178aa77e63f09359ac303f2309491cb8c08d2b4f15  guix-build-5195baa60087/output/aarch64-linux-gnu/bitcoin-5195baa60087-aarch64-linux-gnu.tar.gz
1d7800e753b22c70b8df715e6dab4f5b3ed469d5a2d56f24ceb915e6544b0140  guix-build-5195baa60087/output/arm-linux-gnueabihf/SHA256SUMS.part
0a9b989b60ae9ccff384b06b8ee2612b97055de649ebff496b594436b29d7126  guix-build-5195baa60087/output/arm-linux-gnueabihf/bitcoin-5195baa60087-arm-linux-gnueabihf-debug.tar.gz
fadc3e1df58ccc3f2704d0977aef336398aefff2124a0e325803d8faad82e830  guix-build-5195baa60087/output/arm-linux-gnueabihf/bitcoin-5195baa60087-arm-linux-gnueabihf.tar.gz
e6f0c6b388499cb144f0c80ee2bc96b9377a7989e2977a519115c119a4bae6ce  guix-build-5195baa60087/output/arm64-apple-darwin/SHA256SUMS.part
c66a1167dc514386cef452645c813f3f365405acb3a9615bbd86ca5922c97f98  guix-build-5195baa60087/output/arm64-apple-darwin/bitcoin-5195baa60087-arm64-apple-darwin-unsigned.tar.gz
17eccde10dd1a7a79bd24c471369a63b9e8d237194273737d1f1a1b573438e02  guix-build-5195baa60087/output/arm64-apple-darwin/bitcoin-5195baa60087-arm64-apple-darwin-unsigned.zip
3de3103ebf84b7f03b70fc79b919cdc874c30cb01ff9338556491e0b096dcb57  guix-build-5195baa60087/output/arm64-apple-darwin/bitcoin-5195baa60087-arm64-apple-darwin.tar.gz
dc6019dd78c0968b65687d1651f6fc52588e3f58b15eb83b1fb5c7d482edbf16  guix-build-5195baa60087/output/dist-archive/bitcoin-5195baa60087.tar.gz
79772282913fd981455c52b248f862e4603c23b72f1564ae09ece6e4d1c32827  guix-build-5195baa60087/output/powerpc64-linux-gnu/SHA256SUMS.part
960674739647971d84488bd46aa152957462ae734b08e779bb4f6af189442dc6  guix-build-5195baa60087/output/powerpc64-linux-gnu/bitcoin-5195baa60087-powerpc64-linux-gnu-debug.tar.gz
9c58aeae8577f6d81e32f2ecdce930078fd1ad55097b1001b578802d3d769f57  guix-build-5195baa60087/output/powerpc64-linux-gnu/bitcoin-5195baa60087-powerpc64-linux-gnu.tar.gz
3b6b2c5cef989031bbd7c4f65acc50936a937d6154611283e4cb830df1ca7f1c  guix-build-5195baa60087/output/riscv64-linux-gnu/SHA256SUMS.part
3139e8d02ad68dd3f3b4687b8c5797bace5a6bf5ecc0e09d51db5d213a259d00  guix-build-5195baa60087/output/riscv64-linux-gnu/bitcoin-5195baa60087-riscv64-linux-gnu-debug.tar.gz
1c06e11fc2fd0fb47fd0c66b0bbf0e4b2fdd7e517d252105490ba5c2f456e92d  guix-build-5195baa60087/output/riscv64-linux-gnu/bitcoin-5195baa60087-riscv64-linux-gnu.tar.gz
39ec562a649dd65cb1533eb7da4f42eb87ac980af57b775369011c5b3c8b2a1a  guix-build-5195baa60087/output/x86_64-apple-darwin/SHA256SUMS.part
b87bbdcce5b9fa84922ec23336aa465eedd5d2d7e38644c2ea5dddb6c46e72b6  guix-build-5195baa60087/output/x86_64-apple-darwin/bitcoin-5195baa60087-x86_64-apple-darwin-unsigned.tar.gz
c621b236f852ba8a8e4b0d070194638984af13e758a39496b87776f2a4ac6510  guix-build-5195baa60087/output/x86_64-apple-darwin/bitcoin-5195baa60087-x86_64-apple-darwin-unsigned.zip
af6ed0a67813f64a44aab312b822e2ce6024df68f4803ad3f1bc66197227b4da  guix-build-5195baa60087/output/x86_64-apple-darwin/bitcoin-5195baa60087-x86_64-apple-darwin.tar.gz
4becad22f284eb29ee337acac78f5c2b330529776331147a6312e3072783b0b8  guix-build-5195baa60087/output/x86_64-linux-gnu/SHA256SUMS.part
8bcfa52392628ac42ed37e3bb55a3af0cfdf455c2cdc1f40ae1680302055baf1  guix-build-5195baa60087/output/x86_64-linux-gnu/bitcoin-5195baa60087-x86_64-linux-gnu-debug.tar.gz
0c3b2d9047780afaa2322346507d03f4972c788c61c65523a7b4442d93da63b0  guix-build-5195baa60087/output/x86_64-linux-gnu/bitcoin-5195baa60087-x86_64-linux-gnu.tar.gz
b7b04e109703e8cae38f5b67eada0c14511a751beeaafd05de53680c77f08f87  guix-build-5195baa60087/output/x86_64-w64-mingw32/SHA256SUMS.part
42f5616e055991115d51c45013d60184c1e4d1e8b90fc7fcf7c37bbd41a4544a  guix-build-5195baa60087/output/x86_64-w64-mingw32/bitcoin-5195baa60087-win64-debug.zip
540b6e8c433ddc15698ea0d8102679a859c1f884c8dc0e09910932df9fb600e9  guix-build-5195baa60087/output/x86_64-w64-mingw32/bitcoin-5195baa60087-win64-setup-unsigned.exe
d65c00ea2e5c6e62d14808947442520d5821429384071a166e43a156708d2414  guix-build-5195baa60087/output/x86_64-w64-mingw32/bitcoin-5195baa60087-win64-unsigned.tar.gz
e2bf352cf412d254d05676ed506465adc053f84c78588328e2db405f4f520292  guix-build-5195baa60087/output/x86_64-w64-mingw32/bitcoin-5195baa60087-win64.zip

@fanquake fanquake merged commit 1cc3aa4 into bitcoin:master May 2, 2024
16 checks passed
@fanquake fanquake deleted the miniupnpc_2_2_7 branch May 2, 2024 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants