Skip to content

Conversation

dickolsson
Copy link

@dickolsson dickolsson commented Jan 15, 2024

This is a fix against the master / 1.1-dev branch.

It fixes build issues on OpenBSD related to the lack of full dual-stack IPv6 1 and EVFILT_USER 2.

Everything builds fine. There are a few test failures on OpenBSD but they seem unrelated to the changes introduced here.

Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the port!

@@ -40,6 +40,11 @@ KJ_BEGIN_HEADER
#endif
#endif

#if __OpenBSD__
// OpenBSD doesn't have full dual-stack IPv6.
#define AI_V4MAPPED 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think we should be defining this symbol in a header that applications include.

It looks like this is only used in two places. In async-io-unix.c++ there's already an #if that avoids using this on Android, perhaps we should simply extend that to include OpenBSD? The symbol is also used in async-io-test.c++, where I think we can safely #define it to 0 if it's not already defined.

@@ -1063,10 +1065,12 @@ void UnixEventPort::captureChildExit() {
}

void UnixEventPort::wake() const {
#if !__OpenBSD__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this outright breaks support for cross-thread events on OpenBSD, causing them to fail to be delivered.

I think it would be better to remove OpenBSD from the list of platforms where we define KJ_USE_KQUEUE. It will then fall back to the poll()-based implementation, which supports cross-thread events correctly.

If we want to support kqueue on OpenBSD, we'll need to implement some other mechanism for wakeups here. If OpenBSD's kqueue supports per-thread signals correctly (like FreeBSD but not MacOS) then we could send a signal instead (using SIGUSR1, or the signal set with setReservedSignal()). If signals don't work I guess we could create a pipe that the thread waits on, and write a byte to it to trigger a wakeup.

@@ -41,7 +41,7 @@
#include <arpa/inet.h>
#endif

#if __FreeBSD__
#if __FreeBSD__ || __OpenBSD__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just move this include to the group that is included on all non-Windows platforms. There should be no harm and maybe it fixes other systems as well. In fact, I sad the same thing when these lines were added originally, but the submitter pushed back:

#1846 (comment)

theStack added a commit to theStack/capnproto that referenced this pull request May 30, 2025
The motivation for this commit is to fix the build for OpenBSD,
but it may also solves the same potential problem for other systems
without causing any harm.

Suggested already twice, see
capnproto#1846 (comment)
capnproto#1907 (comment)
kentonv added a commit that referenced this pull request Jun 5, 2025
Fix build on OpenBSD (second take, revival of #1907)
@dickolsson
Copy link
Author

Thank you @theStack and @kentonv for tackling this elsewhere!

@dickolsson dickolsson closed this Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants