build: add option for external mpgen binary #142

1. 
   theuni commented at 5:34 pm on January 31, 2025: contributor

   The parent project or user can supply EXTERNAL_MPGEN, which will override the one set here. If not set, the internally built one is used instead.

   This is useful for cross builds, especially when using libmultiprocess as a subdirectory. Parent projects can similarly define a cache var.

   Note that the internal binary is still built, but it can be skipped by building the multiprocess target directly.

   This is the simplest impl of this I could come up with. Trying to pass an optional option for the binary into target_capnp_sources is not very ergonomic with CMake, so I’ve opted for just using a global here instead.

   It would probably make sense to disable the internal mpgen target if the external option is used, but that adds a good bit of complexity, so I haven’t done it here. Happy to do it as a follow-up if desired.

2. build: add option for external mpgen binary

   This is useful for cross builds.
   
   Note that the internal binary is still built, but it can be skipped by building
   the multiprocess target directly.

   75cf04a6ed
3. 
   theuni commented at 5:41 pm on January 31, 2025: contributor

   Now that #140 has been merged, after this and #141 (or some solution for that problem), we can enable an in-tree built for libmultiprocess with no need for the target build in depends.
4. ryanofsky approved
5. 
   ryanofsky commented at 8:58 pm on January 31, 2025: collaborator

   Code review ACK 75cf04a6ed4870d2023f005a9f0a9ef3c81e9996 Looks great!

   I will probably merge this Monday unless it would help to have it merged sooner, in case @hebasto has feedback.

   I think it is good that specifying EXTERNAL_MPGEN does not remove the mpgen build target. But if it is being built unnecessarily (doesn’t seem like it is) that would be good to fix. In general I don’t like options that enable and disable build targets (see #74), and think all targets that can be built should be available, built when needed, and not built when not needed. Defining targets conditionally turns cmakelists files into spaghetti and makes them more difficult to maintain, and harder to understand and use.

6. 
   ryanofsky commented at 8:59 pm on January 31, 2025: collaborator

   I can also update the depends build in https://github.com/bitcoin/bitcoin/pull/31741 to use this if you didn’t already do that. (EDIT: nvm I see you have a branch already)
7. in CMakeLists.txt:25 in 75cf04a6ed

   21@@ -22,6 +22,8 @@ if(Libmultiprocess_ENABLE_CLANG_TIDY)
   22   set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}")
   23 endif()
   24 
   25+set(EXTERNAL_MPGEN "" CACHE STRING "Use the supplied mpgen binary rather than the one built internally")
   

   ---

   ---

   

   hebasto commented at 0:22 am on February 1, 2025:

   1. The value of EXTERNAL_MPGEN must be a full path to satisfy the if(EXIST...) condition. Why not make this more explicit by using the FILEPATH type and mentioning in the help string?

   2. As a user-facing cache variable, it should have a library-specific prefix, like this one:https://github.com/chaincodelabs/libmultiprocess/blob/1e06ff07cdeadc78b2c022750f7b5cbc88d56b1d/CMakeLists.txt#L18

   3. style nit: A full stop at the end of the help string?

8. in cmake/TargetCapnpSources.cmake:70 in 75cf04a6ed

   67+    set(MPGEN_BINARY "${EXTERNAL_MPGEN}")
   68+    if(NOT EXISTS "${MPGEN_BINARY}")
   69+      message(FATAL_ERROR "EXTERNAL_MPGEN: \"${MPGEN_BINARY}\" does not exist.")
   70+    endif()
   71+  elseif(TARGET Libmultiprocess::multiprocess)
   72+    set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>)
   

   ---

   ---

   

   hebasto commented at 0:25 am on February 1, 2025:

   I’m fairly certain that a generation expression isn’t necessary here:

   0    set(MPGEN_BINARY Libmultiprocess::mpgen)
   

9. 
   hebasto commented at 0:26 am on February 1, 2025: member

   Approach ACK 75cf04a6ed4870d2023f005a9f0a9ef3c81e9996.
10. ryanofsky referenced this in commit f4ce0f0511 on Feb 3, 2025
11. 
    ryanofsky commented at 4:47 pm on February 3, 2025: collaborator

    I’m planning to merge this soon in its current form to be able to clear the backlog of PRs in this repository and update https://github.com/bitcoin/bitcoin/pull/31741, but hebasto’s suggestions in https://github.com/chaincodelabs/libmultiprocess/pull/142#pullrequestreview-2588097796 make sense and I implemented them in a followup
12. ryanofsky merged this on Feb 3, 2025
13. ryanofsky closed this on Feb 3, 2025

    ---

14. ryanofsky referenced this in commit 21b92b69c9 on Feb 3, 2025
15. ryanofsky referenced this in commit f09c50118f on Feb 3, 2025
16. ryanofsky referenced this in commit 8959938ed4 on Feb 3, 2025
17. ryanofsky referenced this in commit 37fb62c3a5 on Feb 4, 2025
18. 
    hebasto commented at 12:20 pm on February 4, 2025: member

    @ryanofsky

    In general I don’t like options that enable and disable build targets (see #74), and think all targets that can be built should be available, built when needed, and not built when not needed.

    I’m curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

    (I assume, that your phrase “In general” extends this approach scope beyond this project).

19. 
    ryanofsky commented at 1:42 pm on February 4, 2025: collaborator

    @ryanofsky

    I’m curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

    Ideally, I think there should be WITH_XXXX options that control whether or not cmake searches for each XXXX dependency and targets are not declared if their dependencies are not present.

20. ryanofsky referenced this in commit 1d75538a32 on Feb 5, 2025
21. ryanofsky referenced this in commit 9437e6846f on Feb 7, 2025
22. ryanofsky referenced this in commit a4a8f7a7ba on Feb 7, 2025
23. ryanofsky referenced this in commit 3a95817ece on Feb 10, 2025
24. 
    hebasto commented at 3:16 pm on February 10, 2025: member

    First, I apologise for using this PR as a forum for general discussion.

    @ryanofsky I’m curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

    Ideally, I think there should be WITH_XXXX options that control whether or not cmake searches for each XXXX dependency and targets are not declared if their dependencies are not present.

    I’m still not convinced, though. Consider a general case with a set of targets, T, a set of dependencies, D, and a non-trivial mapping from T to D.

    When a user wishes to build a subset of targets, T’, they must somehow to calculate the corresponding subset of dependencies, D’—a task that does not appear to be intended for users.

    For an additional example from the LLVM project, simply grep for option(LLVM_BUILD_.

25. Sjors referenced this in commit 6aabfcb615 on Feb 10, 2025
26. 
    ryanofsky commented at 3:49 pm on February 10, 2025: collaborator

    re: https://github.com/chaincodelabs/libmultiprocess/pull/142#issuecomment-2648337815

    First, I apologise for using this PR as a forum for general discussion.

    No problem, I encourage making issues here just to discuss things, and it seems fine to use this PR, even though opening a new issue could be a little better in the future.

    When a user wishes to build a subset of targets, T’, they must somehow to calculate the corresponding subset of dependencies, D’—a task that does not appear to be intended for users.

    I don’t know what use case you are thinking of but this doesn’t correspond to any scenario I can remember from my experience.

    Usually I want to build all targets, or I want to build one or two specific targets. When I want to build all targets I want to be able to run “make all”. When I want to build individual targets I want to run “make thing1 thing2”. In neither of these cases do I want to resort to using cmake and toggling targets on and off. I want to run cmake once and then run “make” after that to build the things I need when I need them.

    So BUILD_xxx opttions seem mostly useless to me I don’t really understand why someone would want them.

    But WITH_xxx options are incredibly useful because they allow the project to use a lot of dependencies while still being usable on systems that don’t have all the dependencies installed. So when I do run cmake (again, hopefully just one time) it is helpful to be able to turn on/off dependencies depending on what is available in my current environment. It is also useful to be able to see what extra dependencies are available that I might want to install to enable more features.

27. Sjors referenced this in commit 1746618e08 on Feb 13, 2025
28. ryanofsky referenced this in commit 83e40d3b52 on Feb 24, 2025
29. ryanofsky referenced this in commit 8619f03ec2 on Feb 24, 2025
30. ryanofsky referenced this in commit cbb7b41c20 on Feb 24, 2025
31. fanquake referenced this in commit 01f7715766 on Feb 25, 2025
32. fanquake referenced this in commit ba0a4391ff on Feb 25, 2025
33. janus referenced this in commit 86cb86b050 on Sep 1, 2025

Contributors
theuni ryanofsky hebasto