Replace extension-module feature with PYO3_MAKE_EXTENSION_MODULE

[davidhewitt]

This is another try at an alternative to the extension-module feature.

Inspired by PyO3/maturin#325, this changes the extension-module feature to instead be an environment variable PYO3_MAKE_EXTENSION_MODULE.

Users building extension modules are expected to set this during cargo build to turn off linking libpython. I will update maturin and setuptools-rust to do this automatically. This way I hope 99% of users will no longer have linking errors, and everything should just work magically.

[davidhewitt]

(Test failures are expected and will be resolved after I update setuptools-rust. First I'd like someone to review this idea to confirm that it's a sensible proposal.)

[pganssle]

It seems weird to me that this is an environment variable. It seems like an inherent property of the project (which would go into per-project configuration), not something that you would ever want to configure in your environment by setting a variable.

[davidhewitt]

It seems like an inherent property of the project (which would go into per-project configuration)

I agree with you regarding this statement.

However, as discussed in #904 #771 the current design of using a feature as per-project configuration is problematic. I would argue that this proposed environment variable is the mechanism for pyo3 to build an extension module correctly, and the per-project configuration lives elsewhere:

• maturin and setuptools-rust can set it when they are configured to build an extension module.
• users who don't want to use those tools can write an alternative wrapper to build (shell script or whatever) which sets this environment variable.

[birkenfeld]

Seems to be a reason for me to try out maturin :)

[pganssle]

@davidhewitt Understood, but I still don't think an environment variable is the right way to transmit this information to the compiler. This is something that, per-project, you either always want on, or never want on. If there's no way to transmit this information other than via environment variable, then I guess it's just a bit of a wart, but I think it's worth considering other options.

What about some sort of configuration file other than a feature?

I'm not super familiar with the Cargo.toml file — is there a place to put project or tool configuration not intended to be processed by Cargo.toml? If so, you could have a pyo3-config subtable (or whatever namespace is appropriate) and parse the file in build.rs to determine whether or not to do the build. Alternatively, it could go in any other kind of config file, or you could even just have a sentinel file whose existence indicates that it's an extension module: if pyo3-extension-module exists in the repo root or the crate root or something, build.rs does the linking, otherwise don't.

I guess the one downside to this "configure it in a file" approach is that setuptools-rust and maturin might not be able to easily automatically set it if you forget or if you never intend to build directly via cargo, but there might be easy work-arounds for that (for example, auto-generating the sentinel file as part of the sdist or wheel build if it's not present).

[davidhewitt]

This is something that, per-project, you either always want on, or never want on.

Unfortunately, this is exactly what turned out not to be true for extension-module. If you're writing a python extension (i.e. lib.rs) then on unix you must not link to libpython. However, you still can have many binaries, (main.rs, or extras in src/bin/) which all need to link to libpython or they won't function correctly.

And the same is true for tests - even unit tests written in lib.rs need to link to libpython to be able to execute properly with cargo test. This is the cause of the error reported in #846 - as well as why we advertise a workaround in the faq.

What about some sort of configuration file other than a feature?

I could be wrong, but I came to the conclusion that any sort of static configuration falls to this same issue. We don't want the extension-module for the whole project configuration, but actually just when building the extension module lib for packaging.

This is why I came to the conclusion that an evironment variable is the best way to go.

FWIW, I agree it's a bit of a wart, but:

• I think that most users using maturin or setuptools-rust will never need to know this environment variable exists.
• I have a vague hope that in the future, cargo linker args might be flexible enough that build.rs is able to emit enough specific configuration that everything "just works" without needing to have this environment variable. There's some movement in PRs like Finish implementation of -Zextra-link-arg. rust-lang/cargo#8441, but I think it's a long way off before that functionality is stabilised.

(EDIT: adding the missing not in the second paragraph.)

[kngwyu]

Hmm... 🤔
I think it's not so bad but I also feel it somewhat overhack to make tools set this variable.

[davidhewitt]

Maybe it's just me, I feel it's exactly the job of tools to set this variable.

We make the base package support all combinations users might want using configuration. Then tools manage as much of that configuration as possible to make users' lives easier.

[gilescope]

I agree with David - I don't think having it as a default feature is helpful. Somehow if the packaging tools can do the right thing that would be great. This seems like an alternative to PyO3/maturin#325

[konstin]

I believe that rust-lang/cargo#8441 would be the best solution and also the only one that consistently does the right thing (excluding proc macros that compile python to bytecode maybe, but that's some very special case). It should be possible to build cargo of that pull request and check if that works for pyo3.

There should be some precaution against users defaulting to binary linking because they forgot to set the environment variable or use cargo through some tool/IDE.

For maturin this change should be easy to implement, but you should also consider that there are many that use cargo build and cargo check

[davidhewitt]

I took a go with rust-lang/cargo#8441. Sadly it appears that in its current form the linker flags don't get passed to dependencies, so this still won't help us here.

There should be some precaution against users defaulting to binary linking because they forgot to set the environment variable or use cargo through some tool/IDE.

For maturin this change should be easy to implement, but you should also consider that there are many that use cargo build and cargo check

I have been thinking further on this point. I don't think it matters if the extension uses binary linking for local development? So as long as we document clearly how to package extensions properly (e.g. either set this env var when building, or use a supported too), then I think we have taken appropriate precaution.

[konstin]

I took a go with rust-lang/cargo#8441. Sadly it appears that in its current form the linker flags don't get passed to dependencies, so this still won't help us here.

This seems to have been fixed after your comment in rust-lang/cargo@6382d8b

[davidhewitt]

This seems to have been fixed after your comment in rust-lang/cargo@6382d8b

Indeed it is, and it works beautifully. Closing this in favour of #1123