oaubert / python-vlc

Python vlc bindings
GNU Lesser General Public License v2.1
381 stars 108 forks source link

Rewrite of the bindings' generator using Tree-sitter #275

Closed yanns1 closed 2 weeks ago

yanns1 commented 3 months ago

The most important change is the rewrite of Parser using Tree-sitter. The goal is to make the parsing of libvlc's header files more robust, maintainable and powerful than the current regex-based parsing.

In addition, a few secondary improvements have been made:

oaubert commented 3 months ago

Great! But... I tried the described method (on Debian/testing) python3 dev_setup.py failed with

Clone vendored C Tree-sitter grammar... Done.
Create a virtual environment in .venv... Done.
Upgrade pip... Traceback (most recent call last):
  File "/home/oaubert/doc/projects/mast/ptrans/python-vlc/dev_setup.py", line 39, in <module>
    proc = run(cmd, capture_output=True, check=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['.venv/bin/python3', '-m', 'pip', 'install', '--upgrade', 'pip']' returned non-zero exit status 1.
(.venv) oaubert@gnozyme:mast/ptrans/python-vlc>which python3
/home/oaubert/doc/projects/mast/ptrans/python-vlc/.venv/bin/python3
(.venv) oaubert@gnozyme:mast/ptrans/python-vlc>python3 -m pip install --upgrade pip
ERROR: Can not perform a '--user' install. User site-packages are not visible in this virtualenv.

I could solve this by editing .venv/pyvenv and changing include-system-site-packages to true, but it is not quite satisfactory.

The first point would be to have the dev_setup.py display error messages when there are errors, and do some more checks: do not execute venv if it is already created, etc. Maybe a plain shell script would be easier. I tried in a python3 docker image, and the devscript executed correctly. But the make failed with a OSError: build/c.so: cannot open shared object file: No such file or directory Indeed, build is a link to /home/oaubert/.cache/tree-sitter/lib which does not exist.

yanns1 commented 3 months ago

Ok for making better errors and checking if venv already present etc.

We made a Python script so as to have one script work on multiple OS, but indeed a shell script might be easier.

For the error on pip upgrade, I suspect it is because the environment variable PIP_USER is set to 1/true (did you run it in Docker? because others seem to have the issue to, see https://stackoverflow.com/q/73962053). Solution: env PIP_USER=false in your Dockerfile (similar issue: https://github.com/gitpod-io/gitpod/issues/1997)

For the OSError, I don't understand how build became a link. This is the method Language.build_library from py-tree-sitter that create build if necessary when creating the parser c.so.

oaubert commented 3 months ago

My bad for the OSError - I had modified generate.py some time ago, and it kept it.