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

tox 4 breaks generative env def with -pyXXX fragments if basepython is defined #2838

Closed
gibizer opened this issue Jan 9, 2023 · 12 comments · Fixed by #2840
Closed

tox 4 breaks generative env def with -pyXXX fragments if basepython is defined #2838

gibizer opened this issue Jan 9, 2023 · 12 comments · Fixed by #2840

Comments

@gibizer
Copy link

gibizer commented Jan 9, 2023

Issue

[tox]
[testenv]
base_python = python3
[testenv:functional{,-py38,-py39,-py310}]
[testenv:other]

This tox.ini is worked before in tox 3.28.0 to use the default python3 binary for other env while use the specific python binary for the functional envs. With tox 4.6.2. this result in conflict.

Environment

Provide at least:

  • OS:
root@3974fdf51f12:/tmp/repro# cat /etc/*rele*      
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.1 LTS"
PRETTY_NAME="Ubuntu 22.04.1 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.1 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy
  • pip list of the host Python where tox is installed:
root@3974fdf51f12:/tmp/repro# pip list 
Package       Version
------------- -------
cachetools    5.2.1
chardet       5.1.0
colorama      0.4.6
distlib       0.3.6
filelock      3.9.0
packaging     23.0
pip           22.0.2
platformdirs  2.6.2
pluggy        1.0.0
py            1.11.0
pyproject_api 1.4.0
setuptools    59.6.0
six           1.16.0
tomli         2.0.1
tox           4.2.6
virtualenv    20.17.1
wheel         0.37.1

Output of running tox

Provide the output of tox -rvv:

root@3974fdf51f12:/tmp/repro# tox -rvv -e functional-py310
.pkg: 58 W remove tox env folder /tmp/repro/.tox/.pkg [tox/tox_env/api.py:321]
functional-py310: 58 E failed with env name functional-py310 conflicting with base python python3 [tox/session/cmd/run/single.py:55]
  functional-py310: FAIL code 1 (0.00 seconds)
  evaluation failed :( (0.02 seconds)

Minimal example

If possible, provide a minimal reproducer for the issue:

[tox]
[testenv]
base_python = python3
[testenv:functional{,-py38,-py39,-py310}]
[testenv:other]
@stephenfin
Copy link
Contributor

stephenfin commented Jan 9, 2023

I think this is expected behaviour. You need to set [tox] ignore_base_python_conflict = true for this to work:

[tox]
ignore_base_python_conflict = true
[testenv]
base_python = python3
[testenv:functional{,-py38,-py39,-py310}]
[testenv:other]

However, when this is set I see the following:

❯ tox -e functional-py38
functional-py38: skipped because could not find python interpreter with spec(s): functional-py38
  functional-py38: SKIP (0.02 seconds)
  evaluation failed :( (0.06 seconds)

That seems wrong. I wonder if I introduced a bug? fwiw, py38 works just fine:

❯ tox -e py38
  py38: OK (0.24 seconds)
  congratulations :) (0.27 seconds)

EDIT: And if I drop [testenv] basepython, things also work as expected:

[testenv]
[testenv:functional{,-py38,-py39,-py310}]
[testenv:other]
❯ tox -e functional-py38
  functional-py38: OK (0.03 seconds)
  congratulations :) (0.07 seconds)
❯ source .tox/functional-py38/bin/activate
❯ python --version
Python 3.8.16

@gibizer
Copy link
Author

gibizer commented Jan 9, 2023

Yes, non generative targets works. Somehow during the generative target having a python version triggers the conflict with the basepython = python3

@gibizer
Copy link
Author

gibizer commented Jan 9, 2023

I believe in tox 2.38 I did not need ignore_base_python_conflict = true to make this work.

@stephenfin
Copy link
Contributor

Applying the following diff:

diff --git src/tox/tox_env/python/api.py src/tox/tox_env/python/api.py
index 9aa1f839..83d0bb3e 100644
--- src/tox/tox_env/python/api.py
+++ src/tox/tox_env/python/api.py
@@ -145,6 +146,8 @@ class Python(ToxEnv, ABC):
     def _validate_base_python(env_name: str, base_pythons: list[str], ignore_base_python_conflict: bool) -> list[str]:
         elements = {env_name}  # match with full env-name
         elements.update(env_name.split("-"))  # and also any factor
+        print(f'got env_name: {env_name}')
+        print(f'got base_pythons: {base_pythons}')
         for candidate in elements:
             spec_name = PythonSpec.from_string_spec(candidate)
             if spec_name.implementation and spec_name.implementation.lower() not in INTERPRETER_SHORT_NAMES:
@@ -234,8 +237,10 @@ class Python(ToxEnv, ABC):
         base_pythons: list[str] = self.conf["base_python"]
 
         if self._base_python_searched is False:
+            print(f'searching for: {base_pythons}...')
             self._base_python_searched = True
             self._base_python = self._get_python(base_pythons)
+            print(f'found: {self._base_python}')
             if self._base_python is not None and self.journal:
                 value = self._get_env_journal_python()
                 self.journal["python"] = value

With [testenv] basepython defined:

❯ tox -e functional-py38
got env_name: functional-py38
got base_pythons: ['python3']
searching for: ['functional-py38']...
found: None
functional-py38: skipped because could not find python interpreter with spec(s): functional-py38
  functional-py38: SKIP (0.01 seconds)
  evaluation failed :( (0.04 seconds)

Without [testenv] basepython defined:

❯ tox -e functional-py38
I called validate_base_python with ['py38']
got env_name: functional-py38
got base_pythons: ['py38']
searching for: ['py38']...
found: PythonInfo(implementation='CPython', version_info=VersionInfo(major=3, minor=8, micro=16, releaselevel='final', serial=0), version='3.8.16 (default, Dec  7 2022, 00:00:00) \n[GCC 12.2.1 20221121 (Red Hat 12.2.1-4)]', is_64=True, platform='linux', extra={'executable': PosixPath('/usr/bin/python3.8')})
  functional-py38: OK (0.02 seconds)
  congratulations :) (0.06 seconds)

So our logic in tox.tox_env.python.api.Python._validate_base_python is incorrect. I suspect this is based on the fact that we return the entire env name if there is a conflict, rather than just the factor that we found to conflict with a base_python setting:

return [env_name] # ignore the base python settings

Clearly functional-py38 is not a valid base_python value, though one of the factors (py38) is.

stephenfin added a commit to stephenfin/tox that referenced this issue Jan 9, 2023
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 tox-dev#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: tox-dev#2838
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 9, 2023
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 tox-dev#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: tox-dev#2838
@gaborbernat
Copy link
Member

gaborbernat commented Jan 9, 2023

With tox 4.6.2. this result in conflict.

Impressive that you're using that new version, considering our latest release is 4.2.6. EDIT: Sorry for the snarky comment, I've just seen too many times people saying this worked with 3.x so must with 4.x. 🤦 It's a new major version, it's expected that there are some breaking changes 🤔

This tox.ini is worked before in tox 3.28.0 to use the default python3 binary for other env while use the specific python binary for the functional envs.

Just because it happened to work with 3.x does not mean it was correct.

Yes, non generative targets works. Somehow during the generative target having a python version triggers the conflict with the basepython = python3

I'd consider this a bug; it should always fail.

I think this is expected behaviour. You need to set [tox] ignore_base_python_conflict = true for this to work:

I'd recommend not using that flag and instead fixing your configuration file; I'm marking it deprecated and will remove it at some point in the not too far future.

@stephenfin
Copy link
Contributor

stephenfin commented Jan 9, 2023

With tox 4.6.2. this result in conflict.

Impressive that you're using that new version, considering our latest release is 4.2.6.

@gaborbernat That reads kind of snarky. @gibizer clearly meant 4.2.6. I'll just helpfully point to our own Code of Conduct and emphasise:

Examples of behavior that contributes to creating a positive environment include:

  • Using welcoming and inclusive language
  • Being respectful of differing viewpoints and experiences
  • Gracefully accepting constructive criticism
  • Focusing on what is best for the community
  • Showing empathy towards other community members

(emphasis mine)

This tox.ini is worked before in tox 3.28.0 to use the default python3 binary for other env while use the specific python binary for the functional envs.

Just because it happened to work with 3.x does not mean it was correct.

Unfortunately Hyrum's Law is a thing, and it becomes doubly important when you have large communities relying on this behaviour. When I added ignore_basepython_conflict, I was very careful to make it opt-in to avoid breaking users and allow them time to fix things. To me, that was an example of how to migrate users away from broken configuration to a better configuration without inflicting serious pain on them. To me, this (breaking the feature without warning) is an example of how not to do that. You're killing us in OpenStack-land 😞

At the very least, the upgrade doc should note this, no?

Yes, non generative targets works. Somehow during the generative target having a python version triggers the conflict with the basepython = python3

I'd consider this a bug; it should always fail.

I think this is expected behaviour. You need to set [tox] ignore_base_python_conflict = true for this to work:

I'd recommend not using that flag and instead fixing your configuration file; I'm marking it deprecated and will remove it at some point in the not too far future.

Please re-consider this. This is a really desirable feature for de-duplicating tox configuration files, especially when multiple testenv sections are involved. Just because it's now full of bugs post-rewrite doesn't mean it's less useful, it just means the bugs need fixing.

@gaborbernat
Copy link
Member

gaborbernat commented Jan 9, 2023

Please re-consider this. This is a really desirable feature for de-duplicating tox configuration files, especially when multiple testenv sections are involved. Just because it's now full of bugs post-rewrite doesn't mean it's less useful, it just means the bugs need fixing.

People seem to be abusing it to create configurations that are confusing, so I'm really not a fan of it. It's not about the bugs, it's about being recommended as a quick fix for fixing confusing configurations.

This is a really desirable feature for de-duplicating tox configuration files,

I'd prefer if we come up with another way to solve this problem.

Unfortunately Hyrum's Law is a thing, and it becomes doubly important when you have large communities relying on this behaviour.

There's a reason we released this a 4.0 and not 3.x. Not just because is a full rewrite, but also because we intend to not support everything 3 happened to support.

When I added ignore_basepython_conflict, I was very careful to make it opt-in to avoid breaking users and allow them time to fix things.

Hence, why I'll not remove this option just today. But I'm also not going to leave it for long. People should find different solutions to this problem, and use this as a stop gap for a few months at most.

At the very least, the upgrade doc should note this, no?

Yes, most definitely!

@stephenfin
Copy link
Contributor

Please re-consider this. This is a really desirable feature for de-duplicating tox configuration files, especially when multiple testenv sections are involved. Just because it's now full of bugs post-rewrite doesn't mean it's less useful, it just means the bugs need fixing.

People seem to be abusing it to create configurations that are confusing, so I'm really not a fan of it. It's not about the bugs, it's about being recommended as a quick fix for fixing confusing configurations.

It seems pretty simply: factor version > base_python version. Clearly enough people find it useful too, since I see over 1k matches on GitHub alone. I'm sure some of those are cargo culted but the ability to provide a default for all environments absent an overriding python version factor has been super handy. There are literally 100s of developers working in the OpenStack community that rely on this everyday.

This is a really desirable feature for de-duplicating tox configuration files,

I'd prefer if we come up with another way to solve this problem.

By all means, please suggest one. If it works and we can implement it before we deprecate this feature then I've no issues switching. I'm not aware of any plans to remove this yet though.

Unfortunately Hyrum's Law is a thing, and it becomes doubly important when you have large communities relying on this behaviour.

There's a reason we released this a 4.0 and not 3.x. Not just because is a full rewrite, but also because we intend to not support everything 3 happened to support.

Python 3 isn't that long ago. We can't be forgetting the lessons of that transition so quickly. I realise some things are not going to be undone retrospectively but I really don't think we should be piling on the misery by e.g. removing a setting like this.

As a general aside, all of our lives (especially yours, I imagine 😄) would have been so much easier if this tox 4 transition had been a gradual, iterative process, like what the SQLAlchemy folks are doing with the 1.4 release before 2.0. The ship has sailed but this is proving really painful.

When I added ignore_basepython_conflict, I was very careful to make it opt-in to avoid breaking users and allow them time to fix things.

Hence, why I'll not remove this option just today. But I'm also not going to leave it for long. People should find different solutions to this problem, and use this as a stop gap for a few months at most.

See above ☝️

At the very least, the upgrade doc should note this, no?

Yes, most definitely!

@gaborbernat
Copy link
Member

By all means, please suggest one. If it works and we can implement it before we deprecate this feature then I've no issues switching. I'm not aware of any plans to remove this yet though.

Can you fill in a feature request issue and explain the problem you're trying to solve in detail? Thanks!

As a general aside, all of our lives (especially yours, I imagine 😄) would have been so much easier if this tox 4 transition had been a gradual, iterative process, like what the SQLAlchemy folks are doing with the 1.4 release before 2.0. The ship has sailed but this is proving really painful.

With a full revwrite is hard to do this. I've been sitting on version 4.0 for over a year now, and many people have been commenting to just release it otherwise will never be released 🤷 you'll only find out what unexpected features your users are using once you force them to use the new version. I've had 3 calls to verify before RC before releasing but not many people did, and clearly no one using this unexpected feature.

@rekby
Copy link

rekby commented Jan 10, 2023

My CI test env was broken with the issue while update tox from 4.2.4 to 4.2.5 (not while upgrade from 3.x to 4.x).

Line with tox version in test yaml (in later commits I removed base python line from tox config and update version to 4.2.6).

Action with issue log: https://github.com/ydb-platform/ydb-python-sdk/actions/runs/3881058691/jobs/6619662667

@gibizer
Copy link
Author

gibizer commented Jan 10, 2023

@gaborbernat @stephenfin thanks for pincking this up. I'm happy that this is progressing.

@stephenfin
Copy link
Contributor

By all means, please suggest one. If it works and we can implement it before we deprecate this feature then I've no issues switching. I'm not aware of any plans to remove this yet though.

Can you fill in a feature request issue and explain the problem you're trying to solve in detail? Thanks!

Sure, see #2846.

As a general aside, all of our lives (especially yours, I imagine smile) would have been so much easier if this tox 4 transition had been a gradual, iterative process, like what the SQLAlchemy folks are doing with the 1.4 release before 2.0. The ship has sailed but this is proving really painful.

With a full revwrite is hard to do this. I've been sitting on version 4.0 for over a year now, and many people have been commenting to just release it otherwise will never be released shrug you'll only find out what unexpected features your users are using once you force them to use the new version. I've had 3 calls to verify before RC before releasing but not many people did, and clearly no one using this unexpected feature.

That's fair and I sympathise. This being the case though, perhaps as these bugs are identified it would be better to fix the and then consider deprecating the problematic behaviour rather than saying "tough luck, we broke it by accident but won't fix it". Especially when people (like me) are showing up and offering to fix said bugs, rather than asking you to do it for them.

stephenfin added a commit to stephenfin/tox that referenced this issue Jan 10, 2023
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 tox-dev#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: tox-dev#2838
gaborbernat pushed a commit that referenced this issue Jan 10, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fixes #2838
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 20, 2023
* Update magnum-ui from branch 'master'
  to 452ca7da23f4489bc0dd25cfd031d4f1db04dfa6
  - Fix tox
    
    tox4 errors if basepython and python in other envs are different. [1]
    
    Bumped tox minversion as allowlist_externals is only supported in 3.18
    [2]
    
    [1] tox-dev/tox#2838
    [2] tox-dev/tox#2730
    
    Change-Id: I9d24395a7cc5d8423a58ec1e3ed8468ca6984e77
openstack-mirroring pushed a commit to openstack/magnum-ui that referenced this issue Feb 20, 2023
tox4 errors if basepython and python in other envs are different. [1]

Bumped tox minversion as allowlist_externals is only supported in 3.18
[2]

[1] tox-dev/tox#2838
[2] tox-dev/tox#2730

Change-Id: I9d24395a7cc5d8423a58ec1e3ed8468ca6984e77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants