Don't include absolute paths when using --embed-positions#6755
Don't include absolute paths when using --embed-positions#6755da-woods merged 8 commits intocython:masterfrom
--embed-positions#6755Conversation
scoder
left a comment
There was a problem hiding this comment.
It looks like you always call .as_posix() on the return value of get_relative_path(). So, why return a pathlib.Path instead of a plain string directly?
| def get_relative_path(self): | ||
| return Path(self.get_description()) | ||
|
|
||
| get_error_description = get_description | ||
|
|
||
| def get_filenametable_entry(self): |
There was a problem hiding this comment.
We previously used "stringsource>" here and you changed it to self.name. Why?
There was a problem hiding this comment.
I'm not sure how the previous code worked in this regard. If you would use stringsource> here, then above in the Compilor/<x>Nodes you would call Path("stringsource>") which doesn't look right. I suspect that it got filtered out by the isabs check, which then falls back to the description > name.
Co-authored-by: scoder <stefan_ml@behnel.de>
|
@scoder Anything left to do here? |
sagemathgh-40686: Install pyx sources with meson Currently the meson build doesn't install the pyx source files, as opposed to the setuptools build. This has caused/is causing several issues: - The method `sage_getsource` as well as ipython's own source viewer `??` don't work for cython methods. This creates an inconsistency with python methods, and breaks several tests when the sage source is not available (eg. distro packages). - It breaks the doc builder sagemath#39973 (comment) - It used to cause issues when accessing some methods (sagemath#39735), now worked around. The only reason why these issues are not present in sage-the-distro is that cython wrongly embeds the absolute paths of source files, which masks the issues as long as the source is present in its original location. But as soon as cython/cython#6755 is released this will affect all types of sage build. In any case, I believe being able to inspect the methods implementation from the sage interface itself is a valuable feature for mathematicians, which are its main user base. I have only done one dir so far so I don't waste my time in case this is rejected or a different implementation is proposed. I will do the rest when I get the OK. Fixes sagemath#39874 URL: sagemath#40686 Reported by: Antonio Rojas Reviewer(s): Tobias Diez
sagemathgh-40686: Install pyx sources with meson Currently the meson build doesn't install the pyx source files, as opposed to the setuptools build. This has caused/is causing several issues: - The method `sage_getsource` as well as ipython's own source viewer `??` don't work for cython methods. This creates an inconsistency with python methods, and breaks several tests when the sage source is not available (eg. distro packages). - It breaks the doc builder sagemath#39973 (comment) - It used to cause issues when accessing some methods (sagemath#39735), now worked around. The only reason why these issues are not present in sage-the-distro is that cython wrongly embeds the absolute paths of source files, which masks the issues as long as the source is present in its original location. But as soon as cython/cython#6755 is released this will affect all types of sage build. In any case, I believe being able to inspect the methods implementation from the sage interface itself is a valuable feature for mathematicians, which are its main user base. I have only done one dir so far so I don't waste my time in case this is rejected or a different implementation is proposed. I will do the rest when I get the OK. Fixes sagemath#39874 URL: sagemath#40686 Reported by: Antonio Rojas Reviewer(s): Tobias Diez
|
Ping. Having this merged is important to fix issues with the meson build of sagemath. |
|
@tobiasdiez Do you think you can rebase this patch? For me, it does not apply cleanly on top of 3.2.4. |
|
For me it does not apply cleanly on top of master branch: I had to edit by hand a few changes to |
da-woods
left a comment
There was a problem hiding this comment.
It looks good to me.
My main worry is that I don't quite know where all these paths are actually used. I think we have:
- code objects (probably mostly cosmetic but I imagine some people are using them in debugger type tools)
- tracebacks (again, mostly cosmetic but not helpful if they break)
- docstrings (mostly cosmetic, probably some people using them. I think this is what sage is trying to use)
My view is that we should probably merge it into the master branch (on the basis that it's consistent with what we try to do generally) and see if it causes any issues. But it's definitely the sort of change that feels a bit like it's going to cause unexpected problems.
Cython/Compiler/ModuleNode.py
Outdated
There was a problem hiding this comment.
Probably want to remove these - I think they're both unused (although isabs was used before your change)
There was a problem hiding this comment.
Thanks, removed them.
Remove unused imports from ModuleNode.py
|
Thanks. I think it makes sense to merge this and see how it works. |
|
Thanks for the review and merge! |
) Currently, invoking cython with `embed-positions` from a folder not containing the source code, will result in the absolute paths being embedded. This is fixed by always embedding the relative paths. Since there are a couple of places where a relative path of a `SourceDescriptor` is needed, a new method `get_relative_path` is introduced and reused.
Currently, invoking cython with
embed-positionsfrom a folder not containing the source code, will result in the absolute paths being embedded. This is fixed by always embedding the relative paths.Since there are a couple of places where a relative path of a
SourceDescriptoris needed, a new methodget_relative_pathis introduced and reused.