Be stricter about our Python factors #2830
Conversation
We are relying on 'PythonSpec.from_string_spec' from 'virtualenv' to determine if a factor is actually a Python factor that implies a specific base python. The regex 'PythonSpec.from_string_spec' is too loose for our purposes so use our own. Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: tox-dev#2657
We currently allow users to request a Python factor without an interpreter short name prefix. This is highly unusual and rather error prone, given it's likely to conflict with e.g. versions of other packages. We shouldn't really support this so start logging a warning, effectively deprecating this and allowing us to remove this functionality later. Signed-off-by: Stephen Finucane <stephen@that.guru>
|
Need to add tests and a release note. Next week's work though. |
|
@stephenfin do you plan picking up this today, or shall I cut the new release without this? |
|
I'll wrap this up shortly. |
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
|
I cannot accept #2848 as is for the reasons outlined in it. We can fine-tune the range of valid specs, though; but that's a conversation for that PR. |
| # TODO: Drop in a future version | ||
| # PythonSpec.from_string_spec set implementation to None for py and python |
There was a problem hiding this comment.
Remove the TODO. We haven't decided if we'll do or not.
| if impl is None and not factor.startswith("py") and not factor.startswith("python"): | ||
| logging.warning( | ||
| "identified the %s factor in the %s testenv as a Python spec but the interpreter implementation " | ||
| "was not specified. This will be a ignored in future versions of tox. Please prefix this factor " | ||
| "with an interpreter short name (one of: %s)", | ||
| factor, | ||
| env_name, | ||
| ", ".join(list(INTERPRETER_SHORT_NAMES) + ["py"]), | ||
| ) |
There was a problem hiding this comment.
Remove this warning please. tox r -e 3.8 I think should be valid and we should default to py prefix in that case.
| # TODO: This is acceptable to 'PythonSpec.from_string_spec' but is | ||
| # clearly not valid :( |
There was a problem hiding this comment.
Remove todo 👍 we can discuss this in an issue, don't need code comments for it.
| ("2000", "2000"), | ||
| ("4000", None), | ||
| ], | ||
| ids=lambda a: "|".join(a) if isinstance(a, list) else str(a), |
|
Thanks for the review. I think doing our own parsing for factors [*] is a better than plugging holes in virtualenv's [*] Specifically factors. |
Fixes #2657 by only using non-prefixed factors that look like Python versions, rather than accepting virtually anything.
We also opt to discourage non-prefixed factors since these are far too likely to conflict with other non-Python factors and the current behavior is very magical. We can fully remove this functionality in a future release.
Thanks for contribution
Please, make sure you address all the checklists (for details on how see
development documentation)!
tox -e fix)docs/changelogfolder