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

Switch to maturin. #8815

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Switch to maturin. #8815

merged 1 commit into from
Jul 5, 2024

Conversation

alex
Copy link
Member

@alex alex commented Apr 24, 2023

It seems to be much faster at doing things locally.

@alex
Copy link
Member Author

alex commented Apr 24, 2023

TODO before mergable:

  • Get this working in CI...
  • Figure out how to handle version numbers
  • Make sure the abi3 stuff works with PyPy
  • Update wheel builder to no longer use setup.py directly (this should be done in a separate PR)
  • Figure out what the correct behavior with respect to MANIFEST.in is
  • Maturin stable release
  • Figure out how maturin's MSRV interacts with our MSRV

@alex alex force-pushed the maturin branch 8 times, most recently from 0ed4d74 to 78c881c Compare April 26, 2023 23:37
@messense
Copy link

Hey, maturin's sdist support is be a bit hacky so you may run into obscure problems, feel free to ping me or open an issue if something doesn't work.

@alex
Copy link
Member Author

alex commented Apr 27, 2023 via email

@alex
Copy link
Member Author

alex commented Apr 30, 2023

@messense is there a way to disable the rewriting things to be local_dependencies? My preference is to just include everything at its normal location with include.

@messense
Copy link

messense commented May 2, 2023

is there a way to disable the rewriting things to be local_dependencies?

No way ATM, I do want to support it to avoid PyO3/maturin#1442.

@alex
Copy link
Member Author

alex commented May 2, 2023 via email

@messense
Copy link

messense commented May 2, 2023

Would it be useful to file a bug for that, or should I just follow 1442?

Yeah, file a bug is useful.

@alex
Copy link
Member Author

alex commented May 4, 2023

@messense Is it intentional that maturin doesn't set the PYO3_PYTHON env var when running cargo? setuptools-rust does (https://github.com/PyO3/setuptools-rust/blob/main/setuptools_rust/build.py#L600), and we use it in our build.rs (I think you even wrote that code :-))

Specifically, this is happening when pip wheel invokes ['maturin', 'pep517', 'build-wheel', '-i', '/Users/alex_gaynor/projects/cryptography/.venv/bin/python3.11', '--compatibility', 'off'], so I'd expect the -i value to go into PYO3_PYTHON.

@messense
Copy link

messense commented May 5, 2023

  🔗 Found pyo3 bindings with abi3 support for Python ≥ 3.7
  🐍 Not using a specific python interpreter

I think it has something to do with abi3, we can certainly pass down the interpreter path if it's specified. I can take a look this weekend.

@alex
Copy link
Member Author

alex commented May 5, 2023

Thanks!

bors bot added a commit to PyO3/maturin that referenced this pull request May 6, 2023
1592: Always set `PYO3_PYTHON` if interpreter is runnable regardless of abi3 r=messense a=messense

It can be useful in build scripts, like pyca/cryptography#8815 (comment)

Co-authored-by: messense <messense@icloud.com>
bors bot added a commit to PyO3/maturin that referenced this pull request May 6, 2023
1592: Always set `PYO3_PYTHON` if interpreter is runnable regardless of abi3 r=messense a=messense

It can be useful in build scripts, like pyca/cryptography#8815 (comment)

Co-authored-by: messense <messense@icloud.com>
bors bot added a commit to PyO3/maturin that referenced this pull request May 6, 2023
1592: Always set `PYO3_PYTHON` if interpreter is runnable regardless of abi3 r=messense a=messense

It can be useful in build scripts, like pyca/cryptography#8815 (comment)

Co-authored-by: messense <messense@icloud.com>
@messense
Copy link

messense commented May 6, 2023

Please try maturin 1.0.0b9.

@alex
Copy link
Member Author

alex commented May 6, 2023

Ok, we are green here, huzzah!

The remaining things we need are: a) Maturin stable release (this currently uses a beta), b) @reaperhulk we need to discuss MSRV implications:

maturin currently has an MSRV of 1.64, which is obviously higher than our MSRV. It also has wheels for every possible platform it seems, https://pypi.org/project/maturin/1.0.0b9/#files

@alex
Copy link
Member Author

alex commented May 13, 2023

After discussion with @reaperhulk, we think the MSRV is too aggressive. Going to hold this in draft for now.

@alex alex marked this pull request as draft May 13, 2023 00:28
@alex alex force-pushed the maturin branch 2 times, most recently from d65784b to 3fd7767 Compare February 24, 2024 20:12
@alex alex marked this pull request as ready for review February 24, 2024 20:12
@alex alex changed the title Attempt to switch to maturin. Switch to maturin. Feb 24, 2024
@alex alex force-pushed the maturin branch 2 times, most recently from 5c13ed1 to 78f9e71 Compare February 25, 2024 23:08
@alex alex added this to the Forty Fourth Release milestone Feb 26, 2024
@alex alex force-pushed the maturin branch 2 times, most recently from 7bb47e8 to 0f22527 Compare March 6, 2024 00:42
@alex alex force-pushed the maturin branch 2 times, most recently from dc027af to dc1924e Compare March 30, 2024 18:28
It seems to be much faster at doing things locally.
@reaperhulk reaperhulk merged commit 5b23baa into pyca:main Jul 5, 2024
83 checks passed
@alex alex deleted the maturin branch July 5, 2024 18:57
@h-vetinari
Copy link

This is probably worth a release note? (and should be remilestoned to 43?)

@alex
Copy link
Member Author

alex commented Jul 6, 2024

There's no public API implicated here -- the only installation method we have ever documented support for is pip (and perhaps by extension other installers that are PEP517 compatible).

Good catch re:milestone though.

@h-vetinari
Copy link

Your call of course. There are cases where someone might hit this (e.g. installation without build isolation, as distributors are wont to do), but it's not hard to find out and fix. Still, I'd tend to add it for completeness 🤷‍♂️

@eli-schwartz
Copy link

Am I correct in understanding that now maturin is used it is no longer possible to build non-abi3 wheels if you're building locally for a single python version?

@alex
Copy link
Member Author

alex commented Aug 11, 2024

I think it may be possible with build-args (ala https://github.com/pyca/cryptography/blob/main/.github/workflows/wheel-builder.yml#L130), by simply not setting abi3. But I haven't tested, nor did we ever previously test building non-abi3 artifacts.

If there's something we can do to improve this, let us know.

@eli-schwartz
Copy link

It assumed it did work before, because when you simply run the build backend without any particular options, the previous version installed

/usr/lib/python3.12/site-packages/cryptography/hazmat/bindings/_rust.cpython-312-x86_64-linux-gnu.so

whereas the new version installs

/usr/lib/python3.12/site-packages/cryptography/hazmat/bindings/_rust.abi3.so

I futzed around a bit in the current source, and was able to convince maturin to build cpython-312-x86_64-linux-gnu.so extensions by deleting all the features = ["abi3"] from various toml files and removing pyo3/abi3-py37 from pyproject.toml, so maybe I'm wrong to think the old version was actually optimized for a specific python version (and instead it was building against the abi3 API/ABI but ignoring the "abi3" extension name?).

As far as I can tell, it's not actually possible to disable a single feature in cargo, though there's an open issue from 2016 asking for the ability. (?)

I know when I helped someone implement Limited API support in meson, the resulting implementation took:

  • an attribute on the config for an extension module, asking for Limited API support and specifying the minimum version, which internally handles -DPy_LIMITED_API=0x3........... for you
  • a builtin (not project-specific) build option python.allow_limited_api which can be manually toggled by the person doing a local build to opt out of the whole thing entirely

which I figured was a good balance.

@eli-schwartz
Copy link

I think it may be possible with build-args (ala https://github.com/pyca/cryptography/blob/main/.github/workflows/wheel-builder.yml#L130), by simply not setting abi3.

It appears this is set in pyproject.toml anyway. 🤷 If I try to remove it on its own, maturin spawns an error message:

💥 maturin failed
  Caused by: You have selected the `abi3` feature but not a minimum version (e.g. the `abi3-py36` feature). maturin needs a minimum version feature to build abi3 wheels.

@alex
Copy link
Member Author

alex commented Aug 11, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants