build: add option for external mpgen binary #142
Conversation
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.
There was a problem hiding this comment.
Code review ACK 75cf04a 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.
|
I can also update the depends build in bitcoin/bitcoin#31741 to use this if you didn't already do that. (EDIT: nvm I see you have a branch already) |
| @@ -22,6 +22,8 @@ if(Libmultiprocess_ENABLE_CLANG_TIDY) | |||
| set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}") | |||
| endif() | |||
|
|
|||
| set(EXTERNAL_MPGEN "" CACHE STRING "Use the supplied mpgen binary rather than the one built internally") | |||
There was a problem hiding this comment.
-
The value of
EXTERNAL_MPGENmust be a full path to satisfy theif(EXIST...)condition. Why not make this more explicit by using theFILEPATHtype and mentioning in the help string? -
As a user-facing cache variable, it should have a library-specific prefix, like this one:
libmultiprocess/CMakeLists.txt
Line 18 in 1e06ff0
-
style nit: A full stop at the end of the help string?
| message(FATAL_ERROR "EXTERNAL_MPGEN: \"${MPGEN_BINARY}\" does not exist.") | ||
| endif() | ||
| elseif(TARGET Libmultiprocess::multiprocess) | ||
| set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>) |
There was a problem hiding this comment.
I'm fairly certain that a generation expression isn't necessary here:
| set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>) | |
| set(MPGEN_BINARY Libmultiprocess::mpgen) |
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_sourcesis 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.