-
Notifications
You must be signed in to change notification settings - Fork 100
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
Conversation
There was a problem hiding this 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.
py-limited-api = "auto" | ||
args = ["--profile", "release-lto"] | ||
|
||
[[tool.setuptools-rust.binaries]] # executable to be installed in `$venv/bin` |
There was a problem hiding this comment.
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]]
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
Hi @davidhewitt, thank you very much for the review and thoughtful comments.
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? |
I would be interested in leaving at least 1 example ( |
Thanks, I think just |
(Also looks like |
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. |
16bb854
to
06e69c1
Compare
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. |
06e69c1
to
e7db473
Compare
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 |
e7db473
to
f2866e4
Compare
|
||
/// Calls Python (see https://pyo3.rs for details) | ||
#[pyfunction] | ||
fn demo(py: Python) -> PyResult<()> { |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
f2866e4
to
22e291e
Compare
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 |
macOS fixed in #351 |
Change examples in accordance to review comments in #348
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 usingpyproject.toml
instead.For an example of how this would work, see
examples/hello-world-pyprojecttoml/pyproject.toml
.Approach:
setuptools
exposes asetuptools.finalize_distribution_options
hook that allows reusing the logic implemented insetuptools_rust.setuptools_ext.rust_extensions
for modifying thedist
object based on thepyproject.toml
config.setup.py
is currently used bysetuptools_rust
topyproject.toml
Cargo.toml
info to automatically fill in the fields in thedist
object, even things likename
orversion
), but this is the most obvious/easier approach, so I just went with it.RustExtension
andRustBin
are translated directly topyproject.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"
.RustExtension
andRustBin
, the user can specify[[tool.setuptools-rust.ext-modules]]
and[[tool.setuptools-rust.binaries]]
.Remarks:
pyproject.toml
, please feel free to close this PR.