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

feat: Add Python bindings #269

Merged
merged 12 commits into from
Jan 3, 2024
Merged

feat: Add Python bindings #269

merged 12 commits into from
Jan 3, 2024

Conversation

DataTriny
Copy link
Member

@DataTriny DataTriny commented Aug 11, 2023

Adds a new crate containing bindings for the Python programming language.

Wheels will be automatically uploaded to pypi.org for the most common platforms/architectures.

The pygame example is on par with the SDL example, except for Linux where hit-testing is probably not possible due to pygame not exposing window real size.

I'm not 100% sure about the unsendable types: is it enough to just document the fact that they must only be used from the main thread? Or should we protect users more?

TODO

  • Create a PYPI_TOKEN repository secret

@mwcampbell
Copy link
Contributor

Can we configure pyo3 to rename the enum variants so they follow the ALL_CAPS_SNAKE_CASE style that's typical of Python, rather than the CamelCase style of Rust?

@mwcampbell
Copy link
Contributor

I suggest we format the example using Black.

@DataTriny
Copy link
Member Author

Can we configure pyo3 to rename the enum variants so they follow the ALL_CAPS_SNAKE_CASE style that's typical of Python, rather than the CamelCase style of Rust?

There is no automatic way of achieving this unfortunately. We'd have to add #[pyo3(name = "...")] to every enum variants.

I thought it was not a good idea to create new enums for this single purpose.

@mwcampbell
Copy link
Contributor

I really don't want to ship the Python bindings with that obviously unidiomatic style. That's the kind of thing that would be really tedious to fix later, particularly for users. I suggest implementing a derive macro to automatically add the PyO3-specific attribute on all the enum variants. This means adding a crate like accesskit_macros (could be in a top-level macros directory). Such a crate should only be in the dependencies of the base accesskit crate if the pyo3 feature is enabled.

@mwcampbell
Copy link
Contributor

Can we remove Black from the requirements.txt file that we provide with the example? Users don't need Black in order to run the example.

@DataTriny
Copy link
Member Author

I suggest implementing a derive macro to automatically add the PyO3-specific attribute on all the enum variants.

After some testing, I'm not even sure if it's possible. If I add #[pyo3] attributes from a proc macro, the common crate refuses to compile because it doesn't understand the link between the #[pyo3] attribute and the #[pyclass] attribute.

Manually adding the full attribute that would be needed, that is #[cfg_attr(feature = "pyo3", pyo3(name = "UNKNOWN"))] to the first variant of the Role enum, triggers exactly the same error:

error: cannot find attribute `pyo3` in this scope
  --> common/src/lib.rs:57:34
   |
57 |     #[cfg_attr(feature = "pyo3", pyo3(name = "UNKNOWN"))]
   |                                  ^^^^
   |
   = note: `pyo3` is in scope, but it is a crate, not an attribute

Unless I miss something, implementing this directly inside PyO3 might be the only solution.

@DataTriny
Copy link
Member Author

I've opened PyO3/pyo3#3383 in that direction. But if we want to quickly ship something, duplicating the enums might be the only option.

@mwcampbell
Copy link
Contributor

I'm not in a hurry to release the Python bindings. Let's take the time to do it right. So I think we should wait for the response to the new PyO3 issue and see if either we or the PyO3 developers can implement something that helps all PyO3 users with this problem.

@DataTriny
Copy link
Member Author

My work to support renaming enum variants was merged into PyO3 so I'm switching to temporarily pull the master branch. There is discussion to release PyO3 0.20 in a few weeks.

As always, if someone can test the pygame example on macOS and report back here that would be nice. I would be surprised if it doesn't work out of the box since it's SDL under the hood.

@mwcampbell
Copy link
Contributor

Unfortunately, the Pygame example does not run on macOS. Attempting to instantiate the macOS adapter fails with the following error:

TypeError: argument 'window': 'PyCapsule' object cannot be interpreted as an integer

@DataTriny
Copy link
Member Author

Thanks @mwcampbell for the feedback.

Looking at pygame's source code it's now obvious. It's actually the safest way to pass a pointer, but I'm not sure we should only expect to receive a PyCapsule here. I'll see how other windowing libraries do this.

@LeonarddeR
Copy link
Contributor

I suggest we format the example using Black.

May be a bit controversial, but I'd suggest Ruff instead. Though not yet stable, it is supposed to be a Black replacement in the end. Moreover, it is also Rust and it has a linter on board as well.

@DataTriny
Copy link
Member Author

While rebasing this I played a bit with ruff this morning. My understanding is that it doesn't do formatting but linting. As such it would not replace black. It was able to point me to a couple of issues in the pygame example, the question is: do we want to run these two tools just for one Python file? This would mean adding another step in the formatting CI job.

@DataTriny DataTriny force-pushed the python_bindings branch 2 times, most recently from 531ac2a to e746a50 Compare October 14, 2023 11:12
@DataTriny
Copy link
Member Author

@mwcampbell The Pygame example should work on macOS hopefully now. Can you please confirm?

@LeonarddeR
Copy link
Contributor

While rebasing this I played a bit with ruff this morning. My understanding is that it doesn't do formatting but linting. As such it would not replace black. It was able to point me to a couple of issues in the pygame example, the question is: do we want to run these two tools just for one Python file? This would mean adding another step in the formatting CI job.

Ruff has an experimental formatter that should be pretty similar to Black in its output. While it is not yet stable, given this project doesn't have that much Python code, I think it should be sufficient.

Copy link

@simlay simlay left a comment

Choose a reason for hiding this comment

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

Hope you don't mind a random drive by review, I saw this and felt like I might have some useful words. Feel free to disregard my comments if they're outside the goals of this PR. It's cool to see more Rust-Python wheels!

- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: 3.7
Copy link

Choose a reason for hiding this comment

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

Python 3.7 was EOL'd on 2023-06-07. Also, this should probably match the python version in the release job which could be part of the matrix.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was OK when I started this PR, but it should be updated indeed.

Comment on lines 42 to 46
uses: PyO3/maturin-action@v1
with:
target: ${{ matrix.rust-target }}
manylinux: auto
args: --release --out dist --sdist
Copy link

Choose a reason for hiding this comment

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

It's my understanding (I could be wrong) that this will only build the wheel for the python version specified in the actions/python@v4 action (in this case 3.7). Getting a good matrix can be rough. In the past, I've used pypa/cibuildwheel as seen here which definitely has some negatives (the whole build runs in a docker container which can have different libraries than the host).

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. In fact, here is the link to the download page of a test upload I did: https://test.pypi.org/project/accesskit/#files

Copy link

Choose a reason for hiding this comment

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

TIL about test.pypi.org. Cool.

Well, I don't work on the Python client for Fluvio anymore (and it looks like there hasn't been a release in a while) but it's my recollection that pypa/cibuildwheel action took a long time (~30 minutes) because each of the python versions was done in serial.

One thing that I also recall being a pretty rough time was if there's OpenSSL 3 in the dependency tree. This uses a different version of perl than OpenSSL 1 and the pypa images didn't have it. I'm currently failing at finding the github issue for this :/

You might be able to get away with just adding a python-version list to your matrix and have them run in parallel. It's unclear what kind of target OS/arch/python version support you're looking for. pypa/cibuildwheel has a lot of python versions and targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have to check, maybe we have to link to OpenSSL on Linux through the zbus crate.

I see that some Rust projects only publish pre-compiled wheels for one Python version, is it acceptable? I know pretty much nothing about how Python packaging works, but I know that it didn't take long for my system to generate the wheel using the tarball. It includes the pre-built native binaries so at least the user doesn't have to do that.

Again, thank you very much for stopping by.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, zbus doesn't depend on OpenSSL. On Linux, we don't depend on any C libraries other than the base system libraries (libc, pthread, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't 100% sure as I am not familiar with D-Bus authentication mechanism, but no OpenSSL involved here thankfully!

Copy link
Member Author

Choose a reason for hiding this comment

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

Coming back to this: we actually only need to build wheels for the oldest Python version we want to support because I'm using the limited ABI3 feature which provide backward compatibility. Since we are not doing much from the Python side, it's best to enable it and only build for, let's say, Pyton 3.9.

@mwcampbell
Copy link
Contributor

Thanks @simlay for the review. We certainly need more feedback from people like you who have previous experience building Python packages with Rust.

@mwcampbell
Copy link
Contributor

Finally verified that the pygame example runs on macOS. Reviewing the code now.

@mwcampbell
Copy link
Contributor

I'm not sure I understand why it's necessary to place pyproject.toml at the root of the repository, rather than under bindings/python. Is it just so we can include the license files in the built packages, or is there another reason as well?

@mwcampbell
Copy link
Contributor

The doc comment for the Python Windows SubclassingAdapter class mentions the main thread. I suppose this was copied from the macOS version, but it's not applicable here.

There are still two references to Python 3.7: in the new CI workflow, and in the README for the pygame example.

My review is now complete.

@DataTriny
Copy link
Member Author

@mwcampbell I believe to have addressed all your comments.

I needed to test the CI on my fork before pushing it. I've attached the wheels to the GitHub release as well.

As for the macOS adapter, I went with a PyCapsule as it would be more cumbersome to create a c_void_p from Rust. I still had to cast the pointer though...

@DataTriny
Copy link
Member Author

I'd like to have #324 merged before we release accesskit_python 0.1 as the API of the Unix adapter will have to be updated.

@mwcampbell
Copy link
Contributor

The latest changes look good to me. So now I just have to figure out how we'll publish on PyPI.

@DataTriny
Copy link
Member Author

@mwcampbell I currently don't have access to a Linux desktop environment to test the pygame example. If you can do it before merging, otherwise I'll take care of it later. Thanks.

@DataTriny
Copy link
Member Author

The pygame example still works on Linux!

@mwcampbell
Copy link
Contributor

Just one more question before we merge this: It looks like the CI workflow doesn't currently use PyPI's "trusted publisher" feature. How hard would it be to use that feature?

@mwcampbell mwcampbell merged commit 52560da into main Jan 3, 2024
5 checks passed
@mwcampbell mwcampbell deleted the python_bindings branch January 3, 2024 21:35
@mwcampbell mwcampbell mentioned this pull request Jan 3, 2024
@DataTriny
Copy link
Member Author

I've setup the workflow to use this feature. You'll need to configure it on the PyPI side as described on this page.

Information you will have to provide as shown in a screenshot on the page:

  • Repository name: accesskit
  • Workflow name: python-bindings.yml
  • Environment name: release

Not sure about switching from your personal account to an organization though.

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.

4 participants