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

The extension-module feature fails to be additive #771

Open
nagisa opened this issue Feb 18, 2020 · 11 comments · May be fixed by #2135
Open

The extension-module feature fails to be additive #771

nagisa opened this issue Feb 18, 2020 · 11 comments · May be fixed by #2135
Labels

Comments

@nagisa
Copy link
Contributor

nagisa commented Feb 18, 2020

In Rust it is the best practice to have features to be strictly additive. extension-module, instead, subtracts from the crate by not linking python. This can result in weird and difficult to resolve failure modes in e.g. workspaces.

For instance consider…

💥 Reproducing

[workspace]
members = [
    "extension",
    "some_binary",
]

where both extension and some_binary use as a dependency pyo3, but extension enables the extension-module feature, while some_binary explicitly disables the features.

When cargo build is run from the workspace root, then both of the binaries will get a pyo3 with extension-module feature enabled and some_binary will fail to link.

This behaviour is entirely valid from cargo’s standpoint – it dictates that features should be additive and thus there must not be a way to write any single crate in a way that would fail when additional features are enabled in its dependencies.

To be honest, I don’t see a good way to fix it, other than spiting pyo3 into two separate crates.

@konstin
Copy link
Member

konstin commented Feb 20, 2020

The underlying problem are two cargo bugs.

One is the behavior of workspaces, as explained in rust-lang/cargo#5928 (comment):

This is a conflation of a number of preexisting bugs. They're not super well indexed unfortunately. Cargo unions all features which means with --all it would union all the features requested by anything, and a different bug means that Cargo never compiles multiple versions of a crate with different sets of features meaning that a crate is only compiled once with one set of features.

In that sense it's not compatible with features today in Cargo to have a set of binaries that only work with a feature and a set that only work without, and then build them all at once.

This might get solved by rust-lang/cargo#7820.

The other problem is that cargo currently doesn't have target-dependent linker args (rust-lang/cargo#5881). This would be solved by rust-lang/cargo#7811, but this seems to be stuck in a stalled RFC (rust-lang/cargo#7811 (comment)).

@nagisa
Copy link
Contributor Author

nagisa commented Feb 21, 2020

One is the behavior of workspaces, as explained in ...

However Rust (and Cargo) project was operating presuming features to be additive (see also rust-lang/cargo#4328) and a ton functionality assumes that enabling additional features is valid. A large many of workflows like cargo test --all-features and even outside of cargo assume this.

While some of the links do suggest very relevant solutions, given their state I’m think pyo3 should be investigating ways to resolve this and similar issues on its end.

@davidhewitt
Copy link
Member

davidhewitt commented Feb 21, 2020

I haven't thought through this fully yet, but I was kind of hoping that rust-lang/cargo#7811 would potentially allow us to remove the extension-module feature entirely.

(Edit: fixed link to ticket I meant)

@programmerjake
Copy link
Contributor

I haven't thought through this fully yet, but I was kind of hoping that rust-lang/cargo#781 would potentially allow us to remove the extension-module feature entirely.

That seems like the wrong issue number.

@davidhewitt
Copy link
Member

Heh thanks, I meant rust-lang/cargo#7811 (copied the wrong link from @konstin 's post above). Have edited my original comment.

@konstin
Copy link
Member

konstin commented Feb 21, 2020

While some of the links do suggest very relevant solutions, given their state I’m think pyo3 should be investigating ways to resolve this and similar issues on its end.

I did investigate, and I didn't find any good solution, which is exactely why I opened rust-lang/cargo#5881 back then.

One key problem that we have have to support two cases: I call them the bin case and lib case. The lib is used for building native python modules. The bin case is used for writing rust applications that somehow integrate a python interpreter and for the native python modules with cargo test. Both targets need different linker arguments, that will lead to errors when set on the other target.

The other key problem is that we need to set the linker args dependent on the current environment.

The only way to dynamically define linker arguments depending on some condition (in this case the target) I am aware of are build scripts. Since pyo3's build script will only be run once per workspace, it will always be incorrect for one of the two crates. I unfortunately do not see any solution that could potentially work for workspaces.

@nagisa
Copy link
Contributor Author

nagisa commented Feb 21, 2020

2 pyo3 crates for the separate use-cases (pyo3-extension to if you’re writing an extension and pyo3 otherwise) would help to an extent. It still would fail to work for a crate that has both a library and binary output, but it would not fail to work in a workspace where you have a binary and extension as entirely separate entities.

Another option would be to move the attachment of the relevant linker arguments to the user crate. e.g. pyo3 could expose

macro_rules! link_python { 
    () => {
        #[link(name=env!("PYO3_PYTHON_BINARY_LINK_ARG"))] // envvar exported by the build script
        extern {}
    }
}

and the binaries using python would be responsible for invoking this macro somewhere inside (usually main.rs?) that’s used only in the binary but not the library. Would work in either the workspace and multiple-output-crate case.

The latter option is what we currently (in its non-macro form) use in our project that originally prompted this issue.

@davidhewitt
Copy link
Member

Another option would be to move the attachment of the relevant linker arguments to the user crate. e.g. pyo3 could expose

A trick like this is interesting. It would be best if we can find a solution which just works automagically without the user having to do anything.

Though a one-liner for downstream binaries (and tests) would be better than having certain combinations of crates completely incompatible with each other, which is where we are now.

@kngwyu
Copy link
Member

kngwyu commented Mar 9, 2020

How about using bin_module, which behaves contrary to extension-module?

Another option would be to move the attachment of the relevant linker arguments to the user crate.

Then #[pyo3::main] macro would be good, like #[tokio::main](though it's a bit overdone)

@konstin
Copy link
Member

konstin commented Mar 9, 2020

How about using bin_module, which behaves contrary to extension-module?

This would cause the same problem, just that this time extension modules would link libpython where they shouldn't.

2 pyo3 crates for the separate use-cases (pyo3-extension to if you’re writing an extension and pyo3 otherwise) would help to an extent. It still would fail to work for a crate that has both a library and binary output, but it would not fail to work in a workspace where you have a binary and extension as entirely separate entities.

I tried this, and astoundingly enough this seems to work. This definitely looks worth exploring, even though it still needs to be checked whether this also works for cargo test.

Another option would be to move the attachment of the relevant linker arguments to the user crate. e.g. pyo3 could expose

macro_rules! link_python { 
    () => {
        #[link(name=env!("PYO3_PYTHON_BINARY_LINK_ARG"))] // envvar exported by the build script
        extern {}
    }
}

and the binaries using python would be responsible for invoking this macro somewhere inside (usually main.rs?) that’s used only in the binary but not the library. Would work in either the workspace and multiple-output-crate case.

The latter option is what we currently (in its non-macro form) use in our project that originally prompted this issue.

Does this allow passing global linker like the shared library linking we need to do?

@nagisa
Copy link
Contributor Author

nagisa commented Mar 9, 2020

Does this allow passing global linker like the shared library linking we need to do?

It is possible, but unstable: rust-lang/rust#29596

I tried this, and astoundingly enough this seems to work. This definitely looks worth exploring, even though it still needs to be checked whether this also works for cargo test.

This approach won’t work with cargo test, unless I’m missing some obvious hack on how to depend on pyo3 and not plain pyo3-extension when building a test.

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