Re-enable external signer support for Windows

luke-jr commented at 5:57 am on May 12, 2022: member

configure check is modified to actually verify Boost::Process works (without needing extra libs), rather than hardcoding it off for Windows unconditionally.

For now, depends does some trivial patching of boost::process to use std::filesystem instead of boost::filesystem, and also deletes boost::filesystem entirely to be sure it doesn’t find its way into builds.

Also, BOOST_PROCESS_USE_STD_FS is defined since it looks like that’s the approach upstream boost is planning on (https://github.com/klemens-morgenstern/boost-process/commit/85cbc29726e4f4169036ab0b9b905f7287950334)

DrahtBot added the label Build system on May 12, 2022

DrahtBot added the label Utils/log/libs on May 12, 2022

configure: Detect compatibility of Boost.Process rather than hardcode non-Windows af5fee0b87

Restore still-needed workaround for Boost.Process incompatibility w/ mingw-w64 a7fa0da861

depends: Patch boost to remove filesystem module and use std::filesystem instead (for Boost.Process) d9eb2a1405

luke-jr force-pushed on May 12, 2022

fixup! configure: Detect compatibility of Boost.Process rather than hardcode non-Windows 06548df560

Sjors commented at 8:31 am on May 12, 2022: member

I wonder if this is still worth doing* in light of #24907. Then again, actually replacing Boost::Process might take a while.

* = well, you already did it, so it’s just about review and maintenance.

in depends/packages/boost.mk:9 in 06548df560

3@@ -4,6 +4,11 @@ $(package)_download_path=https://boostorg.jfrog.io/artifactory/main/release/$($(
4 $(package)_file_name=boost_$(subst .,_,$($(package)_version)).tar.bz2
5 $(package)_sha256_hash=fc9f85fc030e233142908241af7a846e60630aa7388de9a5fafb1f3a26840854
6 
7+define $(package)_preprocess_cmds
8+  rm boost/filesystem* -rf &&\
9+  sed -i.old 's/\(#include <\)boost\/filesystem[^>]*>/\1filesystem>/; s/boost\(::filesystem\)/std\1/g' `find boost/process -type f`

fanquake commented at 10:12 am on May 12, 2022:

If there’s a patch available somewhere upstream, we should be applying that, not re-introducing sed usage into depends, which we’ve worked to almost fully remove.

luke-jr commented at 8:32 pm on May 12, 2022:

sed is much cleaner, and has not been removed.

fanquake commented at 8:40 pm on May 12, 2022:

sed+some regex is not cleaner than a well-documented patch, from upstream.

Yes it has been removed, to the furthest extent possible. As far as I’m aware, the only usages left are where we can’t use patch files, because the values being substituted are only available at runtime. Clearly this isn’t an instance of having to do that.

fanquake commented at 11:01 am on May 12, 2022: member

Concept 0. I don’t think we should be increasing our Boost Process usage, given there is agreement to move away from it entirely. If you disagree, please comment in #24907.

Regardless, bending over backwards to force Windows support back in, given it’s clear that it’s not really supported upstream (Windows fixes/improvements just seem to be ignored, i.e https://github.com/boostorg/process/pull/240, https://github.com/boostorg/process/pull/237, https://github.com/boostorg/process/pull/236) or tested (the Windows CI for the upstream change failed, although a perpetually failing CI also seems to be the norm for Process), and there’s has been no effort put into getting a fixed (for mingw-w64) version of Process into any recent Boost release (so users could drop the __kernel_entry workarounds).

One other point worth noting is that since the release of 23.0, I don’t think we’ve had anyone complain or comment on external signing support for Windows “going missing”. Maybe that’s because no-one has upgraded yet, and/or they’ve just failed to notice. Or maybe it’s because external singing on Windows didn’t really have any users.

Then again, actually replacing Boost::Process might take a while.

There’s no particular reason it should take a while. The people who care about, and want to use, or build features on top of external signing should probably be working on/thinking about a migration already. It’s pretty clear that Process is not the kind of dependency we want in Bitcoin Core over the long term, so if the status quo persists, I’d assume Process and external signing could eventually just be removed.

luke-jr commented at 8:34 pm on May 12, 2022: member

I don’t think we should be increasing our Boost Process usage, given there is agreement to move away from it entirely. If you disagree, please comment in #24907.

I don’t particularly care if we move away from it, but I don’t see any better alternatives (cpp-subprocess, for example, doesn’t officially support Windows, and doesn’t seem to have a simple way to port the fix in #22417).

The people who care about, and want to use, or build features on top of external signing should probably be working on/thinking about a migration already. It’s pretty clear that Process is not the kind of dependency we want in Bitcoin Core over the long term, so if the status quo persists, I’d assume Process and external signing could eventually just be removed.

That’s an absurd attitude to take.

luke-jr referenced this in commit ae42a1665a on May 21, 2022

luke-jr referenced this in commit b17d2e9823 on May 21, 2022

luke-jr referenced this in commit 8b12eb658e on May 21, 2022

DrahtBot commented at 1:42 pm on May 24, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25696 (build: Re-enable external signer on Windows 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.

fanquake commented at 1:57 pm on July 25, 2022: member

I still am not convinced that this is something we should be doing, but the approach in #25696 would be more preferable in any case. I think this PR could be closed in favour of that one.

luke-jr commented at 5:56 pm on July 25, 2022: member

I still am not convinced that this is something we should be doing, but the approach in #25696 would be more preferable in any case. I think this PR could be closed in favour of that one.

Its configure check is broken currently, but sure, it can copy that from this, or vice-versa.

fanquake commented at 9:00 am on July 26, 2022: member

Ok. I’m going to close this in favour of the other PR for now then. Any required changes can be picked there.

fanquake closed this on Jul 26, 2022

bitcoin locked this on Jul 26, 2023

Re-enable external signer support for Windows #25111

Conflicts