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

Validate Requires-Python metadata against the "mindeps" build in CI config #809

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Aug 16, 2023

Rationale

This is my first crack at solving a problem I've dealt with in this and other projects in the past:

Suppose you drop support for a python version (e.g. 3.7, which is EOL and which we should drop soon).
There are numerous fields and locations which need updating. CI, tox, classifiers, etc.
The trouble with doing this is that python_requires/Requires-Python (one is the key in setuptools, the other is the field in PKG-INFO) has very particular semantics and is the most important of all of these fields.
Failing to set your new python version boundary correctly results in solvers always having some chance of picking up on your new version even on an unsupported python version.

Ideally updates are done perfectly and there's never any worry, but I've seen this go wrong and I've come close to doing it wrong myself.

Happily, there's a clear directionality at play: if you no longer test on a python version, you shouldn't say you support that version.
We have a specific build (mindeps) whose purpose is to check the lowest versions of our requirements, including the meta-requirement that is the python version.
So all we need to do is make sure that "mindeps" is testing our Requires-Python.

Approach & Details

Part I: get_python_requires.py

First, I wrote a script to build a virtualenv, install build, python -m build --sdist, and extract the Requires-Python data from the sdist. It keeps the virtualenv for build cached and puts the sdist in a temp dir so that we're always building the parts we need afresh but we're not doing a full virtualenv construction on every call.
This is structured as a standalone component which I might test and ship separately at some point as its own packaged application. (Doing so would allow use of packaging to do more sophisticated version parsing, etc.)

Now I can reliably pull Requires-Python from any PEP 517 package, including our own. It will work if we switch to hatch or flit or poetry or distutils2023 (no, that's not real, don't worry 😉 ).

Part II: ensure_min_python_is_tested.py

Now we need to know what version we're testing. There are several sources of info we could check (e.g. tox list), but for this I have intentionally chosen only the mindeps CI build. We can expand it in the future.

To do this, I need a YAML parser. It was nice to make get_python_requires.py "pure" stdlib python which wraps its need for build, but that won't work here.
So this will always be called via tox. Once the YAML parser is available, pulling out test-mindeps and the python-version it uses is pretty easy.
Check against get_python_requires.py and we're done!

Part III: Cleanup

There are some minor changes to finalize.

  • cache that build virtualenv inside of the .tox dir so that we don't dirty up everyone's homedirs
  • add this test to CI tests

There are additional improvements we could allow. e.g. If build were installed in the surrounding env, get_python_requires.py would not need to build its own venv. I can pursue anything like this if it is important to anyone reviewing this.


📚 Documentation preview 📚: https://globus-sdk-python--809.org.readthedocs.build/en/809/

@sirosen sirosen added the no-news-is-good-news This change does not require a news file label Aug 16, 2023
This does a temporary sdist build and extracts the result in order to
get the (singular) '>=' lower bound.

If the expectations of the script are violated, an error is raised.

The approach is to be "slow but robust" here. We could simply parse
`setup.cfg`, but that makes the toolchain sensitive to changes in
packaging tooling. The PKG-INFO data should be reliable in all cases.
This check requires a YAML parser to pull the github workflow data
and crawl to the correct node.
Once there, the check will assert that the version in the CI build
*exactly* matches the lower bound in `Requires-Python`.

This leverages the `get_python_requires.py` script to extract the
requirements dir.
Allow passing a cache directory explicitly to
`get_python_requires.py`. Subsequently, the wrapping script,
`ensure_min_python_is_tested.py` can insist on an env var which
provides the cache dir path.
Finally, `tox.ini` gets a final tweak to set this value via setenv to
the `envdir` (the virtualenv dir which also contains the tmp and log
dirs).

The choice of an env var over parsing an argument is merely that it is
marginally simpler to read an env var and fail on its absence than it
is to do the same with an argument. (At the very least, *any* argument
parsing opens the question of whether or not argparse should be used.)
mddj is a public packaged version of the same functionality (in Alpha).

Pulling it out of this repo lets us rely on it without having to
carry the maintenance burden in globus-sdk of all of the relevant logic.
@sirosen
Copy link
Member Author

sirosen commented Aug 22, 2023

I've rebuilt the get_python_requires.py script into a public alpha package, the MetaData DJ (mddj).
As a result, this changeset now doesn't carry that project in tree at all.
We just install mddj==0.0.3 and call it with python -m mddj read python-requires --lower-bound.

This uses a new feature of `mddj` to read the tox factor list and
confirm that the minimum version declared in our requires-python data
is also synchronized with our tox.ini
@sirosen
Copy link
Member Author

sirosen commented Sep 26, 2023

Now that mddj supports parsing tox factors from tox --listenvs, I've bumped the version of it to 0.0.6 and made it part of the checks.
Currently:

$ mddj read tox min-version
3.7

@sirosen sirosen merged commit 933d677 into globus:main Oct 4, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants