Re-enable external signer support for Windows #25111
Conversation
…tem instead (for Boost.Process)
…ardcode non-Windows
|
I wonder if this is still worth doing* in light of #24907. Then again, actually replacing Boost::Process might take a while.
|
fanquake
left a comment
fanquake
left a comment
There was a problem hiding this comment.
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 boostorg/process#240, boostorg/process#237, boostorg/process#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.
|
|
||
| define $(package)_preprocess_cmds | ||
| rm boost/filesystem* -rf &&\ | ||
| sed -i.old 's/\(#include <\)boost\/filesystem[^>]*>/\1filesystem>/; s/boost\(::filesystem\)/std\1/g' `find boost/process -type f` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sed is much cleaner, and has not been removed.
There was a problem hiding this comment.
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.
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).
That's an absurd attitude to take. |
… non-Windows Github-Pull: bitcoin#25111 Rebased-From: aa8f5bdc47eab8b561c9b8a33496abd214c40d5c
…mingw-w64 Github-Pull: bitcoin#25111 Rebased-From: f2eb262a8ac74509560eee7a9eef198e9447331e
…tem instead (for Boost.Process) Github-Pull: bitcoin#25111 Rebased-From: 2a53dce0b66294f3ad43e08cbecf428a818b3f7e
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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. |
|
Ok. I'm going to close this in favour of the other PR for now then. Any required changes can be picked there. |
4 participants
configurecheck 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_FSis defined since it looks like that's the approach upstream boost is planning on (klemens-morgenstern/boost-process@85cbc29)