Simplify cythonization of many sage extensions.

robertwb commented 10 years ago

No need to enumerate modules if we don't use special flags.

Component: build

Author: Robert Bradshaw, Jeroen Demeyer

Branch/Commit: 0681b66

Reviewer: Nathann Cohen, Jean-Pierre Flori

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

robertwb commented 10 years ago

Branch: u/robertwb/ticket/15410

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Commit: 94f5a9f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

[94f5a9f](https://github.com/sagemath/sagetrac-mirror/commit/94f5a9f) Simplify cythonization of sage/structure

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 94f5a9f to 5219984

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

`[5219984](https://github.com/sagemath/sagetrac-mirror/commit/5219984)`	`Simplify category, coding, and ext cythonization.`
`[708ee36](https://github.com/sagemath/sagetrac-mirror/commit/708ee36)`	`Simplify geometry, games, functions, crypto cythonization.`
`[d5bb198](https://github.com/sagemath/sagetrac-mirror/commit/d5bb198)`	`Simplify graphs, groups cythonization.`
`[ee6bff2](https://github.com/sagemath/sagetrac-mirror/commit/ee6bff2)`	`Simplify cythonization for media, matroids, and misc.`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 5219984 to 7caecd0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

[7caecd0](https://github.com/sagemath/sagetrac-mirror/commit/7caecd0) Trivial cythonization simplification.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:7

Hmmmm... Looks like the 'gmp' dependency in sage.quadratic_forms.count_local_2 was useless ?

Nathann

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 7caecd0 to 7703d9e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

[7703d9e](https://github.com/sagemath/sagetrac-mirror/commit/7703d9e) Add missing gmp library dependency.

robertwb commented 10 years ago

comment:9

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...

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

Reviewer: Nathann Cohen

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:10

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/Differential-Equations.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

jdemeyer commented 10 years ago

Author: Robert Bradshaw

vbraun commented 10 years ago

comment:13

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

8f46c63a-2596-4880-ba93-a09d8ba6c056 commented 10 years ago

comment:14

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).

robertwb commented 10 years ago

comment:15

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?

8f46c63a-2596-4880-ba93-a09d8ba6c056 commented 10 years ago

comment:16

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.

vbraun commented 10 years ago

comment:17

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.

8f46c63a-2596-4880-ba93-a09d8ba6c056 commented 10 years ago

comment:18

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.

robertwb commented 10 years ago

comment:19

I'm unable to reproduce this issue with a full rebuild (and your logs are gone). Are you still seeing this?

jpflori commented 10 years ago

comment:21

Should we go on with this ticket? That is use wildcards and move dependency tracking ot the cython file themselves?