cmake: Check for makensis and zip tools before using them for optional deploy targets #32019

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:250307-makensis changing 2 files +26 −12
  1. hebasto commented at 4:23 pm on March 7, 2025: member

    For x86_64-w64-mingw32 and *-apple-darwin targets, the optional deploy target requires dedicated tools: makensis and zip, respectively.

    This PR introduces a uniform checks for those tools when attempting to build the deploy target, ensuring they are not required for configuring and building any other targets.

    Here is an example of workflow for x86_64-w64-mingw32:

     0$ # `nsis` is not installed
 1$ cmake -B build -G "GNU Makefiles" --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
 2$ cmake --build build -j $(nproc)
 3$ cmake --build build -t deploy
 4
 5Error: NSIS not found.
 6Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
 7for example, by setting the MAKENSIS_EXECUTABLE variable or another relevant CMake variable.
 8Then re-run cmake to regenerate the build system.
 9
10Built target deploy
11$ sudo apt install nsis
12$ cmake -B build
13$ cmake --build build -t deploy
14...
15[100%] Generating bitcoin-win64-setup.exe
16[100%] Built target deploy

    Fixes #32018.

  2. hebasto added the label Build system on Mar 7, 2025
  3. DrahtBot commented at 4:23 pm on March 7, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32019.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, fanquake
    Stale ACK laanwj, mabu44

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)

    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.

  4. hodlinator approved
  5. hodlinator commented at 10:11 pm on March 7, 2025: contributor

    tACK deea0988720aab65b3cc6d80b50bbf0d537adb24

    Tested on WSL.

    Without:

    0$ cmake --build build --target deploy -j16
1...
2[100%] Generating bitcoin-win64-setup.exe
3gmake[3]: makensis: No such file or directory
4gmake[3]: *** [CMakeFiles/deploy.dir/build.make:81: bitcoin-win64-setup.exe] Error 127
5gmake[2]: *** [CMakeFiles/Makefile2:530: CMakeFiles/deploy.dir/all] Error 2
6gmake[1]: *** [CMakeFiles/Makefile2:537: CMakeFiles/deploy.dir/rule] Error 2
7gmake: *** [Makefile:244: deploy] Error 2

    With PR (but still no nsis installed):

    0$ cmake --build build --target deploy -j16
1...
2CMake Error at cmake/module/Maintenance.cmake:49 (find_program):
3  Could not find MAKENSIS_EXECUTABLE using the following names: makensis
4Call Stack (most recent call first):
5  CMakeLists.txt:628 (add_windows_deploy_target)

    Works after installing nsis.

  6. laanwj approved
  7. laanwj commented at 5:04 am on March 8, 2025: member
    Code review ACK deea0988720aab65b3cc6d80b50bbf0d537adb24
  8. mabu44 commented at 7:35 pm on March 8, 2025: none

    tACK deea0988720aab65b3cc6d80b50bbf0d537adb24

    Got same result as @hodlinator on Debian.

  9. fanquake commented at 3:29 pm on March 9, 2025: member
    Now that nsis is required, the docs about installing nsis in build-windows.md should be updated.
  10. hebasto force-pushed on Mar 9, 2025
  11. hebasto commented at 4:33 pm on March 9, 2025: member

    Now that nsis is required, the docs about installing nsis in build-windows.md should be updated.

    Thanks for pointing this out!

    Since the deploy target is optional, the NSIS tool shouldn’t be a strict requirement for a successful configuration.

    I’ve reworked this PR so that the error message “Error: NSIS not found” is only printed when attempting to build the deploy target.

  12. hebasto marked this as a draft on Mar 9, 2025
  13. hebasto force-pushed on Mar 9, 2025
  14. DrahtBot added the label CI failed on Mar 9, 2025
  15. hebasto marked this as ready for review on Mar 9, 2025
  16. fanquake commented at 4:48 pm on March 9, 2025: member

    the NSIS tool shouldn’t be a strict requirement for a successful configuration.

    Then the same should be done for macOS and zip, otherwise it’s still inconsistent.

  17. DrahtBot removed the label CI failed on Mar 9, 2025
  18. hebasto renamed this:
    cmake: Check for `makensis` tool before using it
    cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets
    on Mar 9, 2025
  19. hebasto commented at 7:15 pm on March 9, 2025: member

    the NSIS tool shouldn’t be a strict requirement for a successful configuration.

    Then the same should be done for macOS and zip, otherwise it’s still inconsistent.

    Sure thing! Added.

    The PR description has been updated.

  20. hodlinator approved
  21. hodlinator commented at 12:59 pm on March 10, 2025: contributor

    re-ACK 0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9

    Good that nsis and zip could be made optional unless deploying.

    Only re-tested Windows-side, but Mac side diff looks equivalent.

    Without nsis:

    0/mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
1Error: NSIS not found
2Built target deploy

    I needed to re-run cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake for the build to pick up that makensis was available. It would be good if we could invalidate the cache upon failure, or indicate in the error message that one has to regenerate the build config.

  22. DrahtBot requested review from laanwj on Mar 10, 2025
  23. hebasto commented at 1:29 pm on March 10, 2025: member

    re-ACK 0ecd2e0

    Good that nsis and zip could be made optional unless deploying.

    Only re-tested Windows-side, but Mac side diff looks equivalent.

    Without nsis:

    0/mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
1Error: NSIS not found
2Built target deploy

    I needed to re-run cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake for the build to pick up that makensis was available. It would be good if we could invalidate the cache upon failure

    I had exactly the same idea while working on this implementation (also see a discussion in #31942). But the configuration step should be re-run after installing a dependency to let the deploy target choose another implementation.

    Perhaps @purpleKarrot could suggest a better approach?

    or indicate in the error message that one has to regenerate the build config.

    Agree. Mind suggesting a wording for the error message?

  24. hodlinator commented at 1:38 pm on March 10, 2025: contributor

    or indicate in the error message that one has to regenerate the build config.

    Agree. Mind suggesting a wording for the error message?

    Felt I was going out on a limb using the term “build config”, was thinking something like:

    0Error: NSIS not found. Install it (make it available to find_program()) and regenerate the build config.
  25. hebasto force-pushed on Mar 10, 2025
  26. hebasto commented at 3:00 pm on March 10, 2025: member
    Addressed the feedback from @hodlinator. Thank you!
  27. Sjors commented at 3:17 pm on March 10, 2025: member
    Tested on macOS 15.3.1 that cmake --build build --target deploy still works. It’s impossible (?) to test the missing condition, because/usr/bin/zip comes with the OS and not even sudo can delete it. Seems fine to check though, because who knows what Apple does in their next OS upgrade. And I guess this also covers cross and guix compiling?
  28. Sjors commented at 3:25 pm on March 10, 2025: member
    Oh this code path isn’t used at all when building natively? E.g. on macOS I can do ZIP_EXECUTABLE=$HOME/temp/zip2 cmake --build build --target deploy which doesn’t fail.
  29. hebasto commented at 3:56 pm on March 10, 2025: member

    Oh this code path isn’t used at all when building natively?

    Correct.

  30. laanwj commented at 3:58 pm on March 10, 2025: member

    Tested windows path without NSIS

    0$ ninja -j4
1[224/224] Linking CXX executable src/test/test_bitcoin.exe (successful build)
2$ ninja deploy
3[1/1] cd /home/user/src/bitcoin-windows/build && /usr/bin/cmake -E echo && /usr/bin/cm... -E echo "Then re-run cmake to regenerate the build system." && /usr/bin/cmake -E echo
4
5Error: NSIS not found.
6Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
7for example, by setting the MAKENSIS_EXECUTABLE variable or another relevant CMake variable.
8Then re-run cmake to regenerate the build system.

    Configuration and build succeeds, but deploy fails with a message. This is the expected behavior.

  31. purpleKarrot commented at 9:51 pm on March 10, 2025: contributor

    Perhaps @purpleKarrot could suggest a better approach?

    CPack?

  32. in cmake/module/Maintenance.cmake:50 in b86eb5f45a outdated 
    43@@ -44,6 +44,20 @@ endfunction()
44 
45 function(add_windows_deploy_target)
46   if(MINGW AND TARGET bitcoin-qt AND TARGET bitcoind AND TARGET bitcoin-cli AND TARGET bitcoin-tx AND TARGET bitcoin-wallet AND TARGET bitcoin-util AND TARGET test_bitcoin)
47+    find_program(MAKENSIS_EXECUTABLE makensis)
48+    if(NOT MAKENSIS_EXECUTABLE)
49+      add_custom_target(deploy
50+        COMMAND ${CMAKE_COMMAND} -E echo
    fanquake commented at 6:39 am on March 11, 2025:
    0~ This seems far too verbose/exposing implementation details, and we don’t do this for any other dep.
    hodlinator commented at 9:24 am on March 11, 2025:

    I think the code is noisy, but like that the text is precise, could skip the example though. Tried to make the code less noisy too but best I could come up with while still using echo was:

    0        COMMAND ${CMAKE_COMMAND} -E echo &&
1                ${CMAKE_COMMAND} -E echo "Error: NSIS not found." &&
2                ${CMAKE_COMMAND} -E echo "Please install NSIS and/or ensure that its executable is accessible to the find_program() command." &&
3                ${CMAKE_COMMAND} -E echo "Then re-run cmake to regenerate the build system." &&
4                ${CMAKE_COMMAND} -E echo
    hebasto commented at 6:25 pm on March 11, 2025:

    This seems far too verbose…

    Having an optional dependency, the absence of which is revealed during the configuration stage, is quite unusual and requires better communication to the user.

    This seems … exposing implementation details

    MAKENSIS_EXECUTABLE is a cache variable that the user is expected to set when necessary, like any other cache variable. It cannot be considered as “implementation details”.

    we don’t do this for any other dep

    Other deps are handled during the configuration step, which differs from this scenario with optional dependencies.

    fanquake commented at 0:04 am on March 12, 2025:

    find_program() command.

    Details like this are completely unnecessary.

    fanquake commented at 8:31 am on March 12, 2025:
    We also don’t do this for other similar targets, like docs. Where we just print "Error: Doxygen not found".
    laanwj commented at 11:09 am on March 12, 2025:

    i kind of liked the mention of MAKENSIS_EXECUTABLE, it is my experience that it can be frustrating to find out what exact environment variable to set to point the build system at the right locations if you don’t have everything installed in standard locations.

    But yes maybe error messages aren’t the best place for documentation.

    hebasto commented at 11:25 am on March 13, 2025:
    Thanks! Reworked.
  33. hebasto commented at 6:17 pm on March 11, 2025: member

    Perhaps @purpleKarrot could suggest a better approach?

    CPack?

    https://github.com/bitcoin/bitcoin/blob/dbc89b604c4dae9702f1ff08abd4ed144a5fcb76/cmake/module/Maintenance.cmake#L47-L48 :)

  34. hodlinator approved
  35. hodlinator commented at 1:55 pm on March 12, 2025: contributor

    re-ACK b86eb5f45a6df456d6aa80d62dd5b720ecfe917c

    Would prefer something a bit more terse as mentioned in #32019 (review), but not a blocker for me.

  36. cmake: Check for `makensis` tool before using it 0aeff29951
  37. cmake: Require `zip` only for `deploy` target 1f9b2e150c
  38. hebasto force-pushed on Mar 13, 2025
  39. hebasto commented at 11:25 am on March 13, 2025: member

    The recent feedback regarding verbosity has been addressed.

    All error messages about a missed optional executable are now consistent.

    Rebased on top of bitcoin/bitcoin#31161.

  40. hodlinator commented at 3:38 pm on March 13, 2025: contributor

    re-ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65

    We’re back to an exact copy of 0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9, only rebased, which I previously ACKed.

    If you retouch and have remaining patience I would still prefer:

    0Error: NSIS not found
1Satisfy the dependency and re-run CMake to regenerate the build system.

    Tried to figure out a way to get cmake to return an error code after printing the error, but without success.

  41. fanquake added this to the milestone 29.0 on Mar 14, 2025
  42. fanquake approved
  43. fanquake commented at 9:07 am on March 18, 2025: member
    ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65
  44. fanquake merged this on Mar 18, 2025
  45. fanquake closed this on Mar 18, 2025
  46. hebasto deleted the branch on Mar 18, 2025


hebasto DrahtBot hodlinator laanwj mabu44 fanquake Sjors purpleKarrot


laanwj

Labels
Build system

Milestone
29.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-31 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me