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

Force cython 2.x #665

Closed
wants to merge 5 commits into from
Closed

Force cython 2.x #665

wants to merge 5 commits into from

Conversation

cmacdonald
Copy link
Contributor

As noted in #663, Jnius does not build currently. This is because of the release last month of Cython 3.0.

There are two issues:
(1) Cython 3.0 wont compile include files (.pxi). I have excluded these
(2) Jnius compiled using Cython 3.0 fails with AttributeError: 'Class' object has no attribute '_JavaClass__cls_storage'. I cant easily see why this is occurring.

I think we should do a new 1.5.1 release as soon as possible using this PR, as anyone installing Jnius in an environment without prebuilt binaries will currently not succeed. Pegging to Cython <3.0 will address the main problem for now. We can create another issue for Cython 3 support.

@misl6
Copy link
Member

misl6 commented Aug 6, 2023

These .pxi and .pxd files should not be completely ignored, otherwise, if a change is done on a .pxd or .pxi file, but the .pyx file is left untouched, the build will not be performed.

But, we all agree that .pxi and .pxd files should not be considered sources.

Instead, an approach, similar to the one we currently use on pyobjus should be used:

https://github.com/kivy/pyobjus/blob/9d66904623df5e0772dad807a6a14c340f22fc9e/setup.py#L122-L128

(Tests are passing on Cython 3.x, so IMHO there's no need to force Cython 0.x)

Yeah, have a lot of deprecation warnings, but that will be a slow migration process for the whole Python community.

@cmacdonald
Copy link
Contributor Author

(Tests are passing on Cython 3.x, so IMHO there's no need to force Cython 0.x)

Are you sure that tests and installation is passing/working on Cython 3? Its definitely not my recent experience, even on the Github Actions for this PR.

I'm happy to revert setup.py if we are forcing old Cython.

Instead, an approach, similar to the one we currently use on pyobjus should be used:

I think your point is to use a depends= kwarg to declare the .pxi files, right?

@cmacdonald
Copy link
Contributor Author

Any thoughts @misl6 ?

@misl6
Copy link
Member

misl6 commented Aug 24, 2023

I think your point is to use a depends= kwarg to declare the .pxi files, right?

Yes.

Are you sure that tests and installation is passing/working on Cython 3? Its definitely not my recent experience, even on the Github Actions for this PR.

I've tested it locally by using the depends= approach for .pxi and .pxd files, and everything seems to pass (so the only issue with Cython3 seems that .pxi and .pxd files should not be placed into the sources list)

@cmacdonald
Copy link
Contributor Author

cmacdonald commented Sep 6, 2023

I have added depends= kwarg as requested. Github actions for this PR are passing.

I, respectfully, disagree with the assessment on Cython 3. I have separated that into a separate PR (#669), based on this PR, for which the github actions are failing.

@cmacdonald
Copy link
Contributor Author

Closing this PR in favour of #669.

@cmacdonald cmacdonald closed this Sep 13, 2023
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