Opened 3 years ago

Closed 3 years ago

#10751 closed defect (fixed)

Upgrading 4.5.3 -> 4.6.2.alpha4 fails

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-4.7
Component: build Keywords: cython upgrade dependencies
Cc: vbraun Merged in: sage-4.7.alpha1
Authors: Volker Braun Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

This is a problem I discovered when upgrading from sage-4.5.3 to a candidate 4.6.2.alpha4. Probably, the problem has existed for a while, it just "surfaced" or got noticed now.

The problem is that the file sage/algebras/quatalg/quaternion_algebra_element.cpp is not recreated from the corresponding .pyx file when upgrading. This then causes a compile error.

The file sage/algebras/quatalg/quaternion_algebra_element.pyx includes (using Cython's include statement) sage/ext/gmp.pxi and the latter file was changed some Sage versions ago.

The following happens:

$ ./sage -b

----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
scons: `install' is up to date.
=================== setup.py ========================
Updating Cython code....
Time to execute 0 commands: 1.31130218506e-05 seconds
Finished compiling Cython code (time = 0.0120220184326 seconds)
running install
running build
running build_py
running build_ext
building 'sage.algebras.quatalg.quaternion_algebra_element' extension
building 'sage.calculus.riemann' extension
[...]
building 'sage.symbolic.pynac' extension
building 'sage.symbolic.ring' extension
gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -O3 -Wall
-Wstrict-prototypes -fPIC
-I/mnt/usb1/scratch/jdemeyer/sage-4.5.3-4.6.2.alpha4/local/include/FLINT/ -I/mnt/usb1/scratch/jdemeyer/sage-4.5.3-4.6.2.alpha4/local/include
-I/mnt/usb1/scratch/jdemeyer/sage-4.5.3-4.6.2.alpha4/local/include/csage
-I/mnt/usb1/scratch/jdemeyer/sage-4.5.3-4.6.2.alpha4/devel/sage/sage/ext
-I/mnt/usb1/scratch/jdemeyer/sage-4.5.3-4.6.2.alpha4/local/include/python2.6
-c sage/algebras/quatalg/quaternion_algebra_element.cpp -o
build/temp.linux-x86_64-2.6/sage/algebras/quatalg/quaternion_algebra_element.o
-w
[trouble...]

Depends on #10233

Apply trac_10751_fix.patch

That is, first apply trac_10233_fix_cython_include_path.patch from the dependency and then trac_10751_fix.patch.

Attachments (2)

trac_10751_setup_misses_cython_dependencies.patch (38.0 KB) - added by vbraun 3 years ago.
Initial patch
trac_10751_fix.patch (4.1 KB) - added by vbraun 3 years ago.
Updated patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by vbraun

  • Cc vbraun added

Changed 3 years ago by vbraun

Initial patch

comment:2 Changed 3 years ago by vbraun

  • Authors set to Volker Braun
  • Keywords dependencies added
  • Status changed from new to needs_work

Here is the initial version of my rewrite. The basic idea is to split the dependency tracking off into the sage library (sage/misc/dependencies.py) so it can be reused, documented, and doctested. And instead of stuffing filenames as strings into dictionaries, classes encapsulating source file names are introduced. Let me know if you have any comments on these design decisions.

I still want to polish the code a bit more, but the current patch fixes Jeroen's bug:

[vbraun@volker-two hg]$ touch sage/ext/gmp.pxi
[vbraun@volker-two hg]$ sage -b

----------------------------------------------------------
sage: Building and installing modified Sage library files.


Installing c_lib
scons: `install' is up to date.
Updating Cython code....
Building sage/algebras/quatalg/quaternion_algebra_element.pyx because it depends on sage/ext/gmp.pxi.
Building sage/combinat/expnums.pyx because it depends on sage/ext/gmp.pxi.
Building sage/ext/multi_modular.pyx because it depends on sage/ext/gmp.pxi.
Building sage/matrix/matrix_cyclo_dense.pyx because it depends on sage/ext/gmp.pxi.
Building sage/matrix/matrix_integer_dense.pyx because it depends on sage/ext/gmp.pxi.
Building sage/matrix/matrix_rational_dense.pyx because it depends on sage/ext/gmp.pxi.
Building sage/matrix/matrix_rational_sparse.pyx because it depends on sage/ext/gmp.pxi.
Building sage/matrix/misc.pyx because it depends on sage/ext/gmp.pxi.
Building sage/modular/modform/eis_series_cython.pyx because it depends on sage/ext/gmp.pxi.
Building sage/quadratic_forms/count_local_2.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/integer.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/integer_ring.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/fast_arith.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/fraction_field_FpT.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/rational.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_base_coercion.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_capped_absolute_element.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_capped_relative_element.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_fixed_mod_element.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_generic_element.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/padic_printing.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/pow_computer.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/padics/pow_computer_ext.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/polynomial/polynomial_integer_dense_flint.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/polynomial/polynomial_rational_flint.pyx because it depends on sage/ext/gmp.pxi.
Building sage/rings/polynomial/real_roots.pyx because it depends on sage/ext/gmp.pxi.
Execute 26 commands (using 4 threads)

comment:3 Changed 3 years ago by fbissey

It's big. I guess the split in depency.py may make thing more manageable in the long term. We how to discuss the potential removal of debian stuff on the the list, #5903 doesn't say anything about setup.py and module_list.py (which I was thinking of cleaning).

The only mistake I can see from a first look is on line 529 of dependency.py

sage: gmp = SourceFile('/home/vbraun/opt/sage-4.6.2.alpha4/local/include/gmp.h')

I don't think we want to hardcode your setup :)

comment:4 Changed 3 years ago by robertwb

We should switch Sage to use http://wiki.cython.org/enhancements/distutils_preprocessing . This would let us get rid of most of the manual module_list.py declarations as well.

comment:5 Changed 3 years ago by jdemeyer

  • Priority changed from blocker to major

comment:6 Changed 3 years ago by vbraun

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

Robert's distutils-preprocessing seems to be the best way to do things, and we should investigate that once the requisite Cython-14.1 is included.

In the meantime, I've made a small patch that fixes the dependency discovery bug (tested by touching sage/ext/gmp.pxi as above). This leaves the setup.py code in its current somewhat messy state, but I think we should go with the minimal bugfix until we can give distutils-preprocessing a try.

Also, enables parallel compile by default. I didn't even know we supported that. You can still disable it via setting the MAKE='make -j1' environment variable. Rebuilding the Sage library dropped from half an hour to 5 minutes ;-)

comment:7 Changed 3 years ago by vbraun

  • Description modified (diff)

comment:8 Changed 3 years ago by vbraun

  • Description modified (diff)

Hmm patch bot still lists both patches erroneously.

Apply trac_10751_fix.patch

comment:9 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

make -j without argument to -j means: use unlimited number of threads. Therefore, the code

pos = MAKE.rfind('-j') 
if pos>=0: 
    nthreads = int(MAKE[pos+2:].split()[0])

should be changed to

pos = MAKE.rfind(' -j')
if pos >= 0:
    try: 
        nthreads = int(MAKE[pos+3:].split()[0])
    except ValueError:
        # If there is no argument to -j, use maximum number of threads
        nthreads = 2*cpu_count

In any case, the ValueError must be handled somehow.

Note also that I have added a space before -j.

comment:10 Changed 3 years ago by jdemeyer

also IndexError should be caught in the same except clause.

comment:11 Changed 3 years ago by vbraun

Ok, fixed. Also handling the make -j=N case now.

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:13 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think you need to add a try/except as follows:

try:
    if MAKE[pos+3] == '=':   # make -j=N is the same as make -jN 
        pos += 1
except IndexError:
    pass 

comment:14 Changed 3 years ago by vbraun

  • Status changed from needs_work to needs_review

Yep, that if clause needs to be moved into the existing try/except block. Updated patch follows...

Changed 3 years ago by vbraun

Updated patch

comment:15 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Looks good to me.

comment:16 Changed 3 years ago by jdemeyer

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