Opened 9 years ago

Closed 8 years ago

#11376 closed enhancement (fixed)

Remove the hardcoding of python version in setup.py and SConstruct to build sage_clib and sage itself

Reported by: fbissey Owned by: GeorgSWeber
Priority: major Milestone: sage-4.7.2
Component: build Keywords:
Cc: strogdon, leif Merged in: sage-4.7.2.alpha0
Authors: François Bissey Reviewers: Mariah Lenox
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11373 Stopgaps:

Description (last modified by fbissey)

Currently to build sage (the spkg) four files are involved in major way:

  1. SConstuct to build sage-clib
  2. setup.py
  3. module_list.py
  4. sage/misc/cython.py

The last three to build the sage python extensions. setup.py, and SConstruct both have the version of python used hardcoded in them, module_list.py doesn't refer to the python version anywhere. cython.py mention the python version explicitly in doctest but smartly find the version used for building all by itself.

In this ticket I bring this smartness to SConstruct and setup.py. The advantage of doing this is that sage then can be build with either python-2.6 or 2.7 (and 3.x in the future) without patching. Of course it may still require patches to fix doctest and syntax in some parts - but you would be able to build sage in the first place without having to patch the build system. It has been found to be an impediment to testing in #9958.

Depend: #11373

Apply:

Attachments (8)

trac_11376-build-sage_clib.patch (2.0 KB) - added by fbissey 9 years ago.
Remove the python version hardcoding in SConstruct, remove debian cruft as well.
trac_11376-build-sage_clib-4.7.patch (2.0 KB) - added by fbissey 9 years ago.
a version of this patch that works with 4.7.rc4
trac_11376-build_setuppy.patch (3.8 KB) - added by fbissey 9 years ago.
Make setup.py python version smart, remove debian cruft. (made dependent on #11373)
trac_11376-SConstruct.patch (2.1 KB) - added by fbissey 9 years ago.
updated 20110603 #2
trac_11376-setuppy.patch (4.1 KB) - added by fbissey 9 years ago.
updated 20110603 take 2
trac_11376-msic_cythonpy.patch (1.6 KB) - added by fbissey 9 years ago.
updated 20110603 take 2
trac_11376-fix_cythonpy_doctest.patch (983 bytes) - added by fbissey 9 years ago.
fix the cython.py doctest for the appearance on numpy
trac_11376-msic_cythonpy-p2.patch (1.6 KB) - added by fbissey 9 years ago.
Patch to misc/cython.py with the right number of slashes

Download all attachments as: .zip

Change History (40)

Changed 9 years ago by fbissey

Remove the python version hardcoding in SConstruct, remove debian cruft as well.

comment:1 Changed 9 years ago by fbissey

  • Authors set to François Bissey

I also remove the Debian cruft from SConstruct and setup.py at the same time.

comment:2 Changed 9 years ago by fbissey

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by strogdon

  • Cc strogdon added

comment:4 Changed 9 years ago by mariah

  • Status changed from needs_review to needs_work

patch trac_11376-build-sage_clib.patch failed to apply to sage-4.7.rc4:

sage: hg_sage.apply("/home/mariah/trac_11376-build-sage_clib.patch")
cd "/home/mariah/sage/sage-4.7.rc4-x86_64-Linux-core2-fc/devel/sage" && hg status
cd "/home/mariah/sage/sage-4.7.rc4-x86_64-Linux-core2-fc/devel/sage" && hg status
cd "/home/mariah/sage/sage-4.7.rc4-x86_64-Linux-core2-fc/devel/sage" && hg import   "/home/mariah/trac_11376-build-sage_clib.patch"
applying /home/mariah/trac_11376-build-sage_clib.patch
patching file c_lib/SConstruct
Hunk #3 FAILED at 133
1 out of 3 hunks FAILED -- saving rejects to file c_lib/SConstruct.rej
abort: patch failed to apply
sage:

comment:5 follow-up: Changed 9 years ago by fbissey

Thanks for looking at this Mariah. It doesn't apply because it was based on 4.7.1.alpha1. It was based on 4.7.1.alpha1 because of #11167 I cannot produce a patch that will work equally on 4.7.rc and 4.7.1.alpha. That's unfortunate but such is life. In fact I may have to rebase trac_11376-build_setuppy.patch because of #11373 in 4.7.1.alpha2.

Changed 9 years ago by fbissey

a version of this patch that works with 4.7.rc4

comment:6 Changed 9 years ago by fbissey

Just in case you still want to test it against 4.7.rc4 use this patch trac_11376-build-sage_clib-4.7.patch instead.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by mariah

  • Status changed from needs_work to needs_review

Replying to fbissey:

Thanks for looking at this Mariah. It doesn't apply because it was based on 4.7.1.alpha1.

Apologies for the noise. I did not realize that 4.7.1.alpha1 was out. I have not seen any announcement of 4.7.1-alpha1 on sage-release. Is there someplace I should be looking for such announcements?

comment:8 in reply to: ↑ 7 Changed 9 years ago by fbissey

Replying to mariah:

Replying to fbissey:

Thanks for looking at this Mariah. It doesn't apply because it was based on 4.7.1.alpha1.

Apologies for the noise. I did not realize that 4.7.1.alpha1 was out. I have not seen any announcement of 4.7.1-alpha1 on sage-release. Is there someplace I should be looking for such announcements?

No apologies required. It isn't officially out but Jeroen has "prototypes" alpha0, alpha1 and alpha2 already on http://sage.math.washington.edu/home/release/. It is a bit a pain to know that if you do something against 4.7.rc4 it will have to be put back to "needs work" once 4.7 is finally out and Jeroen officially announce the next alpha.

comment:9 Changed 9 years ago by fbissey

I will add that it is perfectly acceptable to me if you want to leave this ticket for now and come back to it once a 4.7.1.alpha is released.

comment:10 Changed 9 years ago by fbissey

Now that 4.7.1.alpha0 is released this ticket should be in a reviewable state.

Changed 9 years ago by fbissey

Make setup.py python version smart, remove debian cruft. (made dependent on #11373)

comment:11 Changed 9 years ago by fbissey

  • Description modified (diff)

Decided to avoid problem with #11373 so make it depend on it.

comment:12 follow-up: Changed 9 years ago by jhpalmieri

The python documentation for sys.version says "Do not extract version information out of it, rather, use version_info and the functions provided by the platform module." It looks to me as though you could use

platform.python_version()  # gives '2.6.4'
platform.python_version().rstrip('.', 1)[0]  # gives '2.6'
ver = platform.python_version_tuple()  # gives ('2', '6', '4')
ver[0] + '.' + ver[1]  # gives '2.6'

or

ver = sys.version_info
str(ver[0]) + '.' + str(ver[1])   # gives '2.6'

I'll take a look at the rest of the patches...

comment:13 in reply to: ↑ 12 Changed 9 years ago by fbissey

Replying to jhpalmieri:

The python documentation for sys.version says "Do not extract version information out of it, rather, use version_info and the functions provided by the platform module." It looks to me as though you could use

platform.python_version()  # gives '2.6.4'
platform.python_version().rstrip('.', 1)[0]  # gives '2.6'
ver = platform.python_version_tuple()  # gives ('2', '6', '4')
ver[0] + '.' + ver[1]  # gives '2.6'

or

ver = sys.version_info
str(ver[0]) + '.' + str(ver[1])   # gives '2.6'

I'll take a look at the rest of the patches...

OK I will rewrite that then. It was purely a case of stealing code from somewhere else. Looks cool in misc/cython.py that will be useful here. I should bring cython.py in line too.

comment:14 Changed 9 years ago by jhpalmieri

By the way, I should have provided a link: see http://docs.python.org/library/sys.html#sys.version for information about sys.version, and pointers to the other options.

comment:15 Changed 9 years ago by fbissey

  • Description modified (diff)

New version of patches all using the platform module. I also included sage/misc/cython.py to switch it from sys to platform. The cython patch in #9958 will be updated consequently.

comment:16 follow-up: Changed 9 years ago by jhpalmieri

Can there ever be version numbers like '2.10'? Choosing the first three characters of the version string seems riskier than removing everything starting from the second dot.

comment:17 in reply to: ↑ 16 Changed 9 years ago by fbissey

Replying to jhpalmieri:

Can there ever be version numbers like '2.10'? Choosing the first three characters of the version string seems riskier than removing everything starting from the second dot.

2.10 in particular no. But your general point is valid. I will address this. Thanks for pointing this out.

comment:18 Changed 9 years ago by fbissey

It also looks like I made a big typo in the placement of a % in one of the patch as well. I'll get to it ASAP (about 4hours from now).

comment:19 Changed 9 years ago by fbissey

  • Description modified (diff)

Patch updated to use .rstrip('.', 1)[0] as it was the quickest thing to do. Fixed the syntax errors in the setup.py patch as well.

Changed 9 years ago by fbissey

updated 20110603 #2

Changed 9 years ago by fbissey

updated 20110603 take 2

Changed 9 years ago by fbissey

updated 20110603 take 2

comment:20 Changed 9 years ago by fbissey

  • Description modified (diff)

OK so I should have tested what John said before adopting it, even at half past midnight. He meant rsplit not rstrip.

comment:21 Changed 9 years ago by fbissey

  • Dependencies set to #11373

comment:22 Changed 9 years ago by mariah

  • Status changed from needs_review to needs_work

On skynet/eno (x86_64-Linux-core2-fc), I applied the three patches to sage-4.7.1.alpha2, did 'sage -b', then 'make testlong'. I got the following failure:

eno% ./sage -t  -long -force_lib "devel/sage/sage/misc/cython.py"
sage -t -long -force_lib "devel/sage/sage/misc/cython.py"   
**********************************************************************
File "/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/misc/cython.py", line 146:
    sage: pyx_preparse("")
Expected:
    ('\ninclude "interrupt.pxi"  # ctrl-c interrupt block support\ninclude "stdsage.pxi"  # ctrl-c interrupt block support\n\ninclude "cdefs.pxi"\n',
    ['mpfr',
    'gmp',
    'gmpxx',
    'stdc++',
    'pari',
    'm',
    'curvesntl',
    'g0nntl',
    'jcntl',
    'rankntl',
    'gsl',
    '...blas',
    ...,
    'ntl',
    'csage'],
    ['.../local/include/csage/',
    '.../local/include/',
    '.../local/include/python2.6/',
    '.../devel/sage/sage/ext/',
    '.../devel/sage/',
    '.../devel/sage/sage/gsl/'],
    'c',
    [])
Got:
    ('\ninclude "interrupt.pxi"  # ctrl-c interrupt block support\ninclude "stdsage.pxi"  # ctrl-c interrupt block support\n\ninclude "cdefs.pxi"\n', ['mpfr', 'gmp', 'gmpxx', 'stdc++', 'pari', 'm', 'curvesntl', 'g0nntl', 'jcntl', 'rankntl', 'gsl', 'gslcblas', 'atlas', 'ntl', 'csage'], ['/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/csage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/python2.6/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//lib/python2.6/site-packages/numpy/core/include', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/ext/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/gsl/'], 'c', [])
**********************************************************************
File "/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/misc/cython.py", line 188:
    sage: inc
Expected:
    ['bar',
    '.../local/include/csage/',
    '.../local/include/',
    '.../local/include/python2.6/',
    '.../devel/sage/sage/ext/',
    '.../devel/sage/',
    '.../devel/sage/sage/gsl/']
Got:
    ['bar', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/csage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/python2.6/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//lib/python2.6/site-packages/numpy/core/include', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/ext/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/gsl/']
**********************************************************************
1 items had failures:
   2 of  10 in __main__.example_2

comment:23 Changed 9 years ago by fbissey

Hi Mariah,

I fixed this test as part of #9958 in http://trac.sagemath.org/sage_trac/attachment/ticket/9958/trac_9958-fix-misc_cythonpy.patch I thought it was appearing because of python-2.7.

If it shows up for you with python-2.6 it is strange that it didn't before the patch.

In any case if it shows up for you the above patch will have to be split and moved to this ticket. Just a few minutes.

Changed 9 years ago by fbissey

fix the cython.py doctest for the appearance on numpy

comment:24 Changed 9 years ago by fbissey

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

OK new patch added.

comment:25 Changed 9 years ago by strogdon

What about the 

/localinclude/  and  /locallib/

in the returned results? Could this also cause a failure? I'm seeing this also in failure of the cython.py test when the patches of #9958 are applied. And the numpy fix is already in those patches.

comment:26 Changed 9 years ago by mariah

  • Status changed from needs_review to needs_work

On skynet/eno (x86_64-Linux-core2-fc), I applied all four patches to sage-4.7.1.alpha1, did 'sage -b', but I still get the error

sage -t -long -force_lib "devel/sage/sage/misc/cython.py"   
**********************************************************************
File "/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/misc/cython.py", line 146:
    sage: pyx_preparse("")
Expected:
    ('\ninclude "interrupt.pxi"  # ctrl-c interrupt block support\ninclude "stdsage.pxi"  # ctrl-c interrupt block support\n\ninclude "cdefs.pxi"\n',
    ['mpfr',
    'gmp',
    'gmpxx',
    'stdc++',
    'pari',
    'm',
    'curvesntl',
    'g0nntl',
    'jcntl',
    'rankntl',
    'gsl',
    '...blas',
    ...,
    'ntl',
    'csage'],
    ['.../local/include/csage/',
    '.../local/include/',
    '.../local/include/python2.6/',
    '.../local/lib/python2.6/site-packages/numpy/core/include',
    '.../devel/sage/sage/ext/',
    '.../devel/sage/',
    '.../devel/sage/sage/gsl/'],
    'c',
    [])
Got:
    ('\ninclude "interrupt.pxi"  # ctrl-c interrupt block support\ninclude "stdsage.pxi"  # ctrl-c interrupt block support\n\ninclude "cdefs.pxi"\n', ['mpfr', 'gmp', 'gmpxx', 'stdc++', 'pari', 'm', 'curvesntl', 'g0nntl', 'jcntl', 'rankntl', 'gsl', 'gslcblas', 'atlas', 'ntl', 'csage'], ['/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/csage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/python2.6/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//lib/python2.6/site-packages/numpy/core/include', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/ext/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/gsl/'], 'c', [])
**********************************************************************
File "/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/misc/cython.py", line 189:
    sage: inc
Expected:
    ['bar',
    '.../local/include/csage/',
    '.../local/include/',
    '.../local/include/python2.6/',
    '.../local/lib/python2.6/site-packages/numpy/core/include',
    '.../devel/sage/sage/ext/',
    '.../devel/sage/',
    '.../devel/sage/sage/gsl/']
Got:
    ['bar', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/csage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//include/python2.6/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/local//lib/python2.6/site-packages/numpy/core/include', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/ext/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/', '/home/mariah/sage/sage-4.7.1.alpha2-x86_64-Linux-core2-fc-review-11376/devel/sage/sage/gsl/']
**********************************************************************
1 items had failures:
   2 of  10 in __main__.example_2
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/mariah/.sage//tmp/.doctest_cython.py
         [4.7 s]
 
----------------------------------------------------------------------
The following tests failed:

Changed 9 years ago by fbissey

Patch to misc/cython.py with the right number of slashes

comment:27 Changed 9 years ago by fbissey

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

Thanks for pointing it out Steve I had missed it completely. We should have a uniform definition of SAGE_LOCAL, sometimes it has a final "/" and sometimes it doesn't. So I updated the misc_cythonpy patch to -p2. Hope we got everything now.

comment:28 Changed 8 years ago by mariah

  • Reviewers set to Mariah Lenox
  • Status changed from needs_review to positive_review

Applied all patches to sage-4.7.1.alpha2, did 'sage -b', and 'make testlong'. All tests passed. Positive review! (finally!)

comment:29 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:30 Changed 8 years ago by fbissey

A little word of thanks, in the right bug this time (note to self don't do this again at 1:30am):

Thank you Mariah for the review. Thank you John for the suggestion to make it better. Thank you Steve for pointing the obvious.

Thank you all for making this reach this stage. Hopefully this will be in 4.7.2.alpha0.

comment:31 Changed 8 years ago by leif

  • Cc leif added

comment:32 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.