Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip building trees for nodes with invalid tag name #5690

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

oleksandr-pavlyk
Copy link
Contributor

This change attempts to exclude arguments with invalid tag names from being inserted into the TreeBuilder as per comment from gh-5552.

It does resolve the exception reported in gh-5552 and in gh-5686, but I am not sure whether this is the best way to implement the idea to skip nodes with invalid names.

Please feel free to suggest improvements.

This change attempts to exclude arguments with invalid tag names
from being inserted into the TreeBuilder
@oleksandr-pavlyk
Copy link
Contributor Author

@da-woods @scoder Please look this change over and provide guidance.

Copy link
Contributor

@da-woods da-woods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing it's missing is a test - probably just to check that a generator expression doesn't break it the build

def f(a, b):
    return (x for x in range(a, b))

is probably sufficient to hit the bug I think.

Unfortunately I'm not really sure how gdb mode is tested: probably in Cython/Debugger/Tests but I've never looked in there myself

Cython/Debugger/DebugWriter.py Outdated Show resolved Hide resolved
Documented the purpose of is_valid_tag function in its docstring.
@oleksandr-pavlyk
Copy link
Contributor Author

I attempted to validate the fix by locally running cygdb, but could not get it to work.

python ../cython/cython.py --cplus --gdb a.pyx --output-file a.cxx
g++ -fPIC a.cxx $(python3-config --includes) $(python3-config --ldflags) -shared -o a$(python3-config --extension-suffix)

Then

(dev_dpctl) opavlyk@opavlyk-mobl:~/repos/cython-crash$ cygdb
GNU gdb (Ubuntu 9.2-0ubuntu1~20.04.1) 9.2
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
There was an error in Python code originating from the file /home/opavlyk/miniconda3/envs/dev_dpctl/lib/python3.9/site-packages/Cython/Debugger/Cygdb.py
It used the Python interpreter /usr/bin/python
Traceback (most recent call last):
  File "<string>", line 11, in <module>
ModuleNotFoundError: No module named 'Cython'

@tornaria
Copy link
Contributor

tornaria commented Sep 9, 2023

Works for me. It fixes the minimal example I made up for #5552, but in fact it fixes compilation of the whole of sagemath.

A test as suggested above (or as the one made up for #5552) would be a good thing. Bear in mind that the failure only occurs when lxml python package is installed.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we really need to change start() and end() as well, but it's probably better to be consistent.

Cython/Debugger/DebugWriter.py Outdated Show resolved Hide resolved
Cython/Debugger/DebugWriter.py Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Sep 9, 2023

Looks good to me now.

@oleksandr-pavlyk oleksandr-pavlyk marked this pull request as ready for review September 9, 2023 13:09
@matusvalo matusvalo added this to the 3.0.3 milestone Sep 11, 2023
@scoder scoder merged commit 72172c6 into cython:master Sep 14, 2023
76 checks passed
@scoder
Copy link
Contributor

scoder commented Sep 14, 2023

Thanks

tornaria added a commit to tornaria/void-packages that referenced this pull request Oct 10, 2023
cython/cython#5690
cython/cython#5725

Both are merged in cython 3.0.3, however that release breaks scipy.
dimpase referenced this pull request in sagemath/sage Oct 10, 2023
ahesford pushed a commit to void-linux/void-packages that referenced this pull request Oct 10, 2023
cython/cython#5690
cython/cython#5725

Both are merged in cython 3.0.3, however that release breaks scipy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants