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

Replace extension-module feature with pyo3::main and pyo3::test #921

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

This is an experiment to remove extension-module feature, with its known linking problems. See #904 #771 #341

Instead, to enable linking to libpython when needed, I introduce two proc macro attributes
#[pyo3::main] and #[pyo3::test] which force linking using the macro suggested in #771

To sweeten the deal, both of these attributes can automatically acquire the GIL if you make the first argument py: Python. e.g.:

#[pyo3::main]
fn main(py: Python) {
  // use py! :)
}

This implementation is not yet complete - I hard-coded libpython3.8 to test on my laptop. Needs to use proper detection of the python library. It also breaks windows where this hack isn't needed.

I'm happy with #[pyo3::main], though I wonder if adding this link arg to every #[pyo3::test] is overkill - an alternative would be to have a pyo3_test_link_python!() macro or so which could be included once in each test file.

@davidhewitt
Copy link
Member Author

@m-ou-se I'm not quite sure if this solves what you need for inline-python. I think it might - users of inline-python will just have to add #[pyo3::main] where appropriate?

@programmerjake
Copy link
Contributor

it might be a good idea to be able to apply #[pyo3::main] or similar to a function other than fn main(), since there are systems that can't use the default main function, e.g. for libSDL on Android where the main is essentially provided by libSDL and the Android Java libraries.

@kngwyu
Copy link
Member

kngwyu commented May 12, 2020

Awesome, thank you!
I'll add detailed comments later, but from a glance:

  1. Do we need

            #[link(name = "python3.8")]
            extern "C" {}

    for each test function?

  2. We should also allow #[pyo3::main]fn main() {}

@programmerjake
Copy link
Contributor

another idea is to just merge them both into #[pyo3::entry] or #[pyo3::calls_python] or some other suitable name

@davidhewitt
Copy link
Member Author

davidhewitt commented May 12, 2020

Do we need [...] for each test function?

No, we only need this #[link] attribute once per executable really. So an alternative I was considering was that somewhere in a test file, users must add:

#[cfg(test)]
pyo3::link_python!();

I think it does make sense for #[pyo3::main] to include this #[link], as that should only ever appear once.

another idea is to just merge them both into #[pyo3::entry] or #[pyo3::calls_python] or some other suitable name

I quite like #[pyo3::main] because it's an obvious name that hits 99% of use cases. I think for the niche use cases you describe above I'd rather recommend they just call pyo3::link_python!() explicitly.

@davidhewitt
Copy link
Member Author

Also, I quite like having #[pyo3::test] separate to #[pyo3::main] because the test macro can then generate the #[test] attribute, as well as acquire the GIL if needed.

But maybe if we prefer just calling pyo3::link_python!() once per test file, and remove #[link] from #[pyo3::test], I should split it into a separate PR.

Let me know what design you all like best 😊

@programmerjake
Copy link
Contributor

honestly, I'm fine with having #[pyo3::main/test] as long as the link macro is also exposed for the corner cases.

@m-ou-se
Copy link
Contributor

m-ou-se commented May 12, 2020

I don't think an attribute like that is the way to go.

Some of my projects use Python somewhere in some module, but not directly from main. Attaching the attribute to main seems like the wrong place. It automatically locking the GIL is also not always what I'd want.

It also requires the use of a procedural macro, making the recently introduced macros feature a bit useless, as you'd always need a separate proc_macro crate with all the dependencies (quote, proc_macro2, syn, ..) again. I'd much prefer a simple macro_rules!{} macro like pyo3::link_python!().

But even then, it seems not like a very ergonomic solution. Everybody who forgets this macro (including all existing code) will just get a long linker error with no proper explanation. :(

Maybe this is something to be solved on the side of Cargo/Rustc instead. This is not the only problem when compiling plugins/extensions (whether for Python or other things). There's the problem of -undefined dynamic_lookup on MacOS, there's the problem of the name of the compiled library being libabc.so instead of abc.so, and the problem of only allowing one plugin/extension per package/Cargo.toml, because they are a library and not binaries to Cargo.

Maybe it's time to propose an addition to Cargo, allowing something like

[[bin]]
crate-type = ["plugin"]

and then letting build scripts specify flags for these separately. Or something in that direction.

@davidhewitt
Copy link
Member Author

davidhewitt commented May 12, 2020

I agree that the error case for forgetting to link is not very ergonomic.

A cargo plugin sounds great, though I guess it could be a long time before the design and implementation of it is complete.

Maybe a better workaround for now is to keep extension-module and add the pyo3::link_python!() macro? That shouldn't break existing code and will let those that run into problem cases have a way to compile.

@m-ou-se
Copy link
Contributor

m-ou-se commented May 12, 2020

Maybe a better workaround for now is to keep extension-module and add the pyo3::link_python!() macro? That shouldn't break existing code and will let those that run into problem cases have a way to compile.

@davidhewitt That might be the least surprising option for now. None of the solutions are perfect, but at least this one means that the regular use cases don't change, and the macro only needs to be used in the edge cases.

@davidhewitt
Copy link
Member Author

Yikes, accidentally re-used the same branch name for a new PR... Closing this PR. I plan to revisit the linking macro, but otherwise I think this is dead code.

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