cmake: Add `MULTIPROCESS` option #118

hebasto · 2024-03-09T17:30:20Z

Also a new "Linux 64-bit, multiprocess" CI task has been added.

Also see: bitcoin#29665.

pablomartin4btc

cr ACK a08579d

New CI added as mentioned in description;
Setting multiprocess option ON;
Added new macro generate_from_capnp in cmake/module/GenerateMPCode.cmake;
Added new CMakeLists.txt for IPC directory;
Added new IPC library into linking for bitcoin-gui targets when multiprocess is enabled.

Doc reference for multiprocess and IPC.

Other than that, new CI is running successfully:

Where in the CI details can be seeing the multiprocess optional package enabled in the configuration summary (Generate build system task):

...

Optional packages:
  GUI ................................. Qt5
  external signer ..................... ON
  multiprocess ........................ ON
...

hebasto · 2024-03-11T09:16:01Z

@ryanofsky

You may also be interested in this PR :)

ryanofsky

Code review ACK a08579d.

Seems like a nice change. I do agree with the "TODO: Switch to find_package() command" comment. It would be nice to avoid using pkgconfig and ideally find the package using the ${libdir}/cmake/libmultiprocess-*.cmake files that are installed.

cmake/module/GenerateMPCode.cmake

src/ipc/CMakeLists.txt

hebasto · 2024-03-11T20:25:26Z

I do agree with the "TODO: Switch to find_package() command" comment. It would be nice to avoid using pkgconfig and ideally find the package using the ${libdir}/cmake/libmultiprocess-*.cmake files that are installed.

Unfortunately, those target import files do not propagate dependency targets. As such, they have to be handled separately which increases complexity.

hebasto · 2024-03-11T21:46:50Z

@ryanofsky

Thank you for your valuable review!

I've reworked the branch basing on your suggestion. Now it looks much cleaner :)

pablomartin4btc

re ACK 8e739cd

Addressed @ryanofsky's feedback since my last review, mainly replacing macro generate_from_capnp capnp_relpath by function target_capnp_sources.

ryanofsky

Code review ACK 8e739cd

re: #118 (comment)

Unfortunately, those target import files do not propagate dependency targets. As such, they have to be handled separately which increases complexity.

Hmm, what does this mean specifically? It seems like the installed cmake files are calling add_library and add_executable to add targets and setting the needed properties. Does something need to be improved in https://github.com/chaincodelabs/libmultiprocess/blob/master/CMakeLists.txt, because current install(EXPORT...) lines are not enough?

cmake/module/GenerateMPCode.cmake

ryanofsky · 2024-03-12T19:21:09Z

depends/toolchain.cmake.in

+    set(MULTIPROCESS ON CACHE STRING "")
+  endif()
+  if(NOT MPGEN_PREFIX)
+    set(MPGEN_PREFIX "${CMAKE_FIND_ROOT_PATH}/native" CACHE PATH "")


I don't really understand what this is doing, might be helpful to add a comment. Particularly what is the last "" argument?

EDIT: I guess last "" arguments are for documentation? Are these arguments actually required? If they are it seems like it would be helpful to change them to something like "Set by depends build" to document where they are coming from

This command uses the following signature:

set(<variable> <value>... CACHE <type> <docstring> [FORCE])

where the <docstring> is mandatory. It allows the user to override its value with -DMPGEN_PREFIX=... in the command line.

What about "libmultiprocess codegen tool prefix"?

What about "libmultiprocess codegen tool prefix"?

Well "libmultiprocess codegen tool prefix" already seems to be the documentation string for this option in cmake/optional.cmake so that seems very good.

But I'm confused about why any documentation string is required at all in depends/toolchain.cmake.in which should just be overriding values, not defining new options. If I understand https://cmake.org/cmake/help/latest/command/set.html correctly, in-memory values naturally override cached values, so it seems like the ideal thing would be to stop specifying CACHE <type> <docstring> arguments in this file, and just set the values in memory. If writing the cache here really is necessary though, I'd think appropriate doc string might be be something like "Overridden by depends build" to indicate that cached values have been overridden.

If writing the cache here really is necessary though...

It is. Otherwise, the user won't be able to override a build option via command line.
For example, passing -DMULTIPROCESS=OFF won't have any effect.

This is a specific of toolchain files.

... I'd think appropriate doc string might be be something like "Overridden by depends build" to indicate that cached values have been overridden.

Docstrings are available via -LH command-line option or in cmake-gui. And you are right that they shouldn't be empty.

However, its content must just be copy-pasted from the option() commands from the main CMakeLists.txt file. Otherwise, for example, when the user passes -DMULTIPROCESS=OFF, it ends with:

// Overridden by depends build MULTIPROCESS:STRING=OFF

which is misleading.

... you are right that they shouldn't be empty.

Fixed in #122.

ryanofsky · 2024-03-12T19:26:08Z

cmake/module/GenerateMPCode.cmake

+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/license/mit/.
+
+function(target_capnp_sources target)


This function seems generally useful, so it would be great to move this module to https://github.com/chaincodelabs/libmultiprocess/tree/master/cmake /module, and use it upstream to replace hardcoded add_custom_command calls, and install it so it could be reused by the bitcoin build.

ryanofsky

Code review ACK 3542d08

ryanofsky · 2024-03-14T20:20:19Z

depends/toolchain.cmake.in

+    set(MULTIPROCESS ON CACHE STRING "")
+  endif()
+  if(NOT MPGEN_PREFIX)
+    set(MPGEN_PREFIX "${CMAKE_FIND_ROOT_PATH}/native" CACHE PATH "")


What about "libmultiprocess codegen tool prefix"?

Well "libmultiprocess codegen tool prefix" already seems to be the documentation string for this option in cmake/optional.cmake so that seems very good.

But I'm confused about why any documentation string is required at all in depends/toolchain.cmake.in which should just be overriding values, not defining new options. If I understand https://cmake.org/cmake/help/latest/command/set.html correctly, in-memory values naturally override cached values, so it seems like the ideal thing would be to stop specifying CACHE <type> <docstring> arguments in this file, and just set the values in memory. If writing the cache here really is necessary though, I'd think appropriate doc string might be be something like "Overridden by depends build" to indicate that cached values have been overridden.

hebasto · 2024-03-18T11:04:04Z

Unfortunately, those target import files do not propagate dependency targets. As such, they have to be handled separately which increases complexity.

Hmm, what does this mean specifically? It seems like the installed cmake files are calling add_library and add_executable to add targets and setting the needed properties. Does something need to be improved in https://github.com/chaincodelabs/libmultiprocess/blob/master/CMakeLists.txt, because current install(EXPORT...) lines are not enough?

The targets from the CapnProto package are not imported. Resolved in bitcoin-core/libmultiprocess#96:

include(CMakeFindDependencyMacro)
find_dependency(CapnProto)

840e494 fixup! build: Generate `share/toolchain.cmake` in depends (Hennadii Stepanov) Pull request description: See discussion in #118 (comment). Can be tested in the following ways: - applying the `-LH` command line option - using `cmake-gui` - direct observing of the `CMakeCache.txt` file content ACKs for top commit: pablomartin4btc: tACK 840e494 Tree-SHA512: fd441457dc4df44b54c5fe98119786126db6b592f58ce7bca642a65921ffb9572d0e2afca07471666f425b647ef69f860228b8a0d61d02468d41edccad25627b

hebasto · 2024-03-18T11:26:06Z

Rebased due to the conflicts and undrafted.

Added lacking docstrings.

hebasto · 2024-03-26T10:48:51Z

Rebased.

66643d8 cmake, refactor: Use `target_capnp_sources` for examples (Hennadii Stepanov) bd2dfe2 cmake, refactor: Use `target_capnp_sources` for `mptest` target (Hennadii Stepanov) d9ec22f cmake: Add `LibmultiprocessMacros` module (Hennadii Stepanov) Pull request description: This PR introduces the `LibmultiprocessMacros` module that is used internally and might be exported as a part of the (future) `LibmultiprocessGen` package (it is a subject of the follow-up [PR](#96)). Also see a discussion in hebasto/bitcoin#118. ACKs for top commit: ryanofsky: Code review ACK 66643d8 Tree-SHA512: b97fcb2ce7b2085b3a041c422e3f51f06b8bf7a0abaa5baa7f39db2bf03491447bf7af073c95fb8bb6bc602d30e0c8856ae523954980cdb85afd87442c0b909a

hebasto · 2024-03-30T07:15:38Z

Reworked in a clean CMake's way using the recent updates in the https://github.com/chaincodelabs/libmultiprocess repository.

However, not using the last commit due to an issue with package search.

Not using the latest commit 3f8483b61a5eee3e958328d90f25f29a1a61ae7b due to bitcoin-core/libmultiprocess#97 (comment)

Add "Linux 64-bit, multiprocess" task.

hebasto · 2024-04-03T16:30:18Z

Rebased.

hebasto · 2024-04-03T19:28:01Z

@ryanofsky

While I'm still looking into the recent changes in the upstream repo, do you think it is reasonable to proceed with this PR as it is to unblock the following work on the CI stuff?

Anyway, we're likely to revisit this code, once the upstream packaging improvements conclude.

ryanofsky · 2024-04-03T20:18:08Z

While I'm still looking into the recent changes in the upstream repo, do you think it is reasonable to proceed with this PR as it is to unblock the following work on the CI stuff?

Of course, do you need anything from me?

hebasto · 2024-04-03T20:19:18Z

While I'm still looking into the recent changes in the upstream repo, do you think it is reasonable to proceed with this PR as it is to unblock the following work on the CI stuff?

Of course, do you need anything from me?

Approval?

:)

ryanofsky · 2024-04-03T20:34:34Z

Hmm, I'm not sure if you want me to literally ACK the PR since it is just going into a private branch. The thing I still don't understand is why the depends system is defining cache options like set(MULTIPROCESS ON CACHE STRING "") that don't have documentation or just duplicate options in the top level cmake file. I don't understand why same options would be defined two places, instead of just once or why we would want two copies of the same documentation in different files. I don't think this should block anything and you should definitely proceed with the work, but I just don't understand everything that is going on yet.

hebasto · 2024-04-03T20:43:06Z

The thing I still don't understand is why the depends system is defining cache options...

If depends set such options as non-cache variables, the user won't be able to override them in the command line, for example, with -DMULTIPROCESS=OFF.

... set(MULTIPROCESS ON CACHE STRING "") that don't have documentation...

That is no longer the case in general. But documentation is missed for MULTIPROCESS and I'm going to fix it shortly in this PR.

hebasto · 2024-04-03T20:44:56Z

I don't understand why same options would be defined two places, instead of just once or why we would want two copies of the same documentation in different files.

I don't have a better implementation for now.

fanquake · 2024-04-03T20:47:23Z

If depends set such options as non-cache variables, the user won't be able to override them in the command line, for example, with -DMULTIPROCESS=OFF.

So this is to accomodate someone who does a depends build, specifically enabling multiprocess, who then wants to use the toolchain file, but also disable multiprocess?

ryanofsky · 2024-04-03T20:48:24Z

I don't have a better implementation for now.

Understood, and I don't have one either. I think you should definitely proceed with you have and if there is anything I can do to help just let me know.

hebasto · 2024-04-03T20:48:32Z

If depends set such options as non-cache variables, the user won't be able to override them in the command line, for example, with -DMULTIPROCESS=OFF.

So this is to accomodate someone who does a depends build, specifically enabling multiprocess, who then wants to use the toolchain file, but also disable multiprocess?

Yes, it is.

fanquake · 2024-04-03T20:51:11Z

Shouldn't they just not enable multiprocess in the first place?

hebasto · 2024-04-03T20:53:12Z

Shouldn't they just not enable multiprocess in the first place?

Of course, they should not.

I'll take more time to think about it.

hebasto · 2024-04-08T13:20:50Z

I'm going to merge this PR as it is.

However, handling of the build configuration variables in the depends toolchain file should be revisited.

hebasto mentioned this pull request Mar 9, 2024

CMake-based build system tracking issue bitcoin/bitcoin#28607

Closed

16 tasks

hebasto added the enhancement New feature or request label Mar 9, 2024

hebasto force-pushed the 240309-cmake-CI branch from cb66647 to a08579d Compare March 9, 2024 19:01

pablomartin4btc approved these changes Mar 11, 2024

View reviewed changes

ryanofsky approved these changes Mar 11, 2024

View reviewed changes

cmake/module/GenerateMPCode.cmake Outdated Show resolved Hide resolved

src/ipc/CMakeLists.txt Outdated Show resolved Hide resolved

hebasto force-pushed the 240309-cmake-CI branch from a08579d to 8e739cd Compare March 11, 2024 21:44

pablomartin4btc approved these changes Mar 12, 2024

View reviewed changes

ryanofsky approved these changes Mar 12, 2024

View reviewed changes

hebasto force-pushed the 240309-cmake-CI branch from 8e739cd to 3542d08 Compare March 12, 2024 19:47

hebasto marked this pull request as draft March 12, 2024 19:49

ryanofsky approved these changes Mar 14, 2024

View reviewed changes

This was referenced Mar 15, 2024

cmake: Introduce LibmultiprocessMacros module bitcoin-core/libmultiprocess#95

Merged

cmake: Add docstrings for options when cross-compiling #122

Merged

hebasto force-pushed the 240309-cmake-CI branch from 3542d08 to 0ef581f Compare March 18, 2024 11:24

hebasto marked this pull request as ready for review March 18, 2024 11:25

hebasto force-pushed the cmake-staging branch from 700eaae to 8078427 Compare March 26, 2024 09:07

hebasto force-pushed the 240309-cmake-CI branch from 0ef581f to 69d3f7a Compare March 26, 2024 10:48

hebasto force-pushed the 240309-cmake-CI branch from 69d3f7a to 4b5fb0d Compare March 30, 2024 06:39

hebasto force-pushed the cmake-staging branch from a3313e2 to 418b4f4 Compare April 3, 2024 15:40

hebasto added 2 commits April 3, 2024 17:29

depends: Bump libmultiprocess source

aae82b3

Not using the latest commit 3f8483b61a5eee3e958328d90f25f29a1a61ae7b due to bitcoin-core/libmultiprocess#97 (comment)

cmake: Add MULTIPROCESS option

c24647c

fixup! ci: Test CMake edge cases

6c64fc2

Add "Linux 64-bit, multiprocess" task.

hebasto force-pushed the 240309-cmake-CI branch from 4b5fb0d to 6c64fc2 Compare April 3, 2024 16:29

hebasto merged commit c1e1967 into cmake-staging Apr 8, 2024
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Add `MULTIPROCESS` option #118

cmake: Add `MULTIPROCESS` option #118

hebasto commented Mar 9, 2024 •

edited

Loading

pablomartin4btc left a comment

hebasto commented Mar 11, 2024

ryanofsky left a comment

hebasto commented Mar 11, 2024

hebasto commented Mar 11, 2024

pablomartin4btc left a comment

ryanofsky left a comment •

edited

Loading

ryanofsky Mar 12, 2024 •

edited

Loading

hebasto Mar 12, 2024

ryanofsky Mar 14, 2024

hebasto Mar 16, 2024

hebasto Mar 16, 2024 •

edited

Loading

ryanofsky Mar 12, 2024 •

edited

Loading

ryanofsky left a comment

ryanofsky Mar 14, 2024

hebasto commented Mar 18, 2024

hebasto commented Mar 18, 2024

hebasto commented Mar 26, 2024

hebasto commented Mar 30, 2024

hebasto commented Apr 3, 2024

hebasto commented Apr 3, 2024

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024 •

edited

Loading

hebasto commented Apr 3, 2024

fanquake commented Apr 3, 2024 •

edited

Loading

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024

fanquake commented Apr 3, 2024

hebasto commented Apr 3, 2024

hebasto commented Apr 8, 2024

cmake: Add MULTIPROCESS option #118

cmake: Add MULTIPROCESS option #118

Conversation

hebasto commented Mar 9, 2024 • edited Loading

pablomartin4btc left a comment

Choose a reason for hiding this comment

hebasto commented Mar 11, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

hebasto commented Mar 11, 2024

hebasto commented Mar 11, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

ryanofsky Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

hebasto Mar 12, 2024

Choose a reason for hiding this comment

ryanofsky Mar 14, 2024

Choose a reason for hiding this comment

hebasto Mar 16, 2024

Choose a reason for hiding this comment

hebasto Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

ryanofsky Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Mar 14, 2024

Choose a reason for hiding this comment

hebasto commented Mar 18, 2024

hebasto commented Mar 18, 2024

hebasto commented Mar 26, 2024

hebasto commented Mar 30, 2024

hebasto commented Apr 3, 2024

hebasto commented Apr 3, 2024

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024 • edited Loading

hebasto commented Apr 3, 2024

fanquake commented Apr 3, 2024 • edited Loading

ryanofsky commented Apr 3, 2024

hebasto commented Apr 3, 2024

fanquake commented Apr 3, 2024

hebasto commented Apr 3, 2024

hebasto commented Apr 8, 2024

cmake: Add `MULTIPROCESS` option #118

cmake: Add `MULTIPROCESS` option #118

hebasto commented Mar 9, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Mar 12, 2024 •

edited

Loading

hebasto Mar 16, 2024 •

edited

Loading

ryanofsky Mar 12, 2024 •

edited

Loading

hebasto commented Apr 3, 2024 •

edited

Loading

fanquake commented Apr 3, 2024 •

edited

Loading