multiprocess: Add bitcoin wrapper executable #31375

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/wrap changing 11 files +272 −36
  1. ryanofsky commented at 5:57 pm on November 26, 2024: contributor

    Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don’t cause confusion.

    Idea and implementation of this were discussed in #30983


    This PR is part of the process separation project.

  2. multiprocess: Add bitcoin wrapper executable
    Intended to make bitcoin command line features more discoverable and allow
    installing new multiprocess binaries in libexec/ instead of bin/ so they don't
    cause confusion.
    
    Idea and implementation of this were discussed in
    https://github.com/bitcoin/bitcoin/issues/30983
    
    It's possible to debug this and see what programs it is trying to call by running:
    
    strace -e trace=execve -s 10000 build/src/bitcoin ...
    b06c7ad0ae
  3. test: Support BITCOIN_CMD environment variable
    Support new BITCOIN_CMD environment variable in functional test to be able to
    test the new bitcoin wrapper executable and run other commands through it
    instead of calling them directly.
    c5dad301d0
  4. ci: Run multiprocess tests through wrapper executable da108a6e5b
  5. DrahtBot commented at 5:57 pm on November 26, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31375.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK Sjors

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30975 (Add multiprocess binaries to release build by Sjors)
    • #30125 (test: improve BDB parser (handle internal/overflow pages, support all page sizes) by theStack)
    • #29838 (Feature: Use different datadirs for different signets by BrandonOdiwuor)
    • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  6. DrahtBot added the label CI failed on Nov 26, 2024
  7. DrahtBot commented at 6:23 pm on November 26, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33557375133

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. ryanofsky force-pushed on Nov 27, 2024
  9. ryanofsky commented at 5:15 pm on November 27, 2024: contributor
    Updated 19ae652376faca65d972c12cb51cfc8af0560c9e -> 63df9f3deb35be79496f7c240e3303e1d96c6832 (pr/wrap.3 -> pr/wrap.4, compare) with fixes for windows and fuzz CI errors, and lint and tidy fixes Updated 63df9f3deb35be79496f7c240e3303e1d96c6832 -> da108a6e5be220654a65b6613ee7eb2c4ddc8677 (pr/wrap.4 -> pr/wrap.5, compare) fixing windows include error, fs lint error, and previous releases test bug
  10. ryanofsky force-pushed on Nov 27, 2024
  11. DrahtBot removed the label CI failed on Nov 27, 2024
  12. in src/bitcoin.cpp:188 in b06c7ad0ae outdated
    183+    // Otherwise if exe is installed in a bin/ directory, first look for target
    184+    // executable in libexec/
    185+    (exe_dir.filename() == "bin" && try_exec(fs::path{exe_dir.parent_path()} / "libexec" / fs::PathFromString(args[0]).filename())) ||
    186+    // Otherwise look for target executable next to current exe
    187+    try_exec(exe_dir / fs::PathFromString(args[0]).filename()) ||
    188+    // Otherwise just look on the system path.
    


    Sjors commented at 9:29 am on November 28, 2024:
    b06c7ad0ae91102fe8cdcac6f86d627aace2219b: this is potentially confusing. If I build without gui, I expect build/src/bitcoin gui to fail. Right now it would launch any bitcoin-qt in my $PATH.
  13. Sjors commented at 9:40 am on November 28, 2024: member

    Concept ACK

    I think it would be more clear to move build/src/bitcoin-{node,gui} to build/src/libexec, rather than use a different file organization for CMake builds than for installs.

    The “Win64 native, VS 2022” job still seems unhappy.

  14. hebasto commented at 10:49 am on November 28, 2024: member

    The “Win64 native, VS 2022” job still seems unhappy.

    https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-170#posix-function-names:

    To turn off deprecation warnings for these functions, define the preprocessor macro _CRT_NONSTDC_NO_WARNINGS. You can define this macro at the command line by including the option /D_CRT_NONSTDC_NO_WARNINGS.

    We have already used this macro:https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/cmake/leveldb.cmake#L87-L89


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me