Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiprocess for non depends #65

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sjors
Copy link
Owner

@Sjors Sjors commented Oct 7, 2024

No description provided.

Sjors and others added 4 commits October 7, 2024 10:25
This update brings in the following changes:
chaincodelabs/libmultiprocess#111: doc: Add internal design section
chaincodelabs/libmultiprocess#113 Add missing include to util.h
This update brings in the following changes:

chaincodelabs/libmultiprocess#116  shutdown bugfix: destroy RPC system before running cleanup callbacks
@Sjors Sjors force-pushed the 2024/10/multiprocess-non-depends branch 2 times, most recently from 891008e to db3c723 Compare October 7, 2024 09:00
@Sjors
Copy link
Owner Author

Sjors commented Oct 7, 2024

See chaincodelabs/libmultiprocess#117 and chaincodelabs/libmultiprocess#68 for most of the failures.

Cap’n Proto can be installed through distros, but multiprocess can only be built from source. Currently this PR just clones the repo and installs it, which is what a non-depends user would do. But we should probably have a better alternative.

Perhaps a good approach is to have a convenient way to install multiprocess using depends.

Or to just build it, with every other package disabled, and point our regular build system towards it. I haven't tried the latter yet.

@ryanofsky
Copy link

I responded to chaincodelabs/libmultiprocess#117 but hopefully that should be easy to fix with a small tweak to this PR.

I think fixing chaincodelabs/libmultiprocess#68 will probably be easiest with a change to libmultiprocess, but I'll have to refresh my memory of that issue and see what that might look ilke.

On idea of using depends system to install multiprocess library in a way where it could be used by a non-depends build or a not 100% depends build, that seems a like it could be tricky because I think depends system isn't currently set to set up to mix inside and outside dependencies, so it might need new configuration options to do that. Also the way the depends build currently passes package paths to the bitcoin build is through a toolchain.cmake file, but I'm not sure if requiring the depends toolchain for non-depends build would be too limiting. If so the bitcoin build might need to be changed to find depends packages a different way.

I think maybe a better long term approach might be to drop libmultiprocess package from depends, and make libmultiproces into a git subtree like leveldb and secp256k1, building it basically as part of bitcoin. This idea was suggested by achow a long time ago, before we were using cmake, and now that we are using cmake it seems worth pursuing. But I don't have a clear idea of how it should be implemented or what cmake wizardry is required to stitch together separate cmake projects into the same build without installing them. And I also haven't used git subtree before.

An intermediate approach might just be to make building and installing libmultiprocess from source easier. For example, maybe I could just add a simple top level Makefile that calls cmake --build cmake --install so building and installing from source could just be done with one command make install instead of multiple mkdir, cd, cmake, make commands.

@Sjors
Copy link
Owner Author

Sjors commented Oct 7, 2024

I think depends system isn't currently set to set up to mix inside and outside dependencies

I vaguely remember someone tried to make that work in the past.

@fanquake
Copy link

fanquake commented Oct 7, 2024

I vaguely remember someone tried to make that work in the past

It somewhat used to be a thing, but was removed (and won't be re-added), because it defeats the point of depends being a self contained dependency builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants