PEP 517 integration

[pyo3]

PEP 517 integration

[ijl]

pip 19.0 supports PEP 517 now: https://pip.pypa.io/en/stable/news/.

[konstin]

I've finished sdist support in 78c516f and published a beta version with the (as far as I tested) complete PEP 517 support. Testing and experience reports with the new beta are welcome! The usage is documented in the readme, even though you need to specify pyo3-pack==0.7.0-beta.1 instead of just pyo3-pack or use pip install --pre.

[konstin]

I've tried implementing PEP 517 and also written a bit about that in the blog post, but it failed because pip hasn't implemented that interface yet. The current status is that you can build source distributions with disabled sdist feature, but can't install them. Writing and testing the PEP 517 interface is also blocked on pip, especially since I expect the implementation to differ from the PEP.

[althonos]

Yes I stumbled on your blog post after opening the issue 😉

Setuptools already has an implementation although I'm not sure if it's really used anywhere.

[ijl]

@konstin do you have any thoughts on how this should go? I see the pyo3-pack wheel just distributes a script right now, and this would need a library as well.

[konstin]

In the first step, we need to build an sdist. I had some support for this but dropped it. You can see the (likely reusable parts) at 96b0187#diff-3e8b6c785f497d88e8869ef9110c91ea and 96b0187#diff-b507bbbbb0351464abee9a55d2f759ed (and 96b0187 in general).

For the bindings actually implementing pep 517 we have afaik three options:

1. Python calling pyo3-pack with subprocess.
   Pro: Easy to implement. I'd say we can do everything in less than a hundred lines of python. We can even implement prepare_metadata_for_build_wheel with a new subcommand and some minor refactoring.
   Con: We need to ensure on all platforms and environments that we can find the pyo3-pack binary, that stdout/stderr work and that we can return the final sdist/wheel name.
2. cffi
   Pro: None of the frictions of the python script
   Con: cffi itself can't handle default arguments (because c can't), so we'd need to add a python script besides the cffi bindings, which pyo3-pack currently can't do. Worse is that there is afaik no proper error handling support
3. pyo3
   Pro: Pure rust solution (as in a single shared lib / no .py files) and proper error handling
   Con: We're stuck on nightly only pyo3 compiles on stable and need to publish a lot of wheels

cffi seems to be the worst option, but I don't have preference for pyo3 or subprocess. For subprocess, we can either add the script to the pyo3-pack package or make a miniature standalone package that depends on pyo3-pack. For the pyo3 version I think the best is to add an additional crate to this repo.

For all three solutions, we need to check what pip does when an exception is raised in one of the hooks. We also need to make check in pyo3-pack that cargo is installed and print an explanation otherwise (this can actually be done independent of pep 517).

[konstin]

I've implemented the wheel part of PEP 517 in 671fcdb, which also contains an untested sdist writer. The implementation uses the subprocess-solution.

I've filed tox-dev/tox#1344 for isolated builds with tox.

[omerbenamram]

Hi @konstin!
Thanks for your work on this :)
I've been playing with https://github.com/indygreg/PyOxidizer, and I wanted to bundle an application that uses a pyo3-packed rust library in it's deps.

PyOxidezer doesn't support wheels, so it was a good time try out sdist.
Unfortunely pip is really bad when it comes to unicode (pypa/pip#6566)

I've created a pyproject.toml file like:

[build-system]
requires = ["pyo3-pack>=0.7.0-beta.1", "toml"]
build-backend = "pyo3_pack"

and tried to pip install .

It seems to be this line:
https://github.com/PyO3/pyo3-pack/blob/master/pyo3_pack/__init__.py#L114

Which blows up because of the emojis in the output (windows doesn't like emojis yet).

WDYT?

[konstin]

Thank you for testing @omerbenamram! Seems I've got bitten by windows default encoding not being utf8 once again. That should be fixed in e7a6a09, and I've published 0.7.0-beta.2 with the fix.

[ijl]

I see building the .tar.gz requires pyproject.toml be packaged (via include). If it's not specified in the manifest, pip complains setup.py can't be found. Perhaps you can either error on build, or include it automatically, to avoid publishing broken artifacts?

[ijl]

I notice that if target/wheels doesn't exist, then pyo3-pack build fails with:

🔗 Found pyo3 bindings
🐍 Found CPython 3.7m at python3.7
💥 pyo3-pack failed
No such file or directory (os error 2)

I need to first create it via build --no-sdist.

[konstin]

@ijl Could you explain how you got a .tar.gz? I thought I checked for a pyproject.toml before building a source distribution.

Re the second issue: Good catch, I'll fix that.

[ijl]

@konstin pyo3-pack build --release --strip, then pip install target/wheels/....tar.gz. It would be nice to not need to build the wheel, too. Maybe --format bdist,sdist (by default) or something instead of --no-sdist?

[konstin]

target/wheels missing should be fixed with 027c33e

[ijl]

Thanks for fixing that. Oh, and I mean there was a pyproject.toml present, but my error was not having it include in Cargo.toml.

[konstin]

Oh, I see now. I've add a check in dd712c7. Thanks for reporting that!

[omerbenamram]

@konstin It's now a UnicodeEncodeError 🤔

[konstin]

@omerbenamram I've tried another workaround for the encoding errors in #153, which makes it pass on machine (German / cp1252). It still prints a warning however about the output not being cp1252 and print surrogates instead of emoji, even though the terminal supports utf8.

It would be nice to not need to build the wheel, too. Maybe --format bdist,sdist (by default) or something instead of --no-sdist?

@ijl Could you explain the use case for only building an sdist?

[ijl]

@konstin I want to have a test in which I build an sdist and install it via pip. Building the project twice would be unnecessarily slow.

[ijl]

I see this as working well.

I think it would be prudent to make the documented usage build a non-manylinux release wheel with pinned versions:

[build-system]
build-backend = "pyo3_pack"
requires = ["pyo3-pack>=0.7<0.8", "toml>=0.10,<0.11"]

[tool.pyo3-pack]
cargo-extra-args = "--release"
manylinux = "off"

[omerbenamram]

@konstin it works! sweet :)

[konstin]

@konstin I want to have a test in which I build an sdist and install it via pip. Building the project twice would be unnecessarily slow.

Is there a reason not to use tox with build isolation, which builds an sdist before building a wheel, or pip install .? Or could you reuse the sdist from the initial project build? The functionality for just building a wheel is already implemented as part of the PEP 517 backend, but apparently there's no way that pip will give you an sdist. Maybe a pyo3-pack sdist command would be the best solution for now.

I see this as working well.

I think it would be prudent to make the documented usage build a non-manylinux release wheel with pinned versions:

[build-system]
build-backend = "pyo3_pack"
requires = ["pyo3-pack>=0.7<0.8", "toml>=0.10,<0.11"]

[tool.pyo3-pack]
cargo-extra-args = "--release"
manylinux = "off"

I've changed the implementation to always use release mode and opened a thread on the python discourse on the topic. I've also exposed the strip option. Having manylinux = "off" is definitely a good idea; I've also added that.

[ijl]

I have it right now as pyo3-pack build and pip install target/wheels/*.tar.gz, which reuses the initial build's sdist artifact. I'm not using tox, and to run pip install . would verify the repository has all the files it needs but not that the sdist does. pyo3-pack sdist would be useful, but for me this is also good enough now.

[konstin]

I've created pypa/pip#6669 for solving this properly in pip.

[ijl]

beta.5 works perfectly for my use case, including pyo3-pack sdist. Thank you for adding that.

[konstin]

Thank you all for the beta testing and the feedback!

The only thing missing is the tox integration, which fails due to a bug in tox; I've opened #176 to track that. For everything else that comes up, please open a new issue.