build: explicitly disable support for external signing on Windows

fanquake commented at 5:00 am on January 14, 2022: member

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: https://github.com/klemens-morgenstern/boost-process/issues/164.

Resolves #24036.

fanquake added the label Build system on Jan 14, 2022

fanquake requested review from Sjors on Jan 14, 2022

fanquake requested review from achow101 on Jan 14, 2022

fanquake requested review from hebasto on Jan 14, 2022

MarcoFalke added the label DrahtBot Guix build requested on Jan 14, 2022

in configure.ac:1427 in 905d0ca808 outdated

1423+      dnl workarounds. See 67669ab425b52a2b6be3d2f3b3b7e3939b676a2c.
1424+      use_external_signer=no
1425+    ;;
1426+    *openbsd*)
1427+      dnl Boost Process doesn't support OpenBSD
1428+      use_external_signer=no

luke-jr commented at 6:58 am on January 14, 2022:

This will silently disable it even if the user specifies --enable-external-signer. Should error instead.

Sjors commented at 12:45 pm on January 14, 2022:

That would make more sense to me as well, assuming I understand correctly that Boost Process will simply be absent on OpenBSD - rather than silently broken.

fanquake commented at 12:55 pm on January 14, 2022:

assuming I understand correctly that Boost Process will simply be absent on OpenBSD

No, Boost Process is present on OpenBSD, but is broken.

Sjors commented at 12:58 pm on January 14, 2022:

build-openbsd.md says that Boost Process is not absent, but will cause the build to fail. In any case an error makes more sense, ~or~ as well as setting the default to false on OpenBSD.

luke-jr changes_requested

luke-jr commented at 7:00 am on January 14, 2022: member

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.

fanquake commented at 7:32 am on January 14, 2022: member

Dropping a dependency isn’t a good reason for losing functionality.

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.

luke-jr commented at 8:03 am on January 14, 2022: member

This is just something we disagree on. Dropping a dependency can be a perfectly good reason to (likely temporarily) lose some (still experimental) functionality,

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…)

hebasto commented at 8:36 am on January 14, 2022: member

Concept ACK.

It’s not a reason to remove the functionality from everyone who disagrees with you.

To be precise:

not “to remove” but “to remove temporarily”
not “the functionality” but “the experimental functionality”

fanquake commented at 9:39 am on January 14, 2022: member

(It’s also really no different than removing it entirely to avoid the dependency on boost::process…)

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.

Sjors commented at 12:42 pm on January 14, 2022: member

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 RunCommandParseJSON without Boost::Process.

fanquake commented at 1:56 pm on January 14, 2022: member

It could make sense to punt this until after the v23 branch-off and mention the upcoming deprecation in the v23 release notes.

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.

and/or for the upstream issue to resolve itself, and/or for someone to reimplement RunCommandParseJSON without Boost::Process.

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.

Sjors commented at 2:09 pm on January 14, 2022: member

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.

build: disable external signer on Windows e2ab9f83f8

fanquake force-pushed on Jan 15, 2022

fanquake renamed this:
~~build: explicitly disable support for external signing on Windows & OpenBSD~~
build: explicitly disable support for external signing on Windows
on Jan 15, 2022

kallewoof commented at 7:43 am on January 15, 2022: member

utACK e2ab9f83f8655bf09ea392beeee36b2bbe29769b

Sjors commented at 10:42 am on January 16, 2022: member

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 system_tests.cpp, but that’s fine given the above (and probably anyway, for when the feature is brought back).

in configure.ac:1426 in e2ab9f83f8

1422+      dnl since Boost 1.71.0, Process does not work with mingw-w64 without
1423+      dnl workarounds. See 67669ab425b52a2b6be3d2f3b3b7e3939b676a2c.
1424+      if test "$use_external_signer" = "yes"; then
1425+        AC_MSG_ERROR([External signing is not supported on Windows])
1426+      fi
1427+      use_external_signer="no";

hebasto commented at 3:30 am on January 18, 2022:

nit:

0      use_external_signer="no"

hebasto approved

hebasto commented at 3:31 am on January 18, 2022: member

ACK e2ab9f83f8655bf09ea392beeee36b2bbe29769b, tested on Linux Mint 20.2 (x86_64).

achow101 commented at 3:44 am on January 18, 2022: member

ACK e2ab9f83f8655bf09ea392beeee36b2bbe29769b

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.

fanquake merged this on Jan 20, 2022

fanquake closed this on Jan 20, 2022

fanquake deleted the branch on Jan 20, 2022

sidhujag referenced this in commit d5a1cc52db on Jan 20, 2022

MarcoFalke removed the label DrahtBot Guix build requested on Jan 27, 2022

hebasto commented at 8:54 am on February 3, 2022: member

FWIW, native MSVC builds still support external signers, did not test all functionality though.

fanquake referenced this in commit 62c864605a on Jul 28, 2022

sidhujag referenced this in commit 59b247d7b0 on Jul 29, 2022

fanquake referenced this in commit f765d4e232 on Aug 4, 2022

sidhujag referenced this in commit 1372bb62f5 on Aug 4, 2022

DrahtBot locked this on Feb 3, 2023

build: explicitly disable support for external signing on Windows #24065