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

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:

$ # `nsis` is not installed
$ cmake -B build -G "GNU Makefiles" --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
$ cmake --build build -j $(nproc)
$ cmake --build build -t deploy

Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE variable or another relevant CMake variable.
Then re-run cmake to regenerate the build system.

Built target deploy
$ sudo apt install nsis
$ cmake -B build
$ cmake --build build -t deploy
...
[100%] Generating bitcoin-win64-setup.exe
[100%] Built target deploy

Fixes #32018.

hebasto added the label Build system on Mar 7, 2025

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.

hodlinator approved

hodlinator commented at 10:11 PM on March 7, 2025: contributor

tACK deea0988720aab65b3cc6d80b50bbf0d537adb24

Tested on WSL.

Without:

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

With PR (but still no nsis installed):

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

Works after installing nsis.

laanwj approved

laanwj commented at 5:04 AM on March 8, 2025: member

Code review ACK deea0988720aab65b3cc6d80b50bbf0d537adb24

mabu44 commented at 7:35 PM on March 8, 2025: none

tACK deea0988720aab65b3cc6d80b50bbf0d537adb24

Got same result as @hodlinator on Debian.

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.

hebasto force-pushed on Mar 9, 2025

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.

hebasto marked this as a draft on Mar 9, 2025

hebasto force-pushed on Mar 9, 2025

DrahtBot added the label CI failed on Mar 9, 2025

hebasto marked this as ready for review on Mar 9, 2025

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.

DrahtBot removed the label CI failed on Mar 9, 2025

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

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.

hodlinator approved

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:

/mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
Error: NSIS not found
Built 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.

DrahtBot requested review from laanwj on Mar 10, 2025

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:
/mnt/c/Users/hodlinator/bitcoin$ cmake --build build --target deploy
Error: NSIS not found
Built 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?

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:

Error: NSIS not found. Install it (make it available to find_program()) and regenerate the build config.

hebasto force-pushed on Mar 10, 2025

hebasto commented at 3:00 PM on March 10, 2025: member

Addressed the feedback from @hodlinator. Thank you!

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?

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.

hebasto commented at 3:56 PM on March 10, 2025: member

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

Correct.

laanwj commented at 3:58 PM on March 10, 2025: member

Tested windows path without NSIS

$ ninja -j4
[224/224] Linking CXX executable src/test/test_bitcoin.exe (successful build)
$ ninja deploy
[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

Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE variable or another relevant CMake variable.
Then re-run cmake to regenerate the build system.

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

purpleKarrot commented at 9:51 PM on March 10, 2025: contributor

Perhaps @purpleKarrot could suggest a better approach?

CPack?

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:

        COMMAND ${CMAKE_COMMAND} -E echo &&
                ${CMAKE_COMMAND} -E echo "Error: NSIS not found." &&
                ${CMAKE_COMMAND} -E echo "Please install NSIS and/or ensure that its executable is accessible to the find_program() command." &&
                ${CMAKE_COMMAND} -E echo "Then re-run cmake to regenerate the build system." &&
                ${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 12: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.

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 :)

hodlinator approved

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.

cmake: Check for `makensis` tool before using it 0aeff29951

cmake: Require `zip` only for `deploy` target 1f9b2e150c

hebasto force-pushed on Mar 13, 2025

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.

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:

Error: NSIS not found
Satisfy 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.

fanquake added this to the milestone 29.0 on Mar 14, 2025

fanquake approved

fanquake commented at 9:07 AM on March 18, 2025: member

ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65

fanquake merged this on Mar 18, 2025

fanquake closed this on Mar 18, 2025

hebasto deleted the branch on Mar 18, 2025