Reintroduce external signer support for Windows

hebasto commented at 6:06 pm on April 14, 2024: member

Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.

Partially reverts:

After this PR, we can proceed to actually remove the unused code from src/util/subprocess.hpp.

DrahtBot commented at 6:06 pm on April 14, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack

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:

#30952 (test: Use shell builtins in run_command test case by achow101)
#30315 (Stratum v2 Transport by Sjors)
#29346 (Stratum v2 Noise Protocol by Sjors)

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 ci/test/00_setup_env_win64.sh:19 in cf6a4b2c0a outdated

15@@ -16,4 +16,4 @@ export GOAL="deploy"
16 # Prior to 11.0.0, the mingw-w64 headers were missing noreturn attributes, causing warnings when
17 # cross-compiling for Windows. https://sourceforge.net/p/mingw-w64/bugs/306/
18 # https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe
19-export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests CXXFLAGS=-Wno-return-type"
20+export BITCOIN_CONFIG="--enable-reduce-exports --enable-external-signer --disable-gui-tests CXXFLAGS=-Wno-return-type"

fanquake commented at 6:15 pm on April 14, 2024:

ci: remove –enable-external-signer from win64 job

Why this is being reverted? This is already applied globally to the CI (and I’d say pointlessly so).

hebasto commented at 6:17 pm on April 14, 2024:

Thanks! Dropped.

fanquake commented at 6:19 pm on April 14, 2024:

Great, can you remove it from the global config here as well. Given it’s not required or actually testing anything.

hebasto commented at 7:54 pm on April 14, 2024:

Great, can you remove it from the global config here as well. Given it’s not required or actually testing anything.

I agree. Implemented.

hebasto force-pushed on Apr 14, 2024

fanquake commented at 10:42 am on April 17, 2024: member

There are no conflicts with #29865.

Looks like there are now?

hebasto commented at 3:30 pm on April 18, 2024: member

There are no conflicts with #29865.

Looks like there are now?

Well, there is a conflict in the code, which is being removed in #29865. It won’t be a problem for either PR if the other one goes first.

hebasto force-pushed on Apr 23, 2024

hebasto commented at 5:35 pm on April 23, 2024: member

Rebased.

hebasto force-pushed on Apr 23, 2024

DrahtBot added the label CI failed on Apr 23, 2024

DrahtBot commented at 7:48 pm on April 23, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/24165845040

DrahtBot removed the label CI failed on Apr 23, 2024

laanwj added the label Windows on Apr 24, 2024

laanwj added the label Wallet on Apr 24, 2024

hebasto force-pushed on Apr 24, 2024

hebasto commented at 4:34 pm on April 24, 2024: member

Rebased on top of the merged #29910.

hebasto added the label Needs CMake port on Apr 24, 2024

in src/util/subprocess.h:1015 in 0202082747 outdated

1007@@ -1012,7 +1008,7 @@ class Popen
1008 /*
1009   ~Popen()
1010   {
1011-#ifdef __USING_WINDOWS__

Sjors commented at 8:47 am on April 26, 2024:

#29961 removes this, that’s the only conflict I found.

Sjors commented at 9:50 am on April 26, 2024: member

Tested the Guix build on Windows 11.

Code changes look reasonable, but Windows stuff is not my expertise…

DrahtBot added the label Needs rebase on Apr 28, 2024

hebasto force-pushed on Apr 28, 2024

hebasto commented at 5:30 am on April 28, 2024: member

Rebased to resolve conflicts with the merged bitcoin/bitcoin#29774.

hebasto commented at 5:33 am on April 28, 2024: member

cc @achow101 @theStack

Code changes look reasonable, but Windows stuff is not my expertise…

cc @sipsorcery :)

DrahtBot removed the label Needs rebase on Apr 28, 2024

theStack commented at 5:05 pm on April 30, 2024: contributor

Concept ACK

in src/test/system_tests.cpp:34 in db73ac8c51 outdated

28@@ -29,25 +29,44 @@ BOOST_AUTO_TEST_CASE(dummy)
29 
30 BOOST_AUTO_TEST_CASE(run_command)
31 {
32+#ifdef WIN32
33+    // https://www.winehq.org/pipermail/wine-devel/2008-September/069387.html
34+    auto hntdll = GetModuleHandleA("ntdll.dll");
35+    assert(hntdll);
36+    const bool wine_runtime = GetProcAddress(hntdll, "wine_get_version");

sipsorcery commented at 6:55 pm on May 2, 2024:

This check will fail on Windows. I’m guessing that’s expected but just checking?

hebasto commented at 7:07 pm on May 2, 2024:

The goal is to detect that binaries, which are cross-compiled for Windows, are running in Wine.

sipsorcery commented at 7:31 pm on May 2, 2024:

Does that mean this PR is intended to be a fix for Windows or for wine?

hebasto commented at 8:30 pm on May 2, 2024:

For both. Wine is used to run test suite on Linux after cross-compiling for Windows. This need roots from the fact that Wine and Windows runtimes produce different error messages: https://github.com/bitcoin/bitcoin/blob/db73ac8c517f91ed5175b3635eb37ed56852ff91/src/test/system_tests.cpp#L80

hebasto commented at 8:32 pm on May 2, 2024:

FWIW, the same behaviour was before #29489.

hebasto force-pushed on May 2, 2024

hebasto commented at 8:46 pm on May 2, 2024: member

Rebased on top of the merged #29961.

Sjors commented at 11:50 am on May 6, 2024: member

Briefly retested 83c6332dc7b8145b19d725830b26dc4ef21e4b61 which still works.

DrahtBot added the label Needs rebase on May 7, 2024

hebasto force-pushed on May 7, 2024

hebasto commented at 8:07 pm on May 7, 2024: member

Rebased due to the conflict with merged bitcoin/bitcoin#29494.

DrahtBot removed the label Needs rebase on May 7, 2024

DrahtBot added the label CI failed on May 7, 2024

DrahtBot commented at 9:29 pm on May 7, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/24699627882

fanquake commented at 3:21 am on May 8, 2024: member

https://github.com/bitcoin/bitcoin/pull/29868/checks?check_run_id=24699627882:

0In file included from common/run_command.cpp:13:
1./util/subprocess.h: In member function ‘int subprocess::Popen::wait()’:
2./util/subprocess.h:1062:7: error: unused variable ‘ret’ [-Werror=unused-variable]
3 1062 |   int ret = WaitForSingleObject(process_handle_, INFINITE);
4      |       ^~~

hebasto force-pushed on May 8, 2024

DrahtBot removed the label CI failed on May 8, 2024

Sjors commented at 7:07 am on May 9, 2024: member

I assume you added b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?

Is there something you can remove from cpp-subprocess based on your comment? #29961 (comment)

fanquake commented at 8:18 am on May 9, 2024: member

I assume you added https://github.com/bitcoin/bitcoin/commit/b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?

It’s there because setting something that is already on, to on, is pointless, and doesn’t test anything. In either case, external signer is also not special, so it’s not clear why, as a feature, it’s being explcitly enabled in the global CI config.

hebasto commented at 10:20 am on May 9, 2024: member

@Sjors

Is there something you can remove from cpp-subprocess based on your comment? #29961 (comment)

Popen::poll() and Popen::pid() might go.

hebasto commented at 10:28 am on May 9, 2024: member

In either case, external signer is also not special…

At some point, we might want to get rid of the #ifdef ENABLE_EXTERNAL_SIGNER all together.

fanquake referenced this in commit b47c393d8a on May 11, 2024

hebasto force-pushed on May 11, 2024

hebasto commented at 10:54 am on May 11, 2024: member

Rebased due to the conflict with merged bitcoin/bitcoin#30081.

DrahtBot added the label Needs rebase on May 30, 2024

hebasto force-pushed on May 30, 2024

hebasto commented at 8:57 am on May 30, 2024: member

Rebased to resolve a conflict with bitcoin/bitcoin#30049.

DrahtBot removed the label Needs rebase on May 30, 2024

DrahtBot added the label CI failed on May 30, 2024

DrahtBot removed the label CI failed on May 30, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

maflcko removed the label Needs CMake port on Aug 29, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot added the label CI failed on Aug 30, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot removed the label Needs rebase on Aug 30, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot removed the label CI failed on Aug 31, 2024

in src/test/system_tests.cpp:78 in dc08930ec6 outdated

72@@ -54,8 +73,13 @@ BOOST_AUTO_TEST_CASE(run_command)
73     }
74     {
75         // Return non-zero exit code, with error message for stderr
76+#ifdef WIN32
77+        const std::string command{"cmd.exe /c dir nosuchfile"};
78+        const std::string expected{wine_runtime ? "File not found." : "File Not Found"};

maflcko commented at 7:37 am on September 2, 2024:

Not sure. Won’t this lead to more issue of the kind of #30608? If yes, it would be good to fix this first, and then extend the tests.

Why not simply call python with python -c 'import sys; print("err", file=sys.stderr); sys.exit(2)'?

maflcko commented at 6:50 pm on September 3, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/30788

Sjors commented at 11:47 am on September 2, 2024: member

Tested dc08930ec68b465420cc5e1157e532fcb5cf8c87 again on Windows.

x86_64 guix hashes

 001d11755aaf73fd378b563afa5f54286fbe9c8e2a3b6259a61d21b7cbb5bf9f4  guix-build-dc08930ec68b/output/aarch64-linux-gnu/SHA256SUMS.part
 12baf27e4f865ed23b97d7f353bbcf6f8fd5629c8d7cec26ee02b048406d3418b  guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu-debug.tar.gz
 2a686ed4a50b371a3ef2d20791e4fdf1f1258404f1012efc132a906808835628b  guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu.tar.gz
 3dbea3292cca17491164781477fdc6c34e189456156aecc58cfe07763989a09b9  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/SHA256SUMS.part
 485f6d8f7377f1210c460b49766fe29075dbd54c4ab0a23c93d7d81605e82cf58  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/bitcoin-dc08930ec68b-arm-linux-gnueabihf-debug.tar.gz
 538730c75787366e4b9719123bcac53db01cce8c22701bc994a8e34b5d8c17415  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/bitcoin-dc08930ec68b-arm-linux-gnueabihf.tar.gz
 66ce75b2084be1e1afcddc3353ba67b5f848bbeb24639beba3f5e82ba4d79679f  guix-build-dc08930ec68b/output/arm64-apple-darwin/SHA256SUMS.part
 7bfb4d7c4d12a8bd24c1d3d0806c3ef41693211efa991138f92cc47e6b81f0b85  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin-unsigned.tar.gz
 8673bfb079316b5f793401f10038739fe7947f33c41013ebc1cfbea53ee6f6949  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin-unsigned.zip
 94f5884559b3f880e7152b6b320b8f400079ee9ec413caf25ddc682c4a489e691  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin.tar.gz
107e60b134b8b59271cc4df1b31506f73ee02bf19e28207e40989190f18e4bb68d  guix-build-dc08930ec68b/output/dist-archive/bitcoin-dc08930ec68b.tar.gz
11f94e934d10242c09488235fdfd3ce14b4ed9fa3148c75e33fa1e4cdf59db6cd1  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/SHA256SUMS.part
1280562f673316e701ed3b601fc0fb8f9af39eb1e7d17edbdc436db6e108bc8031  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/bitcoin-dc08930ec68b-powerpc64-linux-gnu-debug.tar.gz
13967d0b7d316d19a3bc7496aaa0162c65c235bdd14b490786f45026b8ad08cb4d  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/bitcoin-dc08930ec68b-powerpc64-linux-gnu.tar.gz
144bc31242d17bc8653ab647699f101b3073c1e364ce2d2a8685694fa4df39948f  guix-build-dc08930ec68b/output/riscv64-linux-gnu/SHA256SUMS.part
158fe6e6def3df24dcd20b6bd52857fbb28c84753a3db2816522521ab7787c97b7  guix-build-dc08930ec68b/output/riscv64-linux-gnu/bitcoin-dc08930ec68b-riscv64-linux-gnu-debug.tar.gz
16cea272797e4f1528903efe6b56d2fc77cf3564e67133a9bc674169f0cac13d98  guix-build-dc08930ec68b/output/riscv64-linux-gnu/bitcoin-dc08930ec68b-riscv64-linux-gnu.tar.gz
17312ac2af7056f6b807d5143be9f4814bbbe2f947e2f09cd41b066ebf74879861  guix-build-dc08930ec68b/output/x86_64-apple-darwin/SHA256SUMS.part
187d71cd457dc5d85ec9c38fe9779535c5a70fb9a3420a06719be16b3d265174f9  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin-unsigned.tar.gz
19e055caed7299e4dbc9dc7e03f19d4a3e92224099223760e49e66b9c9186953fb  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin-unsigned.zip
207b776bef6e3e99bf124b03c074649f9a82881afd84db2c3baed063996e57149b  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin.tar.gz
217a138f4fa30b3654a21ecc4db58788eeaf91d053c3bb5b8e991c0cc40578212d  guix-build-dc08930ec68b/output/x86_64-linux-gnu/SHA256SUMS.part
2235435eeddcd6b5185b6f8a5cf665ca143c9b0e1b38d7a30cbb6fe4e04d92e861  guix-build-dc08930ec68b/output/x86_64-linux-gnu/bitcoin-dc08930ec68b-x86_64-linux-gnu-debug.tar.gz
235ded07c774f55349ea892ed030acbe11bd5823b2956bf08f7aac43abd66b39f2  guix-build-dc08930ec68b/output/x86_64-linux-gnu/bitcoin-dc08930ec68b-x86_64-linux-gnu.tar.gz
240c4dd1a1c1cbe8def8cb13240b0456d4d094c9639d6533de530d606b4f3b2586  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/SHA256SUMS.part
2559c27d3edc82b84cff8af895ebf80b250a9e3b0c2ef08b5a1cb1720fbe913113  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-debug.zip
26f533c0800645b8704c528bd3f13d65558964aeff820dc8ff21360b1847dc774b  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-setup-unsigned.exe
27030082937bb216365d5bfea074ae8ba3290268d122718cc04f675bc6c0881f7d  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-unsigned.tar.gz
288d6f24a1aaf2079a3c2e5562678bfd8513d741b6b27ef5432b7b78b4325def91  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64.zip

subprocess: Fix cross-compiling for Windows 2f939519f1

subprocess: Make `Popen::wait()` return process exit code on Windows

This behavior is expected and consistent with the non-Windows
implementation.

The code is borrowed from `Popen::poll()`.

47d49ea90f

subprocess: Fix quote issue on Windows 7d0fcb2fa6

subprocess, refactor: Replace custom `__USING_WINDOWS__` with `WIN32`

The `WIN32` macro is used across the entire code base to designate
building for Windows.

847d10e38a

hebasto force-pushed on Sep 4, 2024

hebasto marked this as a draft on Sep 4, 2024

hebasto force-pushed on Sep 4, 2024

DrahtBot added the label CI failed on Sep 4, 2024

hebasto marked this as ready for review on Sep 4, 2024

in src/test/system_tests.cpp:43 in 53d426e563 outdated

38         const UniValue result = RunCommandParseJSON("");
39         BOOST_CHECK(result.isNull());
40     }
41     {
42+#ifdef WIN32
43+        const UniValue result = RunCommandParseJSON("cmd.exe /c echo {\"success\": true}");

luke-jr commented at 11:40 pm on September 4, 2024:

Why is an extra cmd.exe needed?

hebasto commented at 6:44 pm on September 5, 2024:

It is not “extra”, but required. Consider this output:

0C:\Users\hebasto>where echo
1INFO: Could not find files for the given pattern(s).
2
3C:\Users\hebasto>where cmd
4C:\Windows\System32\cmd.exe

in src/test/system_tests.cpp:83 in 53d426e563 outdated

78+        std::string expected;
79+        if (wine_runtime) {
80+            command = "cmd.exe /c dir nosuchfile";
81+            expected = "File not found.";
82+        } else {
83+            command = "py -3 -c 'import sys; print(\"err\", file=sys.stderr); sys.exit(2)'";

luke-jr commented at 11:41 pm on September 4, 2024:

Seems like echo err 1>&2 should be portable?

luke-jr changes_requested

DrahtBot removed the label CI failed on Sep 5, 2024

in src/test/system_tests.cpp:81 in 53d426e563 outdated

72@@ -54,8 +73,20 @@ BOOST_AUTO_TEST_CASE(run_command)
73     }
74     {
75         // Return non-zero exit code, with error message for stderr
76+#ifdef WIN32
77+        std::string command;
78+        std::string expected;
79+        if (wine_runtime) {
80+            command = "cmd.exe /c dir nosuchfile";
81+            expected = "File not found.";

maflcko commented at 4:56 am on September 5, 2024:

Please don’t re-introduce https://github.com/bitcoin/bitcoin/issues/30608

hebasto marked this as a draft on Sep 5, 2024

hebasto force-pushed on Sep 5, 2024

test: Reintroduce Windows support in `system_tests/run_command` test b083581226

build: Re-enable external signer support for Windows c00c849c64

ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option

This option is enabled by default.

5541ef02f7

hebasto force-pushed on Sep 5, 2024

DrahtBot commented at 1:20 am on September 6, 2024: contributor

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

Make sure to run all tests locally, according to the documentation.

The failure may 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.

DrahtBot added the label CI failed on Sep 6, 2024

in src/test/system_tests.cpp:85 in 5541ef02f7

81+            command = "cmd.exe /c \"echo err 1>&2 && exit 1\"";
82+            expected = "err";
83+        } else {
84+            command = "cmd.exe /c \"echo err 1>&2 && exit 1\"";
85+            expected = "err";
86+        }

luke-jr commented at 4:50 pm on September 6, 2024:

These are identical now. wine_runtime seems no longer needed at all?

DrahtBot added the label Needs rebase on Sep 24, 2024

DrahtBot commented at 5:17 pm on September 24, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Reintroduce external signer support for Windows #29868

Code Coverage

Reviews

Conflicts