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

PEP 517 integration #2

Closed
althonos opened this issue Jul 21, 2018 · 26 comments
Closed

PEP 517 integration #2

althonos opened this issue Jul 21, 2018 · 26 comments
Labels
enhancement New feature or request

Comments

@althonos
Copy link
Member

The recent PEP 517 specifies an interface for alternative build systems to be defined, which is exactly what's happening here.

To do that, pyo3-pack would need to provide a very simple Python interface with the following hooks:

  • build_wheel to build a wheel
  • build_sdist to build a gztarred archive of the source code
    (actual semantics are defined in the PEP).

Then, releasing the pyo3-pack module to PyPI would allow users to specify the build system in pyproject.toml, making it easier to build a Rust wheel without setuptools.

@althonos althonos added the enhancement New feature or request label Jul 21, 2018
@konstin
Copy link
Member

konstin commented Jul 27, 2018

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.

@konstin konstin added the blocked Something is blocking this label Jul 27, 2018
@althonos
Copy link
Member Author

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
Copy link
Contributor

ijl commented Jan 23, 2019

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

@ijl
Copy link
Contributor

ijl commented Jan 24, 2019

@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 konstin removed the blocked Something is blocking this label Jan 24, 2019
@konstin
Copy link
Member

konstin commented Jan 24, 2019

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
Copy link
Member

konstin commented Jun 21, 2019

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.

konstin added a commit that referenced this issue Jun 22, 2019
@konstin
Copy link
Member

konstin commented Jun 22, 2019

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.

@omerbenamram
Copy link
Contributor

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 .

image

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).

image

WDYT?

@konstin
Copy link
Member

konstin commented Jun 25, 2019

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
Copy link
Contributor

ijl commented Jun 25, 2019

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
Copy link
Contributor

ijl commented Jun 25, 2019

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
Copy link
Member

konstin commented Jun 25, 2019

@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
Copy link
Contributor

ijl commented Jun 25, 2019

@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
Copy link
Member

konstin commented Jun 25, 2019

target/wheels missing should be fixed with 027c33e

@ijl
Copy link
Contributor

ijl commented Jun 25, 2019

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
Copy link
Member

konstin commented Jun 25, 2019

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

@omerbenamram
Copy link
Contributor

@konstin It's now a UnicodeEncodeError 🤔

image

@konstin
Copy link
Member

konstin commented Jun 27, 2019

@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
Copy link
Contributor

ijl commented Jun 27, 2019

@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
Copy link
Contributor

ijl commented Jun 28, 2019

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
Copy link
Contributor

@konstin it works! sweet :)

@konstin
Copy link
Member

konstin commented Jun 30, 2019

@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
Copy link
Contributor

ijl commented Jul 1, 2019

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
Copy link
Member

konstin commented Jul 1, 2019

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

@ijl
Copy link
Contributor

ijl commented Jul 4, 2019

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

@konstin
Copy link
Member

konstin commented Aug 11, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants