Fix various issues with missing interpreters #2828

stephenfin · 2023-01-06T14:58:35Z

This is a somewhat hefty PR that resolves a number of issues related to missing interpreters. Full details are provided in the commit messages and I would recommend reading these separately. These commits are:

Correctly skip/fail env for cached base_python values
Remove unnecessary assert
Note change in behaviour when all envs are skipped
Ignore missing interpreters when classifying env type
Return non-zero error code on skipped env

In summary, we fix #2811, #2826, and #2827. Tests are provided for all of these fixes. We also add a note to the documentation highlighting the change in behavior introduced in #2206, which I initially took to be a bug.

This PR could be split into multiple PRs but all three fixes are required to get a sane configuration for projects that set usedevelop=true and request missing Python interpreters somehow.

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

ran the linter to address style issues (tox -e fix)
wrote descriptive pull request text
ensured there are test(s) validating the fix
added news fragment in docs/changelog folder
updated/extended the documentation

We were correctly raising the Skip or NoInterpreter exceptions upon first access of the 'base_python' property of the 'Python' class, however, subsequent calls would simply return None. This meant we did not trigger our exception flow as expected and resulted in a traceback like so: ❯ tox --skip-missing-interpreter=true -e py33 py33: internal error Traceback (most recent call last): File "/tox/src/tox/session/cmd/run/single.py", line 45, in _evaluate tox_env.setup() File "/tox/src/tox/tox_env/api.py", line 248, in setup self._setup_env() File "/tox/src/tox/tox_env/python/runner.py", line 106, in _setup_env super()._setup_env() File "/tox/src/tox/tox_env/python/api.py", line 186, in _setup_env self.ensure_python_env() File "/tox/src/tox/tox_env/python/api.py", line 190, in ensure_python_env conf = self.python_cache() ^^^^^^^^^^^^^^^^^^^ File "/tox/src/tox/tox_env/python/virtual_env/api.py", line 77, in python_cache base = super().python_cache() ^^^^^^^^^^^^^^^^^^^^^^ File "/tox/src/tox/tox_env/python/api.py", line 228, in python_cache "version_info": list(self.base_python.version_info), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'version_info' py33: FAIL code 2 (0.00 seconds) evaluation failed :( (0.06 seconds) (from an environment without Python 3.3) Correct this so that we also raise these exceptions on subsequent accesses. ❯ tox --skip-missing-interpreter=true -e py33 py33: skipped because could not find python interpreter with spec(s): py33 py33: SKIP (0.00 seconds) evaluation failed :( (0.06 seconds) Note that this fix emphasises a change in behavior in tox 4. With the above configuration, tox 3 would have skipped the environment a reported success. By comparison, tox 4 reports a failure. This behavior change was introduced in tox-dev#2206. A future change will note this in the documentation. Also note that this is still not complete. Running the above command with '--skip-missing-interpreters=false' instead will currently result in an unhandled exception. This is an existing issue that will need to be addressed separately. Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#2826

By using the property rather than the attribute, we can rely on an exception being raised and no longer need to manually assert this ourselves. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin · 2023-01-06T15:01:28Z

src/tox/tox_env/python/api.py

+        if self._base_python is None:
+            if self.core["skip_missing_interpreters"]:
+                raise Skip(f"could not find python interpreter with spec(s): {', '.join(base_pythons)}")
+            raise NoInterpreter(base_pythons)


Note for reviewers: This is a new source of errors but we handle it below...

stephenfin · 2023-01-06T15:02:28Z

src/tox/tox_env/python/package.py

-            run_py = cast(Python, run_env).base_python
+            try:
+                run_py = cast(Python, run_env).base_python
+            except NoInterpreter:


Note for reviewers: This is where we handle those new errors I mentioned above. We also handle "old" errors: you could previously get here by running e.g. tox -e py33 on an environment that didn't provide python3.3.

stephenfin · 2023-01-06T15:03:32Z

tests/tox_env/python/test_python_runner.py

-    result = project.run("--skip-missing-interpreters", cli)
-    assert result.code == 0 if expected else 1
+    result = project.run(f"--skip-missing-interpreters={cli}")
+    assert result.code == (0 if expected else -1)


Note for reviewers: I've called this out in the commit message but this was a bug I only spotted when I copied the logic and found it wasn't working as expected. I suspect there are probably other places that we do this around here but I only fixed this one to keep the PR sane.

This behavior change was introduced in tox-dev#2206 and fixed tox-dev#2195. Call it out in the upgrade docs. Signed-off-by: Stephen Finucane <stephen@that.guru>

We now separate environments into either run or packaging environments [1]. As noted in 'tox.session.env_select.EnvSelector._defined_envs' [2], the name of the environment is not enough to determine what type of environment it is and we must actually build the environment and inspect it. This allows us to prevent users *running* these packaging environments (e.g. 'tox -e .pkg'). Part of this process of building an environment is validating the base python. If this validation fails (i.e. the Python version does not exist), we will raise 'tox.tox_env.python.api.NoInterpreter'. We were not handling this exception, and thus the process of determining the types of each environment would cause a failure if any environment requested a Python version we did not support, even if we weren't actually trying to run this environment. The fix for this is simple: handle the exception and simply ignore these unsupported environments. While we're here, fix some issues with an existing test that were noticed while adding new tests. [1] https://tox.wiki/en/latest/upgrading.html#packaging-configuration-and-inheritance [2] https://github.com/tox-dev/tox/blob/af35384bb2ee/src/tox/session/env_select.py#L173 Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#2811

In tox-dev#2206, we said that tox should fail if all envs are skipped. This is broken when only a single env runs. We print an error message but return 0. Correct this behavior. Signed-off-by: Stephen Finucane <stephen@that.guru> Closes: tox-dev#2827

Signed-off-by: Stephen Finucane <stephen@that.guru>

* use min version 4.2.5, for fixes [1][2][3] * passenv fixed as space-separated list is not allowed anymore * dock target uses requirements from requirements.txt as well as doc/requirements.txt * skipsdist is not supported * whitelist_externals has been removed in favour of allowlist_externals * reno was added to doc/requirements.txt to fix the releasenotes target * update setup.cfg to install aodh from tarball in the requirements The tarball wasn't being installed when specified in tox.ini, and the [extras] section in setup.cfg needed updating to support installing from a URL [1] tox-dev/tox#2754 [2] tox-dev/tox#2824 [3] tox-dev/tox#2828 Change-Id: I4122d0d05f297f864318e80392e6c77fb2e9fdcf

* Update python-aodhclient from branch 'master' to 02176deb25b3a8f1bc02eb3d70f0a50ce22f02f7 - Make tox.ini tox 4.0 compatible * use min version 4.2.5, for fixes [1][2][3] * passenv fixed as space-separated list is not allowed anymore * dock target uses requirements from requirements.txt as well as doc/requirements.txt * skipsdist is not supported * whitelist_externals has been removed in favour of allowlist_externals * reno was added to doc/requirements.txt to fix the releasenotes target * update setup.cfg to install aodh from tarball in the requirements The tarball wasn't being installed when specified in tox.ini, and the [extras] section in setup.cfg needed updating to support installing from a URL [1] tox-dev/tox#2754 [2] tox-dev/tox#2824 [3] tox-dev/tox#2828 Change-Id: I4122d0d05f297f864318e80392e6c77fb2e9fdcf

* Update aodh from branch 'master' to 84d59a198257ea6f3b839ca5d96cbaa74f2f3b76 - Make tox.ini tox 4.0 compatible * use min version 4.2.5, for fixes [1][2][3] * passenv fixed as space-separated list is not allowed anymore * doc target uses requirements.txt as well as docs/requirements.txt * skipsdist is not supported * Add usedevelop = False so that aodh-api gets installed Update setup.cfg: [files] -> [options] [1] tox-dev/tox#2754 [2] tox-dev/tox#2824 [3] tox-dev/tox#2828 Change-Id: I2422dc17e6c73ef346de80e57cdf61ef5d271d69

* use min version 4.2.5, for fixes [1][2][3] * passenv fixed as space-separated list is not allowed anymore * doc target uses requirements.txt as well as docs/requirements.txt * skipsdist is not supported * Add usedevelop = False so that aodh-api gets installed Update setup.cfg: [files] -> [options] [1] tox-dev/tox#2754 [2] tox-dev/tox#2824 [3] tox-dev/tox#2828 Change-Id: I2422dc17e6c73ef346de80e57cdf61ef5d271d69

This PR contains the following updates: | Package | Type | Update | Change | Pending | |---|---|---|---|---| | [tox](https://togithub.com/tox-dev/tox) ([changelog](https://tox.wiki/en/latest/changelog.html)) | dev | patch | `4.2.4` -> `4.2.5` | `4.3.5` (+8) | --- ### Release Notes <details> <summary>tox-dev/tox</summary> ### [`v4.2.5`](https://togithub.com/tox-dev/tox/releases/tag/4.2.5) [Compare Source](https://togithub.com/tox-dev/tox/compare/4.2.4...4.2.5) #### What's Changed - Fix various issues with missing interpreters by [@stephenfin](https://togithub.com/stephenfin) in [https://github.com/tox-dev/tox/pull/2828](https://togithub.com/tox-dev/tox/pull/2828) **Full Changelog**: tox-dev/tox@4.2.4...4.2.5 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box  Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>

stephenfin added 2 commits January 6, 2023 14:30

Remove unnecessary assert

e01b8e3

By using the property rather than the attribute, we can rely on an exception being raised and no longer need to manually assert this ourselves. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin requested a review from gaborbernat as a code owner January 6, 2023 14:58

psf-chronographer bot added the bot:chronographer:provided label Jan 6, 2023

stephenfin commented Jan 6, 2023

View reviewed changes

stephenfin added 3 commits January 6, 2023 15:05

Note change in behaviour when all envs are skipped

8457a43

This behavior change was introduced in tox-dev#2206 and fixed tox-dev#2195. Call it out in the upgrade docs. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin force-pushed the issues/2811 branch from b9eb563 to 5505c82 Compare January 6, 2023 15:08

Add missing attributions to changelog entries

3039214

Signed-off-by: Stephen Finucane <stephen@that.guru>

gaborbernat approved these changes Jan 6, 2023

View reviewed changes

gaborbernat merged commit c790c60 into tox-dev:main Jan 6, 2023

stephenfin deleted the issues/2811 branch January 6, 2023 16:13

mikelane mentioned this pull request Jan 9, 2023

Bump tox from 4.2.4 to 4.2.6 mikelane/reddit-get#128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various issues with missing interpreters #2828

Fix various issues with missing interpreters #2828

stephenfin commented Jan 6, 2023 •

edited

Loading

stephenfin Jan 6, 2023

stephenfin Jan 6, 2023 •

edited

Loading

stephenfin Jan 6, 2023 •

edited

Loading

Fix various issues with missing interpreters #2828

Fix various issues with missing interpreters #2828

Conversation

stephenfin commented Jan 6, 2023 • edited Loading

Thanks for contribution

stephenfin Jan 6, 2023

Choose a reason for hiding this comment

stephenfin Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

stephenfin Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

stephenfin commented Jan 6, 2023 •

edited

Loading

stephenfin Jan 6, 2023 •

edited

Loading

stephenfin Jan 6, 2023 •

edited

Loading