Add abi3-py* features #1263

kngwyu · 2020-10-31T07:41:37Z

For #1195, but still needs some design enhancements.
This PR adds some feature flags that can be used to specify the minimum required version when building abi3 wheel.
E.g., you can build cp36-abi3-manylinux1 wheel with Python3.8 if you use abi3-py36 feature.
I still have some questions:

Should this be set by features or environmental variables?
How do we support cp3-abi3 wheels? I think it's better for tools (maturin or setuptools-rust) to allow this renaming with some configuration. cc: @konstin

For folks who are not familiar with PEP425 (like me!), here's the link: https://www.python.org/dev/peps/pep-0425

documentation

kngwyu · 2020-11-10T09:49:02Z

Now I think that using the environmental variable is easier for users and tools... 🤔

davidhewitt · 2020-11-10T10:10:56Z

Environment variables are definitely much easier to set. Also that way my build configuration won't be affected by thirdparty packages (any package I include in my dependencies could incorrectly enable these features).

It's also kinda hard to understand how these features interact. If I set abi3-py36 and abi3-py38 but not any other feature, what does it mean? Is my wheel only supported for python 3.8 and up? Or am I restricted to only using python 3.6 features?

A single environment variable PYO3_ABI3_VERSION might be easier to understand.

konstin · 2020-11-11T12:35:15Z

I believe that we need some place, likely in Cargo.toml or pyproject.toml, that specifies the minimum supported python version. pyo3 should have cfgs that enfore that only symbols available at that version are available. setuptools-rust and maturin can use this to set the appropriate Requires-Python and abi tag. I like the solution with features particularly because we can read those with cargo metadata.

It's also kinda hard to understand how these features interact. If I set abi3-py36 and abi3-py38 but not any other feature, what does it mean? Is my wheel only supported for python 3.8 and up? Or am I restricted to only using python 3.6 features?

I'd define it so that abi3-py36 means that all symbols that are 3.7+ are removed by cfgs.

davidhewitt · 2020-11-11T17:08:40Z

I believe that we need some place, likely in Cargo.toml or pyproject.toml, that specifies the minimum supported python version.

I could be wrong, but I don't think it's possible for pyo3's build.rs to read metadata of downstream crates in a reliable way?

kngwyu · 2020-11-12T03:28:53Z

I don't think it's possible for pyo3's build.rs to read metadata of downstream crates in a reliable way?

I think @konstin meant that maturin or some tools can read the metadata.

setuptools-rust and maturin can use this to set the appropriate Requires-Python and abi tag.

That makes sense.

kngwyu · 2020-11-12T03:45:28Z

I confirmed this works with maturin's pyo3_pure example. My system Python is 3.8, and the built wheel works in Python 3.6 after renamed.

kngwyu · 2020-11-12T03:47:07Z

build.rs

@@ -9,7 +9,10 @@ use std::{
    str::FromStr,
 };

-const PY3_MIN_MINOR: u8 = 5;
+/// Minimum required Python version.
+const PY3_MIN_MINOR: u8 = 6;


Not related to this PR, but we dropped 3.5 support in #1250.

davidhewitt

This is cool, but I am quite concerned that changing the Python ABI dynamically (upwards) only works when building extension-module on linux.

This kinda makes me worry that it shouldn't be set using features and an environment variable is better. That way maturin and setuptools-rust can set the environment variable when it's appropriate without breaking cargo test etc.

davidhewitt · 2020-11-12T09:22:46Z

build.rs

+/// Minimum required Python version.
+const PY3_MIN_MINOR: u8 = 6;
+/// Maximum Python version that can be used as minimum required Python version with abi3.
+const ABI3_MAX_MINOR: u8 = 9;


Binaries trying to link against a higher python version than the actual installed libpython will fail because symbols will mismatch. So this new feature can only work for extension modules.

(And even worse, only extension modules on linux - because on windows they always link!)

This would mean that an extension module which uses the abi3-pyXY features would further break cargo test even after #1123 , and would also not be cross-platform.

davidhewitt · 2020-11-12T09:24:01Z

build.rs

-    }
+        // Check any `abi3-py3*` feature is set. If not, use the interpreter version.
+        (PY3_MIN_MINOR..=ABI3_MAX_MINOR)
+            .find_map(|i| env::var_os(format!("CARGO_FEATURE_ABI3_PY3{}", i)).map(|_| i))


Nit:

Suggested change

.find_map(|i| env::var_os(format!("CARGO_FEATURE_ABI3_PY3{}", i)).map(|_| i))

.find(|i| env::var_os(format!("CARGO_FEATURE_ABI3_PY3{}", i)).is_some())

Nice catch 👍

kngwyu · 2020-11-12T13:30:38Z

'limited to extension-module' problem
- I don't think it's so problematic. abi3 does not mean so much in embedding cases.
Windows problem
- Nice point. It's certainly problematic. But I want to test this on Windows (by CI?) before moving to another solution.

davidhewitt · 2020-11-12T18:46:25Z

I don't think it's so problematic. abi3 does not mean so much in embedding cases.

👍 - maybe should we set the feature abi3 = ["extension-module"] in Cargo.toml?

Windows problem

Note that as well as windows, also tests will not link with cargo test without our default features workaround.

I was hoping that after #1123 we would be able to run cargo test without such a workaround.

kngwyu · 2020-11-14T08:14:46Z

maybe should we set the feature abi3 = ["extension-module"] in Cargo.toml?

Sounds too restrictive to do so.

kngwyu · 2020-11-14T08:20:09Z

Binaries trying to link against a higher python version than the actual installed libpython will fail because symbols will mismatch

Can this happen even with abi3?
I didn't observe it fails in Linux + linking setting (i.e., no 'externsion-module' and not manylinux).

cc: @alex

alex · 2020-11-14T14:41:06Z

Yes, if you build an ABI3 wheel that targets 3.9 as the minimum version, it may fail when you try to load it against a 3.6 CPython. (Note that it'll only fail if you use a symbol that was recently added to the ABI3 ABI, if you target a higher minimu version but don't use any newer symbols, it'll probably just work.)

kngwyu · 2020-11-14T15:28:49Z

Note that it'll only fail if you use a symbol that was recently added to the ABI3 ABI, if you target a higher minimu version but don't use any newer symbols, it'll probably just work.

Ah, it makes sense, thanks. Maybe we should prohibit setting a higher version.

alex · 2020-11-15T15:44:29Z

guide/src/building_and_distribution.md

+E.g., if you set `abi3-py36` feature, you can build `cp36-abi3-manylinux2020_x86_64.whl` using Python 3.8.
+
+To ensure ABI compatibility, we don't allow setting a minimum version higher than the system Python version.
+E.g., if you set `abi3-py38` and try to compile the crate with Python 3.8, it just fails.


Suggested change

E.g., if you set `abi3-py38` and try to compile the crate with Python 3.8, it just fails.

E.g., if you set `abi3-py39` and try to compile the crate with Python 3.8, it just fails.

I think?

Oops, nice catch, thanks.

kngwyu · 2020-11-15T15:47:14Z

I'm leaning towards setting this as a feature flag.
They just change the #[cfg(Py_*)] and don't seem so problematic as extension-module.

davidhewitt · 2020-11-15T16:11:35Z

Ah, it makes sense, thanks. Maybe we should prohibit setting a higher version.

I think @konstin highlighted an interesting case where someone can in theory use this setup to build an abi3-py39 wheel without _any_ Python on their system (or maybe only e.g. Python 3.6 installed). This would work because extension-module doesn't actually need to link anything, so we can just use the pyo3::ffi definitions.

But I agree that in 99% of cases a higher version is a problem, because it means that tests (e.g) might unexpectedly fail to link. So I suggest that if we detect in build.rs that the abi3 version > python version, we emit a friendly error message informing so and invite the user to set an enviroment variable e.g. PYO3_ALLOW_NEWER_PYTHON=1 to opt-in to this.

I still want to push a concern with features vs environment-variables. Imagine we have a library lib and an extension module extension-module.

lib/Cargo.toml

[dependencies]
pyo3 = { version = "...", features = ["abi3-py37"] }

extension-module/Cargo.toml

[dependencies]
lib = "..."
pyo3 = { version = "...", features = ["extension-module", "abi3-py36"] }

Now we want to use maturin (or setuptools-rust) to build extension-module. The packaging tool can scrape the abi3 version from the metadata for extension-module, and detects Python 3.6.

So the packaging tool makes a Python 3.6 wheel.

However this is a bug: lib sets the abi3-py37 feature, and so pyo3 will allow use of Python 3.7 features to be in the built extension module. The wheel might not actually work on Python 3.6 at all.

If instead maturin were to set an environment variable PYO3_ABI_VERSION=3.6 then lib would not accidentally be able to break our wheel - instead lib would just fail to compile.

kngwyu · 2020-11-15T17:34:53Z

So I suggest that if we detect in build.rs that the abi3 version > python version, we emit a friendly error message informing so and invite the user to set an enviroment variable e.g. PYO3_ALLOW_NEWER_PYTHON=1 to opt-in to this.

For now I don't think we need to support such a rare case. I'm willing to change the behavior if a user complains about, though.

However this is a bug: lib sets the abi3-py37 feature, and so pyo3 will allow use of Python 3.7 features to be in the built extension module. The wheel might not actually work on Python 3.6 at all.

I tried that and confirm that it just fails to compile (and noticed the bug of feature definitions...).

I think env var is also good, but I also think that it would add additional works for tools.
If we use the feature for this, what tools do is just two things: read the feature flag and name the wheel.
But, if we use an env var, tools additionally have to define the interface of the minimum required Python version: e.g., by pyproject.toml.

davidhewitt · 2020-11-15T18:48:23Z

For now I don't think we need to support such a rare case. I'm willing to change the behavior if a user complains about, though.

👍

I think env var is also good, but I also think that it would add additional works for tools.
If we use the feature for this, what tools do is just two things: read the feature flag and name the wheel.
But, if we use an env var, tools additionally have to define the interface of the minimum required Python version: e.g., by pyproject.toml.

I guess, but tools are great for doing lots of work for the user so I'm not sure that's such a big problem! 😄

Anyway I guess to learn more about this design the best way now might be to merge it? We can then try out the implementations in setuptools-rust and maturin and see how it goes.

(We can give a little time between merging and releasing to experiment... or even release a pyo3 0.13.0-alpha1 to try it out?)

konstin · 2020-11-16T19:41:38Z

I finally got around to prototype what I had in mind: diff. The idea is that you always have to set a minimum version, as the code will have one implicitly anyway. This then makes it possible on maturin's side to build without checking for python interpreter (diff). This makes it possible to build for your minimum target version without the error prone process of finding python versions, with which I had a lot of trouble in maturin. It should also be faster since we avoid running python twice. (We could go even further and move pyo3's build script into a separate crate that is only built when abi3 is not used, but that's out of scope for now)

kngwyu · 2020-11-17T05:54:13Z

@konstin
Thanks. Does this also work for Windows?

konstin · 2020-11-18T08:54:49Z

Unfortunately not :/ Do you know why we do link(name = "pythonXY") on windows and whether it's possible to link the cdylib without reading the .lib file?

kngwyu · 2020-11-19T13:11:40Z

Do you know why we do link(name = "pythonXY") on windows and whether it's possible to link the cdylib without reading the .lib file?

No, I don't explicitly understand the difference. Just found some notes but still investigating.

davidhewitt · 2020-11-19T23:19:25Z

why we do link(name = "pythonXY") on windows

If I recall (from experimentation last time I wondered this) #[link(name = "pythonXY")] is only needed on extern blocks that contain static mut variables. I'm not sure whether that's us misconfiguring things in build.rs, a bug in rustc, or a limitation in msvc's linker.

I'll try and see if I can reproduce this in a minimal repro for rust upstream.

whether it's possible to link the cdylib without reading the .lib file

Unfortunately I think msvc linker needs to resolve all the symbols from somewhere at build time.

Looks like there's an option we could try setting (similar to macos): https://docs.microsoft.com/en-us/cpp/build/reference/force-force-file-output?view=msvc-160 ... but it might just break stuff.

kngwyu · 2020-12-01T14:42:52Z

Updated: I added PYO3_NO_INTERPRETER env var to allow building a wheel without calling Python on *NIX.
Tools like maturin can make use of this feature.

kngwyu · 2020-12-05T05:59:26Z

FYI: How about releasing 0.13 after merging this PR? cc: @davidhewitt

davidhewitt

Yeah, I'm happy to do the release. I made a ticket #1306 just to make sure we include everything we want in this release.

For this PR, I've made a final review with a few suggested improvements to finish it up. I'm still not totally sure if feature flags or an environment variable is better, but I think it's fine for us to merge this now and learn about it. It shouldn't be such an annoying breaking change if we decide to use an environment variable later.

CHANGELOG.md

davidhewitt · 2020-12-05T09:16:19Z

build.rs

@@ -820,7 +836,25 @@ fn check_target_architecture(interpreter_config: &InterpreterConfig) -> Result<(
    Ok(())
 }

+fn abi3_without_interpreter() -> Result<()> {


I think this is a good start, in the future I think we can replace this with something more flexible.

See this discussion with the PyOxidiser maintainer who suggested if PyOxidiser used PyO3 they would like the ability to set an environment variable with a path to a JSON file which configures PyO3: indygreg/PyOxidizer#324 (comment)

guide/src/building_and_distribution.md

davidhewitt · 2020-12-05T09:34:18Z

guide/src/building_and_distribution.md

+E.g., if you set `abi3-py38` and try to compile the crate with Python 3.6, it just fails.
+
+As an advanced feature, you can build PyO3 wheel without calling Python interpreter with
+the environment variable `PYO3_NO_INTERPRETER` set, but this only works on *NIX.


Maybe to match PYO3_PYTHON, it's better to call this new environment variable PYO3_NO_HOST_PYTHON ?

Sound nice 👍

I decided to use a shorter PYO3_NO_PYTHON.

Co-authored-by: David Hewitt <[email protected]>

davidhewitt

LGTM! Just one remaining nit.

davidhewitt · 2020-12-06T21:16:15Z

build.rs

@@ -850,7 +850,7 @@ fn abi3_without_interpreter() -> Result<()> {

 fn main() -> Result<()> {
    // If PYO3_NO_INTEPRETER is set with abi3, we can build PyO3 without calling Python (UNIX only).


Suggested change

// If PYO3_NO_INTEPRETER is set with abi3, we can build PyO3 without calling Python (UNIX only).

// If PYO3_NO_PYTHON is set with abi3, we can build PyO3 without calling Python (UNIX only).

kngwyu · 2020-12-08T03:51:47Z

Thank you for your kind reviews, now the docs are much better 😃

kngwyu force-pushed the abi3-min-python branch from b517964 to b94755c Compare October 31, 2020 08:07

kngwyu force-pushed the abi3-min-python branch 2 times, most recently from d3d2333 to 9895282 Compare November 12, 2020 03:43

kngwyu marked this pull request as ready for review November 12, 2020 03:45

kngwyu commented Nov 12, 2020

View reviewed changes

davidhewitt requested changes Nov 12, 2020

View reviewed changes

kngwyu mentioned this pull request Nov 12, 2020

Building with maturin in manylinux fails because of missing PYO3_CROSS_LIB_DIR #1264

Closed

davidhewitt mentioned this pull request Nov 14, 2020

PyO3 support indygreg/PyOxidizer#324

Open

kngwyu force-pushed the abi3-min-python branch from 9895282 to ca68209 Compare November 15, 2020 15:32

Add abi3-py* features

597119d

kngwyu force-pushed the abi3-min-python branch from ca68209 to ae79b92 Compare November 15, 2020 15:42

alex reviewed Nov 15, 2020

View reviewed changes

Note abi3-py* features in the guide and Add CHANGELOG

93282e9

kngwyu force-pushed the abi3-min-python branch from ae79b92 to 93282e9 Compare November 15, 2020 15:47

Correct the definitions of abi3-py* features

eae0d86

davidhewitt approved these changes Nov 15, 2020

View reviewed changes

Introduce PYO3_NO_INTERPRETER variable for build script

1b83850

konstin mentioned this pull request Dec 1, 2020

abi3 binding wheel can only be installed for a specific version of python PyO3/maturin#382

Closed

davidhewitt mentioned this pull request Dec 5, 2020

0.13 Release #1306

Closed

davidhewitt approved these changes Dec 5, 2020

View reviewed changes

Apply suggestions from @davidhewitt

6da6bc9

Co-authored-by: David Hewitt <[email protected]>

davidhewitt approved these changes Dec 6, 2020

View reviewed changes

Rename PYO3_NO_INTERPRETER by PYO3_NO_PYTHON

4914372

kngwyu force-pushed the abi3-min-python branch from 2b3267a to 4914372 Compare December 7, 2020 08:30

kngwyu merged commit 9aa70f7 into master Dec 8, 2020

davidhewitt mentioned this pull request Dec 19, 2020

Specifying the minimum supported python version #1195

Closed

messense deleted the abi3-min-python branch March 18, 2021 02:23

	.find_map(\|i\| env::var_os(format!("CARGO_FEATURE_ABI3_PY3{}", i)).map(\|_\| i))
	.find(\|i\| env::var_os(format!("CARGO_FEATURE_ABI3_PY3{}", i)).is_some())

	E.g., if you set `abi3-py38` and try to compile the crate with Python 3.8, it just fails.
	E.g., if you set `abi3-py39` and try to compile the crate with Python 3.8, it just fails.

		@@ -850,7 +850,7 @@ fn abi3_without_interpreter() -> Result<()> {

		fn main() -> Result<()> {
		// If PYO3_NO_INTEPRETER is set with abi3, we can build PyO3 without calling Python (UNIX only).

	// If PYO3_NO_INTEPRETER is set with abi3, we can build PyO3 without calling Python (UNIX only).
	// If PYO3_NO_PYTHON is set with abi3, we can build PyO3 without calling Python (UNIX only).

Add abi3-py* features #1263

Add abi3-py* features #1263

Conversation

kngwyu commented Oct 31, 2020 • edited Loading

kngwyu commented Nov 10, 2020

davidhewitt commented Nov 10, 2020

konstin commented Nov 11, 2020

davidhewitt commented Nov 11, 2020

kngwyu commented Nov 12, 2020

kngwyu commented Nov 12, 2020

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Nov 12, 2020

davidhewitt commented Nov 12, 2020

kngwyu commented Nov 14, 2020

kngwyu commented Nov 14, 2020

alex commented Nov 14, 2020

kngwyu commented Nov 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Nov 15, 2020

davidhewitt commented Nov 15, 2020

kngwyu commented Nov 15, 2020

davidhewitt commented Nov 15, 2020

konstin commented Nov 16, 2020

kngwyu commented Nov 17, 2020

konstin commented Nov 18, 2020

kngwyu commented Nov 19, 2020

davidhewitt commented Nov 19, 2020 • edited Loading

kngwyu commented Dec 1, 2020

kngwyu commented Dec 5, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Dec 8, 2020

kngwyu commented Oct 31, 2020 •

edited

Loading

davidhewitt commented Nov 19, 2020 •

edited

Loading