Opened 4 years ago

Closed 3 years ago

#21459 closed defect (fixed)

Fix building of sage_setup.autogen.interpreters output on Cygwin/Windows

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords:
Cc: tscrim, gouezel, jpflori Merged in:
Authors: Jeroen Demeyer Reviewers: Jean-Pierre Flori, Erik Bray
Report Upstream: N/A Work issues:
Branch: 18348bf (Commits) Commit: 18348bf417a6e05284e25803b28f394da2e90f28
Dependencies: #20730 Stopgaps:

Description (last modified by embray)

This adds a new file generated for each interpreter, called interp_<name>.h. Its purpose is to provide the correct __declspec(dllexport/import) on exported function definitions, so that they have the correct link-time behavior when building the interpreter wrappers themselves.

This is a workaround to #19868. Ideally this would be fixed directly in Cython so that such a workaround is not needed, but after exploring the issue it seems that for now providing such a workaround is easier than fixing Cython.

Explicit use of the interp_<name>.h headers replaces the implicit cdef extern: statements in the wrapper_<name>.pxd definition files. This ensures that the function declarations (with the correct __declspec attribute) are included from the .h files, instead of being written directly into Cython-generated C files. The end result has no visible differences for users of the library.

This currently depends on #20730 but doesn't strictly have to. It was just easier to understand the code and see what needed to be changed where after it was refactored.

Change History (49)

comment:1 Changed 4 years ago by embray

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by tscrim

  • Cc tscrim gouezel jpflori added

comment:3 Changed 4 years ago by cheuberg

  • Status changed from needs_review to needs_work

Does not merge with 7.5.rc2

comment:4 Changed 4 years ago by git

  • Commit changed from df25c5c1cd5b0fe2540480469e8a138ea6a1ba6d to 794519185206abd03356077eb9d6c39919c6ae74

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

9eb811fOne more whitespae fix
89b48beAdd some __main__ modules for ease of testing.
55853a7Just import everything into the top-level __init__ to rebuild the original sage_setup.autogen.interpreters namespace. This enures that all the tests pass.
9e5cf75Add from __future__ import print_function, absolute_import to all (non-empty) modules for forward compatibility.
45b2847Change the name attributes of the interpreter implementations to class
3530046Deduplicate code in anticipation of adding more generated file(s) for interpreters.
04c22e9More programmatic discovery of interpreter types rather than manually listing them
e44d5b0Write internally used interp_ header files and use them appropriately in the generated wrapper_.pyx files.
160a13cFix bugs in the tests
7945191Expected change in test

comment:5 Changed 4 years ago by embray

  • Milestone changed from sage-7.4 to sage-8.0
  • Status changed from needs_work to needs_review

Rebased--would be nice to get this fixed finally. This workaround is still perhaps not an ideal solution to the problem, but it's been working well for quite some time now on my Cygwin branch.

comment:6 Changed 4 years ago by embray

If anyone wants to review this it's really only the last 3 commits on this branch that matter. The rest is from #20730

Last edited 4 years ago by embray (previous) (diff)

comment:7 Changed 4 years ago by embray

  • Priority changed from minor to blocker

comment:8 Changed 4 years ago by jdemeyer

This issue keeps coming up, can't we just fix it in Cython? This is at least the third time that Sage needs a workaround for the same Cython on Cygwin bug (the one which causes the module_api test in Cython to fail, which may or may not be fixed by https://github.com/cython/cython/pull/360).

comment:9 Changed 4 years ago by embray

I've already explained multiple times here and there (I thought on this ticket but apparently not?) it's easier to fix this here, now, than to fix it properly in Cython. The PR you referenced does not fix this issue.

Last edited 4 years ago by embray (previous) (diff)

comment:10 Changed 4 years ago by jdemeyer

Yes, but fixing the same issue independently in Sage N times takes O(N) time while fixing it in Cython takes O(1) time. Although I'm not making any claim about the constants...

Anyway, let me look at the Cython issue, maybe I see something that you don't.

comment:11 Changed 4 years ago by embray

I don't think it's worth your time. AFAIK this is the only place in Sage now or any time soon this needs to be addressed again. I'm sure it can be fixed in Cython and am sure I could do it too, it was just annoying last I tried and this is much simpler. Please, this ticket has been open for 7 months and I'm tired of rebasing it. Just let it be...

Last edited 4 years ago by embray (previous) (diff)

comment:12 Changed 4 years ago by embray

You can open a separate issue for the general problem and assign it to me if you want, but I really don't think it's a high priority.

comment:13 Changed 4 years ago by jdemeyer

I don't want to be annoying, but this ticket depends on #20730 which depends on #20238, so it might not be merged soon anyway.

comment:14 follow-up: Changed 4 years ago by embray

What is holding up #20238? We could also invert that dependency considering that #20730 is much simpler.

comment:15 Changed 4 years ago by embray

The only reason that dependency exists in the first place is due to a minor merge conflict that can be resolved in either direction. I know you have good intentions in wanted to solve the problem generally and I agree, but it's not worth holding up any longer...

comment:16 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

Replying to embray:

What is holding up #20238?

Lack of reviewers.

comment:17 Changed 3 years ago by tscrim

Perhaps what we should ask is: Erik, are you willing to review #20238, and Jeroen, are you willing to review #20730? (Both in the near future.) Otherwise, I would say we should invert the dependency.

comment:18 Changed 3 years ago by jdemeyer

It is still my opinion that it would be better to fix this ticket in Cython instead of adding yet another workaround in Sage. So in this view, it doesn't actually depend on #20730 and #20238.

comment:19 Changed 3 years ago by embray

This is such a minor fix though--it took me like an hour to make at the most. Seven months ago. I feel like I've spent more time arguing about this than I spent working on the fix in the first place--that's why I'm frustrated. It would be easy to just merge this fix and then work on the "general" problem as time permits (the "general" problem, which I'll remind, currently does not affect any other code that I know of--as you suggested the constants here are not such that O(1) < O(N)).

comment:20 Changed 3 years ago by embray

I mean, Jeroen, if you think you can fix the issue in Cython obviously that's your prerogative and I'd welcome such a fix if it can be done in time for Sage 8.0. Otherwise I'm happy to help with that too but not until after Sage 8.0 is out. My main issue here is just getting all open Cygwin blockers fixed for 8.0, including this one, and I don't care how it's fixed. I just don't want to spend time on chasing a general fix right now when I already have this one. It's not like I actually care about merging this code for its own sake.

comment:21 Changed 3 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

Let's get this in as a temporary but simple workaround.

comment:22 Changed 3 years ago by jdemeyer

It seems that this needs to be rebased.

For the record, I guess I can fix the problem using https://github.com/cython/cython/pull/1650 and https://github.com/cython/cython/pull/1687 and with the following trivial patch:

  • src/sage_setup/autogen/interpreters.py

    diff --git a/src/sage_setup/autogen/interpreters.py b/src/sage_setup/autogen/interpreters.py
    index 31526e6..4f8774f 100644
    a b interp_{{ s.name }}(args 
    32673267
    32683268        w(je("""
    32693269# {{ warn }}
    3270 # distutils: sources = sage/ext/interpreters/interp_{{ s.name }}.c
    32713270{{ s.pyx_header }}
    32723271
    32733272include "cysignals/memory.pxi"
    cdef extern from "Python.h": 
    32863285
    32873286from sage.ext.fast_callable cimport Wrapper
    32883287
    3289 cdef extern:
     3288cdef extern from "|interp_{{ s.name }}.c":
    32903289    {{ myself.func_header(cython=true) -}}
    32913290{% if s.err_return != 'NULL' %}
    32923291 except? {{ s.err_return }}

(Only tested on Linux)

comment:23 Changed 3 years ago by embray

  • Status changed from positive_review to needs_work

comment:24 Changed 3 years ago by embray

What does the pipe symbol mean in cdef extern from "|interp_{{ s.name }}.c":? I don't recall seeing that syntax before.

comment:25 Changed 3 years ago by embray

Nevermind; answered my own question by actually reading your links.

comment:26 Changed 3 years ago by tscrim

Needs rebasing.

comment:27 Changed 3 years ago by embray

I know--I still want to test out Jeroen's idea for fixing this. In the short term I think taking this patch as-is is more likely, since his fix involves bikeshedding in Cython. But who knows--maybe we can come to a conclusion on that quickly.

comment:28 Changed 3 years ago by embray

I tried building with a Cython with Jeroen's two patches applied, and with his suggested patch above, and early signs are good for this solving the problem.

I'm still running full ptestlong just to be sure, but it compiled with no problems--bravo!

Assuming this works, the question remains what to do in the short term. There's still not a consensus on accepting Cython-1650. And even if it were accepted there's no consensus yet on the syntax.

1) We could accept my fix in the short term, if needed, and back it out later. But that's slightly annoying, especially since it adds new generated files.

2) Alternatively, we could patch Cython for now assuming that Jeroen's patch will be accepted at some point, but that's problematic for downstream support, especially considering the syntax uncertainties.

I prefer 1) for now because I don't like relying on patches, especially when it means building is impossible downstream without those patches, and further still given that it's a not completely insignificant patch (it's a new feature to Cython, not just a bug fix).

That said, I'm okay with either way.

comment:29 Changed 3 years ago by git

  • Commit changed from 794519185206abd03356077eb9d6c39919c6ae74 to 954772aa3f9f69ba5b986f64d7feed5a99a189cf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2029901Write internally used interp_ header files and use them appropriately in the generated wrapper_.pyx files.
d2f26fdFix bugs in the tests
954772aExpected change in test

comment:30 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Went ahead and rebased my existing branch, so that it's good to go if need be. But I'll await further feedback on how to move forward with this...

comment:31 Changed 3 years ago by jpflori

I agree with you that 1) is not very satisfactory but is the way to go before the discussion about cython settles down.

Could you use the :trac: syntax for http://trac.sagemath.org/ticket/19868 And mention the way 2) should fix the wrongly generated headers in the docstrings, including ticket numbers on cython tracker?

comment:32 Changed 3 years ago by embray

Sure, I'll fix that.

comment:33 Changed 3 years ago by jdemeyer

There are various changes on this patch which seem unrelated to Cygwin support: are those really needed? For example

  • src/sage_setup/autogen/interpreters/specs/rr.py

    diff --git a/src/sage_setup/autogen/interpreters/specs/rr.py b/src/sage_setup/autogen/interpreters/specs/rr.py
    index f4d3dc5..fe7a5cb 100644
    a b class RRInterpreter(StackInterpreter): 
    184183                        S=self.mc_stack,
    185184                        P=self.mc_py_constants)
    186185        self.pg = pg
     186        self.h_header = '#include <mpfr.h>'
    187187        self.c_header = ri(0,
    188188            '''
    189             #include <mpfr.h>
    190189            #include "interpreters/wrapper_rr.h"
    191190            ''')

For the record: I prefer the solution of patching Cython and hoping that upstream Cython will fix it. Especially because the Sage patch here is quite invasive (adding new auto-generated files).

I might be able to get my branch working without https://github.com/cython/cython/pull/1650 if that turns out to be problematic. But https://github.com/cython/cython/pull/1687 is really crucial for my approach to work.

comment:34 Changed 3 years ago by embray

I don't think you can get it working without PR 1650 either. I mean all power to you if you can, but I tried and failed some time ago. You know Cython's inner workings better than I do though.

The change you mention, however, was necessary for some reason. I just don't remember what it was because I worked on it, oh, 8 months ago. But I'm fine with dumping it if you can put together an alternative branch including the necessary patches to Cython. I'm still not super happy about the | syntax though, or the possibility of needing to change it later. Another problem is that it's being put in unconditionally, which really this is only a problem on Windows, and yet you'd force downstream packagers (e.g. on Debian) to accept this patch to Cython in order for Sage to build at all. So I don't think it's a good idea unless you can find some other workaround.

comment:35 Changed 3 years ago by jpflori

@Eric: could you try moving back the headers include and check it still work?

I would be very happy if Sage 8.0 could support Cygwin...

comment:36 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/cygwin/interpreters to u/jdemeyer/cygwin/interpreters

comment:37 Changed 3 years ago by jdemeyer

  • Commit changed from 954772aa3f9f69ba5b986f64d7feed5a99a189cf to f6cfbfc4f2f25aba8b8f405c1b383d975a25af86

This seems to work without needing any additional Cython patches...

It's still a slight hack, but at least a simple hack.


New commits:

f6cfbfcFix build of interpreters on Cygwin

comment:38 Changed 3 years ago by embray

Okay, I can see why this works. But while you call it simple (and it is, in terms of impact), it's not very explicit what it's doing or why. I don't mind that, if it works, for now, but it definitely needs more explanation about why it's needed and what it's doing (and under what circumstances it could eventually be removed).

comment:39 Changed 3 years ago by jdemeyer

I am correct when interpreting your comment as "I accept Jeroen's patch, provided that some documentation is added"?

comment:40 Changed 3 years ago by embray

Yes.

comment:41 Changed 3 years ago by git

  • Commit changed from f6cfbfc4f2f25aba8b8f405c1b383d975a25af86 to 3ed8029dd68701b38be6623d9a2556d9a8d24c04

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

3ed8029Expand comment

comment:42 Changed 3 years ago by jdemeyer

Ping?

comment:43 Changed 3 years ago by embray

Shouldn't it at least have a link back to this ticket? Or if nothing else, we should open a new ticket to not forget to implement a "better" fix if/when the needed capabilities are in Cython.

comment:44 Changed 3 years ago by git

  • Commit changed from 3ed8029dd68701b38be6623d9a2556d9a8d24c04 to 18348bf417a6e05284e25803b28f394da2e90f28

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

18348bfExpand expanded comment

comment:45 Changed 3 years ago by tscrim

Just doing commit pushes don't send trac e-mails unless you are in cc I've found.

comment:46 Changed 3 years ago by tscrim

(That was meant as a ping for Erik to note that Jeroen pushed another commit.)

comment:47 Changed 3 years ago by embray

  • Authors changed from Erik Bray to Jeroen Demeyer
  • Reviewers changed from Jean-Pierre Flori to Jean-Pierre Flori, Erik Bray
  • Status changed from needs_review to positive_review

comment:48 Changed 3 years ago by embray

Dashed off a ticket to remind to fix this later: #23258

comment:49 Changed 3 years ago by vbraun

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