depends: build libnatpmp with CMake #29708
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
Concept ACK. |
This was referenced Mar 23, 2024
f6ccb79 to
1b0102e
Compare
3f82379 to
289c8c5
Compare
This includes once CMake related change I upstreamed: miniupnp/libnatpmp#43.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
289c8c5 to
3c1ae3e
Compare
|
Guix build (x86_64): |
hebasto
reviewed
Apr 20, 2024
There was a problem hiding this comment.
Approach ACK 3c1ae3e.
I've verified compiler flags for different scenarios, including cross-compiling for Windows and providing DEBUG=1. CMake ones lack -Wall -Wextra flags, but that's not a blocker.
Also this PR limits the installed headers to ones that are actually used in our code, which is fine.
|
My Guix builds: |
hebasto
approved these changes
Apr 20, 2024
m3dwards
reviewed
Apr 24, 2024
| endef | ||
|
|
||
| define $(package)_build_cmds | ||
| $(MAKE) libnatpmp.a $($(package)_build_opts) | ||
| $(MAKE) natpmp |
There was a problem hiding this comment.
Perhaps this is what we want but I thought it worth mentioning that there is a difference on my Mac between this and having cmake call make with: cmake --build . --target natpmp.
$(MAKE) from autotools for me calls GNU MAKE 3.81 and cmake delegates to gmake being GNU MAKE 4.4.1.
Should we call cmake instead and allow it to decide the best make to delegate to?
There was a problem hiding this comment.
Possibly, but if we are going to do that, we could do it globally/all at once, or even set something at a higher level in depends. Not sure if we have to change this during these migrations.
|
My Guix builds: a134241bc9ff9823ce078ea10dbac544ff63536ee3f66fb557414ca191b1d62d guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/SHA256SUMS.part
f5a46ab7a5faae9ce50b2d084cf65d14372b354b9f8369178f1348b70a3b675b guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu-debug.tar.gz
e2353ed4f0738026fe16cd90b503ef2f02f31e0b68ba42ea80924997225ae374 guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu.tar.gz
78060899627e9449977151dce2b8125ef3f17b1050e0396c9f907ef963f55ab1 guix-build-3c1ae3ee33d4/output/arm-linux-gnueabihf/SHA256SUMS.part
4f71666871fe889ee484cfeefe577d33760453113542fe39eab2ac2bfd34030e guix-build-3c1ae3ee33d4/output/arm-linux-gnueabihf/bitcoin-3c1ae3ee33d4-arm-linux-gnueabihf-debug.tar.gz
fbc516363e389f477596165f5dd54b652cd868a8da91c5647364b0fa76223807 guix-build-3c1ae3ee33d4/output/arm-linux-gnueabihf/bitcoin-3c1ae3ee33d4-arm-linux-gnueabihf.tar.gz
5811efb8663f1e353525908f9b98594fa88d563af157c988b194fd125c958f4c guix-build-3c1ae3ee33d4/output/arm64-apple-darwin/SHA256SUMS.part
10e7f27dc83c471f1fa2ef5e683ae30b592c9077d6648fee45545f6c7a76b556 guix-build-3c1ae3ee33d4/output/arm64-apple-darwin/bitcoin-3c1ae3ee33d4-arm64-apple-darwin-unsigned.tar.gz
4b9d4714bbec7304969043abd51b0b5391120ec3295c58516741e3d06e550362 guix-build-3c1ae3ee33d4/output/arm64-apple-darwin/bitcoin-3c1ae3ee33d4-arm64-apple-darwin-unsigned.zip
c026a9c6975db5ce11d0005cf0640877ae59d4933a5654e8af80c9e55017b065 guix-build-3c1ae3ee33d4/output/arm64-apple-darwin/bitcoin-3c1ae3ee33d4-arm64-apple-darwin.tar.gz
3c8b6655d2fb37aa1abef62852a602025b089cac54870f5dadd4d3030efcc11e guix-build-3c1ae3ee33d4/output/dist-archive/bitcoin-3c1ae3ee33d4.tar.gz
ffdec3a09f67bf1813dac52021b35a93fe7574507d365db7fd89eeefe6a39086 guix-build-3c1ae3ee33d4/output/powerpc64-linux-gnu/SHA256SUMS.part
af0a7e085c00b1ef6195ed578b03d50a4ac095dcc06076967efde0001e2df6f8 guix-build-3c1ae3ee33d4/output/powerpc64-linux-gnu/bitcoin-3c1ae3ee33d4-powerpc64-linux-gnu-debug.tar.gz
5f6d9a5d629c3c23fa2b9b3d5bf358b58e0bb967303d716edbd237511890820f guix-build-3c1ae3ee33d4/output/powerpc64-linux-gnu/bitcoin-3c1ae3ee33d4-powerpc64-linux-gnu.tar.gz
492661b9b094ef28af4c15fc778ad98bfdd6555084929ebe78bf7f9c2ef7da8d guix-build-3c1ae3ee33d4/output/riscv64-linux-gnu/SHA256SUMS.part
d11cfef1935eb8eca30eb5daf274192527d8efd8e0e1fb89b921b07f7e75cfc7 guix-build-3c1ae3ee33d4/output/riscv64-linux-gnu/bitcoin-3c1ae3ee33d4-riscv64-linux-gnu-debug.tar.gz
5b7b5dc5dba62981b6258dff4d2d7de4ef4fb942d46f791217bfcb68e63371ae guix-build-3c1ae3ee33d4/output/riscv64-linux-gnu/bitcoin-3c1ae3ee33d4-riscv64-linux-gnu.tar.gz
dfe114ec00ac686e72f7c4fdcc186dc59721edacb3f95b4c09359931ec1c156f guix-build-3c1ae3ee33d4/output/x86_64-apple-darwin/SHA256SUMS.part
73e92f868e21325fab2a8209b421078e4bc78d004bb5484b7775d2f93e8e10ce guix-build-3c1ae3ee33d4/output/x86_64-apple-darwin/bitcoin-3c1ae3ee33d4-x86_64-apple-darwin-unsigned.tar.gz
37e9420d74102a5b4e8da8e01b9cdc0321e92e348f2c3ce96968556867f3dd92 guix-build-3c1ae3ee33d4/output/x86_64-apple-darwin/bitcoin-3c1ae3ee33d4-x86_64-apple-darwin-unsigned.zip
83d8297e31a1d18bfe5e17aa06d47a613a6a53361a62b69e784bace682b28757 guix-build-3c1ae3ee33d4/output/x86_64-apple-darwin/bitcoin-3c1ae3ee33d4-x86_64-apple-darwin.tar.gz
4eee423fb1e5a570a5cbbf0a911a785e7760f990bce4e79b10a0173e664b0943 guix-build-3c1ae3ee33d4/output/x86_64-linux-gnu/SHA256SUMS.part
4284bc6e2cd40aa91eb8c190907ee7baa39e999ff3e699f53d3a851be9b0eeea guix-build-3c1ae3ee33d4/output/x86_64-linux-gnu/bitcoin-3c1ae3ee33d4-x86_64-linux-gnu-debug.tar.gz
a88b4ad6be5f03bfe4283c9d5d4a3eec2b74307995c6a5286dd0ab930d2240b5 guix-build-3c1ae3ee33d4/output/x86_64-linux-gnu/bitcoin-3c1ae3ee33d4-x86_64-linux-gnu.tar.gz
4037f7f339beee475220e7261b698356a636889b6482019483632900e646c31c guix-build-3c1ae3ee33d4/output/x86_64-w64-mingw32/SHA256SUMS.part
f2d372664062179a380907f91af47d2d6585ace72498aa2f86d7a14540f737d3 guix-build-3c1ae3ee33d4/output/x86_64-w64-mingw32/bitcoin-3c1ae3ee33d4-win64-debug.zip
ec6a5638012a1b278f84be31004c2820b0305abc41a494671ca5e483e4f1c649 guix-build-3c1ae3ee33d4/output/x86_64-w64-mingw32/bitcoin-3c1ae3ee33d4-win64-setup-unsigned.exe
59eedae949fd796aa6e17bfeee7786ffb8d34fd56c1502a49f3ca6ba0be91d4a guix-build-3c1ae3ee33d4/output/x86_64-w64-mingw32/bitcoin-3c1ae3ee33d4-win64-unsigned.tar.gz
2d02b86f88065fb1023938f8f1e1c012560903ee181d9212558e29ab80ed6530 guix-build-3c1ae3ee33d4/output/x86_64-w64-mingw32/bitcoin-3c1ae3ee33d4-win64.zip |
|
I don't believe any of these are a problem but some differences on flags passed to clang on my Mac were as follows: When using this PR the following extra flags were seen vs master: -MD On master these flags were seen which were not present when using cmake and this PR: -DMACOSX |
|
ACK 3c1ae3e |
TheCharlatan
approved these changes
Apr 25, 2024
There was a problem hiding this comment.
ACK 3c1ae3e
Also checked the natpmp changes since the last update.
fanquake
added a commit
that referenced
this pull request
May 6, 2024
7c69baf depends: pass verbose through to cmake based make (Max Edwards) Pull request description: While testing #29708 I was not able to enable verbose output to check which flags were being given to the compiler. With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake. How to test: ```shell make -C depends libnatpmp V=1 ``` ACKs for top commit: hebasto: ACK 7c69baf. Tested using the folowing command: fanquake: ACK 7c69baf Tree-SHA512: 81cd1326e940c5f14cbde96735fd02b03c1150881ed88d1e8dfa9385dfa12284bfa2cdfe097ce5f43a726c1718afb76ae16f71552ab68c207d74fdc1f7bb46ae
achow101
added a commit
that referenced
this pull request
Aug 5, 2024
ee934d0 doc: Add missed cmake package to build depends (Hennadii Stepanov) Pull request description: CMake is used to build the following packages in depends when cross-compiling for Windows: - `libevent` (#29835) - `libnatpmp` (#29708) - `miniupnpc` (#29707) - `qrencode` (#29725) - `zeromq` (#29723) ACKs for top commit: vostrnad: ACK ee934d0 achow101: ACK ee934d0 TheCharlatan: ACK ee934d0 tdb3: cr ut ACK ee934d0 Tree-SHA512: 7483a680607aa218a375c285859ab19773267c81324de61f457f40057381090b15779534ff0ddb3d981341b9cd9b9e1d4afffda1ec5d5b105ad5bfcac3c7d76a
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 25, 2024
…efiles 7c69baf depends: pass verbose through to cmake based make (Max Edwards) Pull request description: While testing bitcoin#29708 I was not able to enable verbose output to check which flags were being given to the compiler. With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake. How to test: ```shell make -C depends libnatpmp V=1 ``` ACKs for top commit: hebasto: ACK 7c69baf. Tested using the folowing command: fanquake: ACK 7c69baf Tree-SHA512: 81cd1326e940c5f14cbde96735fd02b03c1150881ed88d1e8dfa9385dfa12284bfa2cdfe097ce5f43a726c1718afb76ae16f71552ab68c207d74fdc1f7bb46ae
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 26, 2024
…efiles 7c69baf depends: pass verbose through to cmake based make (Max Edwards) Pull request description: While testing bitcoin#29708 I was not able to enable verbose output to check which flags were being given to the compiler. With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake. How to test: ```shell make -C depends libnatpmp V=1 ``` ACKs for top commit: hebasto: ACK 7c69baf. Tested using the folowing command: fanquake: ACK 7c69baf Tree-SHA512: 81cd1326e940c5f14cbde96735fd02b03c1150881ed88d1e8dfa9385dfa12284bfa2cdfe097ce5f43a726c1718afb76ae16f71552ab68c207d74fdc1f7bb46ae
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This picks up one of the changes from #29232, which is a switch to building libnatpmp with CMake. It includes an update to the most recent version of libnatpmp (miniupnp/libnatpmp@f2433be), which includes (miniupnp/libnatpmp#43).
From an initial look I couldn't find any significant difference between the Autotools and CMake produced libs.