Skip to content
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

Make reraise_kj_exception available to downstream #344

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

LasseBlaauwbroek
Copy link
Contributor

I'm using Pycapnp in a project, where we compile .capnp files directly to Cython instead of using the dynamic interface (for speed). For this, we need access to the reraise_kj_exception C function defined by Pycapnp. This is not possible, because Cython does not automatically make this function available to downstream users.

My previous solution, in #301, was rather flawed. The file capabilityHelper.cpp, where reraise_kj_exception is defined, was bundled into the distribution, so that this file could be included in downstream libraries. This turns out to be a terrible idea, because it redefines a bunch of other things like ReadPromiseAdapter. For reasons not entirely clear to me, this leads to segmentation faults. This PR revers #301.

Instead, in this PR I've made reraise_kj_exception a Cython-level function, that can be used by downstream libraries. The C-level variant has been renamed to c_reraise_kj_exception.

@LasseBlaauwbroek
Copy link
Contributor Author

The CI failures are unrelated. Master is also failing: https://github.com/LasseBlaauwbroek/pycapnp/actions/runs/6704200082/job/18216165343
Seems like something has changed in the ecosystem.

@LasseBlaauwbroek
Copy link
Contributor Author

I have no idea what is going on here. I can reproduce this locally:

  • In a clean virtualenv, when I run pip install . and then python -c "import capnp" I get ModuleNotFoundError: No module named 'capnp.lib.capnp'.
  • Doing the same thing from a different directory does not give the error.
  • When doing pip install -e ., the error also does not occur.

This suggests to me that there is a loadpath issue. Python seems to be looking up the .py files in the local directory. This is normal as far as I know. But then, it cannot find the binary capnp.cpython-*.so file locally. But I'd think it should be looking that up in the lib location of the virtualenv. I checked, and sys.path indeed includes the lib directory of the virualenv.

I'm at a loss here. @haata @tobiasah can you reproduce this? Any idea what is going on?

@haata
Copy link
Collaborator

haata commented Nov 2, 2023

I'll try to take a look later this weekend.

I'm using Pycapnp in a project, where we compile `.capnp` files directly to
Cython instead of using the dynamic interface (for speed). For this, we need
access to the `reraise_kj_exception` C function defined by Pycapnp. This is not
possible, because Cython does not automatically make this function available to
downstream users.

My previous solution, in capnproto#301, was rather flawed. The  file `capabilityHelper.cpp`, where
`reraise_kj_exception` is defined, was bundled into the distribution, so that
this file could be included in downstream libraries. This turns out to be a
terrible idea, because it redefines a bunch of other things like
`ReadPromiseAdapter`. For reasons not entirely clear to me, this leads to
segmentation faults. This PR revers capnproto#301.

Instead, in this PR I've made `reraise_kj_exception` a Cython-level function,
that can be used by downstream libraries. The C-level variant has been renamed
to `c_reraise_kj_exception`.
@LasseBlaauwbroek
Copy link
Contributor Author

Looks like the build issue has resolved itself. Probably some dependency that was temporarily faulty or something like that.

@haata haata merged commit aafec22 into capnproto:master Nov 5, 2023
12 checks passed
@haata
Copy link
Collaborator

haata commented Nov 5, 2023

(Re-read your PR after commenting, makes sense)

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