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.

Sign up for GitHub

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

Jump to bottom

Numeric factor in environment name wrongly causes "conflicting factors" error #2657

Closed
nsoranzo opened this issue Dec 9, 2022 · 2 comments · Fixed by #2849
Closed

Numeric factor in environment name wrongly causes "conflicting factors" error #2657

nsoranzo opened this issue Dec 9, 2022 · 2 comments · Fixed by #2849
Labels
bug:minor does not affect many people or has no big impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@nsoranzo
Copy link

nsoranzo commented Dec 9, 2022

#2597 had the unintended effect of interpreting a numeric factor (e.g. 2105) as a candidate base python, breaking previously valid environment names (e.g. py37-foo-2105) by raising ValueError: conflicting factors py37, 2105 in py37-foo-2105.
Example GitHub workflow build: https://github.com/galaxyproject/planemo/actions/runs/3654111042/jobs/6174245903 for tox.ini https://github.com/galaxyproject/planemo/blob/nsoranzo-patch-1/tox.ini

Originally posted by @nsoranzo in #2597 (comment)
This was referenced Dec 9, 2022
Merged
Merged
@tushar-deepsource
Copy link

I have the same error, but for a slightly different reason.

For an environment named py310-foo-py, I'm getting:

ValueError: conflicting factors py310, py in py310-foo-py

@tushar-deepsource tushar-deepsource mentioned this issue Dec 9, 2022
Closed
@gaborbernat gaborbernat added bug:minor does not affect many people or has no big impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Dec 9, 2022
@gaborbernat gaborbernat added this to the 4.0.x milestone Dec 9, 2022
@gaborbernat
Copy link
Member

gaborbernat commented Dec 9, 2022 •
edited

For an environment named py310-foo-py, I'm getting:

This is not the same; in your case, failure is expected, as explained in another issue.

@altendky altendky mentioned this issue Dec 16, 2022
Closed
nsoranzo added a commit to nsoranzo/tox that referenced this issue Dec 21, 2022
@nsoranzo
Skip factors that don't start with an alphabetic character
c0592f9 
A numeric factor in an environment name can wrongly cause a
"conflicting factors" error.
Fix tox-dev#2657 .
nsoranzo added a commit to nsoranzo/tox that referenced this issue Dec 21, 2022
@nsoranzo
Skip factors that don't start with an alphabetic character
c77405b 
A numeric factor in an environment name can wrongly cause a
"conflicting factors" error.
Fix tox-dev#2657 .
@nsoranzo nsoranzo mentioned this issue Dec 21, 2022
Closed
5 tasks
@stephenfin stephenfin mentioned this issue Jan 6, 2023
Closed
5 tasks
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 6, 2023
@stephenfin
Be stricter about our Python factors
2b3aa2e 
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
@stephenfin stephenfin mentioned this issue Jan 9, 2023
Merged
5 tasks
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 9, 2023
@stephenfin
Only return Python factor on base_python conflict
60fa657 
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 <stephen@that.guru>
Fixes: tox-dev#2838
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 9, 2023
@stephenfin
Only return Python factor on base_python conflict
c26da88 
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 <stephen@that.guru>
Fixes: tox-dev#2838
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 10, 2023
@stephenfin
Only return Python factor on base_python conflict
e075089 
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 <stephen@that.guru>
Fixes: tox-dev#2838
This was referenced Jan 10, 2023
Closed
Merged
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 10, 2023
@stephenfin
Revert to supporting simple Python factors
e001896 
Instead of parsing factors with virtualenv's
'PythonSpec.from_string_spec', use our own regex to support the simple
factors that have been supported since tox v1.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Fixes: tox-dev#2657
Fixes: tox-dev#2848
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 10, 2023
@stephenfin
Add tests for issue tox-dev#2657
2320e6a 
Signed-off-by: Stephen Finucane <stephen@that.guru>
stephenfin added a commit to stephenfin/tox that referenced this issue Jan 10, 2023
@stephenfin
Revert to supporting simple Python factors
4e64555 
Instead of parsing factors with virtualenv's
'PythonSpec.from_string_spec', use our own regex to support the simple
factors that have been supported since tox v1.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Fixes: tox-dev#2657
Fixes: tox-dev#2848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug:minor does not affect many people or has no big impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Milestone
No milestone
3 participants
@gaborbernat @nsoranzo @tushar-deepsource