New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: explicitly disable support for external signing on Windows #24065
build: explicitly disable support for external signing on Windows #24065
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept NACK in regard to removing Windows support. Dropping a dependency isn't a good reason for losing functionality.
Approach NACK for how it handles BSD. Instead, it makes more sense to simply detect availability of boost::process, and do the right thing when it's not available.
This is just something we disagree on. Dropping a dependency can be a perfectly good reason to (likely temporarily) lose some (still experimental) functionality, if it's blocking the progress of a larger project goal. This doesn't means that external signing on Windows will never be supported ever again. I'll just remove the OpenBSD changes, they aren't required, and just distract from the main change. |
Then you are welcome to build without it yourself. It's not a reason to remove the functionality from everyone who disagrees with you. (It's also really no different than removing it entirely to avoid the dependency on boost::process...) |
Concept ACK.
To be precise:
|
I see those as completely different. In this case, we're (potentially temporarily) losing support for a single platform, where usage could be considered much more experimental than the others. In the case you describe, you'd be removing the feature entirely, for all platforms. |
Concept ACK. The feature is both experimental and seems really hard to use on Windows at the moment. This makes me suspect that virtually no users are impacted by this. It could make sense to punt this until after the v23 branch-off and mention the upcoming deprecation in the v23 release notes. That would leave more time for users to object to it, and/or for the upstream issue to resolve itself, and/or for someone to reimplement |
If the assumption is that virtually no one is effected, (which I tend to agree with) I don't think we need to wait until after we have the next release (2 1/2 months), to then wait and see if anyone reads the releases notes, and then potentially objects.
I think either of those are fairly wishful thinking. Given that the issue for the first has been open for ~9 months with little interest, and, from what I've heard, patching the Boost Filesystem usage out of Boost Process isn't particularly straight-forward. Note that even if a change was made to Boost Process now, it's not clear if it would even make it into the next Boost release (which would also be some time after our branch off). In regards to the second, unless one of the near nonexistent Windows users is interested, I'm not sure who else would be motivated to do that right now. |
Avoiding Boost::Process has been a wish expressed by non-Windows users too, but it never got anyone motivated to the point of actually implementing something. |
905d0ca
to
e2ab9f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK e2ab9f8
utACK e2ab9f8 Tested that it's still enabled on macOS. Maybe clarify that it's disabled on cross compiled Windows; on native builds it should still work. There is some Windows specific code left in the functional test suite and in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e2ab9f8, tested on Linux Mint 20.2 (x86_64).
if test "$use_external_signer" = "yes"; then | ||
AC_MSG_ERROR([External signing is not supported on Windows]) | ||
fi | ||
use_external_signer="no"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
use_external_signer="no"; | |
use_external_signer="no" |
ACK e2ab9f8 I agree that this functionality is experimental and likely to be not working on Windows, so disabling it when cross compiling for release is okay. However we should be looking for solutions to re-enable and fix this functionality. |
FWIW, native MSVC builds still support external signers, did not test all functionality though. |
ba9a8e6 test: Drop unused boost workaround (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin/bitcoin#24065 and removes the workaround which has already been removed in other [places](https://github.com/bitcoin/bitcoin/pull/24065/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216). Moreover, this workaround won't be required even if bitcoin/bitcoin#25696 is ever merged. ACKs for top commit: fanquake: ACK ba9a8e6 Tree-SHA512: db19fc1550252d7a82a08f388ff6078c78452365e74b41e7bc36cbbc4d0fed9342636e8f2371bb8e78c9d11ee721d6363bcc21d11787f3aac967a6c4a9cc346f
ba9a8e6 test: Drop unused boost workaround (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin#24065 and removes the workaround which has already been removed in other [places](https://github.com/bitcoin/bitcoin/pull/24065/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216). Moreover, this workaround won't be required even if bitcoin#25696 is ever merged. ACKs for top commit: fanquake: ACK ba9a8e6 Tree-SHA512: db19fc1550252d7a82a08f388ff6078c78452365e74b41e7bc36cbbc4d0fed9342636e8f2371bb8e78c9d11ee721d6363bcc21d11787f3aac967a6c4a9cc346f
…ernal-signer` configure option 8df063e build: Fix help string for `--enable-external-signer` configure option (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin/bitcoin#24065 and fixes the help string according to the actual default value https://github.com/bitcoin/bitcoin/blob/816ca01650f4cc66a61ac2f9b0f8b74cd9cd0cf8/configure.ac#L324-L327 ACKs for top commit: kristapsk: cr utACK 8df063e jarolrod: ACK 8df063e Tree-SHA512: ad3f457a53c9238ddd8ded9efd1224e564e6cb9da8b7ff7733a11e32a7daad5c0f6c6223509218f44944a874470cb0d2447897662eaf4e78c763b30785717c50
…gner` configure option 8df063e build: Fix help string for `--enable-external-signer` configure option (Hennadii Stepanov) Pull request description: This PR is a follow up of bitcoin#24065 and fixes the help string according to the actual default value https://github.com/bitcoin/bitcoin/blob/816ca01650f4cc66a61ac2f9b0f8b74cd9cd0cf8/configure.ac#L324-L327 ACKs for top commit: kristapsk: cr utACK 8df063e jarolrod: ACK 8df063e Tree-SHA512: ad3f457a53c9238ddd8ded9efd1224e564e6cb9da8b7ff7733a11e32a7daad5c0f6c6223509218f44944a874470cb0d2447897662eaf4e78c763b30785717c50
This change explicitly disables support for external signing when targeting Windows and OpenBSD. The driver for this is that Boost Process uses boost::filesystem internally, when targeting Windows, which gets in the way of removing our usage of it (#20744). While we could adjust #20744 to still link against the Boost libs when building for Windows, that would be disappointing, as we wouldn't have cleanly removed the Boost usage we're trying too (including the build infrastructure), and, we'd be in a position where we would be building releases differently depending on the platform, which is something I want to avoid.
After discussion with Sjors, Achow and Hebasto, this seemed like a reasonable step to move #20744 forward (as-is). Note that support for external signing (while already being experimental), could be considered even more experimental on Windows. Also, oddly, we have external-signing explicitly disabled in our Windows (cross-compile) CI, it's not clear why this is the case, as, if it's a feature being built into releases, it should be being built and tested in the CI which is most-like the release process.
There is an issue open upstream, in regards to migrating Boost Process to std::filesystem, or having an option to use it. However there hasn't been much discussion since it was opened ~9 months ago. There is another related issue here: klemens-morgenstern/boost-process#164.
Resolves #24036.