Skip to content

Delay imports of non-stdlib dependencies until time of use #437

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

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

eli-schwartz
Copy link
Contributor

This is a bit of a hack, but we do this to ensure that when setuptools loads entrypoint based hooks it cannot (will not?) crash.

The issue is that setuptools plugins are autoloaded, whether any given project uses them at all or not. So if setuptools-rust is installed, setuptools always tries to use it, and crashes if setuptools-rust is broken.

Of course, setuptools-rust can't be broken, because it's a wonderful project.

BUT.

As it happens, third-party vendors providing setuptools-rust can get into a situation where multiple packages need to be installed, including setuptools-rust, and also build yet other packages from source. In the middle of this, setuptools-rust itself could be installed but in "half-configured" state, i.e. its dependencies were queued for afterwards due to complicated dependency graph magic.

In such a scenario, it should be nominally all right to have an inert package installed, since if nothing actually uses setuptools-rust it doesn't need to work yet. And in fact, it is all right, as long as setuptools can import the autoloaded plugin hooks (and do nothing with them).

Bug: https://bugs.gentoo.org/933553

This is a bit of a hack, but we do this to ensure that when setuptools
loads entrypoint based hooks it cannot (will not?) crash.

The issue is that setuptools plugins are autoloaded, whether any given
project uses them at all or not. So if setuptools-rust is installed,
setuptools always tries to use it, and crashes if setuptools-rust is
broken.

Of course, setuptools-rust can't be broken, because it's a wonderful
project.

BUT.

As it happens, third-party vendors providing setuptools-rust can get
into a situation where multiple packages need to be installed, including
setuptools-rust, and also build yet other packages from source. In the
middle of this, setuptools-rust itself could be installed but in
"half-configured" state, i.e. its dependencies were queued for
afterwards due to complicated dependency graph magic.

In such a scenario, it should be nominally all right to have an inert
package installed, since if nothing actually uses setuptools-rust it
doesn't need to *work* yet. And in fact, it is all right, as long as
setuptools can import the autoloaded plugin hooks (and do nothing with
them).

Bug: https://bugs.gentoo.org/933553
Since setuptools-rust already depends on setuptools, it is reasonable to
assume that even if tomli isn't installed, setuptools is. And setuptools
includes a vendored copy of tomli.

If the copy in setuptools has been devendored, it will be available via
"tomli". If it isn't devendored, it will be available via
"setuptools.extern.tomli" unless setuptools changes their vendoring
approach which has lasted many years so far. Either way, we are sure to
have a fallback tomli without explicitly depending on one, which means
one less dependency to install in the common case.
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I think the motivation is very reasonable, so let's find a way to support this edge case.

To keep the code simpler, are you perhaps willing to adjust this PR to vendor (with appropriate attribution) the parts of semantic_version we actually use? I think we use barely any of it, and maybe a nicer end state is for setuptools_rust to have no external dependencies except setuptools?

try:
from tomli import load as toml_load
except ImportError:
from setuptools.extern.tomli import load as toml_load
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting; I wasn't aware of this namespace and have just asked upstream at pypa/setuptools#4351 (comment)

@eli-schwartz
Copy link
Contributor Author

To keep the code simpler, are you perhaps willing to adjust this PR to vendor (with appropriate attribution) the parts of semantic_version we actually use? I think we use barely any of it, and maybe a nicer end state is for setuptools_rust to have no external dependencies except setuptools?

It's not a huge project. It's 1185 lines of code and looks like it could trivially reduce to 835, which seems hairy.

And I don't really think dependencies are bad per se, at least when you actually intend to use them.

OTOH I have thought for years that version parsing should be part of the stdlib, but the only move on that part was to remove distutils.version without stdlib-replacement as part of removing distutils. 🤷 Even just PEP 440 support would be nice considering it's such a building block for other python tools and has no real churn.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood; let's proceed with this approach then!

Zero dependencies would have been nice for ease of testing, though I agree the semantic_version code doesn't reduce as nicely as I would have liked.

@davidhewitt davidhewitt merged commit 34c5f16 into PyO3:main Jun 11, 2024
39 checks passed
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jun 11, 2024
…x merge

When rebuilding world with python 3.12 it's very easy to end up with a
merge order that results in building setuptools-rust's dependencies for
3.12 only, but not yet merging setuptools-rust. This then breaks all
other packages relying on setuptools, due to plugin autoloading.

Fix this by making setuptools-rust only depend on other packages when it
is actually used, but not in the APIs invoked by the plugin autoloading.
This means that the package only has to be fully merged in time for
other packages that have an explicit BDEPEND on it, which is fine as
that is already the case.

Bug: PyO3/setuptools-rust#437
Closes: https://bugs.gentoo.org/933553
Signed-off-by: Eli Schwartz <[email protected]>
Signed-off-by: Sam James <[email protected]>
@eli-schwartz eli-schwartz deleted the plugin-defer branch June 11, 2024 14:37
@eli-schwartz
Copy link
Contributor Author

Thanks! I've backported this PR now, so that users doing upgrades with complex dependency graphs on Gentoo can have the fix for 1.9.0 as well.

@thesamesam
Copy link

Many thanks for this both!

@davidhewitt
Copy link
Member

Great! I will probably release 1.10.0 soon (maybe with testing on 3.13).

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.

3 participants