Only return Python factor on base_python conflict

[stephenfin]

Consider the following tox.ini file:

[tox]
ignore_base_python_conflict = true

[testenv]
base_python = python3
commands = ...

[testenv:functional{,-py38,-py39,-py310}]
commands = ...

There is a conflict between the base_python value specified in [testenv] base_python and the value implied by the pyXY factors in the functional-pyXY test envs. The Python._validate_base_python function is supposed to resolve this for us and either (a) raise an error if [tox] ignore_base_python_conflict is set to false (default) or (b) ignore the value of [testenv] base_python in favour of the value implied by the pyXY factor for the given test env if [tox] ignore_base_python_conflict is set to true. There's a bug though. Rather than returning the pyXY factor, we were returning the entire test env name (functional-pyXY). There is no Python version corresponding to e.g. functional-py39 so this (correctly) fails.

We can correct the issue by only returning the factor that modified the base_python value, i.e. the pyXY factor. To ensure we do this, we need some additional logic. It turns out this logic is already present in another helper method on the Python class, extract_base_python, so we also take the opportunity to de-duplicate and reuse some logic.

Note that this change breaks the ability of users to use a testenv name like py38-64 (to get the 64 bit version of a Python 3.8 interpreter). Continuing to support this would require much larger change since we'd no longer be able to strictly delimit factors by hyphens (in this case, the entirety of py38-64 becomes a factor).

Also note that this change emphasises issue #2657, as this will now be raised for a factor like py38-64 since tox (or rather, virtualenv) is falsely identifying 64 as a valid Python interpreter identifier. We will fix this separately so the offending test are skipped for now.

Signed-off-by: Stephen Finucane [email protected]
Fixes: #2838

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

[gaborbernat]

Note that this change breaks the ability of users to use a testenv name like py38-64 (to get the 64 bit version of a Python 3.8 interpreter). Continuing to support this would require much larger change since we'd no longer be able to strictly delimit factors by hyphens (in this case, the entirety of py38-64 becomes a factor).

We must support this, and I cannot accept a change that would introduce this regression.

[gaborbernat]

We must support this, and I cannot accept a change that would introduce this regression.

[stephenfin]

Note that this change breaks the ability of users to use a testenv name like py38-64 (to get the 64 bit version of a Python 3.8 interpreter). Continuing to support this would require much larger change since we'd no longer be able to strictly delimit factors by hyphens (in this case, the entirety of py38-64 becomes a factor).

We must support this, and I cannot accept a change that would introduce this regression.

But why? Who is the target user here? Who needs this functionality? fwiw, I added documentation on how factors worked (since there wasn't any before then) way back in #1573 that seems to have been lost in the rewrite, but when researching that there was no suggesting that e.g. py310-64 was a valid factor.

If you need to support this, how do you propose determining whether something is a factor or not? For example, what would the Python versions used in each of the below be?

• py310-64
• py310-65
• functional-py310-1
• functional-py310-a1
• foo-3.10-1-bar
• foo-py3.10.1.2.3
• py-3-10

While you're considering answers for these, please note how much cognitive effort is required for each, compared to simply supporting pyXY.

Personally, I think we're tacking too close to what virtualenv supports which is super loose. We're not talking about virtualenv, we're talking about tox. There's no reason they need to behave identically. If people want to get virtualenv's specific behavior, they can just set base_python:

[testenv:py310]
base_python = python3.10-64

[gaborbernat]

If you need to support this, how do you propose determining whether something is a factor or not? For example, what would the Python versions used in each of the below be?

• py310-64
• py310-65
• functional-py310-1
• functional-py310-a1
• foo-3.10-1-bar
• foo-py3.10.1.2.3
• py-3-10

https://virtualenv.pypa.io/en/latest/user_guide.html#python-discovery

the architecture is either -64 or -32 (missing means any).

So only envs with either of those two factors should be accepted and the rest not.

Personally, I think we're tacking too close to what virtualenv supports which is super loose. We're not talking about virtualenv, we're talking about tox. There's no reason they need to behave identically. If people want to get virtualenv's specific behavior, they can just set base_python:

I'd actually consider that a feature, not a bug. I want tox to differ as little as possible from virtualenv to have only one way of doing thins.

[stephenfin]

If you need to support this, how do you propose determining whether something is a factor or not? For example, what would the Python versions used in each of the below be?

• py310-64
• py310-65
• functional-py310-1
• functional-py310-a1
• foo-3.10-1-bar
• foo-py3.10.1.2.3
• py-3-10

virtualenv.pypa.io/en/latest/user_guide.html#python-discovery

the architecture is either -64 or -32 (missing means any).

So only envs with either of those two factors should be accepted and the rest not.

As I noted above though, this means we can no longer delimit by hyphens alone, yes? We need to lookahead, check if any of the "factors" are 64 or 32, and if so see if we can combine that with the previous factor (if any).

Can you propose something we can slot into the documentation because I'm struggling to explain this in a way that doesn't make tox factors sound even more "magical" (in a bad way).

Personally, I think we're tacking too close to what virtualenv supports which is super loose. We're not talking about virtualenv, we're talking about tox. There's no reason they need to behave identically. If people want to get virtualenv's specific behavior, they can just set base_python:

I'd actually consider that a feature, not a bug. I want tox to differ as little as possible from virtualenv to have only one way of doing thins.

I respectfully disagree. This isn't a helpful goal for normal end-users who only care about tox for its automation of virtualenvs. If power users want a very specific python version (say, the 32 bit version of Python), they can specify base_python as noted above. I just don't see how this is something that's important enough to break the "hyphens are how we delimit" rule.

[gaborbernat]

As I noted above though, this means we can no longer delimit by hyphens alone, yes? We need to lookahead, check if any of the "factors" are 64 or 32, and if so see if we can combine that with the previous factor (if any).

You are correct 😊

. I just don't see how this is something that's important enough to break the "hyphens are how we delimit" rule.

Hmm, I never considered this a hard rule 🤔 and this still can be the case, no; just look ahead. I personally consider it a feature that you can use the same names in virtualenv spec as your tox env names 🤔 So seem we will need to disagree on this.

[stephenfin]

I had a go at doing ☝️ Just to note, I don't like this in the slightest. I feel it's too confusing and reinforces what I feel is a UX regression in tox 4. IMO, the call to PythonSpec.from_string_spec (in tox.tox_env.python.api.Python.extract_base_python) is a mistake that conflicts with the historical definition of what testenv factors look like (hyphen-separated alphanumeric slugs) and it makes our factor parsing too complicated and factors as a concept much more difficult to explain. I feel we should be stricter in supporting pyXY(Y)-style factors only and anything else should be configured via base_python (which by itself would resolve e.g. #2535). Our looseness here has caused multiple issues already (#2657, #2838). But, it's not my project. Do as you wish. At least this fixes the bug.

[gaborbernat]

I had a go at doing ☝️ Just to note, I don't like this in the slightest. I feel it's too confusing and reinforces what I feel is a UX regression in tox 4. IMO, the call to PythonSpec.from_string_spec (in tox.tox_env.python.api.Python.extract_base_python) is a mistake that conflicts with the historical definition of what testenv factors look like (hyphen-separated alphanumeric slugs) and it makes our factor parsing too complicated and factors as a concept much more difficult to explain. I feel we should be stricter in supporting pyXY(Y)-style factors only and anything else should be configured via base_python

Understood, I'll think about it. I'll also invite the other maintainers, @jugmac00 and @asottile, to comment on their opinions.

[stephenfin]

Thanks, I appreciate it 👍

[gaborbernat]

Let's remove the arch support in factors, for now, the 🤔 (the only support I could find was https://tox.wiki/en/legacy/example/basic.html#using-generative-section-names, but that's also just setting basepython instead).

[gaborbernat]

If you remove the last commit I'll accept this 👍 @stephenfin

[jugmac00]

"the correct Python version" - that would be which one? The one from base_python or from the factor? And while I could read the source code, I'd prefer to have it here, too.

As you are very involved in that topic... is this properly documented? Or do you think it would make sense to update the documentation, too?

[stephenfin]

It would be the version derived from the factor. If I run tox -e py310, I expect to get python3.10.

I did have this documented as part of #841 but that seems to have been lost in the rewrite. I can do another PR to (re-)flesh out the documentation. I also need to re-add docs explaining how factors work, which I'd done previously in #1573.

[jugmac00]

@gaborbernat Unfortunately, I am currently lacking the time to dig deeper in this topic. I rarely used base_python before and I just had to look up what ignore_base_python_conflict even means.

I am very happy that there is some in-depth discussion going on, as I think we really should be careful about introducing more breaking changes.

[stephenfin]

Let's remove the arch support in factors, for now, the thinking (the only support I could find was tox.wiki/en/legacy/example/basic.html#using-generative-section-names, but that's also just setting basepython instead).

I think the docs you want are here https://tox.wiki/en/legacy/config.html#tox-environments ? That details the "special'ness" of pyXY factors and how tox generates a factor matrix.

Note, I wrote those based on reverse engineering how tox worked so obviously I'm not a neutral party here, but I already had many years of experience using tox so it wasn't a totally uninformed take on thing 😄

[stephenfin]

If you remove the last commit I'll accept this +1 @stephenfin

Done. I had to force push so I also took the opportunity to rebase.