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

Allow users to use pyproject.toml define RustExtension and RustBin (instead of setup.py) #348

Merged
merged 1 commit into from Aug 11, 2023

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Aug 3, 2023

This PR was created with the discussion of #208 in mind.

It allows users to specify Rust extensions in the same way they would using setup.py, but using pyproject.toml instead.
For an example of how this would work, see examples/hello-world-pyprojecttoml/pyproject.toml.

Approach:

  • setuptools exposes a setuptools.finalize_distribution_options hook that allows reusing the logic implemented in setuptools_rust.setuptools_ext.rust_extensions for modifying the dist object based on the pyproject.toml config.
  • What is implemented in this PR is pretty much a 1:1 translation between the way setup.py is currently used by setuptools_rust to pyproject.toml
    • I suppose it could be done in a smarter way (e.g. one could even try to use Cargo.toml info to automatically fill in the fields in the dist object, even things like name or version), but this is the most obvious/easier approach, so I just went with it.
  • The kwargs for the RustExtension and RustBin are translated directly to pyproject.toml, the only differences are:
    • pyproject.toml uses - instead of _ (by convention, see PEP 517/621)
    • enum objects are represented in a "stringfied manner", e.g. Binding.PyO3 => "PyO3".
  • To differentiate between RustExtension and RustBin, the user can specify [[tool.setuptools-rust.ext-modules]] and [[tool.setuptools-rust.binaries]].
    • I went with these names because they were the most obvious in my opinion but please feel free to change if you prefer something different.
  • The example I added follows what I would implement in a project of mine (a private module implemented in Rust + a layer of Python wrapping things). It also makes it easier.

Remarks:

  • I am not a Rust developer, so my example can be ugly. Please feel free to change it.
  • If the project would like to use a different approach for pyproject.toml, please feel free to close this PR.
  • Please feel free to modify the PR directly if you would like to tweak things around (e.g. any form of bike-shedding)

@abravalheri abravalheri marked this pull request as ready for review August 3, 2023 18:44
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Nice, this is delightfully simple and a greatly appreciated solution to something which I simply hadn't had time to look into.

Given that setup.py configuration is sort of deprecated, how do you feel about going further and migrating all examples (except perhaps for a hello-world-setuppy to test the setup.py form)?

We should also update documentation to cover (recommend?) this layout. Again, I'd be tempted to move the documentation of using setup.py to a secondary page in the docs and make this pyproject.toml form the preferred option.

examples/hello-world-pyprojecttoml/Cargo.toml Show resolved Hide resolved
py-limited-api = "auto"
args = ["--profile", "release-lto"]

[[tool.setuptools-rust.binaries]] # executable to be installed in `$venv/bin`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be [[tool.setuptools-rust.bin]] to match Cargo.toml's [[bin]]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any strong feelings about this change.

Can I leave this one to your criteria?

Once we have an agreement on all the changes in terminology, I can implement all of them in a single batch :P

Copy link
Member

Choose a reason for hiding this comment

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

I think bin is better than binary to match cargo, however I think given the comment below about plurals, let's go for bins perhaps? What do you think of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problems.

I implemented it in 1f58338.
(bins might sound a bit weird, but I think that the argument you made about consistency is important).

examples/hello-world-pyprojecttoml/pyproject.toml Outdated Show resolved Hide resolved
@abravalheri
Copy link
Contributor Author

abravalheri commented Aug 4, 2023

Hi @davidhewitt, thank you very much for the review and thoughtful comments.

Given that setup.py configuration is sort of deprecated, how do you feel about going further and migrating all examples (except perhaps for a hello-world-setuppy to test the setup.py form)?

We should also update documentation to cover (recommend?) this layout. Again, I'd be tempted to move the documentation of using setup.py to a secondary page in the docs and make this pyproject.toml form the preferred option.

I am happy to help with that once we finalize the round of reviews and iron out the parts proposed in this PR. Maybe 2 follow up PRs? What do you think?

@abravalheri
Copy link
Contributor Author

Given that setup.py configuration is sort of deprecated, how do you feel about going further and migrating all examples (except perhaps for a hello-world-setuppy to test the setup.py form)?

I would be interested in leaving at least 1 example (hello-world-setuppy as you suggested) using setup.py, because setup.py is still a perfectly valid configuration file, but deprecated as a primary CLI tool. (I think a nice analogy is that setup.py is becoming a configuration file in the style of conftest.py or noxfile.py: you don't run them directly in the terminal, but they are used under the hood by the tools they are related to).

@davidhewitt
Copy link
Member

Thanks, I think just binaries -> bins and then this is good to merge. Follow-up PRs are fine with me and much appreciated 👍

@davidhewitt
Copy link
Member

(Also looks like black wants to format some more files :) )

@davidhewitt
Copy link
Member

Thanks @abravalheri, this looks good to me. I will rebase this and fix the merge conflict now. The macOS build failure is probably pre-existing, I'll merge this and see if I can work out a way to solve that separately to unblock you from any follow-up PRs you want to do.

@messense
Copy link
Member

The macOS build failure is probably pre-existing

I think the linking error is the same as PyO3/maturin#1080.

There is an implicit dependency from a binary to the library, see also rust-lang/cargo#9235.

@davidhewitt davidhewitt added the CI-no-fail-fast If one job fails, allow the rest to keep testing label Aug 11, 2023
@abravalheri
Copy link
Contributor Author

abravalheri commented Aug 11, 2023

 def pyprojecttoml_config(dist: Distribution) -> None:
-    with open("pyproject.toml", "rb") as f:
-        cfg = toml_load(f).get("tool", {}).get("setuptools-rust")
+    try:
+        with open("pyproject.toml", "rb") as f:
+            cfg = toml_load(f).get("tool", {}).get("setuptools-rust")
+    except FileNotFoundError:
+        return None

Thank you very much @davidhewitt. I oversaw this one :P


/// Calls Python (see https://pyo3.rs for details)
#[pyfunction]
fn demo(py: Python) -> PyResult<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be a warning here because py is unused. Sorry I am not experienced in Rust, so I got confused by the following Python::with_gil(|py| ... statement.

Suggested change
fn demo(py: Python) -> PyResult<()> {
fn demo(_py: Python) -> PyResult<()> {

If a maintainer would like to commit this to the PR, that would trigger the CI straight ahead. Otherwise I can add it myself and wait for someone to trigger the CI.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, I've force-pushed again. (Removed Python::with_gil bit, as we can use py directly from the argument.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Perfect, thank you @davidhewitt.

Still baby steps on Rust, so mainly copying the PyO3 examples and improvising from there :P

@davidhewitt
Copy link
Member

Ok great, so this is all happy now except for the macOS failure this uncovered. I'll merge this now and separately I'll look into a solution for the macOS issue. Thanks @abravalheri

@davidhewitt davidhewitt merged commit b4325fd into PyO3:main Aug 11, 2023
38 of 47 checks passed
@abravalheri abravalheri deleted the pyprojecttoml-defs branch August 11, 2023 10:53
@davidhewitt
Copy link
Member

macOS fixed in #351

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast If one job fails, allow the rest to keep testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants