Reintroduce external signer support for Windows #29868

hebasto · 2024-04-14T18:06:35Z

Based on upstream arun11299/cpp-subprocess#99.

Partially reverts:

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

DrahtBot · 2024-04-14T18:06:38Z

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:

#30049 (build, test, doc: Temporarily remove Android-related stuff by hebasto)

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.

ci/test/00_setup_env_win64.sh

fanquake · 2024-04-17T10:42:24Z

There are no conflicts with #29865.

Looks like there are now?

hebasto · 2024-04-18T15:30:49Z

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 · 2024-04-23T17:35:34Z

Rebased.

DrahtBot · 2024-04-23T19:48:59Z

🚧 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}

hebasto · 2024-04-24T16:34:21Z

Rebased on top of the merged #29910.

Sjors · 2024-04-26T08:47:55Z

src/util/subprocess.h

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


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

Sjors · 2024-04-26T09:50:23Z

Tested the Guix build on Windows 11.

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

hebasto · 2024-04-28T05:30:11Z

Rebased to resolve conflicts with the merged #29774.

hebasto · 2024-04-28T05:33:11Z

cc @achow101 @theStack

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

cc @sipsorcery :)

theStack · 2024-04-30T17:05:48Z

Concept ACK

sipsorcery · 2024-05-02T18:55:12Z

src/test/system_tests.cpp

+    // https://www.winehq.org/pipermail/wine-devel/2008-September/069387.html
+    auto hntdll = GetModuleHandleA("ntdll.dll");
+    assert(hntdll);
+    const bool wine_runtime = GetProcAddress(hntdll, "wine_get_version");


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

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

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

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:

bitcoin/src/test/system_tests.cpp

Line 80 in db73ac8

const std::string expected{wine_runtime ? "File not found." : "File Not Found"};

FWIW, the same behaviour was before #29489.

hebasto · 2024-05-02T20:46:18Z

Rebased on top of the merged #29961.

Sjors · 2024-05-06T11:50:32Z

Briefly retested 83c6332 which still works.

hebasto · 2024-05-07T20:07:30Z

Rebased due to the conflict with merged #29494.

DrahtBot · 2024-05-07T21:29:55Z

🚧 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 · 2024-05-08T03:21:18Z

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

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

Sjors · 2024-05-09T07:07:54Z

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

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

fanquake · 2024-05-09T08:18:40Z

I assume you added b69b9e8 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 · 2024-05-09T10:20:59Z

@Sjors

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

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

hebasto · 2024-05-09T10:28:06Z

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.

5a11d30 refactor, subprocess: Remove unused stream API calls (Hennadii Stepanov) 05b6f87 refactor, subprocess: Remove unused `Popen::child_created_` data member (Hennadii Stepanov) 9e1ccf5 refactor, subprocess: Remove unused `Popen::poll()` (Hennadii Stepanov) 24b53fc refactor, subprocess: Remove `Popen::pid()` (Hennadii Stepanov) Pull request description: This PR continues #29961. Please note that `Popen::poll()` is not required for #29868 anymore. ACKs for top commit: theuni: Easy code review ACK 5a11d30 since it's all removals :) theStack: Code-review ACK 5a11d30 Tree-SHA512: 11f984f8384c337782d058afa80011e88087a1b5a3ed6e4678d492e6c232b706a26312463c5dd8c529aa477497c8afca9f54574e0e36e3edd5675bd5d8424bbb

This behavior is expected and consistent with the non-Windows implementation. The code is borrowed from `Popen::poll()`.

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

This option is enabled by default.

hebasto · 2024-05-11T10:54:08Z

Rebased due to the conflict with merged #30081.

fanquake reviewed Apr 14, 2024

View reviewed changes

ci/test/00_setup_env_win64.sh Outdated Show resolved Hide resolved

hebasto force-pushed the 240414-win-subprocess branch from cf6a4b2 to 5e50b3d Compare April 14, 2024 18:17

fanquake mentioned this pull request Apr 19, 2024

refactor: Rename subprocess.hpp to follow our header name conventions #29910

Merged

hebasto force-pushed the 240414-win-subprocess branch from c05204c to 718e11c Compare April 23, 2024 17:34

hebasto force-pushed the 240414-win-subprocess branch from 718e11c to a0f3fc9 Compare April 23, 2024 19:48

DrahtBot added the CI failed label Apr 23, 2024

DrahtBot removed the CI failed label Apr 23, 2024

laanwj added Windows Wallet labels Apr 24, 2024

hebasto force-pushed the 240414-win-subprocess branch from a0f3fc9 to 0202082 Compare April 24, 2024 16:33

hebasto added the Needs CMake port label Apr 24, 2024

DrahtBot mentioned this pull request Apr 25, 2024

refactor: remove remaining unused code from cpp-subprocess #29961

Merged

Sjors reviewed Apr 26, 2024

View reviewed changes

DrahtBot added the Needs rebase label Apr 28, 2024

hebasto force-pushed the 240414-win-subprocess branch from 0202082 to db73ac8 Compare April 28, 2024 05:29

DrahtBot removed the Needs rebase label Apr 28, 2024

sipsorcery reviewed May 2, 2024

View reviewed changes

hebasto force-pushed the 240414-win-subprocess branch from db73ac8 to 83c6332 Compare May 2, 2024 20:45

DrahtBot mentioned this pull request May 6, 2024

build, test, doc: Temporarily remove Android-related stuff #30049

Open

DrahtBot added the Needs rebase label May 7, 2024

hebasto force-pushed the 240414-win-subprocess branch from 83c6332 to 9353a31 Compare May 7, 2024 20:06

DrahtBot added CI failed and removed Needs rebase labels May 7, 2024

hebasto force-pushed the 240414-win-subprocess branch from 9353a31 to b69b9e8 Compare May 8, 2024 10:11

DrahtBot removed the CI failed label May 8, 2024

hebasto mentioned this pull request May 10, 2024

refactor: Remove unused code from subprocess.h header #30081

Merged

hebasto added 8 commits May 11, 2024 11:52

subprocess: Fix cross-compiling for Windows

d83470f

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

91fe26c

This behavior is expected and consistent with the non-Windows implementation. The code is borrowed from `Popen::poll()`.

subprocess: Fix quote issue on Windows

c840d5c

subprocess, refactor: Replace custom __USING_WINDOWS__ with WIN32

a6450c4

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

test: Reintroduce Windows support in system_tests/run_command test

6f54d07

build: Re-enable external signer support for Windows

172c705

msvc: Re-enable external signer support

191d73b

test: Drop --enable-external-signer configure option

0783589

This option is enabled by default.

hebasto force-pushed the 240414-win-subprocess branch from b69b9e8 to 0783589 Compare May 11, 2024 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce external signer support for Windows #29868

Reintroduce external signer support for Windows #29868

hebasto commented Apr 14, 2024 •

edited

DrahtBot commented Apr 14, 2024 •

edited

fanquake commented Apr 17, 2024

hebasto commented Apr 18, 2024

hebasto commented Apr 23, 2024

DrahtBot commented Apr 23, 2024

hebasto commented Apr 24, 2024

Sjors Apr 26, 2024

Sjors commented Apr 26, 2024 •

edited

hebasto commented Apr 28, 2024

hebasto commented Apr 28, 2024

theStack commented Apr 30, 2024

sipsorcery May 2, 2024

hebasto May 2, 2024

sipsorcery May 2, 2024

hebasto May 2, 2024 •

edited

hebasto May 2, 2024

hebasto commented May 2, 2024

Sjors commented May 6, 2024

hebasto commented May 7, 2024

DrahtBot commented May 7, 2024

fanquake commented May 8, 2024

Sjors commented May 9, 2024

fanquake commented May 9, 2024

hebasto commented May 9, 2024

hebasto commented May 9, 2024

hebasto commented May 11, 2024

Reintroduce external signer support for Windows #29868

Are you sure you want to change the base?

Reintroduce external signer support for Windows #29868

Conversation

hebasto commented Apr 14, 2024 • edited

DrahtBot commented Apr 14, 2024 • edited

Code Coverage

Reviews

Conflicts

fanquake commented Apr 17, 2024

hebasto commented Apr 18, 2024

hebasto commented Apr 23, 2024

DrahtBot commented Apr 23, 2024

hebasto commented Apr 24, 2024

Sjors Apr 26, 2024

Choose a reason for hiding this comment

Sjors commented Apr 26, 2024 • edited

hebasto commented Apr 28, 2024

hebasto commented Apr 28, 2024

theStack commented Apr 30, 2024

sipsorcery May 2, 2024

Choose a reason for hiding this comment

hebasto May 2, 2024

Choose a reason for hiding this comment

sipsorcery May 2, 2024

Choose a reason for hiding this comment

hebasto May 2, 2024 • edited

Choose a reason for hiding this comment

hebasto May 2, 2024

Choose a reason for hiding this comment

hebasto commented May 2, 2024

Sjors commented May 6, 2024

hebasto commented May 7, 2024

DrahtBot commented May 7, 2024

fanquake commented May 8, 2024

Sjors commented May 9, 2024

fanquake commented May 9, 2024

hebasto commented May 9, 2024

hebasto commented May 9, 2024

hebasto commented May 11, 2024

hebasto commented Apr 14, 2024 •

edited

DrahtBot commented Apr 14, 2024 •

edited

Sjors commented Apr 26, 2024 •

edited

hebasto May 2, 2024 •

edited