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

Remove self-dev-dependency from Cargo.toml #1817

Closed
davidhewitt opened this issue Aug 21, 2021 · 5 comments · Fixed by #2039
Closed

Remove self-dev-dependency from Cargo.toml #1817

davidhewitt opened this issue Aug 21, 2021 · 5 comments · Fixed by #2039

Comments

@davidhewitt
Copy link
Member

From the discussion in #1811, it sounds like the use of path = . dev-dependency is ill-advised.

pyo3 = { path = ".", default-features = false, features = ["macros", "auto-initialize"] }

@decathorpe - the use of this arose because cargo test without the features enabled by the self-dev-dependency is more-or-less pointless - we need the auto-initialize feature enabled else every test will fail when trying to get the Python interpreter. Perhaps I can come up with a better solution for that.

FWIW this was recommended in rust-lang/cargo#2911 (comment)

@davidhewitt
Copy link
Member Author

I think a #[pyo3::test] proc-macro which automatically called prepare_freethreaded_python() and Python::with_gil could remove a lot of boilerplate as well as the need for auto-initialize from most tests.

doctests / example would still need an alternative solution. Could just put pyo3::prepare_freethreaded_python() as a hidden statement in the top of all of them for now.

@davidhewitt
Copy link
Member Author

davidhewitt commented Aug 21, 2021

Hmm, this actually makes the problem worse - it means that the tests even stop building without the macros feature enabled.

I'm not sure we can resolve easily without upstream support in Cargo?

@decathorpe
Copy link
Contributor

I think the "easiest" solution would be to either move those tests to tests/foo.rs and specify their required-features in Cargo.toml (if that's possible, e.g. the tests are only using public API), or to wrap unit tests that use non-default features in something like #[cfg(all(feature = "macros", feature = "auto-initialize"))] attributes. This way, they won't be compiled if those features are not enabled, but you can still compile and run them with cargo test --all-features etc.

@davidhewitt
Copy link
Member Author

I agree that's the most theoretically correct path, however it also seems impractical for PyO3's general maintenance.

The auto-initialize feature in particular seems unavoidable to test without.

Pretty much all of the unit tests and doctests in PyO3 need to interact with the Python interpreter, so unless every test added boilerplate to initialize the GIL (i.e. a call to pyo3::prepare_freethreaded_python) then pretty much all of these tests and doctests would be decorcated in cfg attributes.

I also think that most users and maintainers rightly expect that cargo test covers the default functionality of PyO3's codebase, so I think if cargo test --features auto-initialize became the correct way to run the PyO3 test suite, there'd be a lot of confusion / friction for contributors.

Indeed the issue which spawned this discussion uses cargo test without any feature flags: #1811 (comment)

My personal opinion here is that it's extremely desirable to have cargo test cover the majority of PyO3's codebase. On balance, it seems significantly nicer to have this single line in Cargo.toml rather than add crate::prepare_freethreaded_python() to every test.

I'd really like any solution we explore here to maintain the current usefulness of bare cargo test.

Perhaps there's scope for an API like Python::with_gil_init_once which wraps with_gil and first calls prepare_freethreaded_python. It'd be an alternative to the auto-initialize feature, essentially, which we'd use in all tests & doctests.

I don't particularly like the name with_gil_init_once, so ideas for a nice API design on this point would be great.

@decathorpe
Copy link
Contributor

cargo test --features auto-initialize

I think this would be an acceptable workaround for the problem we have in Fedora packaging, as it's trivially easy to just add this flag in our packaging.

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

Successfully merging a pull request may close this issue.

2 participants