Skip to content

Conversation

@yanns1
Copy link
Contributor

@yanns1 yanns1 commented May 17, 2024

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:

  • The use of Tree-sitter allowed us to handle previously blacklisted, overridden or even ignored items.
    This includes functions in _blacklist (see generate.py), nested structs and unions and function pointers as function parameter or struct/union field.
    We thus made some changes to the generator (PythonGenerator) to take advantage of the newly parsed items, and actually generate bindings for them.
  • We moved from epydoc to Sphinx for documentation generation.
    epydoc becoming obsolete and Sphinx becoming the tool of choice in Python, we made the change.
    We thus had to convert libvlc source code comments written in Doxygen to the default Sphinx docstring format (see methods docs_in_sphinx_format).
  • On the development side, we put in place precommit hooks and Github actions to ensure that the code is properly formatted, has no errors and passes tests.
    We also made a little script (dev_setup.py) to quickly setup the project for new contributors (this is documented in the generator's README).

yanns1 and others added 30 commits January 11, 2024 18:39
@oaubert
Copy link
Owner

oaubert commented May 17, 2024

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
Copy link
Contributor Author

yanns1 commented May 17, 2024

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: gitpod-io/gitpod#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
Copy link
Owner

oaubert commented May 17, 2024

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

test2: $(MODULE_NAME)
PYTHONPATH=$(VERSIONED_PATH):$(PROJECT_ROOT) python tests/test.py
PYTHONPATH=$(DEV_PATH):$(PROJECT_ROOT) python tests/test.py
test_bindings2: $(MODULE_NAME)
Copy link
Owner

Choose a reason for hiding this comment

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

The test_* targets in Makefile depend on $(MODULE_NAME). If we try to build the module in a directory with no access to the dev includes (e.g a container where we installed libvlc-dev), $(DEV_INCLUDES) is empty, which causes an error in the $(MODULE_NAME) commands.

@oaubert oaubert merged commit 397b649 into oaubert:master Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants