Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#13031 closed enhancement (fixed)

Use cythonize() from cython for Sage module building.

Reported by: robertwb Owned by: GeorgSWeber
Priority: major Milestone: sage-5.9
Component: build Keywords: sd40.5
Cc: jdemeyer, roed, ohanar, ppurka, kini, mhansen Merged in: sage-5.9.beta4
Authors: Robert Bradshaw, R. Andrew Ohana Reviewers: Jeroen Demeyer, R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13029, #13432 Stopgaps:

Description (last modified by robertwb)

Cython's cythonize is now robust enough to replace our custom dependency logic, with a few extra features such as * syntax for cython extensions and cycache (still needs work).

Apply to the Sage library:

Attachments (12)

13031-cythonize.patch (32.7 KB) - added by robertwb 7 years ago.
13031-cythonize-5.1.beta1.patch (33.4 KB) - added by ohanar 7 years ago.
13031-cythonize-5.1.beta2.patch (129.5 KB) - added by ohanar 7 years ago.
trac13031.patch (138.3 KB) - added by ohanar 7 years ago.
apply to sage library
13031-cythonize-simple.patch (15.0 KB) - added by robertwb 7 years ago.
apply only this patch
trac13031-mutablity.patch (7.0 KB) - added by ohanar 7 years ago.
remove unused mutablity code
trac13031-cythonize-simple-5.8.beta4.patch (15.1 KB) - added by robertwb 7 years ago.
trac13031-cythonize-version.patch (1.2 KB) - added by robertwb 7 years ago.
trac13031-cythonize-simple.patch (15.0 KB) - added by ohanar 7 years ago.
apply to sage library
13031-doctest-fix.patch (1.9 KB) - added by robertwb 7 years ago.
13031-flush.patch (538 bytes) - added by robertwb 7 years ago.
13031-doctest-fix-rebased.patch (1.4 KB) - added by robertwb 6 years ago.

Download all attachments as: .zip

Change History (86)

Changed 7 years ago by robertwb

comment:1 Changed 7 years ago by robertwb

  • Cc jdemeyer roed ohanar ppurka added
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by ppurka

Is the patch based against sage-5.1beta0? I upgraded a sage installation from sage-5.0rc0 to sage-5.1beta0 and it fails to apply there. I will try a sage-5.1beta0 tarball sometime later.

comment:3 Changed 7 years ago by kini

  • Cc kini added

comment:4 Changed 7 years ago by mhansen

  • Cc mhansen added
  • Keywords sd40.5 added

comment:5 Changed 7 years ago by was

  • Status changed from needs_review to needs_work

Changing to needs work since -- as mentioned above -- this doesn't apply to sage-5.1.beta0:

adding 13031-cythonize.patch to series file
applying 13031-cythonize.patch
patching file module_list.py
Hunk #1 FAILED at 144
Hunk #3 FAILED at 223
Hunk #13 FAILED at 834
Hunk #16 FAILED at 1475
Hunk #20 succeeded at 1698 with fuzz 2 (offset 75 lines).
Hunk #21 succeeded at 1712 with fuzz 2 (offset 79 lines).
4 out of 22 hunks FAILED -- saving rejects to file module_list.py.rej
patching file setup.py
Hunk #1 succeeded at 17 with fuzz 2 (offset 0 lines).
Hunk #3 FAILED at 513
1 out of 3 hunks FAILED -- saving rejects to file setup.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 13031-cythonize.patch
wstein@sage:/tmp/wstein/sage-5.1.beta0-boxen-x86_64-Linux/devel/sage/sage$ 

comment:6 Changed 7 years ago by ohanar

Would you please make the following change:

  • (a) a vs. (b) b

    a b  
    1 ext_modules = cythonize(ext_modules, exclude=exclude_modules, nthreads=nthreads, cache=os.path.join(DOT_SAGE, 'cythoncache')))
     1if 'CYCACHE_DIR' in os.environ:
     2    CYCACHE_DIR = os.environ['CYCACHE_DIR']
     3else:
     4    CYCACHE_DIR = os.path.join(DOT_SAGE,'cycache')
     5ext_modules = cythonize(ext_modules, exclude=exclude_modules, nthreads=nthreads, cache=CYCACHE_DIR))

This way we can easily specify the cache directory separately from DOT_SAGE.

comment:7 Changed 7 years ago by robertwb

  • Status changed from needs_work to needs_review

Changed 7 years ago by ohanar

Changed 7 years ago by ohanar

comment:8 Changed 7 years ago by ohanar

  • Authors set to Robert Bradshaw, R. Andrew Ohana
  • Status changed from needs_review to needs_work

I attached an updated patch that

  • is rebased on the final 5.1.beta2
  • fixes a ZeroDivisionError that was preventing documentation from building
  • more throughly cleans up module_list.py with the use of *s
  • does other clean up with module_list.py since this patch will constantly need to be rebased with each development release (may as well clean up everything while we are at it)

That said, there is still some issue with Cython's cythonize, even using the old module_list.py you encounter the issue:

sage: cython('a = 5')
sage: a
Traceback (most recent call last):
...
NameError: name 'a' is not defined

You'll find that tons of doctests fail, and frequently they are having NameErrors like this (although they aren't necessarily calling Sage's cython function).

comment:9 Changed 7 years ago by robertwb

Thanks for looking at this. I fixed the patch, Sage relies on a deprecated "feature" of globals(). Running all tests...

comment:10 Changed 7 years ago by robertwb

FYI, all tests passed for me.

comment:11 Changed 7 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, looks good to me and works well with 5.1.beta2, so I'm going to mark this back as needing review.

As expected, it needs to be rebased again on more recent betas, but since the changes to module_list.py is so massive, I'm not going to bother with rebasing until Jeroen decides which beta he would like to merge it in.

For whoever reviews this, please use 5.1.beta2 as a basis.

Last edited 7 years ago by ohanar (previous) (diff)

comment:12 Changed 7 years ago by ohanar

The new patch fixes an issue with DOT_SAGE not being defined in setup.py.

comment:13 Changed 7 years ago by jhpalmieri

Rather than code like this:

if not os.path.exists(cache_dir): 
   os.mkdir(cache_dir) 

you should use sage.misc.misc.sage_makedirs (or copy-paste the code from there). A try-except block is safer than testing whether than the directory exists first.

comment:14 Changed 7 years ago by ohanar

  • Description modified (diff)

I just rebased on 5.2.beta1 since the dependency was merged into that release. However, in the process I discovered that cycache is not ready for primetime yet:

$ grep 'cdef class MonoDict:' sage -R
sage/sets/disjoint_set.c: * cdef class MonoDict:             # <<<<<<<<<<<<<<
sage/matrix/matrix_modn_dense_float.cpp: * cdef class MonoDict:             # <<<<<<<<<<<<<<

Neither of these modules changed between 5.1.beta2 and 5.2.beta1, but their dependencies changed, so there is some issue with the hashing that needs to be resolved.

For now I've disabled cycache, since it will be trivial to re-add once this ticket gets merged.

comment:15 follow-up: Changed 7 years ago by robertwb

Yes, I think it's perfectly fine to disable cycache for now to get this in (as it will then be much easier to work on). Could you expound on what the issue was though? I'm not quite following your grep is trying to show...

comment:16 in reply to: ↑ 15 Changed 7 years ago by ohanar

Replying to robertwb:

Yes, I think it's perfectly fine to disable cycache for now to get this in (as it will then be much easier to work on). Could you expound on what the issue was though? I'm not quite following your grep is trying to show...

The grep is of some cython code that existed in 5.1.beta2 in sage/structure/coerce_dict.pxd, that is no longer present in 5.2.beta1. However, cython detected that I had a cached version of the disjoint_set module (which didn't change between versions, but imports the coerce_dict module) and incorrectly pulls the the cached version of disjoint_set.c when it actually needs to rebuild it since coerce_dict.pxd changed (same situation for the other module in the grep).

comment:17 Changed 7 years ago by ohanar

OK, rebased. Be nice if someone reviewed this soon.

comment:18 follow-up: Changed 7 years ago by jdemeyer

This probably should be rebased to #13164 and #12883.

comment:19 in reply to: ↑ 18 Changed 7 years ago by ohanar

Replying to jdemeyer:

This probably should be rebased to #13164 and #12883.

I'll do that once they are merged. This ticket so heavily affects module_list.py that I would rather get this reviewed on some clean development build, and then try to figure out with you what tickets I'll need to rebase against to merge this into whatever beta you feel is appropriate. Since this has been waiting review for a bit, I rather not jump the gun and start rebasing against things that haven't been merged yet.

comment:20 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to Sphinx

This seems to cause Sphinx problems:

/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.clear_mpz_globals: Could not parse cython argspec
/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.gmp_randrange: Could not parse cython argspec
/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.init_mpz_globals: Could not parse cython argspec

comment:21 follow-up: Changed 7 years ago by ohanar

Can you please clarify what command you ran?

comment:22 in reply to: ↑ 21 Changed 7 years ago by jdemeyer

Replying to ohanar:

Can you please clarify what command you ran?

This is build from scratch, so it's just make doc. This is probably equivalent to removing the doc/output directory and then doing make doc.

Changed 7 years ago by ohanar

apply to sage library

comment:23 Changed 7 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, rebased on 5.4.beta1.

The issue with sphinx seems to be because of the python functions defined in sage/ext/gmp.pxi. We probably shouldn't have any python function defined in pxi files anyway (since we currently aren't checking them for coverage or anything). There are python functions in other pxi files, but they aren't causing any issues for some reason (Robert, maybe you could explain why?).

Anyway, for right now I removed one of them (that wasn't being used anywhere), and I moved the other two to sage/rings/integer.pyx since that is where the sage library looks for them.

Changed 7 years ago by robertwb

apply only this patch

comment:24 Changed 7 years ago by robertwb

  • Description modified (diff)

OK, I've changed this to only switch to use cythonize and get rid of the sphinx errors. All the other changes make this fail to apply with each new Sage release, and it's a pain to constantly re-base. Once this is in, we can fix up the module list piecemeal and do other cleanup (though I'm unconvinced writing os.path.join('sage','rings','padics','*.pyx') is really preferable to "sage/rings/padics/*.pyx") as was introduced in the latest rebase.

comment:25 Changed 7 years ago by ohanar

  • Reviewers set to Jeroen Demeyer, R. Andrew Ohana
  • Status changed from needs_review to positive_review
  • Work issues Sphinx deleted

Everything looks good and works well.

Last edited 7 years ago by ohanar (previous) (diff)

comment:26 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There is trouble with upgrading:

On rosemary (Linux RHEL 5.6 x86_64), after upgrading from sage-4.5.2, I get a failure during the installation of the Conway spkg:

Traceback (most recent call last):
  File "./spkg-install", line 4, in <module>
    from sage.all import save
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/all.py", line 74, in <module>
    from sage.matrix.all     import *
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/matrix/all.py", line 1, in <module>
    from matrix_space import MatrixSpace, is_MatrixSpace
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/matrix/matrix_space.py", line 33, in <module>
    import matrix
  File "matrix.pyx", line 1, in init sage.matrix.matrix (sage/matrix/matrix.c:2043)
  File "matrix2.pyx", line 1, in init sage.matrix.matrix2 (sage/matrix/matrix2.c:71233)
  File "matrix1.pyx", line 1, in init sage.matrix.matrix1 (sage/matrix/matrix1.c:13429)
  File "mutability.pxd", line 15, in init sage.matrix.matrix0 (sage/matrix/matrix0.c:29523)
ValueError: PyCapsule_GetPointer called with invalid PyCapsule object

Running ./sage -ba-force fixes this.

comment:27 Changed 7 years ago by jdemeyer

Are old-style Cython modules and new-style Cython modules (made with cythonize()) compatible? If yes, the problem is probably with dependency checking (some modules should be rebuilt but aren't).

comment:28 Changed 7 years ago by jdemeyer

Thanks to some sed magic, this is a list of modules rebuilt in sage-5.7.beta2 but which were not rebuilt with this ticket:

sage/combinat/words/word_datatypes.pyx
sage/ext/interactive_constructors_c.pyx
sage/games/sudoku_backtrack.pyx
sage/media/channels.pyx
sage/misc/refcount.pyx
sage/misc/search.pyx
sage/quadratic_forms/quadratic_form__evaluate.pyx
sage/rings/polynomial/symmetric_reduction.pyx
sage/structure/mutability.pyx

In the sage-5.7.beta2 log file, there is

Building modified file sage/combinat/words/word_datatypes.pyx.
Building modified file sage/ext/interactive_constructors_c.pyx.
Building modified file sage/games/sudoku_backtrack.pyx.
Building modified file sage/media/channels.pyx.
Building modified file sage/misc/refcount.pyx.
Building modified file sage/misc/search.pyx.
Building modified file sage/quadratic_forms/quadratic_form__evaluate.pyx.
Building modified file sage/rings/polynomial/symmetric_reduction.pyx.
Building modified file sage/structure/mutability.pyx.

In the log file with #13031, there is no mention of any of these files.

comment:29 Changed 7 years ago by jdemeyer

Files related to mutability.pyx after building Sage with #13031:

-rwxr-xr-x 1 buildbot buildbot 52116 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/lib.linux-x86_64-2.7/sage/structure/mutability.so
-rwxr-xr-x 1 buildbot buildbot 52116 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/sage/structure/mutability.so
-rw-r--r-- 1 buildbot buildbot 68728 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/temp.linux-x86_64-2.7/sage/structure/mutability.o
-rw-r--r-- 1 buildbot buildbot   207 2012-08-23 12:00:08.000000000 -0400 ./devel/sage-main/doc/en/reference/sage/structure/mutability.rst
-rw-r--r-- 1 buildbot buildbot 71416 2012-08-23 11:33:21.000000000 -0400 ./devel/sage-main/sage/structure/mutability.c
-rw-r--r-- 1 buildbot buildbot   529 2010-06-28 12:37:01.000000000 -0400 ./devel/sage-main/sage/structure/mutability.pxd
-rw-r--r-- 1 buildbot buildbot  2062 2012-08-23 11:22:27.000000000 -0400 ./devel/sage-main/sage/structure/mutability.pyx

comment:30 Changed 7 years ago by jdemeyer

The .so is recompiled from an old Cython C file which was

/* Generated by Cython 0.12.1 on Thu Aug 23 11:33:21 2012 */

while the old code would always run cython to re-generate the .c file.

Sounds like #4797...

comment:31 Changed 7 years ago by jdemeyer

Alternatively, we could also disallow upgrades from versions older than sage-4.6. On the other hand, that would probably not be sufficient as some .c files generated by newer versions of Cython would need to be recreated to account for #13896 for example.

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

comment:32 follow-up: Changed 7 years ago by ohanar

So from what I can tell cythonize is in the right -- these files don't need to be rebuilt (they nor any of their dependencies have been changed). So it looks like it is an issue of using old cython generated code with new cython generated code.

The other potential fix for this particular issue is to remove sage.structure.mutablity, since nothing actually uses it (despite getting imported in a few places).

comment:33 Changed 7 years ago by ohanar

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

Changed 7 years ago by ohanar

remove unused mutablity code

comment:34 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:35 in reply to: ↑ 32 Changed 7 years ago by jdemeyer

Replying to ohanar:

So from what I can tell cythonize is in the right -- these files don't need to be rebuilt (they nor any of their dependencies have been changed).

It depends what you mean. One could argue that all Cython files should implicitly depend on the Cython program. Similar how, in an autotools build system, everything depends on config.status (the output of ./configure) or similar how SCons and ccache use the compiler executable as extra dependency of all C files.

comment:36 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Did you fix the issue of C files not being regenerated by a Cython upgrade? Because whether it's the "fault" of Cython or not, it needs to be fixed.

comment:37 in reply to: ↑ 36 ; follow-ups: Changed 7 years ago by ohanar

Replying to jdemeyer:

Did you fix the issue of C files not being regenerated by a Cython upgrade?

No, I removed the single offending orphaned code, all the other code that is not regenerated causes no issues (since they are they are isolated from all of the modified files). The proper way to have this resolved is to have Cython's dependency checking not just check the timestamp of a file but also what version of Cython was used to generate the C file -- but I would rather not have this ticket held up on a future version of Cython.

From what I can tell, the current DependancyTree only works because it is finding false positives, not because it is checking the Cython version of generated files. So until I find the time to look at fixing this in Cython, I think the correct resolution is to implement #4797.

Changed 7 years ago by robertwb

Changed 7 years ago by robertwb

comment:38 Changed 7 years ago by robertwb

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

OK, I've added a bit of code to cache and check the version.

comment:39 Changed 7 years ago by robertwb

  • Description modified (diff)

Apply only trac13031-cythonize-simple-5.8.beta4.patch trac13031-mutablity.patch trac13031-cythonize-version.patch

comment:40 in reply to: ↑ 37 Changed 7 years ago by jdemeyer

Replying to ohanar:

all the other code that is not regenerated causes no issues

Really???????????????????????????????? What about the bug that Cython-0.17.4 fixed?

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

comment:41 in reply to: ↑ 37 Changed 7 years ago by jdemeyer

Replying to ohanar:

From what I can tell, the current DependancyTree only works because it is finding false positives, not because it is checking the Cython version of generated files.

True, but at least it works.

comment:42 Changed 7 years ago by ohanar

  • Status changed from needs_review to positive_review

Ok, Robert's patch looks good to me.

comment:43 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.9

comment:44 Changed 7 years ago by jdemeyer

  • Dependencies changed from #13029 to #13029, #13432
  • Status changed from positive_review to needs_work

This needs to be rebased to #13432.

Changed 7 years ago by ohanar

apply to sage library

comment:45 Changed 7 years ago by ohanar

  • Description modified (diff)
  • Status changed from needs_work to positive_review

I thought I had rebased this, oh well, it should now apply cleanly on top of #13432.

comment:46 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work
Traceback (most recent call last):
  File "./spkg-install", line 4, in <module>
    from sage.all import save
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/all.py", line 80, in <module>
    from sage.misc.all       import *         # takes a while
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/misc/all.py", line 83, in <module>
    from functional import (additive_order,
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/misc/functional.py", line 36, in <module>
    from sage.rings.complex_double import CDF
  File "complex_double.pyx", line 91, in init sage.rings.complex_double (sage/rings/complex_double.c:16796)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 110, in ComplexField
    C = ComplexField_class(prec)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 204, in __init__
    self._populate_coercion_lists_(coerce_list=[complex_number.RRtoCC(self._real_field(), self)])
  File "complex_number.pyx", line 2431, in sage.rings.complex_number.RRtoCC.__init__ (sage/rings/complex_number.c:15890)
  File "map.pyx", line 125, in sage.categories.map.Map.__init__ (sage/categories/map.c:2478)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/categories/homset.py", line 263, in Hom
    H = category.hom_category().parent_class(X, Y, category = category)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/categories/rings.py", line 636, in __new__
    from sage.rings.homset import RingHomset
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/homset.py", line 17, in <module>
    import morphism
  File "morphism.pyx", line 985, in init sage.rings.morphism (sage/rings/morphism.c:12304)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/structure/all.py", line 49, in <module>
    from mutability  import Mutability
ImportError: No module named mutability

comment:47 Changed 7 years ago by robertwb

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

Apply only trac13031-cythonize-simple.patch and trac13031-cythonize-version.patch

We can delete mutability in a separate CL, now that we have Cython version tracking.

comment:48 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work
sage -t devel/sage/sage/misc/dev_tools.py
**********************************************************************
File "devel/sage/sage/misc/dev_tools.py", line 145, in sage.misc.dev_tools.import_statements
Failed example:
    import_statements(ZZ)
Expected:
      ** Warning **: several names for that object: ZZ, Z
    from sage.rings.integer_ring import ZZ
Got:
      ** Warning **: several names for that object: Z, ZZ
    from sage.rings.integer_ring import Z
**********************************************************************
File "devel/sage/sage/misc/dev_tools.py", line 158, in sage.misc.dev_tools.import_statements
Failed example:
    import_statements(ZZ, verbose=False)
Expected:
    from sage.rings.integer_ring import ZZ
Got:
    from sage.rings.integer_ring import Z
**********************************************************************

Changed 7 years ago by robertwb

comment:49 Changed 7 years ago by robertwb

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

comment:50 Changed 7 years ago by jdemeyer

I think you are missing some sys.stdout.flush() statements in cythonize(), as the message

Cythonizing sage/categories/category_singleton.pyx

might come after

gcc -fno-strict-aliasing -std=gnu99 -D_XPG6 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include/csage -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/devel/sage/sage/ext -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include/python2.7 -c sage/categories/category_singleton.c -o build/temp.solaris-2.11-i86pc.32bit-2.7/sage/categories/category_singleton.o -w

which is very confusing, especially when errors occur.

(there is nothing special about this file, it's just an example)

comment:51 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Changed 7 years ago by robertwb

comment:53 Changed 7 years ago by robertwb

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

Granted, there's no reason (other than luck) that we couldn't have hit this before just as well.

comment:54 Changed 6 years ago by ohanar

  • Status changed from needs_review to positive_review

comment:55 Changed 6 years ago by jdemeyer

The file sage/schemes/generic/notes/divisor_stein-joyner.txt is removed in #13062, so 13031-doctest-fix.patch needs to be rebased.

Changed 6 years ago by robertwb

comment:56 Changed 6 years ago by robertwb

  • Description modified (diff)

comment:57 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.9.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:58 Changed 6 years ago by jdemeyer

  • Merged in changed from sage-5.9.beta3 to sage-5.9.beta4

comment:59 Changed 6 years ago by jdemeyer

Robert: any plans to add the version checking upstream to cythonize()?

comment:60 Changed 6 years ago by jdemeyer

I just found some important bug: it seems the dependencies on the files in c_lib/ are no longer correctly handled.

$ touch devel/sage/c_lib/include/interrupt.h && ./sage -b

should rebuild all files depending on interrupt.h but this doesn't work anymore.

In sage-5.9.beta3, one gets instead lots of lines like

Building sage/libs/ecl.pyx because it depends on /mazur/release/merger/sage-5.9.beta3/local/include/csage/interrupt.h.

comment:61 Changed 6 years ago by robertwb

I'm on it.

comment:62 Changed 6 years ago by jdemeyer

Any progress?

comment:63 Changed 6 years ago by robertwb

The fix will be in Cython 0.19.

comment:64 Changed 6 years ago by robertwb

(We could backport it if that takes too long.)

comment:65 Changed 6 years ago by jdemeyer

Thanks. Anyway, good to know it's a Cython bug, not Sage bug.

comment:66 Changed 6 years ago by jdemeyer

Proposal: unmerge this for sage-5.9, merge in again in sage-5.10 together with #14452.

comment:67 Changed 6 years ago by jdemeyer

(or possibly merge #14452 in sage-5.9.rc0)

comment:68 Changed 6 years ago by robertwb

Alternative proposal: cython-0.18.p1. I really would not like to see this unmerged after it finally got in.

comment:69 Changed 6 years ago by jdemeyer

Given that cython-0.19 is released today and we have a workaround for the docbuilder crash at #14452, why not cython-0.19?

comment:70 follow-up: Changed 6 years ago by jdemeyer

There are still serious problems with dependency checking: #14544

comment:71 in reply to: ↑ 70 ; follow-up: Changed 6 years ago by leif

Replying to jdemeyer:

There are still serious problems with dependency checking: #14544

There are also reports on sage-devel that sage -clone was broken in 5.9 (and later), now rebuilding the whole Sage library. Not sure what exactly introduced that though.

comment:72 in reply to: ↑ 71 ; follow-up: Changed 6 years ago by leif

Replying to leif:

There are also reports on sage-devel that sage -clone was broken in 5.9 (and later), now rebuilding the whole Sage library. Not sure what exactly introduced that though.

Looks like just the fact that sage-clone doesn't copy $SAGE_SRC/.cython_version causes this...

(where $SAGE_SRC is [meanwhile] $SAGE_ROOT/devel/sage)


Also (but this doesn't seem to be the cause), sage-clone does not (or no longer, presumably at least since a while) copy Cython-generated header files (*.h), as these do not contain a comment on the first line stating that they were generated by Cython, so the following doesn't work for them:

print "Copying over all Cython auto-generated .c, .cpp and .h files..."
def cpdir(src, dest):
    if not os.path.isdir(dest):
        return
    for F in os.listdir(src):
        if os.path.isdir(src + '/' + F):
            cpdir(src + '/' + F, dest + '/' + F)
        else:
            ext = os.path.splitext(F)[-1]
            if ext in ['.h', '.c', '.cpp']:
                if 'Cython' in open(src + '/' + F).readline():
                    os.link(src + '/' + F, dest + '/' +F)
                    os.utime(dest + '/' +F, None)

cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))

comment:73 in reply to: ↑ 72 ; follow-up: Changed 6 years ago by leif

Replying to leif:

Also (but this doesn't seem to be the cause), sage-clone does not (or no longer, presumably at least since a while) copy Cython-generated header files (*.h), as these do not contain a comment on the first line stating that they were generated by Cython, so the following doesn't work for them:

print "Copying over all Cython auto-generated .c, .cpp and .h files..."
def cpdir(src, dest):
    if not os.path.isdir(dest):
        return
    for F in os.listdir(src):
        if os.path.isdir(src + '/' + F):
            cpdir(src + '/' + F, dest + '/' + F)
        else:
            ext = os.path.splitext(F)[-1]
            if ext in ['.h', '.c', '.cpp']:
                if 'Cython' in open(src + '/' + F).readline():
                    os.link(src + '/' + F, dest + '/' +F)
                    os.utime(dest + '/' +F, None)

cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))

Furthermore, the above code doesn't copy the Cython-generated files from devel/sage/ext/interpreters/, so these also get rebuilt, but that goes back to at least Sage 5.8 (unless I'm missing something).


If I add copying .cython_version to sage-clone, besides the above-mentioned re-"cythonizing"[sic!], many (if not all?) Python modules get re-byte-compiled, too, for whatever reason... (This doesn't seem to be related to Cython, but maybe I'm just missing something here as well.)

comment:74 in reply to: ↑ 73 Changed 6 years ago by leif

Replying to leif:

If I add copying .cython_version to sage-clone ...

  • sage-clone

    diff --git a/sage-clone b/sage-clone
    a b  
    5353
    5454cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))
    5555
     56if os.path.isfile('sage/.cython_version'):
     57    print "Copying over hidden Cython version file..."
     58    os.link('sage/.cython_version', branch+'/.cython_version')
     59
    5660def copy_dtree(src_dir, dest_dir):
    5761    src_root = os.path.abspath(src_dir)
    5862    dest_root = os.path.abspath(dest_dir)
Note: See TracTickets for help on using tickets.