Opened 8 years ago
Closed 6 years ago
#15410 closed enhancement (fixed)
Simplify cythonization of many sage extensions.
Reported by:  robertwb  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  build  Keywords:  
Cc:  Merged in:  
Authors:  Robert Bradshaw, Jeroen Demeyer  Reviewers:  Nathann Cohen, JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  0681b66 (Commits, GitHub, GitLab)  Commit:  0681b66835d257628a3d2f08a8eadad12d21e846 
Dependencies:  Stopgaps: 
Description (last modified by )
No need to enumerate modules if we don't use special flags.
Change History (31)
comment:1 Changed 8 years ago by
 Branch set to u/robertwb/ticket/15410
 Created changed from 11/13/13 08:20:06 to 11/13/13 08:20:06
 Modified changed from 11/13/13 08:20:06 to 11/13/13 08:20:06
comment:2 Changed 8 years ago by
 Commit set to 94f5a9ffdfd0b3784f75cbac46eef382d7273ea4
comment:3 Changed 8 years ago by
 Status changed from new to needs_review
comment:4 Changed 8 years ago by
 Commit changed from 94f5a9ffdfd0b3784f75cbac46eef382d7273ea4 to 52199847e6ece596afd1ed00bc537cb4c6bb217f
comment:5 Changed 8 years ago by
 Commit changed from 52199847e6ece596afd1ed00bc537cb4c6bb217f to 7caecd0ddf6973cb7267f46c2d51198685286ce7
Branch pushed to git repo; I updated commit sha1. New commits:
7caecd0  Trivial cythonization simplification. 
comment:6 Changed 8 years ago by
 Summary changed from Simplify cythonization of sage/structure. to Simplify cythonization of many sage extensions.
comment:7 Changed 8 years ago by
Hmmmm... Looks like the 'gmp' dependency in sage.quadratic_forms.count_local_2
was useless ?
Nathann
comment:8 Changed 8 years ago by
 Commit changed from 7caecd0ddf6973cb7267f46c2d51198685286ce7 to 7703d9e6accea302f46d66200b0b57a5ae5a9446
Branch pushed to git repo; I updated commit sha1. New commits:
7703d9e  Add missing gmp library dependency. 
comment:9 Changed 8 years ago by
Eventually this will be inferred from the cimports (e.g. of integer_ring). There's a mess of includes and pxi files that needs to be sorted out before that can happen...
comment:10 Changed 8 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
Hmmmmmm... Well I applied your three patches, deleted the build/ directory, built Sage again and passes all long tests. I got the following errors, and I believe that none of them comes from your commits
 sage t long misc/trace.py # 2 doctests failed sage t long misc/interpreter.py # 1 doctest failed sage t long combinat/sf/sfa.py # 1 doctest failed sage t long calculus/desolvers.py # 8 doctests failed sage t long libs/symmetrica/symmetrica.pxi # 1 doctest failed sage t long dev/sagedev.py # 5 doctests failed sage t long dev/patch.py # 6 doctests failed sage t long tests/cmdline.py # 14 doctests failed sage t long tests/interrupt.pyx # Time out sage t long /home/ncohen/.Sage/src/doc/en/constructions/calculus.rst # 4 doctests failed sage t long /home/ncohen/.Sage/src/doc/en/prep/Quickstarts/DifferentialEquations.rst # 2 doctests failed  Total time for all tests: 7355.7 seconds cpu time: 4916.7 seconds cumulative wall time: 5432.8 seconds
Soooooooo well. I'd say "good to go" :)
Nathann
comment:11 Changed 8 years ago by
 Milestone changed from sage5.13 to sage6.0
comment:12 Changed 8 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:13 Changed 8 years ago by
 Status changed from positive_review to needs_work
I'm having problems with this ticket, it works on an incremental build but not in a full rebuild. Dies in conway polynomials: http://build.sagemath.org/sage/builders/%20%20fast%20Volker%20Desktop%20%28Fedora%2019%20x86_64%29%20full/builds/5/steps/compile_1/logs/conway
comment:14 Changed 8 years ago by
while there is no *need* to enumerate files, the proposed wildcarding has been flagged as bad practice outside the sage/distutils world. see http://www.gnu.org/software/automake/manual/html_node/Wildcards.html for an explanation. or just view the explicit listing of files as an extra integrity check similar to docchecks. you get the idea...
python distutils requires a lot of overhead in file lists, which is bad. but wildcards are worse.
this ticket does not improve anything. better leave it like that (YMMV).
comment:15 followup: ↓ 16 Changed 8 years ago by
From your link:
Still, these are philosophical objections, and as such you may disagree, or find enough value in wildcards to dismiss all of them. Before you start writing a patch against Automake to teach it about wildcards, let’s see the main technical issue: portability.
You can guess where I fall :).
Portability is not an issue here. The main downside is someone might forget to add/commit the file, but here this is no worse than we are with .py files, and I would say that this inconsistency is bad (.pyx files should be as similar to .py files as possible). Would you propose enumerating the .py files somewhere as an enhancement?
comment:16 in reply to: ↑ 15 Changed 8 years ago by
Replying to robertwb:
Would you propose enumerating the .py files somewhere as an enhancement?
yes.
when i implemented the make/autotools based build system (#15039), i started off with wildcards (which are not a portability issue, since GNU make is a requirement in various other places). only later, i considered integrity/sanity more important than having random files processed/installed/distributed. so I switched over to explicit lists.
unlike distutils, autotools provides some integrity checks ('distcheck') out of the box, i didn't want to miss them. i cant help to note that enhancing/replacing distutils would be far superior to getting rid of the existing file lists.
comment:17 Changed 8 years ago by
The main reason for autotools requiring explicit file lists is that there is no reliable introspection in C/C++. This is very different in Python. There are certainly pros and cons to wildcards, but just because autotools is doing it doesn't count as an argument IMHO.
comment:18 Changed 8 years ago by
autotools (in combination with GNU make) works with wildcards, but it will not end up smarter than distutils with wildcards. reliable introspection should not be used to decide which files belong to a project.
sage (the library) has tests for every implemented function, no matter how trivisl it is. not checking whether all files are present does not seem to fit. if you are not convinced, go ahead and delete the lists.
comment:19 Changed 8 years ago by
I'm unable to reproduce this issue with a full rebuild (and your logs are gone). Are you still seeing this?
comment:20 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:21 Changed 7 years ago by
Should we go on with this ticket? That is use wildcards and move dependency tracking ot the cython file themselves?
comment:22 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:23 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:24 Changed 6 years ago by
 Description modified (diff)
comment:25 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.6
 Type changed from defect to enhancement
comment:26 Changed 6 years ago by
 Description modified (diff)
 Milestone changed from sage6.6 to sage6.8
I'm rebooting this ticket to handle only the trivial cases (where no additional libraries or compiler flags need to be added).
comment:27 Changed 6 years ago by
 Branch changed from u/robertwb/ticket/15410 to u/jdemeyer/ticket/15410
comment:28 Changed 6 years ago by
 Commit changed from 7703d9e6accea302f46d66200b0b57a5ae5a9446 to 0681b66835d257628a3d2f08a8eadad12d21e846
 Status changed from needs_work to needs_review
New commits:
0681b66  Simplify cythonization of many Cython extensions

comment:29 Changed 6 years ago by
 Reviewers changed from Nathann Cohen to Nathann Cohen, JeanPierre Flori
 Status changed from needs_review to positive_review
comment:30 Changed 6 years ago by
See #15412 for a very similar ticket.
comment:31 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/15410 to 0681b66835d257628a3d2f08a8eadad12d21e846
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits: