Opened 9 years ago

Closed 8 years ago

#12728 closed enhancement (fixed)

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

Reported by: Snark Owned by: Snark
Priority: minor Milestone: sage-5.10
Component: build Keywords:
Cc: Merged in: sage-5.10.beta5
Authors: Julien Puydt Reviewers: Volker Braun, Jeroen Demeyer, Julien Puydt
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by vbraun)

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

Attachments (6)

updirectories-sed.sh (1.1 KB) - added by Snark 8 years ago.
Shell script which patches 99,99% of the issues
trac_12728_5.10.beta3.patch (155.4 KB) - added by Snark 8 years ago.
Patch created applying the script agains 5.10.beta3
trac_12728_5.10.beta3_hand.patch (5.8 KB) - added by Snark 8 years ago.
Hand-crafted finalization of the fix
trac_12728_5.10.beta4_auto.patch (155.5 KB) - added by Snark 8 years ago.
Patch created applying the script agains 5.10.beta4
trac_12728_5.10.beta4_hand.patch (5.8 KB) - added by Snark 8 years ago.
Hand-crafted finalization of the fix
trac_12728_docbuild_workaround.patch (1.1 KB) - added by vbraun 8 years ago.
Initial patch

Download all attachments as: .zip

Change History (60)

comment:1 Changed 9 years ago by Snark

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by jdemeyer

I don't like 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.

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

comment:3 Changed 9 years ago by Snark

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

  1. it doesn't give any serious information, so why is it here?
  2. 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.

comment:4 Changed 9 years ago by jdemeyer

Please fill in your real name as Author.

comment:5 Changed 9 years ago by Snark

  • Authors set to Julien Puydt

comment:6 Changed 9 years ago by fbissey

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/'?

comment:7 Changed 9 years ago by Snark

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.

comment:8 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:9 Changed 8 years ago by SimonKing

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.

comment:10 Changed 8 years ago by SimonKing

See #13916.

comment:11 Changed 8 years ago by Snark

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

Changed 8 years ago by Snark

Shell script which patches 99,99% of the issues

comment:12 Changed 8 years ago by Snark

  • Status changed from needs_work to needs_review

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.

comment:13 Changed 8 years ago by jdemeyer

Please make it clear what should be applied.

comment:14 Changed 8 years ago by Snark

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

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

comment:15 Changed 8 years ago by fbissey

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.

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

comment:16 follow-up: Changed 8 years ago by Snark

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.

comment:17 Changed 8 years ago by fbissey

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.

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

  • Description modified (diff)

Replying to Snark:

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.

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

comment:19 follow-up: Changed 8 years ago by Snark

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.

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

Replying to Snark:

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.

comment:21 Changed 8 years ago by Snark

You rejected the patches, but trac doesn't allow to mark them as rejected as far as I know... so they still sit there, unfortunately.

Changed 8 years ago by Snark

Patch created applying the script agains 5.10.beta3

comment:22 Changed 8 years ago by Snark

  • Description modified (diff)

Ok, I applied the script on sage 5.10.beta3 ; it doesn't make new doctests fail (the quadratic forms code doesn't work correctly on this box anyway).

comment:23 Changed 8 years ago by vbraun

  • Description modified (diff)
  • Reviewers set to Volker Braun

Looks good to me!

comment:24 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:25 Changed 8 years ago by leif

You may have made some publicity for it earlier ... ;-)

comment:26 Changed 8 years ago by Snark

If the patch built by my script gets applied for 5.10.beta4, there's even a chance I'll be able to work on the remaining problems for 5.10 :-)

Changed 8 years ago by Snark

Hand-crafted finalization of the fix

comment:27 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:28 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:29 Changed 8 years ago by jdemeyer

If trac_12728_5.10.beta3_hand.patch should be applied, it should be reviewed and mentioned in the ticket description.

comment:30 Changed 8 years ago by jdemeyer

I also don't like two ways of applying a patch, because which one is "correct"? Either only mention the script, or only the patch.

comment:31 Changed 8 years ago by Snark

  • Description modified (diff)

comment:32 Changed 8 years ago by Snark

  • Description modified (diff)

comment:33 Changed 8 years ago by vbraun

Can you rediff the first patch for sage-5.10.beta4

comment:34 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Volker Braun to Volker Braun, Jeroen Demeyer
  • Status changed from needs_review to needs_work
  • Work issues set to rebase

Changed 8 years ago by Snark

Patch created applying the script agains 5.10.beta4

Changed 8 years ago by Snark

Hand-crafted finalization of the fix

comment:35 Changed 8 years ago by Snark

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

comment:36 Changed 8 years ago by Snark

  • Description modified (diff)

comment:37 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks good.

Jeroen, can we merge this in beta5?

comment:38 Changed 8 years ago by jdemeyer

  • Work issues rebase deleted

comment:39 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This causes problems with the HTML documentation:

dochtml.log:[rings_sta] /mazur/release/merger/sage-5.10.beta5/devel/sage/doc/en/reference/rings_standard/sage/rings/integer.rst:11: WARNING: error while formatting signature for sage.rings.integer.parent: 'NoneType' object has no attribute 'find'
dochtml.log:[structure] /mazur/release/merger/sage-5.10.beta5/devel/sage/doc/en/reference/structure/sage/sets/disjoint_set.rst:11: WARNING: error while formatting signature for sage.sets.disjoint_set.OP_represent: 'NoneType' object has no attribute 'find'
dochtml.log:[structure] /mazur/release/merger/sage-5.10.beta5/devel/sage/doc/en/reference/structure/sage/sets/disjoint_set.rst:11: WARNING: error while formatting signature for sage.sets.disjoint_set.PS_represent: 'NoneType' object has no attribute 'find'
dochtml.log:[structure] /mazur/release/merger/sage-5.10.beta5/devel/sage/doc/en/reference/structure/sage/sets/disjoint_set.rst:11: WARNING: error while formatting signature for sage.sets.disjoint_set.SC_test_list_perms: 'NoneType' object has no attribute 'find'

comment:40 Changed 8 years ago by Snark

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...

comment:41 Changed 8 years ago by vbraun

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)

comment:42 Changed 8 years ago by Snark

@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 :

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

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

comment:43 Changed 8 years ago by Snark

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

  1. is it an abstract method, line 1284? No.
  2. is it callable, line 1287? Yes.
  3. try to read the argspec, line 1290 - AttributeError? line 1291, pass (line 1292)!
  4. is it a function, line 1293? No.
  5. is it a method, line 1295? No.
  6. is it a class instance, line 1297? No.
  7. is it a class, line 1311? No.
  8. does it have some attributes, line 1314? No.
  9. fallback to the 'else' of line 1318!
  10. let's call sage_getsource, line 1320, and get None.
  11. line 1321-1324: since we got None, we set func_obj=obj.
  12. try something line 1328 and fail with AttributeError?
  13. catch this exception and try something else line 1331 and fail with TypeError?
  14. catch this exception line 1332
  15. 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]

comment:44 Changed 8 years ago by Snark

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

comment:45 Changed 8 years ago by Snark

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

  1. try to use the _sage_src_ method and raise an AttributeError?, line 1552 (gets pass-ed)
  2. try to use sage_getsourcelines, line 1556 and get None
  3. 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:

  1. try to use the _sage_src_lines_ method and raise an AttributeError? line 1783 (gets pass-ed)
  2. is it a class instance, line 1791? No.
  3. get a nice-looking doctring line 1802, except that the filename reads 'res_pyx.pxi'
  4. 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'))
  5. 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?

comment:46 Changed 8 years ago by Snark

"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.

comment:47 Changed 8 years ago by Snark

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

comment:48 Changed 8 years ago by vbraun

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.

comment:49 Changed 8 years ago by vbraun

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.

comment:50 Changed 8 years ago by vbraun

  • Status changed from needs_work to positive_review

See #14634 for the Cython bug

comment:51 Changed 8 years ago by jdemeyer

  • Dependencies set to #14634
  • Milestone changed from sage-5.10 to sage-pending

Changed 8 years ago by vbraun

Initial patch

comment:52 Changed 8 years ago by vbraun

  • Dependencies #14634 deleted
  • Description modified (diff)
  • Milestone changed from sage-pending to sage-5.10

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

comment:53 Changed 8 years ago by Snark

  • Reviewers changed from Volker Braun, Jeroen Demeyer to Volker Braun, Jeroen Demeyer, Julien Puydt

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!

comment:54 Changed 8 years ago by jdemeyer

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