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

Closed
wants to merge 4 commits into from
Closed

Conversation

Sjors
Copy link
Owner

@Sjors Sjors commented Oct 7, 2024

No description provided.

@Sjors Sjors force-pushed the 2024/10/multiprocess-non-depends branch 3 times, most recently from db3c723 to 0c75631 Compare October 7, 2024 09:14
@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.

Except for Windows.

Co-Authored-By: fanquake <[email protected]>
@Sjors Sjors force-pushed the 2024/10/multiprocess-non-depends branch 2 times, most recently from e97d499 to e04c4b6 Compare November 10, 2024 19:43
@Sjors
Copy link
Owner Author

Sjors commented Nov 10, 2024

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

I still need to try this.

I think fixing chaincodelabs/libmultiprocess#68 will probably be easiest with a change to libmultiprocess

I rebased bitcoin#30975 past the fix for this.

@Sjors Sjors force-pushed the 2024/10/multiprocess-non-depends branch 2 times, most recently from 40519eb to 86fd7c7 Compare November 10, 2024 19:53
@Sjors Sjors force-pushed the 2024/10/multiprocess-non-depends branch from 86fd7c7 to 516f132 Compare November 10, 2024 20:07
@Sjors
Copy link
Owner Author

Sjors commented Nov 10, 2024

Fixed macOS build.

CentOS doesn't have a capnproto package, so for now I'm running it without multiprocess.

Some CI machines need sudo for the cmake --install . step, others don't and they don't have the sudo command. Maybe there's a nicer approach than my workaround for this.

Interesting build failure for fuzzer: https://cirrus-ci.com/task/5727621063901184

21:11:10.356] /usr/bin/ld: /usr/lib/llvm-19/lib/clang/19/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o): in function main': [21:11:10.356] (.text.main+0x16): undefined reference to LLVMFuzzerTestOneInput'

@Sjors
Copy link
Owner Author

Sjors commented Nov 10, 2024

Some undefined behaviour: https://github.com/Sjors/bitcoin/actions/runs/11768122372/job/32777598746?pr=65

@Sjors Sjors closed this Nov 14, 2024
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