test: Remove `system_tests/run_command` runtime dependencies

hebasto commented at 11:03 pm on November 23, 2025: member

system_tests currently rely on cat, echo, false and sh being available in PATH at runtime.

This PR:

Removes these dependencies.
Reduces the number of platform-specific code paths.

The change is primarily motivated by my work on maintaining the bitcoin-core package in Guix. It enables the removal of the existing bash and coreutils native inputs, which in turn makes it possible to drop the implicit dependency on qtbase@5 (see https://codeberg.org/guix/guix/pulls/4386#issuecomment-8613333).

hebasto added the label Tests on Nov 23, 2025

DrahtBot commented at 11:03 pm on November 23, 2025: 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/33929.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, janb84
Concept ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34562 (ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup by furszy)
#34349 (util: Remove brittle and confusing sp::Popen(std::string) by maflcko)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)

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.

in src/test/system_tests.cpp:26 in c996515a6d

21 
22 #ifdef ENABLE_EXTERNAL_SIGNER
23 
24 BOOST_AUTO_TEST_CASE(run_command)
25 {
26+    const std::string test_executable = std::string(boost::unit_test::framework::master_test_suite().argv[0]).append(" --log_level=nothing --report_level=no --run_test=mock_process/");

maflcko commented at 8:45 pm on December 4, 2025:

nit: This will fail with spaces in the dir:

0$ ./a\ space/test_bitcoin -t system_tests
1Running 1 test case...
2unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
3test/system_tests.cpp(29): last checkpoint
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

sedited commented at 8:39 pm on January 19, 2026:

Looks like this is broken by subprocess’s use of split here: https://github.com/bitcoin/bitcoin/blob/master/src/util/subprocess.h#L941 . This makes me wonder if we should get rid of the single command constructor. Seems a bit like a footgun?

maflcko commented at 7:19 am on January 20, 2026:

Fixed in #34349, due to lack of progress here.

hebasto commented at 7:10 pm on January 21, 2026:

Thanks! Rebased on top of #34349.

maflcko approved

maflcko commented at 8:45 pm on December 4, 2025: member

would be nice to fix the space error, but not sure how easy that is.

review ACK c996515a6dcb158d2d3aa88efc499ffff49bea79 🚆

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK c996515a6dcb158d2d3aa88efc499ffff49bea79 🚆
3tIPkSDEjaEB97BQIv8sM9m414fm5sxxOHHqLWNWGBf6fsHuOwabixwP9VIGmJHyf+ljEBjgD2gIKL2G+psZMAA==

bensig commented at 9:05 pm on January 5, 2026: contributor

Concept ACK c996515

Nice approach using the test binary itself as a mock process.

nit: This will fail if the path contains spaces:

0$ "./a space/test_bitcoin" --run_test=system_tests
1  unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)

The executable path in test_executable probably needs quoting.

maflcko commented at 7:25 am on January 6, 2026: member

nit: This will fail if the path contains spaces: @bensig I’ve seen you minimally reword a prior review (here and in other pull requests), which makes you look like an LLM bot. Review is needed and welcome, but it does not help to simply repeat prior comments, such as the motivation from the pull request description, or from other reviews. It would be better to only mention new findings or new points of view.

bensig commented at 6:41 pm on January 6, 2026: contributor

nit: This will fail if the path contains spaces:

@bensig I’ve seen you minimally reword a prior review (here and in other pull requests), which makes you look like an LLM bot. Review is needed and welcome, but it does not help to simply repeat prior comments, such as the motivation from the pull request description, or from other reviews. It would be better to only mention new findings or new points of view.

You got it… I am still learning the ropes here. I did 21+ reviews this week so far… I was simply reiterating something that I verified.. I will minimize my ACKs to only include any verifications that I have done or new information. Thanks for the feedback.

l0rinc commented at 7:41 pm on January 6, 2026: contributor

We’re usually more interested to know that you have verified the change yourself (e.g. recreated the change locally, ended up with same changes; ran all tests/benchmarks locally; searches for similar PRs or occurrences where the change applies, ran the fuzzer for 36 hours; did an IBD against this change; tested it on a big-endian/32-bit/raspberry-pi/windows, etc), i.e. that there’s PoW behind the ack.

hebasto force-pushed on Jan 21, 2026

hebasto commented at 7:09 pm on January 21, 2026: member

Rebased on top of #34349, which resolves #33929 (review).

hebasto marked this as a draft on Jan 21, 2026

hebasto force-pushed on Feb 18, 2026

fanquake referenced this in commit 2706758dc3 on Feb 18, 2026

test: Remove `system_tests/run_command` runtime dependencies a4324ce095

test: Enable `system_tests/run_command` "stdin" test on Windows 97e7e79435

hebasto force-pushed on Feb 18, 2026

hebasto marked this as ready for review on Feb 18, 2026

hebasto commented at 11:08 am on February 18, 2026: member

Rebased on top of merged bitcoin/bitcoin#34349 and undrafted.

maflcko commented at 11:27 am on February 18, 2026: member

I haven’t tested if #33929 (review) is fixed now. Otherwise, lgtm

re-ACK 97e7e79435c69e90cb7f056c704c275421bf0892 👓

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 97e7e79435c69e90cb7f056c704c275421bf0892 👓
3zP5xFqjUJj1nBLHEbiTcRSAqNBtcB/4ZgSd4lH5VNc5AF5ZBN61d44uXmQgt5V0G+VQ8v1GQWpbB5jPE75wmDA==

in src/test/system_tests.cpp:17 in 97e7e79435

13@@ -14,24 +14,28 @@
14 #include <util/subprocess.h>
15 #endif // ENABLE_EXTERNAL_SIGNER
16 
17+#include <boost/cstdlib.hpp>

fanquake commented at 11:28 am on February 18, 2026:

Rather than adding new Boost headers, can’t we just define and document the equivalent of boost::exit_test_failure (201) ourselves, and use it?

hebasto commented at 12:00 pm on February 18, 2026:

Rather than adding new Boost headers…

What are the drawbacks of including an extremely lightweight header?

… can’t we just define and document the equivalent of boost::exit_test_failure (201) ourselves, and use it?

This will bind us to particular implementation details, which is best avoided.

janb84 commented at 3:22 pm on February 19, 2026:

I agree with @fanquake why add a new Boost header for 2x the value 201.

hat are the drawbacks of including an extremely lightweight header?

Maybe I’m mistaken but my belief is that the project intends to remove the dependency on Boost. Seems illogical to add new dependencies.

This will bind us to particular implementation details, which is best avoided.

This value has not changed for 25 years and, given the motivation in the header, it will not change. So you are correct in general, in this specific case I would argue against it.

maflcko commented at 3:32 pm on February 19, 2026:

Maybe I’m mistaken but my belief is that the project intends to remove the dependency on Boost. Seems illogical to add new dependencies.

Not sure how this can be done easily. What alternative testing framework do you suggest? In any case, removing this single header should be the most trivial part.

Personally I like this pull request as-is, but I don’t think it matters enough one way or the other to discuss at length.

hebasto commented at 3:41 pm on February 19, 2026:

I agree with @fanquake why add a new Boost header for 2x the value 201.

hat are the drawbacks of including an extremely lightweight header?

Maybe I’m mistaken but my belief is that the project intends to remove the dependency on Boost.

Given that the header-only Boost.Test framework will be maintained, I strongly doubt that we will stop using it in the foreseeable future.

Seems illogical to add new dependencies.

This dependency is already used implicitly.

This will bind us to particular implementation details, which is best avoided.

This value has not changed for 25 years and, given the motivation in the header, it will not change. So you are correct in general, in this specific case I would argue against it.

The past is no guarantee of the future :)

Plus, wouldn’t that it be a layer violation?

janb84 commented at 5:21 pm on February 19, 2026:

Not sure how this can be done easily. What alternative testing framework do you suggest? In any case, removing this single header should be the most trivial part.

Given that the header-only Boost.Test framework will be maintained, I strongly doubt that we will stop using it in the foreseeable future.

Was just stating, what I understood, what the direct is where the project is going. But can also see why that does not hold for the unit test part of Boost.

Personally I like this pull request as-is, but I don’t think it matters enough one way or the other to discuss at length.

Agree on that part. /fin

hebasto commented at 5:27 pm on February 19, 2026:

Was just stating, what I understood, what the direct is where the project is going.

Also #34586 (comment).

maflcko commented at 5:54 pm on February 19, 2026:

In theory, there could be a discussion about replacing boost test, but it should state the alternative and the benefits/trade-offs of the alternative. Though, that seems best done in a dedicated tracking/meta issue.

hebasto commented at 12:07 pm on February 18, 2026: member

I haven’t tested if #33929 (comment) is fixed now.

FWIW, on my machine:

0$ ./build\ with\ spaces/bin/test_bitcoin -t system_tests
1Running 1 test case...
2
3*** No errors detected

janb84 commented at 3:13 pm on February 20, 2026: contributor

ACK 97e7e79435c69e90cb7f056c704c275421bf0892

Code review looks OK. Test still works on Linux (aarch64) and also tested it on windows (MSVC build):

 0149/152 Test   [#8](/bitcoin-bitcoin/8/): allocator_tests ......................   Passed    0.14 sec
 1150/152 Test  [#90](/bitcoin-bitcoin/90/): script_assets_tests ..................***Skipped   0.15 sec
 2151/152 Test   [#3](/bitcoin-bitcoin/3/): secp256k1_noverify_tests .............   Passed   50.15 sec
 3152/152 Test   [#4](/bitcoin-bitcoin/4/): secp256k1_tests ......................   Passed   78.29 sec
 4
 5100% tests passed, 0 tests failed out of 152
 6
 7Total Test time (real) =  78.54 sec
 8
 9The following tests did not run:
10         90 - script_assets_tests (Skipped)

Or with spaces:

0PS C:\Users\user\source\repos\bitcoin\build\bin\Re lea se> .\test_bitcoin.exe -t system_tests
1Running 1 test case...
2
3*** No errors detected

test: Remove system_tests/run_command runtime dependencies #33929

Code Coverage & Benchmarks

Reviews

Conflicts

test: Remove `system_tests/run_command` runtime dependencies #33929