sage's spkg sources should use correct include paths instead of having ../../../../whatever everywhere #12728

SnarkBoojum · 2012-03-22T13:46:46Z

Problem

There are many places in sage's spkg sources where inclusions are made with a filename which looks like ../../../../../whatever (no exageration: up to five levels!).

Solution
Those should be removed, and the right path added to the include line.

The first patch was obtained by running the attached script and the second one by hand-editing the remaining occurrences.

Apply

Component: build

Author: Julien Puydt

Reviewer: Volker Braun, Jeroen Demeyer, Julien Puydt

Merged: sage-5.10.beta5

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

jdemeyer · 2012-04-02T09:51:39Z

comment:2

I don't like attachment: compile_with_no_up_in_includes.patch. I would prefer the patch to add only SAGE_ROOT/devel/sage and not all those other directories.

SnarkBoojum · 2012-04-02T10:38:20Z

comment:3

There are two problems with "../../../../.." :

it doesn't give any serious information, so why is it here?
the code is supposed to provide features ; it shouldn't worry about building.

It's better to have the build system provide ten "-I" flags than having a thousand places in the code where those ten paths are engraved, for maintainability reasons.

jdemeyer · 2012-07-27T20:40:22Z

comment:4

Please fill in your real name as Author.

SnarkBoojum · 2012-08-06T13:17:37Z

Author: Julien Puydt

kiwifb · 2012-08-08T11:21:30Z

comment:6

Some dedication here. The second patch at least will need rebasing against at least 5.2. I am not sure you should add all these paths to CYTHON_INCLUDE_DIRS. Is there a reason why they are not in their own variable. Do you need all these and not just SAGE_ROOT + '/devel/sage/sage/'?

SnarkBoojum · 2012-08-08T12:15:41Z

comment:7

Yes, it's now old and deprecated. And yes, I think it was needed to add all those paths.

I plan to rework on the matter differently -- the question is when. The plan would be to have some script with a list of the included headers with their full paths and their wanted path, which would normalize the paths in includes. That might still require changing CYTHON_INCLUDE_DIRS, but hopefully adding less paths.

simon-king-jena · 2013-01-06T12:25:51Z

comment:9

On Sage-devel, you reported that by changing the include paths the answers to some doctest in sage.misc.sageinspect changed. I'll see where that comes from, but I guess sageinspect needs a fix concerning temporary Cython code anyway.

simon-king-jena · 2013-01-06T12:59:46Z

comment:10

See #13916.

SnarkBoojum · 2013-01-06T15:37:54Z

comment:11

That shell script has a bug where everything before stdsage.pxi might get eaten -- that is the reason why there's a failing doctest. Not ready for review yet :-P

SnarkBoojum · 2013-01-06T20:07:45Z

Attachment: updirectories-sed.sh.gz

Shell script which patches 99,99% of the issues

SnarkBoojum · 2013-01-07T07:44:31Z

comment:12

Ok, the new version of the updirectories-sed shell script passes all doctests: it needs review. It won't fix 100% of the problem, but the rest can be patched by hand, at it concerns only a handful of cases.

jdemeyer · 2013-01-12T22:00:35Z

comment:13

Please make it clear what should be applied.

SnarkBoojum · 2013-01-13T10:06:22Z

comment:14

The updirectories-sed.sh shell script needs to be reviewed:

read ;
run (from the right directory) ;
compile ;
make ptestlong ;
report success (very likely) or failure (very unlikely).

kiwifb · 2013-01-13T10:11:32Z

comment:15

I think that's not how we want to proceed hence, Jeroen's question. Having the script is nice for testing but once you have run it you should generate a patch with mercurial and post it here for inclusion.
The script is nice for stuff that will be included in the future... there is a ticket somewhere where someone is making a similar mess - I didn't have the heart to shoot him early.

SnarkBoojum · 2013-01-13T10:22:14Z

comment:16

I'll generate an mercurial patch if that is what you want. It will be thousands of lines long, but if you prefer to review that... every taste is in nature.

kiwifb · 2013-01-13T10:32:07Z

comment:17

Size of the patch is just a technical problem. It just makes it harder to find a problem if there is any but no more so than with just applying your script.

jdemeyer · 2013-01-13T10:54:47Z

comment:18

Replying to @SnarkBoojum:

I'll generate an mercurial patch if that is what you want.

No, that's not needed. But you should really explain in the ticket description which patches should be applied/which scripts should be run. From the previous ticket description, it was a absolutely totally utterly completely not clear that the script should be run instead of applying any patches.

SnarkBoojum · 2013-01-13T11:07:22Z

comment:19

I don't like editing the ticket description because that breaks the flow of comments. And notice that running the shell script fixes a huge percentage of the updirectory includes, but doesn't fix them all : I'll still need to hand-edit the remaining ones afterwards.

jdemeyer · 2013-01-13T11:51:26Z

comment:20

Replying to @SnarkBoojum:

I don't like editing the ticket description because that breaks the flow of comments.

So you expect the release manager to read all the comments in every ticket just to know which patches should be applied? That's not going to happen.

And even from reading the comments, it's not that clear that the shell script replaces the usual patch.

SnarkBoojum · 2013-05-23T09:23:13Z

comment:40

Your error message doesn't look at all like it's related to my changes (no error looking like "File not found"), but I'll investigate...

vbraun · 2013-05-23T10:06:36Z

comment:41

This is what broke:

sage: sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent)
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-1-b9940f7f2356> in <module>()
----> 1 sage_getargspec(sage.sets.disjoint_set.OP_represent)

NameError: name 'sage_getargspec' is not defined
sage: sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-ee04891bf854> in <module>()
----> 1 sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent)

/home/vbraun/opt/sage-5.10.beta3/local/lib/python2.7/site-packages/sage/misc/sageinspect.pyc in sage_getargspec(obj)
   1332         except TypeError: # arg is not a code object
   1333         # The above "hopefully" was wishful thinking:
-> 1334             return inspect.ArgSpec(*_sage_getargspec_cython(sage_getsource(obj)))
   1335             #return _sage_getargspec_from_ast(sage_getsource(obj))
   1336     try:

/home/vbraun/opt/sage-5.10.beta3/local/lib/python2.7/site-packages/sage/misc/sageinspect.pyc in _sage_getargspec_cython(source)
    984 
    985     """
--> 986     defpos = source.find('def ')
    987     assert defpos > -1, "The given source does not contain 'def'"
    988     s = source[defpos:].strip()

AttributeError: 'NoneType' object has no attribute 'find'

used to be

sage: sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent)
ArgSpec(args=['n', 'merges', 'perm'], varargs=None, keywords=None, defaults=None)

SnarkBoojum · 2013-05-23T10:19:11Z

comment:42

@vbraun: thanks ; it still doesn't look like a missing file, but at least it's a place I can start digging.

My plan was the following :

freshly unpack a sage-5.10.beta4 tarball ;
make build
apply the patches
./sage -ba-force
make doc-html

now step 5 is to start from this sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent).

SnarkBoojum · 2013-05-23T12:26:32Z

comment:43

Ok, I determined that sage.misc.sageinspect.sage_getargspec(sage.sets.disjoint_set.OP_represent) gets interpreted in sageinspect.py like this:

is it an abstract method, line 1284? No.
is it callable, line 1287? Yes.
try to read the argspec, line 1290 - AttributeError line 1291, pass (line 1292)!
is it a function, line 1293? No.
is it a method, line 1295? No.
is it a class instance, line 1297? No.
is it a class, line 1311? No.
does it have some attributes, line 1314? No.
fallback to the 'else' of line 1318!
let's call sage_getsource, line 1320, and get None.
line 1321-1324: since we got None, we set func_obj=obj.
try something line 1328 and fail with AttributeError
catch this exception and try something else line 1331 and fail with TypeError
catch this exception line 1332
let's do (no try this time): "return inspect.ArgSpec(*_sage_getargspec_cython(sage_getsource(obj)))", because obviously, the fact that a few lines above sage_getsource got us None already won't be a problem! BOOM!

I interpret those findings as meaning that something with my patches has made sage_getsource(obj) return None, and this then uncovered a nice bug in sageinspect. Does this sound sensible? Should I then file a bug against sageinspect.py? [Of course, I'll now investigate why None gets returned]

SnarkBoojum · 2013-05-23T12:29:35Z

comment:44

Oh, I forgot to make explicit the remark that sage_getsource returning None could be related to a "File not found" problem -- so now it makes sense that the problem can be related to my patches. Something which wasn't that clear. So now I'm looking for a problem, and I believe in it :-P

SnarkBoojum · 2013-05-23T12:50:47Z

comment:45

I now determined how sage_getsource(sage.sets.disjoint_set.OP_represent) gets interpreted in sageinspect.py:

try to use the _sage_src_ method and raise an AttributeError, line 1552 (gets pass-ed)
try to use sage_getsourcelines, line 1556 and get None
after a test of the previous value, return None, line 1558

The problems turns into: why does sage_getsourcelines(sage.sets.disjoint_set.OP_represent) return None? Let's follow the trail again:

try to use the _sage_src_lines_ method and raise an AttributeError line 1783 (gets pass-ed)
is it a class instance, line 1791? No.
get a nice-looking doctring line 1802, except that the filename reads 'res_pyx.pxi'
try to use _extract_embedded_position on it... get to line 200... where the filename gets obtained by os.path.join(SAGE_SRC, res.group('FILENAME'))
from there, the rest is history: we have a wrong filename in a wrong path, so it's no surprise things get messy.

So now I see two problems there: first, the only file with a name looking like this is sage/groups/perm_gps/partn_ref/data_structures_pyx.pxi, and second, the way the full path is obtained from the filename looks too naive.

How can I debug the first problem?

SnarkBoojum · 2013-05-23T13:09:21Z

comment:46

"sage.sets.disjoint_set.OP_represent" is defined in sage/groups/perm_gps/partn_ref/data_structures_pyx.pxi, and indeed at line 125 as I see in the docstring.
The line number is correct ; only the filename looks like it has been eaten.

SnarkBoojum · 2013-05-23T13:11:30Z

comment:47

On the non-patched sage-5.10.beta5 the filename is "sage/sets/../groups/perm_gps/partn_ref/data_structures_pyx.pxi".

vbraun · 2013-05-23T14:58:01Z

comment:48

The filename is already wrong in the Cythonized disjoint_set.c:

static char __pyx_doc_4sage_4sets_12disjoint_set_OP_represent[] = "File: pyx.pxi (starting at line 125)\n\n    Demonstration and testing.\n    \n    TESTS::\n\n        [...]        Done.\n\n    ";

So there is nothing in this ticket that is responsible for it.

vbraun · 2013-05-23T15:06:58Z

comment:49

The bug is in Cython Nodes.relative_position, which assumes that you can make the path relative by stripping off len(os.path.abspath(os.getcwd())) characters. Thats of course totally wrong when the file was discovered through a search path.

vbraun · 2013-05-23T15:11:46Z

comment:50

See #14634 for the Cython bug

jdemeyer · 2013-05-23T15:59:47Z

Dependencies: #14634

vbraun · 2013-05-23T17:48:59Z

Initial patch

vbraun · 2013-05-23T17:50:18Z

comment:52

Attachment: trac_12728_docbuild_workaround.patch.gz

I suggest we just revert the two includes that trigger the Cython bug.

vbraun · 2013-05-23T17:50:18Z

Changed dependencies from #14634 to none

SnarkBoojum · 2013-05-23T19:00:15Z

comment:53

I'm happy with your solution. I rebuilt sagelib adding your patch to my two patches, then built the doc from there : perfect. Positive review!

SnarkBoojum · 2013-05-23T19:00:15Z

Changed reviewer from Volker Braun, Jeroen Demeyer to Volker Braun, Jeroen Demeyer, Julien Puydt

jdemeyer · 2013-05-24T09:39:55Z

Merged: sage-5.10.beta5

SnarkBoojum mannequin added this to the sage-5.10 milestone Mar 22, 2012

SnarkBoojum mannequin added c: build labels Mar 22, 2012

SnarkBoojum mannequin self-assigned this Mar 22, 2012

SnarkBoojum mannequin added the s: needs review label Mar 24, 2012

jdemeyer added s: needs work and removed s: needs review labels Aug 8, 2012

SnarkBoojum mannequin added s: needs review and removed s: needs work labels Jan 7, 2013

This comment has been minimized.

Sign in to view

jdemeyer added the s: needs work label May 23, 2013

vbraun added s: positive review and removed s: needs work labels May 23, 2013

jdemeyer removed this from the sage-5.10 milestone May 23, 2013

jdemeyer added the pending label May 23, 2013

This comment has been minimized.

Sign in to view

vbraun added this to the sage-5.10 milestone May 23, 2013

vbraun removed the pending label May 23, 2013

jdemeyer removed the s: positive review label May 24, 2013

jdemeyer closed this as completed May 24, 2013

This was referenced Sep 24, 2013

Make mod_int signed and speed up matrix_modn_dense_float #14627

Closed

Cython sometimes embeds wrong file names in docstrings #14634

Closed

sage's spkg sources should use correct include paths instead of having ../../../../whatever everywhere #12728

sage's spkg sources should use correct include paths instead of having ../../../../whatever everywhere #12728

Comments

SnarkBoojum mannequin commented Mar 22, 2012

jdemeyer commented Apr 2, 2012

SnarkBoojum mannequin commented Apr 2, 2012

jdemeyer commented Jul 27, 2012

SnarkBoojum mannequin commented Aug 6, 2012

kiwifb commented Aug 8, 2012

SnarkBoojum mannequin commented Aug 8, 2012

simon-king-jena commented Jan 6, 2013

simon-king-jena commented Jan 6, 2013

SnarkBoojum mannequin commented Jan 6, 2013

SnarkBoojum mannequin commented Jan 6, 2013

SnarkBoojum mannequin commented Jan 7, 2013

jdemeyer commented Jan 12, 2013

SnarkBoojum mannequin commented Jan 13, 2013

kiwifb commented Jan 13, 2013

SnarkBoojum mannequin commented Jan 13, 2013

kiwifb commented Jan 13, 2013

jdemeyer commented Jan 13, 2013

This comment has been minimized.

SnarkBoojum mannequin commented Jan 13, 2013

jdemeyer commented Jan 13, 2013

SnarkBoojum mannequin commented May 23, 2013

vbraun commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

vbraun commented May 23, 2013

vbraun commented May 23, 2013

vbraun commented May 23, 2013

jdemeyer commented May 23, 2013

vbraun commented May 23, 2013

vbraun commented May 23, 2013

This comment has been minimized.

vbraun commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

SnarkBoojum mannequin commented May 23, 2013

jdemeyer commented May 24, 2013