Opened 5 years ago

Closed 5 years ago

#18494 closed defect (fixed)

Install sage headers and auxiliary files (.h/.pxd/.pxi files)

Reported by: fbissey Owned by:
Priority: critical Milestone: sage-6.8
Component: distribution Keywords:
Cc: jpflori, strogdon, frederichan Merged in:
Authors: François Bissey Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 56929a9 (Commits) Commit: 56929a9f831d77d5022fe8969ca8365db0487285
Dependencies: Stopgaps:

Description (last modified by fbissey)

Currently, Sage doesn't install any files other than .py and compiled cython code. Headers and other auxiliary files that are necessary for compiling user code (such as the cython() command or certain optional packages) are kept in the src directory and we rely on its presence. Since #18027 we also rely on the presence of the src/build/cythonized directory. It would be good if sage could just work without relying on the presence of it source and build directory.

Change History (107)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Summary changed from Install sage headers and auxiliary files (.pxi/pxd) to Install sage headers and auxiliary files (.h/.pxd/.pxi files)

comment:3 Changed 5 years ago by jdemeyer

  • Description modified (diff)

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

I think we only really need the headers that are in sage/ext/ and its sub-folders. That would include stuff like ccobject.h of p_group_cohomology recent fame. We may want to restrict headers to public interfaces to try to prevent that kind of stuff in the future. Apart from the .pxi and .pxd we would need the .pyx if we want to be able to run the doctests without the sources, that's possibly one step further after this ticket.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

we would need the .pyx if we want to be able to run the doctests without the sources

And the .py files of course, but I don't think we want that...

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

I'll take the last point as sarcasm. In any case I discovered that interrupt_api.h is not the only interesting generated header file that need shipping. interrupt_api.h wants interrupt.h which is generated in the same folder.

comment:7 Changed 5 years ago by fbissey

Fields notes:

  • One would expect all .py files to be installed but src/sage/doctest/tests/nodoctest.py isn't.
  • sage/misc/lazy_import_cache.py has a test for the presence of src in the value of get_cache_file() I am not quite sure what is the point of this or why it needs to point to SAGE_SRC honestly. But that's out of this ticket's scope.

Found that out in sage-on-gentoo by installing manually all headers, pxi, pxd, .pyx, .rst files plus generated headers, fixing pxi.h, not installing the sources at all and pointing SAGE_SRC to /usr/lib64/python2.7/site-packages. I am relatively impressed at how well that went but I know for a fact that some doctests were not run by my sage -t --long --all command (there is one doctest currently timing out on my sage-on-gentoo install and it was definitely not run).

Now I have to seriously find a way of mostly generating all that stuff and to put it in data_files in setup.py.

Last edited 5 years ago by fbissey (previous) (diff)

comment:8 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to jdemeyer:

And the .py files of course, but I don't think we want that...

Well, what I really wanted to say is that we shouldn't rely on the installed .py or .pyx files for doctests. I don't see it as a problem if running doctests requires the sources.

comment:9 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to fbissey:

I'll take the last point as sarcasm. In any case I discovered that interrupt_api.h is not the only interesting generated header file that need shipping. interrupt_api.h wants interrupt.h which is generated in the same folder.

True, I forgot about that. In any case, it doesn't change much: just treat those 2 files the same, as auto-generated by Cython.

Last edited 5 years ago by jdemeyer (previous) (diff)

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

The bit about pxi.h is annoying. Once installed it needs the reference to build/cythonized... removed to be usable with an installed interrupt_api.h but it is necessary during the building phase unless setup.py is modified to have a specific build directory in include_dirs. Any other suggestions?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 5 years ago by jdemeyer

Is there a way to change the order of operations? The following would work:

  1. Cythonize
  2. Install headers
  3. Compile modules (using the headers installed in step 2, not the ones generated in step 1)

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

I am not sure about "installing" the header but copying them in SAGE_SRC instead in step 2 would be sufficient (may be that's what you actually meant) if that can be done. I'll look into it.

comment:13 in reply to: ↑ 12 Changed 5 years ago by jdemeyer

Replying to fbissey:

I am not sure about "installing" the header but copying them in SAGE_SRC instead in step 2 would be sufficient

I wouldn't copy stuff to SAGE_SRC. The build system should treat the source directory as read-only as much as possible.

When you talked about "an installed interrupt_api.h" in 10, which installation location did you mean?

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

Ultimately I was thinking SAGE_LOCAL/lib/python2.7/site-packages/sage/ext/interrupt/, site-packages is the final installation destination. I get what you mean by keeping source pristine, we would like building out of source as much as possible, must have hurt to mess up with that to autogenerate the pari stuff, of course you can argue that you prepared the source before putting it through setup.py.

In any case I am not sure what would effectively be a two passes approach is a good idea.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

In any case I am not sure what would effectively be a two passes approach is a good idea.

Well, you need to install the headers in any case. Does it make a big difference to do it before or after building extension modules?

comment:16 in reply to: ↑ 15 Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

In any case I am not sure what would effectively be a two passes approach is a good idea.

Well, you need to install the headers in any case. Does it make a big difference to do it before or after building extension modules?

It does to me, it means that I need to make two ebuilds because I cannot break the order "prepare/configure/build/install/merge". I don't think it matters much to add an entry to include_dirs in setup.py. It is not used for anything but building/installing unlike cython.py.

comment:17 Changed 5 years ago by fbissey

In cython.py

include_dirs = [os.path.join(SAGE_LOCAL,'include','csage'),
                os.path.join(SAGE_LOCAL,'include'), \
                os.path.join(SAGE_LOCAL,'include','python'+platform.python_version().rsplit('.', 1)[0]), \
                os.path.join(SAGE_LOCAL,'lib','python','site-packages','numpy','core','include'), \
                os.path.join(SAGE_SRC,'sage','ext'), \
                os.path.join(SAGE_SRC), \
                os.path.join(SAGE_SRC,'sage','gsl')]

Any ideas why we add SAGE_SRC and SAGE_SRC/sage/gsl? Neither has any headers in it although I am guessing gsl may have had once.

comment:18 Changed 5 years ago by jdemeyer

The correct list of include dirs is the one in src/setup.py. We should define them in one place and use the same list for setup.py and cython.py.

comment:19 Changed 5 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/headers_install
  • Commit set to d76f95f41bb890dc742fe20a3c5880e765ea2fe4
  • Description modified (diff)
  • Status changed from new to needs_review

I disagree. include dirs being the same in setup.py and cython.py is on the nice to have list but there is no compelling reason why install time and runtime absolutely have to match. In this particular case having the same list in cython.py has in setup.py means that you want to use the source instead of the installed files at runtime. I don't find that compelling. I cannot really accept a cythonize/"install headers"/build/"final install" routine which could put the two in agreement. I think that's undue complication an breaking of standard building procedures.

So what the current branch does:

  • clean up the "cythonized" path from pxi.h
  • clean cython.py so it use installed include directories rather than source ones. Prune useless directories as well and use python mechanism as much as possible rather than path "knowledge" (potentially help python2.3 as well)
  • add cythonized include path in setup.py so that pxi.h can find the generated include files at build time
  • add sections to setup.py to install header files under sage/ext including the generated headers. Also install all .pxi and .pxd files
  • also replace one instance of path knowledge by python mechanism in source.py

There is more that could be done but I don't want to extend the scope of the ticket. I have bled a little on one extra file as it is.


New commits:

d76f95fInstll headers and px{i,d} files. Makes cython.py use installed files rather than source, small clean up of path determination.

comment:20 Changed 5 years ago by fbissey

  • Status changed from needs_review to needs_work

Some unexpected doctest failures to remedy, I hadn't tested sage_setup and got unexpected stuff in the notebook.

comment:21 Changed 5 years ago by jdemeyer

Hi, is there any chance to review #9552 first and base this ticket on that?

comment:22 follow-ups: Changed 5 years ago by jdemeyer

I would still like to use as much as possible the same headers in src/setup.py and in cython.py. You could make a function

@cached_function
def sage_include_directories(sage_lib_dir):
    """
    Return the list of include directories for compiling Sage extension modules.
    """
    import numpy   # not a global import!
    return [.......]

returning the include directories for cython() if sage_lib_dir is SAGE_LIB and returning the include directories for src/setup.py if sage_lib_dir is SAGE_SRC. I haven't thought about is that much, but I think that's the right direction to go.

And I do indeed agree that sage/gsl should be removed.

comment:23 Changed 5 years ago by jdemeyer

And what is so special about the two files sage/libs/linkages/padics/*.pxi?

comment:24 Changed 5 years ago by jdemeyer

Minor remark: please fix your indentation (multiples of 4 spaces) and

["add", "a", "space", "after", "commas", "in", "a", "list"]

comment:25 Changed 5 years ago by jdemeyer

I don't understand the two cases here:

if 'ext' in package:
  python_package_data[package] = ['*.pyx', '*.pxd', '*.pxi','*.h']
else:
  python_package_data[package] = ['*.pyx', '*.pxd', '*.pxi']

comment:26 Changed 5 years ago by fbissey

I originally thought only the headers in sage/ext and under where necessary but I hadn't put that theory through a harsh test yet. I am fixing that in the next iteration.

comment:27 Changed 5 years ago by jdemeyer

Also, although installing .pyx files will not hurt, I wonder whether we really want to do that: I don't think that ordinary users are expected to run doctests...

comment:28 Changed 5 years ago by jpflori

  • Cc jpflori added

comment:29 Changed 5 years ago by jpflori

I would not like to see every pyx file duplicated either...

comment:30 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by jpflori

Replying to jdemeyer:

Is there a way to change the order of operations? The following would work:

  1. Cythonize
  2. Install headers
  3. Compile modules (using the headers installed in step 2, not the ones generated in step 1)

At some point we also had #14397. Not sure if there was a followup to this ticket.

comment:31 in reply to: ↑ 30 Changed 5 years ago by jpflori

Replying to jpflori:

Replying to jdemeyer:

Is there a way to change the order of operations? The following would work:

  1. Cythonize
  2. Install headers
  3. Compile modules (using the headers installed in step 2, not the ones generated in step 1)

At some point we also had #14397. Not sure if there was a followup to this ticket.

Hum, I guess so... and that is just what we do now with the cythonized directory.

comment:32 Changed 5 years ago by fbissey

Hum... Well there are several point of views about that but I will remove it because I consider it out of scope, it shouldn't have been in that commit.

My point of view is that sage should stand on its own without the sources. As it is the doctest are only runnable once sage is installed. Ideally we could run the doctest after building but before installing. But I'll keep that bit as my own special case, separate, only in the sage-on-gentoo tree if I want to run the doctests without the source being present.

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

Did I just make sage automatically load numpy by importing it in cython.py?

sage -t --long src/sage/tests/startup.py
**********************************************************************
File "src/sage/tests/startup.py", line 6, in sage.tests.startup
Failed example:
    'numpy' in sys.modules
Expected:
    False
Got:
    True

comment:34 in reply to: ↑ 33 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

Did I just make sage automatically load numpy by importing it in cython.py?

See 22

comment:35 Changed 5 years ago by git

  • Commit changed from d76f95f41bb890dc742fe20a3c5880e765ea2fe4 to df1d4458f6190415e72d2533b5e5780985b035d5

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

df1d445Revert changes breaking doctest in sagenb and forcing numpy import. Ship all headers and do not ship .pyx files.

comment:36 Changed 5 years ago by fbissey

Still have to fix sage_setup. Curiously in sage/doctest/sources.py changing sp from SAGE_LOCAL/lib/python/site-packages to SAGE_LOCAL/lib/python2.7/site-packages (or SAGE_LIB) breaks doctesting of sagenb - in "vanilla" sage but not sage-on-gentoo. This was extra baggage to this ticket so I won't pursue it.

comment:37 in reply to: ↑ 34 Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

Did I just make sage automatically load numpy by importing it in cython.py?

See 22

Sorry somehow a bunch of notifications didn't make it into my inbox. I will review #9952 and I'll certainly rebase on top once it is done.

comment:38 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

I would still like to use as much as possible the same headers in src/setup.py and in cython.py. You could make a function

@cached_function
def sage_include_directories(sage_lib_dir):
    """
    Return the list of include directories for compiling Sage extension modules.
    """
    import numpy   # not a global import!
    return [.......]

returning the include directories for cython() if sage_lib_dir is SAGE_LIB and returning the include directories for src/setup.py if sage_lib_dir is SAGE_SRC. I haven't thought about is that much, but I think that's the right direction to go.

That would be clever. I'll try that.

It is actually quite hard to work out which .pxi/.pxd will never be used which why I included the padics files. There is no other python sources in that directory so to package them I either have to do it as a data_files or introduce a special case in package_data. Any chance of them being obsolete?

I'll check my space after commas and correct the surroundings. I usually follow the style of what I change rather than move to a new style but I guess the space after comma should really be the standard.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

It is actually quite hard to work out which .pxi/.pxd will never be used which why I included the padics files. There is no other python sources in that directory

Would it help to just add a __init__.py file?

Any chance of them being obsolete?

No, mpz.pxi is not obsolete (and API.pxi is really just documentation)

comment:40 in reply to: ↑ 39 Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

It is actually quite hard to work out which .pxi/.pxd will never be used which why I included the padics files. There is no other python sources in that directory

Would it help to just add a __init__.py file?

It should. That should put the folder in python_packages so it should work. Putting on my list.

comment:41 follow-ups: Changed 5 years ago by fbissey

How close are we of getting #17854 merged we have many item queued that clearly touch the same files. I think finishing #17854 should be the top priority there are two many things that will either depends on it or need to be rebased after it or its dependencies are merged.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 5 years ago by jpflori

Replying to fbissey:

How close are we of getting #17854 merged we have many item queued that clearly touch the same files. I think finishing #17854 should be the top priority there are two many things that will either depends on it or need to be rebased after it or its dependencies are merged.

I cannot really say but I see the last dependency of #17854 is getting rid of something NTL related. Our NTL wrapping code is really terrible with regards to what Cython can now do and we should definitely rewrite it all using actual C++ bindings. This is not so difficult but very time consuming... So maybe waiting for #17854 is not the right thing to do.

comment:43 Changed 5 years ago by fbissey

I see. I have a vested interest in libcsage going away and it shows in some of my eagerness. Anyone has an idea why sage trac messages (including the digest) arrive in my inbox as "read"? Very annoying.

comment:44 Changed 5 years ago by strogdon

  • Cc strogdon added

comment:45 in reply to: ↑ 41 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

How close are we of getting #17854 merged

Since it's needs_review, that only depends on whether or not somebody wants to review it.

comment:46 in reply to: ↑ 42 Changed 5 years ago by jdemeyer

Replying to jpflori:

Our NTL wrapping code is really terrible with regards to what Cython can now do and we should definitely rewrite it all using actual C++ bindings. This is not so difficult but very time consuming...

I agree completely with the above statement. Note that #18367 doesn't really clean-up the NTL interface, it just moves it from c_lib to the Sage library.

comment:47 in reply to: ↑ 45 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

How close are we of getting #17854 merged

Since it's needs_review, that only depends on whether or not somebody wants to review it.

It seems to need re-basing but #18367 seems to be clean and ready for a run on the patchbot as far as I can see.

comment:48 in reply to: ↑ 47 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

It seems to need re-basing

It merges cleanly for me... don't get confused by the red Trac link.

comment:49 in reply to: ↑ 48 ; follow-ups: Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

It seems to need re-basing

It merges cleanly for me... don't get confused by the red Trac link.

OK, regardless I have noticed that #18450 is also needed and in need of review. And there seem to be no progress with upstream cython on that one, which puts a damper on the whole thing.

comment:50 in reply to: ↑ 49 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

OK, regardless I have noticed that #18450 is also needed and in need of review.

It is not really needed for #18450, it's mostly a dependency to avoid conflicts.

And there seem to be no progress with upstream cython on that one

My personal opinion is that we shouldn't care too much about what upstream thinks. Note that upstream Cython has not given a single reason why they are against my feature on #18450.

comment:51 in reply to: ↑ 49 Changed 5 years ago by jdemeyer

Replying to fbissey:

And there seem to be no progress with upstream cython on that one, which puts a damper on the whole thing.

But a very tiny damper regardless. It's trivial to fix #18450 such that it does not depend on the non-accepted Cython patch.

comment:52 in reply to: ↑ 50 Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

OK, regardless I have noticed that #18450 is also needed and in need of review.

It is not really needed for #18450, it's mostly a dependency to avoid conflicts.

And there seem to be no progress with upstream cython on that one

My personal opinion is that we shouldn't care too much about what upstream thinks. Note that upstream Cython has not given a single reason why they are against my feature on #18450.

Yes very strange conversation I have to agree. Upstream think their reasons are obvious. Unfortunately while I am sure you don't care it makes my other life a bit too "interesting".

But I should start re-writing stuff for this ticket, if there are conflicts we'll sort them out when we work out the merge order.

comment:53 in reply to: ↑ 22 Changed 5 years ago by fbissey

Replying to jdemeyer:

I would still like to use as much as possible the same headers in src/setup.py and in cython.py. You could make a function

@cached_function
def sage_include_directories(sage_lib_dir):
    """
    Return the list of include directories for compiling Sage extension modules.
    """
    import numpy   # not a global import!
    return [.......]

Least you think I am not working on this. My attention had to be divided between a number of things but I am still working on this. Any recommendation on where to put this function? We do not want to put it in cython.py because we don't want to import any bits of it in setup.py (bad things will happen to you if you do). I don't want to put it anywhere in sage_setup because it should only be used at build time not runtime, same for module_list.py. env.py could be interesting if I can get away with it. The other possibilities:

  • its own file
  • something I haven't thought about

opinions?

Last edited 5 years ago by fbissey (previous) (diff)

comment:54 Changed 5 years ago by jdemeyer

Actually, env.py sounds like a good suggestion. You'll need to move SAGE_INC (currently defined in setup.py) also to env.py.

I'd wait for 6.8.beta3 though to be safe with merge conflicts.

comment:55 Changed 5 years ago by jdemeyer

In src/setup.py, it would be good to define a new variable for the build/cythonized directory.

comment:56 follow-ups: Changed 5 years ago by fbissey

OK, I'll wait beta3 to put the work to "need review" but I could already present stuff for scrutiny, it is quite advanced now. Bringing SAGE_INC to the sage.env sounds a good idea, hopefully it will be its final destination (I am sure it started in module_list.py). If you think a variable for build/cythonized would be useful I can throw that in, suggestion for the name?

comment:57 in reply to: ↑ 56 Changed 5 years ago by jdemeyer

Replying to fbissey:

If you think a variable for build/cythonized would be useful I can throw that in, suggestion for the name?

SAGE_CYTHONIZED?

comment:58 in reply to: ↑ 56 Changed 5 years ago by jdemeyer

Replying to fbissey:

OK, I'll wait beta3 to put the work to "need review" but I could already present stuff for scrutiny, it is quite advanced now.

For me, you don't need to wait to put it to needs_review, just be aware of #9552, #18450, #18367 (and #17854 which isn't reviewed yet).

comment:59 Changed 5 years ago by fbissey

I should be able to rebase/update everything in the next couple of days, including this little surprise

File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 231, in sage.misc.cython.pyx_preparse
Failed example:
    module = sage.misc.cython.import_test("trac11680")  # long time (7s on sage.math, 2012)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.cython.pyx_preparse[9]>", line 1, in <module>
        module = sage.misc.cython.import_test("trac11680")  # long time (7s on sage.math, 2012)
      File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 810, in import_test
        return compile_and_load(TESTS[name])
      File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 763, in compile_and_load
        return cython_import(file, create_local_c_file=False)
      File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 687, in cython_import
        **kwds)
      File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 480, in cython
        raise RuntimeError("Error compiling {}:\n{}\n{}".format(filename, log, err))
    RuntimeError: Error compiling /home/fbissey/.sage/temp/QCD-nzi3/31051/tmp_zyUtdw.pyx:
    running build
    running build_ext
    building '_home_fbissey__sage_temp_QCD_nzi3_31051_tmp_zyUtdw_pyx_0' extension
    creating build
    creating build/temp.linux-x86_64-2.7
    x86_64-pc-linux-gnu-gcc -pthread -fPIC -I/usr/lib64/python2.7/site-packages/sage/libs/flint -I/usr//include/flint -I/usr/include -I/usr/include/python2.7 -I/usr/lib64/python2.7/site-packages/numpy/core/include -I/usr/lib64/python2.7/site-packages -I/usr/lib64/python2.7/site-packages/sage/ext -I/home/fbissey/.sage/temp/QCD-nzi3/31051 -I/usr/include/python2.7 -c _home_fbissey__sage_temp_QCD_nzi3_31051_tmp_zyUtdw_pyx_0.c -o build/temp.linux-x86_64-2.7/_home_fbissey__sage_temp_QCD_nzi3_31051_tmp_zyUtdw_pyx_0.o -w -O2 -O3 -ggdb

    _home_fbissey__sage_temp_QCD_nzi3_31051_tmp_zyUtdw_pyx_0.c:258:37: fatal error: sage/libs/ntl/ntlwrap.cpp: No such file or directory
     #include "sage/libs/ntl/ntlwrap.cpp"
                                         ^
    compilation terminated.
    error: command 'x86_64-pc-linux-gnu-gcc' failed with exit status 1

So that is one extra file to ship, the only one of its kind.

comment:60 Changed 5 years ago by fbissey

  • Branch changed from u/fbissey/headers_install to u/fbissey/trac18494
  • Commit changed from df1d4458f6190415e72d2533b5e5780985b035d5 to 9ddcb7a38ca4caa5f98cfbb896ff6edbfed9a4a1

Pushed newer version of the work, I am still testing things so I am not putting to review but feedback welcome. After that work, I still feel unsatisfied. Like a few year ago I thought the usage of SAGE_ROOT was excessive in sage, I now think the usage of SAGE_SRC is excessive. It is legit for doctesting but there are some cases where the sources are used at runtime when installed files should be used. On the whole with this commit things should work if you set SAGE_SRC=SAGE_LIB with the possible exception of cython debugging which I would have to check.


New commits:

9ddcb7atrac 18494, re-organize things to install the headers and other auxiliary files necessary to run sage, without the build directory or the source directory if

comment:61 Changed 5 years ago by git

  • Commit changed from 9ddcb7a38ca4caa5f98cfbb896ff6edbfed9a4a1 to 912f8ca84b2de02848e0834c97649339cf018ed0

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

912f8caFix the cython include path doctest (properly this time). Make SAGE_CYTHONIZED relative instead of absolute and fixing removing tabs from env.py.

comment:62 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

Fixed the last few doctests, I am putting this to needs_review.

comment:63 Changed 5 years ago by fbissey

Patchbot seem to like it. I am waiting for review before moving back to #17630 based on this ticket.

comment:64 Changed 5 years ago by jdemeyer

To allow for future auto-generation of python_data_files, I would prefer to start out with a list

cython_generated_files = [
    os.path.join(SAGE_CYTHONIZED, 'sage', 'ext', 'interrupt', 'interrupt_api.h'),
    os.path.join(SAGE_CYTHONIZED, 'sage', 'ext', 'interrupt', 'interrupt.h')
]

and then generate python_data_files from that. That would also be more readable than the [(()[()()])] nesting.

comment:65 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Some formatting issues:

  1. use 4 space indents in for package in python_packages:
  2. format the docstring of def sage_include_directories(): according to http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings
  3. remove the redundant \ in def sage_include_directories()
Last edited 5 years ago by jdemeyer (previous) (diff)

comment:66 follow-up: Changed 5 years ago by jdemeyer

SAGE_CYTHONIZED should use an absolute path (relative to SAGE_SRC)

comment:67 Changed 5 years ago by jdemeyer

Instead of an argument phase='runtime' (whose intentions are not so clear to me), I would prefer to see something more explicit like use_sources=False. Using a boolean will also avoid the need for

else:
    raise ValueError('phase not recognised')
    return               # <--- This line is redundant by the way

comment:68 in reply to: ↑ 66 Changed 5 years ago by fbissey

Replying to jdemeyer:

SAGE_CYTHONIZED should use an absolute path (relative to SAGE_SRC)

Ok I initially did that and reverted because it broke a doctest (sage/repl/interpreter.py) but I have no objection to making it absolute and fixing the doctest instead.

All comments sound good to me, I guess my vocabulary on "phase" is packaging speak, your suggestion is simpler.

comment:69 Changed 5 years ago by jdemeyer

In src/sage_setup/clean.py, rename extensions as skip_extensions.

comment:70 Changed 5 years ago by jdemeyer

Replace opj(x) by x

comment:71 Changed 5 years ago by fbissey

I am experiencing a difficulty while writing an "examples" section. The example works fine from the sage prompt but during doctesting I get

sage -t --long src/sage/env.py
**********************************************************************
File "src/sage/env.py", line 164, in sage.env.sage_include_directories
Failed example:
    sage.env.sage_include_directories()
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.env.sage_include_directories[1]>", line 1, in <module>
        sage.env.sage_include_directories()
    AttributeError: 'module' object has no attribute 'sage_include_directories'
**********************************************************************
1 item had failures:
   1 of   3 in sage.env.sage_include_directories
    [12 tests, 1 failure, 0.01 s]

The code tested is

sage: import sage.env
sage: sage.env.sage_include_directories()
['/home/fbissey/sandbox/git-fork/sage-6.8.beta3/local/include',
 '/home/fbissey/sandbox/git-fork/sage-6.8.beta3/local/include/python2.7',
 '/home/fbissey/sandbox/git-fork/sage-6.8.beta3/local/lib/python2.7/site-packages/numpy/core/include',
 '/home/fbissey/sandbox/git-fork/sage-6.8.beta3/local/lib/python2.7/site-packages',
 '/home/fbissey/sandbox/git-fork/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/ext']

comment:72 Changed 5 years ago by fbissey

It was the wrong sage being executed... Commits coming.

comment:73 Changed 5 years ago by git

  • Commit changed from 912f8ca84b2de02848e0834c97649339cf018ed0 to 1bc95ae3b7a76c2d59d955bef67535d97ac2f472

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

1bc95aeIncorporate Jeroen's feedback.

comment:74 Changed 5 years ago by git

  • Commit changed from 1bc95ae3b7a76c2d59d955bef67535d97ac2f472 to 769b69edf7811fd16ed2bf90bd8fe376e17b5006

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

769b69esmall adjustment in cython.py

comment:75 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

comment:76 Changed 5 years ago by jdemeyer

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

Can you make use_sources an actual boolean, not a string with value "True" or "False"?

comment:77 follow-up: Changed 5 years ago by jdemeyer

I don't want to assume that all cython_generated_files are in sage/ext/interrupt, so I don't like this:

python_data_files = [
    (os.path.join(SAGE_LIB, 'sage', 'ext', 'interrupt'), cython_generated_files)
]

Eventually, also files like src/build/cythonized/sage/modular/arithgroup/farey_symbol.h should be installed in the correct directory.

comment:78 in reply to: ↑ 77 Changed 5 years ago by fbissey

Replying to jdemeyer:

I don't want to assume that all cython_generated_files are in sage/ext/interrupt, so I don't like this:

python_data_files = [
    (os.path.join(SAGE_LIB, 'sage', 'ext', 'interrupt'), cython_generated_files)
]

Eventually, also files like src/build/cythonized/sage/modular/arithgroup/farey_symbol.h should be installed in the correct directory.

I guess we could use a slice but that wouldn't be as clean as using a dictionary or specifically named variable. I would go for the dictionary.

comment:79 Changed 5 years ago by git

  • Commit changed from 769b69edf7811fd16ed2bf90bd8fe376e17b5006 to d57f9ecea507a155bc32f15113191386bbc9223b

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

d57f9ecUse true boolean in env.py. Generate python_data_files from a dictionary.

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

  • Status changed from needs_work to needs_review

OK moved to a new design but I feel I just pushed the old ugliness of python_data_files in cython_generated_files with better documentation.

comment:81 Changed 5 years ago by fbissey

seeking new review, would very much wants this or something close to it in the next beta/rc.

comment:82 in reply to: ↑ 80 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

I feel I just pushed the old ugliness of python_data_files in cython_generated_files with better documentation.

I agree and what you did is not quite what I had in mind: the point is that the ext/interrupt should be computed from the os.path.join(SAGE_CYTHONIZED, 'sage', 'ext', 'interrupt', 'interrupt.h') part.

On the other hard, if you're tired of making these changes, they could be postponed to a follow-up ticket.

comment:83 in reply to: ↑ 82 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

I feel I just pushed the old ugliness of python_data_files in cython_generated_files with better documentation.

I agree and what you did is not quite what I had in mind: the point is that the ext/interrupt should be computed from the os.path.join(SAGE_CYTHONIZED, 'sage', 'ext', 'interrupt', 'interrupt.h') part.

On the other hard, if you're tired of making these changes, they could be postponed to a follow-up ticket.

I see what you have in mind now. I may have an idea how to do something like that, but I am giving myself a time limit. If I cannot work it out in the next 24hours, it'll be a follow up ticket.

comment:84 Changed 5 years ago by jdemeyer

  • Priority changed from major to critical

I think this is important also for #12375 and #18514.

comment:85 in reply to: ↑ 83 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to fbissey:

I see what you have in mind now. I may have an idea how to do something like that, but I am giving myself a time limit. If I cannot work it out in the next 24hours, it'll be a follow up ticket.

Excellent. I'll set this ticket to needs_work now. Feel free to set it back to needs_review (with or without making further changes).

comment:86 Changed 5 years ago by git

  • Commit changed from d57f9ecea507a155bc32f15113191386bbc9223b to 4e745ce5c0d80d3f7043910bd9c8c2f1908dd2ef

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

4e745ceGet SAGE_CYTHONIZED ffiles to generate package_data al by themselves

comment:87 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

Seems to be working from the list of files now.

comment:88 Changed 5 years ago by jdemeyer

This comments needs to be updated:

# This is stored as a dictionary with the key being the directory to install the file under sage.

comment:89 follow-up: Changed 5 years ago by jdemeyer

INPUTS and OUTPUTS should be INPUT and OUTPUT.

comment:90 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work
sage -t --long src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    shell.run_cell('1/0')
Expected:
    ---------------------------------------------------------------------------
    .../sage/rings/integer_ring.pyx in sage.rings.integer_ring.IntegerRing_class._div (.../build/cythonized/sage/rings/integer_ring.c:...)()
        ...         cdef rational.Rational x = rational.Rational.__new__(rational.Rational)
        ...         if mpz_sgn(right.value) == 0:
        ...             raise ZeroDivisionError('Rational division by zero')
        ...         mpz_set(mpq_numref(x.value), left.value)
        ...         mpz_set(mpq_denref(x.value), right.value)
    <BLANKLINE>
    ZeroDivisionError: Rational division by zero
Got:
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    <ipython-input-1-6f88eab09598> in <module>()
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    /usr/local/src/sage-config/src/sage/structure/element.pyx in sage.structure.element.RingElement.__div__ (build/cythonized/sage/structure/element.c:18132)()
       2034                 return (<RingElement>self)._idiv_(<RingElement>right)
       2035             else:
    -> 2036                 return (<RingElement>self)._div_(<RingElement>right)
       2037         global coercion_model
       2038         return coercion_model.bin_op(self, right, div)
    <BLANKLINE>
    /usr/local/src/sage-config/src/sage/rings/integer.pyx in sage.rings.integer.Integer._div_ (build/cythonized/sage/rings/integer.c:12151)()
       1748         # This is vastly faster than doing it here, since here
       1749         # we can't cimport rationals.
    -> 1750         return the_integer_ring._div(self, right)
       1751 
       1752     def __floordiv__(x, y):
    <BLANKLINE>
    /usr/local/src/sage-config/src/sage/rings/integer_ring.pyx in sage.rings.integer_ring.IntegerRing_class._div (build/cythonized/sage/rings/integer_ring.c:4900)()
        420         cdef rational.Rational x = rational.Rational.__new__(rational.Rational)
        421         if mpz_sgn(right.value) == 0:
    --> 422             raise ZeroDivisionError('Rational division by zero')
        423         mpz_set(mpq_numref(x.value), left.value)
        424         mpz_set(mpq_denref(x.value), right.value)
    <BLANKLINE>
    ZeroDivisionError: Rational division by zero
**********************************************************************

comment:91 Changed 5 years ago by frederichan

  • Cc frederichan added

comment:92 in reply to: ↑ 89 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

INPUTS and OUTPUTS should be INPUT and OUTPUT.

I really need to get some glasses, and stop reading stuff in the middle of the night too. The failing doctest shouldn't be. It probably means sage/rings/integer_ring.pyx hasn't been rebuilt after you patched. I thought touching setup.py would trigger that.

comment:93 Changed 5 years ago by git

  • Commit changed from 4e745ce5c0d80d3f7043910bd9c8c2f1908dd2ef to f4b868b4f34c5bf7de22c3f66db695c83baa61eb

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

f4b868bFinal (cross fingers) clean up of comments and documentation sections

comment:94 Changed 5 years ago by fbissey

  • Status changed from needs_work to needs_review

I am putting this back to "need_review" unless someone really wants to do something to trigger the rebuild of sage/rings/integer_ring.pyx that I haven't touched but for which the building directory is doctested in a completely unrelated file, I think it should go in.

comment:95 Changed 5 years ago by git

  • Commit changed from f4b868b4f34c5bf7de22c3f66db695c83baa61eb to 56929a9f831d77d5022fe8969ca8365db0487285

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

56929a9Alter the interpreter.py doctest so it will pass whether sage/rings/integer_ring.pyx is rebuilt or not

comment:96 Changed 5 years ago by fbissey

Just one more push, changing the doctest a little bit means that it will pass whether or not we have a rebuild of integer_ring.pyx and it doesn't impact what the test is designed to do I believe.

comment:97 in reply to: ↑ 92 Changed 5 years ago by jdemeyer

Replying to fbissey:

It probably means sage/rings/integer_ring.pyx hasn't been rebuilt after you patched. I thought touching setup.py would trigger that.

No, changing setup.py triggers re-compilation of everything (i.e. running of gcc) but not re-cythonizing. The usual trick to do that is to change what's in the .cython_version file, see line 545 of setup.py.

comment:98 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

I guess this is good to go now.

comment:99 Changed 5 years ago by frederichan

Sorry for the delay but I still have problems with trac12375. I have tried to isolate the problem: (can you reproduce it?)

setup.py:

#!/usr/bin/env sage -python
from sage.env import *
import os

###
try:
    # Sage >= 6.8
    from sage.env import sage_include_directories
except ImportError:
    # Sage < 6.8
    def sage_include_directories(source=True):
        return [
            os.path.join(SAGE_LOCAL, "include"),
            os.path.join(SAGE_LOCAL, "include", "csage"),
            os.path.join(SAGE_SRC),
            os.path.join(SAGE_SRC, "sage", "ext"),
            ]
###

from distutils.core import setup
from distutils.extension import Extension


cmdclass = { }
ext_modules = [ ]

include_dirs=sage_include_directories(True)

from Cython.Distutils import build_ext

ext_modules+=[Extension(
                   "giacpy",                 # name of extension
                   ["giacpy.pyx"], #  our Cython source
                   include_dirs=include_dirs,
                   language="c++")]
cmdclass={'build_ext': build_ext}


setup(
    ext_modules=ext_modules,
    cmdclass=cmdclass
    )

and giacpy.pyx

from sage.rings.integer cimport Integer
from sage.env import * ##without this line it builts
include 'sage/ext/interrupt.pxi'
include 'sage/ext/stdsage.pxi'

gives:

giacpy.cpp: In function ‘int __pyx_import_star_set(PyObject*, PyObject*, char*)’:
giacpy.cpp:3635:55: error: ‘__pyx_convert__from_py_sage_signals_t’ was not declared in this scope
     _signals = __pyx_convert__from_py_sage_signals_t(o); if (PyErr_Occurred()) {__pyx_filename = __pyx_f[3]; __pyx_lineno = 45; __pyx_clineno = __LINE__; goto __pyx_L2_error;};
                

comment:100 Changed 5 years ago by fbissey

Two remarks, in setup.py you have

include_dirs=sage_include_directories(True)

It should be

include_dirs=sage_include_directories()

In giacpy.pyx why would you need from sage.env import *? What do you need from this?

comment:101 Changed 5 years ago by frederichan

I have tried with and without True and got the same pb. But removing from sage.env import * doesn't help my original program, I have just tried to obtain a small code with the same error message.

comment:102 Changed 5 years ago by fbissey

Did it work at all before you included this ticket?

comment:103 Changed 5 years ago by frederichan

it works for instance with sage 6.7. What I have notice on this small example:

rm giacpy.cpp; /usr/local/sage/sage setup.py build_ext

builts (on sage 6.7) but not:

rm giacpy.cpp; /home/fred/dev/sage/develop/sage/sage setup.py build_ext

(on my git)

comment:104 Changed 5 years ago by jdemeyer

@frederichan: it would help if you give us the exact and complete code you have trouble with: update #12375, rebase it on top of #18494 and then we can talk.

comment:105 follow-up: Changed 5 years ago by frederichan

NB: it doesn't work with sage 6.8beta3 without your ticket. It just mean the this ticket doesn't solve my pb.

comment:106 in reply to: ↑ 105 Changed 5 years ago by jdemeyer

Replying to frederichan:

NB: it doesn't work with sage 6.8beta3 without your ticket. It just mean the this ticket doesn't solve my pb.

Yes, I understood that. I'm saying that, regardless of earlier Sage versions, the best way forward is to use #18494 anyway.

comment:107 Changed 5 years ago by vbraun

  • Branch changed from u/fbissey/trac18494 to 56929a9f831d77d5022fe8969ca8365db0487285
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.