- 
                Notifications
    You must be signed in to change notification settings 
- Fork 106
          Allow users to use pyproject.toml define RustExtension and RustBin (instead of setup.py)
          #348
        
          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
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 NoneThank 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.tomlinstead.For an example of how this would work, see
examples/hello-world-pyprojecttoml/pyproject.toml.Approach:
setuptoolsexposes asetuptools.finalize_distribution_optionshook that allows reusing the logic implemented insetuptools_rust.setuptools_ext.rust_extensionsfor modifying thedistobject based on thepyproject.tomlconfig.setup.pyis currently used bysetuptools_rusttopyproject.tomlCargo.tomlinfo to automatically fill in the fields in thedistobject, even things likenameorversion), but this is the most obvious/easier approach, so I just went with it.RustExtensionandRustBinare translated directly topyproject.toml, the only differences are:pyproject.tomluses-instead of_(by convention, see PEP 517/621)enumobjects are represented in a "stringfied manner", e.g.Binding.PyO3 => "PyO3".RustExtensionandRustBin, the user can specify[[tool.setuptools-rust.ext-modules]]and[[tool.setuptools-rust.binaries]].Remarks:
pyproject.toml, please feel free to close this PR.