Opened 2 years ago

Closed 2 years ago

#22728 closed enhancement (fixed)

Patch Cython to find includes better

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: ab97837 (Commits) Commit: ab97837ec3685789a54005fa725863a65866de24
Dependencies: Stopgaps:

Change History (14)

comment:1 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:2 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/patch_cython_to_find_includes_better

comment:3 Changed 2 years ago by jdemeyer

  • Commit set to ab97837ec3685789a54005fa725863a65866de24
  • Status changed from new to needs_review

New commits:

ab97837Cython: automatically add include_dirs for externs

comment:4 follow-up: Changed 2 years ago by vdelecroix

Do you know why nothing is happening on Cyhon side?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by jdemeyer

Replying to vdelecroix:

Do you know why nothing is happening on Cyhon side?

No.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Do you know why nothing is happening on Cyhon side?

No.

Do you think it is worth mentioning that it is a blocker for several projects on their mailing list?

comment:7 in reply to: ↑ 6 Changed 2 years ago by jdemeyer

Replying to vdelecroix:

Do you think it is worth mentioning that it is a blocker for several projects on their mailing list?

Done: https://mail.python.org/pipermail/cython-devel/2017-April/004995.html

comment:8 follow-up: Changed 2 years ago by jdemeyer

On the Cython mailing list, upstream answered

I'm in favor of the idea (basically attempting to resolve header files relative to the cython file including them and transitively passing these paths to modules cimporting them); haven't had time to look at the code yet or think if there could be negative implications.

So we could consider just patching Cython in Sage in order to move forward with #20238.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jdemeyer:

On the Cython mailing list, upstream answered

I'm in favor of the idea (basically attempting to resolve header files relative to the cython file including them and transitively passing these paths to modules cimporting them); haven't had time to look at the code yet or think if there could be negative implications.

So we could consider just patching Cython in Sage in order to move forward with #20238.

If you need the Sage patched version of Cython to install cypari it is not really independent of Sage...

comment:10 in reply to: ↑ 9 Changed 2 years ago by jdemeyer

Replying to vdelecroix:

If you need the Sage patched version of Cython to install cypari it is not really independent of Sage...

True, but that's still better than just keeping #20238 blocked.

comment:11 follow-up: Changed 2 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

IIUC Cypari will still install/work without this patch in principle--this is just needed for some specific use cases in integration with Sage. If it were really needed for cypari to work independently of Sage at the worst cypari could monkey-patch Cython. But considering that this patch is, as far as I can tell, pretty non-controversial and obvious, and is likely to be accepted upstream, I see no problem adding it in Sage as well at least for the sake of progressing on #20238.

comment:12 in reply to: ↑ 11 Changed 2 years ago by jdemeyer

Replying to embray:

IIUC Cypari will still install/work without this patch in principle--this is just needed for some specific use cases in integration with Sage.

To be specific: that patch is needed whenever some package (Sage in this case) wants to use the Cython interface of CyPari2. So you are right that CyPari2 by itself does not need that patch.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:13 Changed 2 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:14 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/patch_cython_to_find_includes_better to ab97837ec3685789a54005fa725863a65866de24
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.