-
Notifications
You must be signed in to change notification settings - Fork 927
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
Add support for OpenBSD #1313
Add support for OpenBSD #1313
Conversation
FYI, I expect you'd want to amend this line in async-test as well, which might help with the failing test: |
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.
Hi @ZhanYF,
Thanks for working on this!
c++/Makefile.am
Outdated
@@ -421,10 +421,14 @@ capnpc_c___SOURCES = src/capnp/compiler/capnpc-c++.c++ | |||
# Also attempt to run ldconfig, because otherwise users get confused. If | |||
# it fails (e.g. because the platform doesn't have it, or because the | |||
# user doesn't have root privileges), don't worry about it. | |||
# | |||
# We need to specify the path for OpenBSD. | |||
install-exec-hook: | |||
ln -sf capnp $(DESTDIR)$(bindir)/capnpc | |||
ldconfig < /dev/null > /dev/null 2>&1 || true |
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.
Do you intentionally want to run ldconfig
bare once, and then a second time with paths, on OpenBSD? Or should this be in an "else" block below?
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.
Technically an 'else' block is not required since the second ldconfig
will overwrite the effect of the first ldconfig
on OpenBSD, but I see how an 'else' block can make things better. Updated.
c++/src/kj/async-io-unix.c++
Outdated
@@ -85,7 +85,7 @@ void setCloseOnExec(int fd) { | |||
} | |||
|
|||
static constexpr uint NEW_FD_FLAGS = | |||
#if __linux__ && !__BIONIC__ | |||
#if __linux__ && KJ_USE_FIBERS |
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.
It looks like you assumed every instance of !__BIONIC__
is guarding fiber code, but there are actually many reasons why we check for Bionic in different parts of the code. Most of these ifdefs are unrelated to fibers. This one here actually only matches Linux, so obviously it doesn't need to be updated to support OpenBSD. Can you remove the changes that aren't fiber-related?
In fact, nothing in this file is fiber-related.
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.
Thanks for letting me know that, I restricted the scope the KJ_USE_FIBERS and reverted the change done on this file. Please take a look :)
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.
Hmm... @ZhanYF I don't see this (& other lines within this file) reverted. Is it possible you forgot to push the updated version or didn't revert it completely?
c++/src/kj/async-io-unix.c++
Outdated
@@ -1483,7 +1483,7 @@ kj::Own<PeerIdentity> SocketAddress::getIdentity(kj::LowLevelAsyncIoProvider& ll | |||
result.uid = creds.uid; | |||
} | |||
|
|||
#elif defined(LOCAL_PEERCRED) | |||
#elif defined(LOCAL_PEERCRED) && __OpenBSD__ |
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.
It looks like your change on this line will actually break platforms other than OpenBSD that use this block of code.
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.
This is a mistake and is now reverted, thanks!
Edit: actually I mean updated, it should be && !__OpenBSD__
on a different branch.
This indeed helped, but there are still some failed tests which I have no idea how to fix, e.g.:
log: Any idea on what might be causing that? |
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.
It might be a good idea to set up CI for OpenBSD (https://github.com/cross-platform-actions/openbsd-builder ?). Otherwise, the bitrot risk seems high.
Regarding the failed tests, is that for the version of code that's pushed or some version you're testing locally? I'm assuming the tests are being run on OpenBSD? I don't know why the tests would be failing on OpenBSD. It looks like it's using the pthread-based implementation & I'm not sure how well that branch is covered by automation. I think you'll have to dig in & debug what's going on if you want to figure that part out. If you're just using plain cap'n'proto messages, then mutexes don't really come up. They're needed if you're using KJ's event loop (& cap'n'proto RPC since it's built on top of that).
That being said, AFAICT, OpenBSD has futex as of 6.2. Not sure if that's the minimum version that can be targeted for your use-case, but, if so, it might be to adjust the macro at the top of mutex.h
to define KJ_USE_FUTEX
to 1 by default on OpenBSD & see if that bypasses the problem.
install-exec-hook: | ||
ln -sf capnp $(DESTDIR)$(bindir)/capnpc | ||
ldconfig < /dev/null > /dev/null 2>&1 || true | ||
|
||
if [ `uname` == 'OpenBSD' ]; then \ |
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: Is this Bash or Bourne shell? If the former, I would prefer using [[ ]]
for the conditional and $()
for the subshell invocation. It's not a big deal here but it's just a safer pattern to just blindly follow & not think about shell nuances. As I said, not a big deal & feel free to ignore if you want (& definitely ignore if the shell is an old-school Bourne shell).
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.
This is an automake file, so we need to assume bourne shell.
c++/src/kj/async-io-unix.c++
Outdated
@@ -85,7 +85,7 @@ void setCloseOnExec(int fd) { | |||
} | |||
|
|||
static constexpr uint NEW_FD_FLAGS = | |||
#if __linux__ && !__BIONIC__ | |||
#if __linux__ && KJ_USE_FIBERS |
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.
Hmm... @ZhanYF I don't see this (& other lines within this file) reverted. Is it possible you forgot to push the updated version or didn't revert it completely?
@@ -1472,7 +1472,7 @@ kj::Own<PeerIdentity> SocketAddress::getIdentity(kj::LowLevelAsyncIoProvider& ll | |||
// seen vague references on the internet saying that a PID of 0 and a UID of uid_t(-1) are used | |||
// as invalid values. | |||
|
|||
#if defined(SO_PEERCRED) | |||
#if defined(SO_PEERCRED) && !__OpenBSD__ |
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.
Should this be reverted as well?
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.
I don't think so, this is intentional as OpenBSD requires struct xucred creds
here. Reverting it will cause the following error:
src/kj/async-io-unix.c++:1476:20: error: variable has incomplete type 'struct ucred'
struct ucred creds;
^
src/kj/async-io-unix.c++:1476:14: note: forward declaration of 'ucred'
struct ucred creds;
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.
It seems like I forget to push the fully reverted version last time, this file should be good now. Thanks for pointing that out.
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.
Can you add a comment here that OpenBSD defines SO_PEERCRED
but uses a different interface for it, hence we're falling back to LOCAL_PEERCRED
?
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.
Agree, done.
c++/src/kj/async.c++
Outdated
@@ -1443,7 +1452,7 @@ FiberStack::FiberStack(size_t stackSizeParam) | |||
// We can create fibers before we convert the main thread into a fiber in FiberBase | |||
KJ_WIN32(osFiber = CreateFiber(stackSize, &StartRoutine::run, this)); | |||
|
|||
#elif !__BIONIC__ | |||
#elif !__BIONIC__ && KJ_USE_FIBERS |
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.
Is this condition redundant? Is this different from simply doing #elif KJ_USE_FIBERS
?
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.
It is indeed redundant here, forgot to change it last time, should be OK now.
c++/src/kj/async.c++
Outdated
@@ -1472,7 +1481,7 @@ FiberStack::FiberStack(size_t stackSizeParam) | |||
FiberStack::~FiberStack() noexcept(false) { | |||
#if _WIN32 || __CYGWIN__ | |||
DeleteFiber(osFiber); | |||
#elif !__BIONIC__ | |||
#elif !__BIONIC__ && KJ_USE_FIBERS |
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.
Same here.
I agree, it's just that the current approach used in cross-platform-actions is relying on macOS for nested virtualization, and since I don't have a mac, the debugging process has not been very smooth. My limited test results show that
The current futex syscall on OpenBSD only supports three operations |
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.
Why did you remove a bunch of CI tests?
@@ -1472,7 +1472,7 @@ kj::Own<PeerIdentity> SocketAddress::getIdentity(kj::LowLevelAsyncIoProvider& ll | |||
// seen vague references on the internet saying that a PID of 0 and a UID of uid_t(-1) are used | |||
// as invalid values. | |||
|
|||
#if defined(SO_PEERCRED) | |||
#if defined(SO_PEERCRED) && !__OpenBSD__ |
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.
Can you add a comment here that OpenBSD defines SO_PEERCRED
but uses a different interface for it, hence we're falling back to LOCAL_PEERCRED
?
Just a quick note that I've integrated some OpenBSD conditionals into KJ_USE_FIBERS in PR #1321 |
Sorry, the CI removal part was a mistake, it was meant to be for my WIP CI test branch. Reverted. |
#1321 has been merged covering a lot of similar ground to your PR. Can you rebase and reduce this PR to just the OpenBSD specifics? |
@@ -77,7 +77,7 @@ $CAPNP convert json:binary $SCHEMA TestAllTypes < $TESTDATA/pretty.json | cmp $T | |||
$CAPNP convert json:binary $SCHEMA TestAllTypes < $TESTDATA/short.json | cmp $TESTDATA/binary - || fail short json to binary | |||
|
|||
$CAPNP convert json:binary $JSON_SCHEMA TestJsonAnnotations -I"$SRCDIR" < $TESTDATA/annotated.json | cmp $TESTDATA/annotated-json.binary - || fail annotated json to binary | |||
$CAPNP convert binary:json $JSON_SCHEMA TestJsonAnnotations -I"$SRCDIR" < $TESTDATA/annotated-json.binary | cmp $TESTDATA/annotated.json - || fail annotated json to binary | |||
$CAPNP convert binary:json $JSON_SCHEMA TestJsonAnnotations -I"$SRCDIR" < $TESTDATA/annotated-json.binary | cmp $TESTDATA/annotated.json - || fail annotated binary to json |
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.
I understand this is not OpenBSD specific but since we are here perhaps we can also fix the naming in this line.
Tested this PR branch on OpenBSD 7.0 snapshot, works fine.
Thanks! |
Hi @ZhanYF, I've put together a point release including your OpenBSD changes: https://capnproto.org/capnproto-c++-0.9.1-rc1.tar.gz Can you verify it works? |
Thanks for that, I can confirm this works on the latest OpenBSD snapshot w/ amd64. P.S.: I've got the GitHub CI pipeline working now, but tests for known-broken features generate a large amount of noise, should I patch those tests out in my pipeline? |
@ZhanYF Hmm, what features are "known-broken"? We could |
Sorry for the delay, I'm busy at the moment so can't investigate all the errors. Here is a list of failed tests from log.test.txt:
They seem to be related to kj, but I don't know how to debug that.
This might be due to the fact that OpenBSD uses LibreSSL instead of OpenSSL, but I'm not sure.
|
KJ only uses futex if
Yikes, looks like some sort of corruption when using mmap?
Oh great, LibreSSL produces different error strings than either upstream OpenSSL or BoringSSL? I guess we'll need to adjust the test to accept any of them.
Were there any error messages associated with these two failures?
To be clear the word "legacy" here refers to how the test was declared (using the googletest syntax vs. KJ syntax), but the test is still very much valid. It looks like OpenBSD may need to be added to the |
Hi, sorry for the looong delay, I got way too busy recently...
You're right, sorry for the confusion. This is not the default and I got those errors because I was testing with kj/mutex.c++:963: pthread_mutex_destroy(&waiter.stupidMutex); strerror(pthreadError) = Invalid argument looks like the
Yes, part of the string is corrupted. I tried to debug it but that rabbit hole is too deep...
agree, after adding those strings to the test the errors went away.
The Xthread related failure is also caused by
Thanks for the pointer! It does seem to be the case and this fixes the following failure: ... but there is still another failure in this category: [ TEST ] kj/async-io-test.c++:174: legacy test: AsyncIo/UnixSocket humm... maybe I should send a PR to fix the libressl/ScmRights failures? sidenote: When testing in a guest VM running on OpenBSD's native hypervisor vmd, the following test will fail, which is likely due to a known bug which leads to clock drifting1. I think we can safely ignore this one for now as I would expect the clock drifting issue to be fixed before any large deployment of vmd...... [ TEST ] kj/time-test.c++:95: all clocks advance in real time |
I think this is because we're using
I guess this suggests that |
- Drop patch (already in version) - Fix build on musl by disabling fibers through the new KJ_USE_FIBERS variable: capnproto/capnproto#1167 capnproto/capnproto#1313 - Update indentation in hash file (two spaces) https://capnproto.org/news Fixes: - http://autobuild.buildroot.org/results/1a54cf9e7223c2bd67a5c85a6f2f42aa98da3a53 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
- Drop patch (already in version) - Fix build on musl by disabling fibers through the new KJ_USE_FIBERS variable: capnproto/capnproto#1167 capnproto/capnproto#1313 - Update indentation in hash file (two spaces) https://capnproto.org/news Fixes: - http://autobuild.buildroot.org/results/1a54cf9e7223c2bd67a5c85a6f2f42aa98da3a53 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> (cherry picked from commit ee3e17a) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
- Drop patch (already in version) - Fix build on musl by disabling fibers through the new KJ_USE_FIBERS variable: capnproto/capnproto#1167 capnproto/capnproto#1313 - Update indentation in hash file (two spaces) https://capnproto.org/news Fixes: - http://autobuild.buildroot.org/results/1a54cf9e7223c2bd67a5c85a6f2f42aa98da3a53 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> (cherry picked from commit ee3e17a) Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
It's been on my to-do list for quite some time now, finally got a chance to do it. This PR aims to:
Tested on OpenBSD 7.0-beta/AMD64.
output of
make check
: https://github.com/ZhanYF/static/blob/main/checkLogany comment? I have to admit I'm not familiar with capnproto/fibers/c++ in general, so please feel free to tweak it :)