bitcoin wrapper: Fix Windows exec so wrapper waits for child process #36105

In 6ca9199eecd9934563351c6b4b11468b75c6d088 - "test: Enable tool_bitcoin.py and interface_gui.py tests on Windows":

The commit message refers to _wspawnvp() and _wexecvp(), however we use the non-wide versions in code.

https://github.com/bitcoin/bitcoin/blob/33a363ea250839ca31ea043b7500789d2e5d844b/doc/AI_POLICY.md#L23-L26

re: #36105 (review)

The commit message refers to _wspawnvp() and _wexecvp(), however we use the non-wide versions in code.

Good catch. These changes are older than #35704 and the commit message was not updated when it was merged.

re: #36105 (review)

Thanks. It's important to me to distinguish words and ideas which are my own from those that come from other sources, so I use co-author tags for this. I think the AI policy was written with a different kind of PR in mind than this one. I understand the changes in this PR and take responsibility for them.

thread #36105 (review):

Thanks for the generous attribution.

Commit ac15f5654aec8ae96fc814cbab331c580f790250 "test: Enable tool_bitcoin.py and interface_gui.py tests on Windows" happens to be the commit I feel is still I don't fully grasp, but it's more what it's referencing than what it's doing.

What it's doing in the scope of interface_gui.py - instead of avoiding to run at all on Windows, only skip when the build is a vcpkg build. It references src/qt/test/CMakeLists.txt but that file is not immediately understandable to me. From looking at https://qt.developpez.com/doc/6.4/qguiapplication/#platformName-prop I think this is how I would make it more understandable:

--- a/src/qt/test/CMakeLists.txt
+++ b/src/qt/test/CMakeLists.txt
@@ -40,12 +40,14 @@ add_test(NAME test_bitcoin-qt
   COMMAND test_bitcoin-qt
 )
 if(WIN32 AND VCPKG_TARGET_TRIPLET)
   set(plugin_path "$<SHELL_PATH:$<PATH:GET_PARENT_PATH,$<PATH:GET_PARENT_PATH,$<TARGET_PROPERTY:Qt6::QWindowsIntegrationPlugin,LOCATION_$<CONFIG>>>>>")
   set_property(TEST test_bitcoin-qt APPEND PROPERTY
     ENVIRONMENT_MODIFICATION
       # On Windows, vcpkg configures Qt with `-opengl dynamic`, which makes
-      # the "minimal" platform plugin unusable due to internal Qt bugs.
+      # the otherwise appropriate headless "minimal" platform plugin unusable
+      # due to internal Qt bugs. So override to "windows" plugin in order to
+      # make it work when built in that configuration (VCPKG_TARGET_TRIPLET).
       QT_QPA_PLATFORM=set:windows
       QT_PLUGIN_PATH=set:${plugin_path}
   )
   unset(plugin_path)

My impression is that src/qt/test/CMakeLists.txt is supposed to make test_bitcoin-qt actually work, even when built with vcpkg.

So why would you be changing the functional test to skip something which should be working?

Well, when I build and run .\build\bin\Release\test_bitcoin-qt.exe I get:

qt.qpa.plugin: Could not find the Qt platform plugin "windows" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Setting it to "minimal" has the same result.

I tried printing the plugin path through adding this before the variable is unset:

  message("QT_PLUGIN_PATH=${plugin_path} (plugin_path)")

and it prints:

QT_PLUGIN_PATH=$<SHELL_PATH:$<PATH:GET_PARENT_PATH,$<PATH:GET_PARENT_PATH,$<TARGET_PROPERTY:Qt6::QWindowsIntegrationPlugin,LOCATION_$<CONFIG>>>>> (plugin_path)

Manually setting:

set QT_PLUGIN_PATH=C:\Users\hodlinator\bitcoin\build\vcpkg_installed\x64-windows\Qt6\plugins

makes .\build\bin\Release\test_bitcoin-qt.exe run fine, both with minimal and windows plugins!

So I wonder if the setting of QT_PLUGIN_PATH in CMakeLists.txt is somehow broken? I'm on CMake 4.4.2.

Having the QT_PLUGIN_PATH set correctly also makes interface_gui.py run fine when modified to not skip!

Yeah, didn't mean to put you in the same category as new-entrant sloppers. But I think it may be good to abide by the same rules or else alter/amend them.

FWIW, in the QML repo, we use the following code:

    file(GENERATE
      OUTPUT $<TARGET_FILE_DIR:bitcoin-qt>/qt.conf
      CONTENT "[Paths]\nPrefix = $<PATH:GET_PARENT_PATH,$<PATH:GET_PARENT_PATH,$<TARGET_FILE_DIR:Qt6::QWindowsIntegrationPlugin>>>\n"
    )

This makes all targets runnable form the build tree on Windows.

It's important to me to distinguish words and ideas which are my own from those that come from other sources, so I use co-author tags for this.

I think it is fine to do this, like it was done previously. See e.g. git log --grep="uggested by": 7209eb77902a8bf170fd7fbf6ec2388fec77b115 says: "Fix was suggested by willcl-ark in...", etc ...

If you want to credit Claude for a specific noteworthy thing, I'd list that specific thing, and if the full commit was authored by Claude, you can say verbatim that it was authored by Claude, but also fully reviewed by you.

I think that is clearer to everyone and hopefully addresses your needs. Otherwise, the co-author tags are only useful to you, but not really useful to anyone else.

re: #36105 (review)

Thanks, added a regex to switch to prose attribution in future commits. But the policy does not make logical sense to me. I don't understand how an accurate Co-Authored-By: line communicates anything about how well an author understands a change. It seems like the policy is asking contributors to be less transparent and obfuscate the source of changes, maybe to avoid them being tagged in github. I'm not very interested in policies or policywriting though, so will leave deep thinking about this to others.

re: #36105 (review)

Good findings here. For now I added the doc improvement. But @hebasto it seems like hodlinator found inconsistencies here that would be good to address:

• QT_PLUGIN_PATH may not be set correctly, and if it is set correctly overriding QT_QPA_PLATFORM is not necessary?
• The claim about 'the "minimal" platform plugin unusable due to internal Qt bugs' is inaccurate?

(hodlinator's print message("QT_PLUGIN_PATH=${plugin_path} (plugin_path)") output is expected though because the variable contains generator expressions, which aren't expanded until cmake's "generate" phase after the configure phase where messages are printed. To see generator expressions you need to output them with something that runs during the generate phase like file(GENERATE OUTPUT <path> CONTENT "$<...>"))

On getting the python test running in the native ci build, generating a qt.conf file as hebasto suggested seems like the best approach. Another approach would be for the python test or ci-windows.py to set the QT_PLUGIN_PATH environment variable themselves, but then they either have to derive that path themselves somehow, or get it from cmake. Getting it from cmake would not be easy because visual studio is a multi-config build system, so the value is not available until the generate phase, so it would need to rely on file(GENERATE) and have more code reading the generated file, basically reinventing the qt.conf approach while being more complicated. I think this is probably better to address in a separate pr since this PR is supposed be a bitcoin.exe bugfix, not change the way bitcoin-qt.exe is run.

Checked https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/spawnvp-wspawnvp?view=msvc-170 but this difference in error codes between runtimes is not documented there. Is it admitted elsewhere?

re: #36105 (review)

Checked https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/spawnvp-wspawnvp?view=msvc-170 but this difference in error codes between runtimes is not documented there. Is it admitted elsewhere?

It's not documented anywhere and was just seen in CI. I was meaning to follow up and get more information about this though, so I'll try to do that soon.

re: #36105 (review)

this difference in error codes between runtimes is not documented there

Followed up and this difference does not seem to be documented anywhere, only observed in CI. Did some more testing though and was able to simplify and write a better comment which is in the latest push

Impressive testing by @ryan8or. ^^ (https://github.com/ryanofsky/bitcoin/actions/runs/34367754505)

One more observation on this, from a windows-2022 runner with the cross-built binaries from the #36190 branch (which carries this PR's commits). When the target executable is missing, the error the user sees differs between runtimes:

MSVCRT: Error: execvp failed to execute '...\bitcoind': Invalid argument
UCRT:   Error: execvp failed to execute '...\bitcoind': No such file or directory

The intermediate lookups in ExecCommand accept EINVAL as "not found" and move on, but the last attempt passes allow_notfound=false, so the raw errno goes into the std::system_error. Mapping EINVAL to ENOENT in that throw as well would make an MSVCRT build report a missing file the same way UCRT does, instead of "Invalid argument".

re: #36105 (review)

Mapping EINVAL to ENOENT in that throw as well would make an MSVCRT build report a missing file the same way UCRT does, instead of "Invalid argument".

When it comes to error reporting I think it's better to just to show actual error reported by operating system or runtime, and trying to translate one error code into another error code before printing it would not be good there. The earlier cases are using EINVAL for error handling not error reporting so they are a bit different.