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

Backport PEP420 namespace package support from Cython 3 #34221

Closed
mkoeppe opened this issue Jul 25, 2022 · 22 comments
Closed

Backport PEP420 namespace package support from Cython 3 #34221

mkoeppe opened this issue Jul 25, 2022 · 22 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 25, 2022

After #33011, downstream packages that use Cython and cimport Sage modules need to activate PEP 420 namespace package support in Cython using with cython_namespace_package_support (as is done in https://github.com/sagemath/sage-prod/blob/develop/src/setup.py#L106 for sagelib).

This feature is standard in the upcoming Cython 3 (#29863). Until downstream packages are updated, we can patch our Cython.

Upstream backport PR:

Depends on #34237

CC: @culler @tscrim @videlec @jhpalmieri

Component: packages: standard

Author: Matthias Koeppe

Branch/Commit: 48a9ba2

Reviewer: Marc Culler, John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/34221

@mkoeppe mkoeppe added this to the sage-9.7 milestone Jul 25, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

New commits:

7e72b2abuild/pkgs/cython/patches/4918.patch: New

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 25, 2022

Commit: 7e72b2a

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

176531abuild/pkgs/cython/package-version.txt: Add patchlevel

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2022

Changed commit from 7e72b2a to 176531a

@culler
Copy link
Contributor

culler commented Jul 26, 2022

comment:5

This solved all of the problems that I ran into with the p_group_cohomology and sage_numerical_backends_coin packages. The problems were caused by cython not finding the .pxd files that were needed for cimport statements. With this patch the cimports worked and I got a successful build of 9.7.beta6 on an arm64 mac with a minimal environment.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 26, 2022

comment:6

Thanks for testing!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

a2f798024237: upgrade; Cython 0.29.32
48a9ba2Merge #34237

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2022

Changed commit from 176531a to 48a9ba2

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 3, 2022

Dependencies: #34237

@jhpalmieri
Copy link
Member

comment:11

This works for me and also fixes the problems with sage_numerical_backends_coin. p_group_cohomology still fails for me ("ld: library not found for -lmodres"), for what that's worth.

@jhpalmieri
Copy link
Member

Reviewer: Marc Culler, John Palmieri

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 6, 2022

comment:12

Thanks!

@culler
Copy link
Contributor

culler commented Aug 8, 2022

comment:14

@jhpalmieri - the p_group_cohomology issue can be fixed by adding a symlink.
See this message on sage-devel.

@jhpalmieri
Copy link
Member

comment:15

Thank you, I added that link to the discussion at #30787.

@kiwifb
Copy link
Member

kiwifb commented Aug 9, 2022

comment:16

For my information, is this needed to build sage now? Or will it be needed once we split sagelib for real?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 9, 2022

comment:17

It's not needed for building sagelib, but it is needed for building other packages as described in the ticket description

@kiwifb
Copy link
Member

kiwifb commented Aug 9, 2022

comment:18

For downstream packages, yes. But I am assuming that once we split sagelib, the modules may need it too - but that's in the future.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 9, 2022

comment:19

The modularized packages such as sagemath-objects don't need it because they use with cython_namespace_package_support just like the monolithic sagelib does.

@kiwifb
Copy link
Member

kiwifb commented Aug 9, 2022

comment:20

Thank you for answering my pesky questions.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants