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

DEP: refactored out runtime dependency on setuptools #258

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Jul 19, 2024

Yesterday we ran into some issues with the latest version of setuptools at astropy (see this issue for context).
In summary, what happened is that setuptools 71 brought in a couple runtime side effects, resulting in some confusing error messages.
My analysis showed that setuptools is only present in the runtime environment that astropy runs tests with because it's brought in by this package, which only requires it for a single line of code, so I refactored it using standard lib importlib.metadata + packagaging (which is already a runtime dependency here), eliminating setuptools as a runtime dependency, which down the road eliminates a source of instabilitly for any package that relies on pytest-doctestplus.

In case it's not clear (it wasn't to me), note that pkg_resources is really part of setuptools and not included in the standard library.

@neutrinoceros neutrinoceros marked this pull request as ready for review July 19, 2024 11:46
@pllim pllim added this to the 1.3.0 milestone Jul 19, 2024
packaging>=17.0

[options.extras_require]
test =
numpy
pytest-remotedata>=0.3.2
setuptools>=30.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it still needed in test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because some tests (e.g. tests/test_doctestplus.py::test_ufunc) include an entire setup.py script that requires setuptools

@pllim
Copy link
Contributor

pllim commented Jul 19, 2024

Just to make sure the failures are related, I reran a previously successful build from 3 days ago: https://github.com/scientific-python/pytest-doctestplus/actions/runs/9950476771

Update: main is green, so failures probably related.

@pllim
Copy link
Contributor

pllim commented Jul 19, 2024

Since this removes a required dependency, might need a change log as well.

@neutrinoceros
Copy link
Contributor Author

yup; failures are related, I'll have a look.

@neutrinoceros
Copy link
Contributor Author

Failures are specifically hit with pre-releases versions of pytest. I'm going to force push the fix. For ease of read, here it is

diff --git a/pytest_doctestplus/utils.py b/pytest_doctestplus/utils.py
index f30e1f5..8a6df5c 100644
--- a/pytest_doctestplus/utils.py
+++ b/pytest_doctestplus/utils.py
@@ -20,7 +20,7 @@ class ModuleChecker:
         except Exception:
             return None
         else:
-            if reqs.specifier.contains(dist_meta.version):
+            if reqs.specifier.contains(dist_meta.version, prereleases=True):
                 return dist_meta
             else:
                 return None

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@pllim
Copy link
Contributor

pllim commented Jul 19, 2024

Looks like Squash & Merge not enabled here, so thanks for squashing!

@pllim pllim merged commit 962de7f into scientific-python:main Jul 19, 2024
22 checks passed
@neutrinoceros neutrinoceros deleted the dep/ditch_setuptools branch July 19, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants