-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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)
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. |
0933a85
to
c46d355
Compare
There was a problem hiding this 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.
…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]>
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. |
Many thanks for this both! |
Great! I will probably release 1.10.0 soon (maybe with testing on 3.13). |
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