Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean and harmonize module_list.py #11377

Closed
kiwifb opened this issue May 25, 2011 · 18 comments
Closed

Clean and harmonize module_list.py #11377

kiwifb opened this issue May 25, 2011 · 18 comments

Comments

@kiwifb
Copy link
Member

kiwifb commented May 25, 2011

module_list.py is quite messy in its current state. There are several things that can be done:

  1. remove the unused debian bits
  2. we have SAGE_LOCAL and SAGE_INC variables but there use iis not uniform
  3. we also have numpy_include_dirs, numpy_depends, flint_depends, singular_depends and ginac_depends and most of them are under-used.

In this ticket I remove old debian stuff, use SAGE_INC, SAGE_LOCAL and the other variables in a uniform fashion removing all instances of SAGE_ROOT +/local/... and so on to replace it by the appropriate variable.

Apply:

CC: @strogdon @robertwb

Component: build

Keywords: sd31

Author: François Bissey

Reviewer: Volker Braun

Merged: sage-4.7.1.alpha4

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

@kiwifb
Copy link
Member Author

kiwifb commented May 25, 2011

Attachment: trac_11377-build_module_listpy.patch.gz

patch to module_list.py

@kiwifb
Copy link
Member Author

kiwifb commented May 25, 2011

Author: François Bissey

@kiwifb
Copy link
Member Author

kiwifb commented May 27, 2011

comment:3

Now that 4.7.1.alpha0 is out this can be reviewed.

@vbraun
Copy link
Member

vbraun commented Jun 13, 2011

comment:4

Looks good to me. But in Sage-4.7.1.alpha2 there were some more extension modules added (under sage.groups.perm_gps.partn_ref). Can you updated your patch for that? I'll review it then asap.

@vbraun
Copy link
Member

vbraun commented Jun 13, 2011

Changed keywords from none to sd31

@kiwifb
Copy link
Member Author

kiwifb commented Jun 13, 2011

comment:6

It is up to date for 4.7.1.alpha2 already. The only possible problems could be with #9989 depending on which one is merged first and whether my suggested changes for it are accepted.

@vbraun
Copy link
Member

vbraun commented Jun 13, 2011

comment:7

The patch applies, but sage.groups.perm_gps.partn_ref.* don't use SAGE_INC and the flint_depends:


    Extension('sage.groups.perm_gps.partn_ref.automorphism_group_canonical_label',
              sources = ['sage/groups/perm_gps/partn_ref/automorphism_group_canonical_label.pyx'],
              libraries = ['gmp', 'flint'],
              include_dirs = [SAGE_ROOT + '/local/include/FLINT/'],
              extra_compile_args = ['-std=c99'],
              depends = [SAGE_ROOT + "/local/include/FLINT/flint.h"]),

@kiwifb
Copy link
Member Author

kiwifb commented Jun 13, 2011

comment:8

I thought I had taken care of that. My apologies it will take a few hours until I can take care of them.

@kiwifb
Copy link
Member Author

kiwifb commented Jun 14, 2011

Attachment: trac_11377-extraflint-dependencies.patch.gz

I missed a number of flint dependencies in the original patch. Apply trac_11377-build_module_listpy.patchafter

@kiwifb
Copy link
Member Author

kiwifb commented Jun 14, 2011

comment:9

It turns out I had missed another instance of flint in sage.set.disjoints.set. So the additional patch takes care of it. It was all done on top of alpha2, hopefully there is nothing wrecking it already in alpha3.

@kiwifb

This comment has been minimized.

@kiwifb
Copy link
Member Author

kiwifb commented Jun 14, 2011

comment:10

In the last patch description the word after shouldn't have come last. It is applied after trac_11377-build_module_listpy.patch.

@vbraun
Copy link
Member

vbraun commented Jun 14, 2011

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jun 14, 2011

comment:11

Looks great!

@jdemeyer
Copy link

Merged: sage-4.7.1.alpha4

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 18, 2011

comment:14

Would have been nice if you'd cc'ed me ;-) (as I have introduced SAGE_INC :-) and always been interested in cleaning the whole mess up, not to mention tickets like #8664, #9914, or the "final" trouble with #4000 ...).

Robert B. originally planned (about 14+ month ago, cf. e.g. #7987) to "automate" most of what's currently hand-coded in module_list.py (by Cython pragmas local to the modules' files), so I mostly made changes that affect only small parts.

Btw, I may have missed something, but neither alpha4 (nor alpha3) have yet been announced; the README.FIRSTs still state

There is no point in testing it because the version can change any time.

@kiwifb
Copy link
Member Author

kiwifb commented Jun 18, 2011

comment:15

Hi Robert,

sorry I didn't know you had introduced it. I was vaguely aware of the existence of an effort like #7987 but didn't know the ticket or that a ticket was actually open.

I must say my primary aim was the elimination of reference to SAGE_ROOT which is a pain for sage-on-gentoo. And of course once started I had to clean the whole thing, I have been waiting for some time with my clean up patch I can tell you. So I am glad it got reviewed positively and all.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 18, 2011

comment:16

Replying to @kiwifb:

And of course once started I had to clean the whole thing, I have been waiting for some time with my clean up patch I can tell you. So I am glad it got reviewed positively and all.

And above all merged! Making larger changes to module_list.py is some kind of Sisyphean task since many tickets (unrelated to the build system) frequently modify parts of it.

Reducing the need for the latter should IMHO be the next step, as it somewhat thwarts concurrent, modular development and maintainability.

I've more than once spent days to figure out and remove useless dependencies (e.g. on libraries) last year, in some cases just to - not much later - see the effort being more or less in vain...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants