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: |
Description (last modified by )
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)
Change History (60)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
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.
comment:4 Changed 9 years ago by
Please fill in your real name as Author.
comment:5 Changed 9 years ago by
comment:6 Changed 9 years ago by
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
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
- Status changed from needs_review to needs_work
comment:9 Changed 8 years ago by
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
See #13916.
comment:11 Changed 8 years ago by
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
comment:12 Changed 8 years ago by
- 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
Please make it clear what should be applied.
comment:14 Changed 8 years ago by
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).
comment:15 Changed 8 years ago by
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.
comment:16 follow-up: ↓ 18 Changed 8 years ago by
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
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
- 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.
comment:19 follow-up: ↓ 20 Changed 8 years ago by
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
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
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.
comment:22 Changed 8 years ago by
- 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
- Description modified (diff)
- Reviewers set to Volker Braun
Looks good to me!
comment:24 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:25 Changed 8 years ago by
You may have made some publicity for it earlier ... ;-)
comment:26 Changed 8 years ago by
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 :-)
comment:27 Changed 8 years ago by
- Status changed from positive_review to needs_work
comment:28 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:29 Changed 8 years ago by
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
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
- Description modified (diff)
comment:32 Changed 8 years ago by
- Description modified (diff)
comment:33 Changed 8 years ago by
Can you rediff the first patch for sage-5.10.beta4
comment:34 Changed 8 years ago by
- 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
Positive review to trac_12728_5.10.beta3_hand.patch
comment:35 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:36 Changed 8 years ago by
- Description modified (diff)
comment:37 Changed 8 years ago by
- Status changed from needs_review to positive_review
Looks good.
Jeroen, can we merge this in beta5?
comment:38 Changed 8 years ago by
- Work issues rebase deleted
comment:39 Changed 8 years ago by
- 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
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
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
@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).
comment:43 Changed 8 years ago by
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]
comment:44 Changed 8 years ago by
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
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?
comment:46 Changed 8 years ago by
"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
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
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
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
- Status changed from needs_work to positive_review
See #14634 for the Cython bug
comment:51 Changed 8 years ago by
- Dependencies set to #14634
- Milestone changed from sage-5.10 to sage-pending
comment:52 Changed 8 years ago by
- 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
- 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
- Merged in set to sage-5.10.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
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.