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

Show warning when abi3 is used with PyPy #1469

Merged
merged 2 commits into from
Mar 6, 2021
Merged

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Mar 5, 2021

Thanks @davidhewitt for contacting to PyPy maintainers.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should definitely include this in the ## Packaging feature at the top of the changelog, because it could break user's builds:

### Packaging
- The `abi3` feature is now a compile error when building for `PyPy`. [#1469](https://github.com/PyO3/pyo3/pull/1469)

On setuptools-rust they'll have to write like this example: https://github.com/PyO3/setuptools-rust/blob/14703b86325e2c53cebec22937e9ca48c7cdeb70/examples/rust_with_cffi/setup.py#L24-L25

src/lib.rs Outdated
Comment on lines 149 to 152
compile_error!(concat!(
"PyPy still does not support abi3. ",
"See https://foss.heptapod.net/pypy/pypy/-/issues/3397 for the discussion."
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of wording changes which I think makes it read easier:

Suggested change
compile_error!(concat!(
"PyPy still does not support abi3. ",
"See https://foss.heptapod.net/pypy/pypy/-/issues/3397 for the discussion."
));
compile_error!(
"PyPy does not yet support abi3. \
See https://foss.heptapod.net/pypy/pypy/-/issues/3397 for more information."
);

@konstin
Copy link
Member

konstin commented Mar 5, 2021

I think it would be good if there was a way to build for pypy when the abi3 feature is enabled; I'd say the abi3 feature should require that only abi3 apis are used, while it should be possible to build non-abi3 wheels. Otherwise someone who wants to ship wheels for both cpython abi3 and pypy would need to add an optional feature for enabling abi3 and make sure cpython wheels are build with the feature and the pypy wheels without it.

@messense
Copy link
Member

messense commented Mar 5, 2021

I think it would be good if there was a way to build for pypy when the abi3 feature is enabled; I'd say the abi3 feature should require that only abi3 apis are used, while it should be possible to build non-abi3 wheels. Otherwise someone who wants to ship wheels for both cpython abi3 and pypy would need to add an optional feature for enabling abi3 and make sure cpython wheels are build with the feature and the pypy wheels without it.

Example: https://github.com/milesgranger/pyrus-cramjam/blob/37306e46ddb288a164af7d65db16eae3e3ce53ff/Makefile#L37

@kngwyu kngwyu changed the title Deny the combination of PyPy and ABI3 Show warning when abi3 is used with PyPy Mar 5, 2021
@kngwyu
Copy link
Member Author

kngwyu commented Mar 5, 2021

Thanks, I changed it to show a warning.
It would look like:
image

In addition, now this PR includes some refactorings of the build script.

@kngwyu kngwyu force-pushed the deny-pypy-and-abi3 branch 2 times, most recently from 6f889b3 to a01169f Compare March 6, 2021 03:43
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - just a couple of final suggestions.

The build script refactoring is very nice 👍

CHANGELOG.md Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@davidhewitt davidhewitt merged commit 4c80ce9 into master Mar 6, 2021
@davidhewitt
Copy link
Member

👍 thanks!

@messense messense deleted the deny-pypy-and-abi3 branch March 18, 2021 02:25
@konstin
Copy link
Member

konstin commented Mar 21, 2021

Thanks, the warning is a good solution 👍

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

Successfully merging this pull request may close these issues.

4 participants