tox4: Factor conditions are not considered for `commands`

[AWhetter]

Running tox v4 against the following tox.ini currently fails: https://github.com/pypa/virtualenv/blob/0262fa6d9a0f8b45c80e6b2f82109ab1250ec336/tox.ini#L45

It fails with the following error:

...
py39: commands[4]> python -m coverage xml -o /home/ashley/workspace/virtualenv/.tox/4/coverage.py39.xml
py39: commands[5]> python -m coverage html -d /home/ashley/workspace/virtualenv/.tox/4/py39/tmp/htmlcov '!py34:' --show-contexts --title virtualenv-py39-coverage
No source for code: '/home/ashley/workspace/virtualenv/!py34:'.
Aborting report output, consider using -i.
py39: exit 1 (0.12 seconds) /home/ashley/workspace/virtualenv> python -m coverage html -d /home/ashley/workspace/virtualenv/.tox/4/py39/tmp/htmlcov '!py34:' --show-contexts --title virtualenv-py39-coverage pid=471636
.pkg: _exit> python /home/ashley/envs/tox4/lib/python3.9/site-packages/tox/util/pep517/backend.py True setuptools.build_meta
  py39: FAIL code 1 (153.51=setup[11.91]+cmd[0.14,135.50,3.64,0.97,1.24,0.12] seconds)
  evaluation failed :( (153.54 seconds)

Running the same tests with tox v3 works fine.

[jugmac00]

The problem does not seem to be the continuation line, but that factors are not considered at all.

I get the same error with:

commands =
  python -m coverage erase
  python -m coverage run -m pytest --junitxml /home/jugmac00/Projects/virtualenv/.tox/4/junit.pypy3.xml tests --int --timeout 600
  python -m coverage combine
  python -m coverage report --skip-covered --show-missing
  python -m coverage xml -o /home/jugmac00/Projects/virtualenv/.tox/4/coverage.pypy3.xml
  python -m coverage html -d /home/jugmac00/Projects/virtualenv/.tox/4/pypy3/tmp/htmlcov '!py36:' --show-contexts --title virtualenv-pypy3-coverage

py39: commands[4]> python -m coverage xml -o /home/jugmac00/Projects/virtualenv/.tox/4/coverage.py39.xml
py39: commands[5]> python -m coverage html -d /home/jugmac00/Projects/virtualenv/.tox/4/py39/tmp/htmlcov '!py36:' --show-contexts --title virtualenv-py39-coverage
No source for code: '/home/jugmac00/Projects/virtualenv/!py36:'.
Aborting report output, consider using -i.
py39: exit 1 (0.10 seconds) /home/jugmac00/Projects/virtualenv> python -m coverage html -d /home/jugmac00/Projects/virtualenv/.tox/4/py39/tmp/htmlcov '!py36:' --show-contexts --title virtualenv-py39-coverage pid=15888
.pkg: _exit> python /home/jugmac00/Projects/tox/src/tox/util/pep517/backend.py True setuptools.build_meta
  py39: FAIL code 1 (161.17=setup[11.32]+cmd[0.10,144.33,3.27,0.97,1.06,0.10] seconds)
  evaluation failed :( (161.20 seconds)

(I just changed py34 to py36 as I do not have py34 installed)

[jugmac00]

This looks like a good place to apply action depending on the given factor(s), as here both the tox_env is available and thus the possible factors, and the command - also we already iterate over the command args:
https://github.com/tox-dev/tox/blob/5118401e6beb9f9b6dbe9bffa847dc99d678b1d7/src/tox/session/cmd/run/single.py#L82-L104

I'd prefer above spot over the command parsing/creation as we would have to pass in the env additionally.

Ok, then we have to search the cmd arg list for factor conditions - looks like the colon at the end is the marker for them.

When we find one, in case of a range, we need to split the factors, and then we need to decide if the following args need to be excluded or included.

Oh my.. I just read about factor groups 😳 Are factor groups a thing for commands?

We have already a factor.py with some helper methods, but I understood this one is used mainly to generate virtualenvs.

From the current tox3 documenation:

!py34-!py36: enum34 # use if neither py34 nor py36 are in the env name

Is this true? or should this be a range and include py35?

And last question... this affects commands... do we have to repeat the same dance for deps, too?

What's your take, @gaborbernat?

[gaborbernat]

This looks like a good place to apply action depending on the given factor(s), as here both the tox_env is available and thus the possible factors, and the command - also we already iterate over the command args:

Oh, no. Definitely not there. factor.py is not a helper method module, is the actual factor filtering module. All filtering needs to happen here https://github.com/tox-dev/tox/blob/rewrite/src/tox/config/loader/ini/__init__.py#L42-L46.

[gaborbernat]

Is this true? or should this be a range and include py35?

It's not range based, just simple factor match.

And last question... this affects commands... do we have to repeat the same dance for deps, too?

No, because the factor filtering is an orthogonal feature to the config value manifestation to Python world.

There's no need to develop anything new here, just need to understand why the filtering is not happening within the lines I've linked above (hence why is a bug ticket).

[jugmac00]

This looks like a good place to apply action depending on the given factor(s), as here both the tox_env is available and thus the possible factors, and the command - also we already iterate over the command args:

Oh, no. Definitely not there. factor.py is not a helper method module, is the actual factor filtering module. All filtering needs to happen here https://github.com/tox-dev/tox/blob/rewrite/src/tox/config/loader/ini/__init__.py#L42-L46.

This will be interesting, as at this stage we do not have a list of command args... not even a knowledge of a thing a like a command...

Ok, so it has to happen here...

https://github.com/tox-dev/tox/blob/5118401e6beb9f9b6dbe9bffa847dc99d678b1d7/src/tox/config/loader/ini/factor.py#L9-L23

And the return value needs to be the command updated to reflect the current env (name).

e.g. given the following tox snippet...

commands =
    python -c "print('hello')" !py34-!py35: -vvvv

for a py39 env we expect...

python -c "print('hello')" -vvvv

for a py35 env we expect...

python -c "print('hello')"

[gaborbernat]

This will be interesting, as at this stage we do not have a list of command args.

Why do you need command args? You only need the env name to see if matches.

[jugmac00]

I head a fresh look on this and the reason why this does not work yet is, that the regex only looks for factor conditions at the very beginning, not in the middle of a line.

@gaborbernat I found no answer in the current docs how many factor conditions we support per line...

Is it enough to support

 python -m coverage html -d /home/jugmac00/Projects/virtualenv/.tox/4/pypy3/tmp/htmlcov '!py36:' --show-contexts --title virtualenv-pypy3-coverage

or do we need to support (this contrived example)

 python -m coverage html -d /home/jugmac00/Projects/virtualenv/.tox/4/pypy3/tmp/htmlcov '!py36:' --show-contexts --title virtualenv-pypy3-coverage '!py38:' --skip-covered

?

[gaborbernat]

I don't follow, can you explain differently?

[jugmac00]

The current code only supports factor conditions at the beginning of a line:

match = re.match(r"^((?P<factor_expr>[\w{}.!,-]+):\s+)?(?P<content>.*?)$", line)

Is it sufficient to support one factor condition per e.g. command line in a tox ini like this?

match = re.match(r"^((?P<pre>.*?)(?P<factor_expr>[\w{}.!,-]+):\s+)?(?P<post>.*?)$", line)

Or do we have to support multiple factor conditions per line, something like

command = do_smth !py36: -x !py37: -y !py38: -z

[gaborbernat]

We match at the beginning of the line only, once; however the factor filtering should happen before the line collapsing, so in your case if the user wrote:

command = 
    do_smth \
    !py36: -x \
    !py37: -y \
    !py38: -z

then all should be good. I belive this should be the case here. So here the bug might be that line 33 https://github.com/tox-dev/tox/blob/5118401e6b/src/tox/config/loader/ini/__init__.py#L33 needs to move to line 46 🤔 and all should be as planned.