[4.12.0][regression] The presence of TOX_PARALLEL_NO_SPINNER=1 in CI suppresses output of single env run tox commands

[webknjaz]

Issue

I have a number for globally set environment variables, one of each is TOX_PARALLEL_NO_SPINNER: https://github.com/ansible/pylibssh/blob/ef77a85/.github/workflows/ci-cd.yml#L75.

In one of the steps, I run tox -e a-single-env -qq | tee a-file.txt. In tox 4.11.4, it produces output from the command specified in tox.ini. Starting with tox 4.12.0, the output is empty.

I'm pretty convinced this is a side effect of #3159.

Previously, setting this var allowed having it in one place, while allowing for parallel and non-parallel runs different job steps, not having to track whether to disable or enable the spinner in each. So I consider this to be a regression.

Environment

• OS: ubuntu-latest @ GHA

tox == 4.12.0

Output of running tox

No output.

Minimal example

$ TOX_PARALLEL_NO_SPINNER=1 tox -e a-single-env --skip-pkg-install -qq

[gaborbernat]

I'm not sure if we can do much about it. What this issue wants is in direct conflict with #3158. Feature for one is a bug for the other one. We just need to pick a winner.

[webknjaz]

Well, it's been recommended to have this env var since tox 3. I'm sure it's present in many GHA workflows today and making workarounds will result in a lot of maintenance burden for the users.

[gaborbernat]

Well, it's been recommended to have this env var since tox 3.

I don't recall this, can you point me to this recommendation?

[webknjaz]

Here's one of the places I've found https://github.com/tox-dev/tox/pull/1311/files#diff-7b3e40da3ae9e1d41262bc01f1899ba172738f4164e053ed8a3f8330f57d5f53R14. But there must be more mentions in issues from 2019.

[webknjaz]

Also, here you specifically say that the CI use case needs an env var: #1220 (comment).

[webknjaz]

And the original feature request: #1184.

[gaborbernat]

Yes, but I did not say that you should also set it for non-parallel targets 😆 Is a regression, yes, but I do not think this was intended to be used in this form. cc @tusharsadhwani

[ssbarnea]

For quite some (months) times I observed tox failing without displaying any output from the called commands, only reporting the exit code, and that unrelated to any parallel options. I was about to file a new bug but I am not sure yet what is causing it.

See https://gist.githubusercontent.com/ssbarnea/3546bca96bdc38ad7faeaffc18291559/raw/37552e53f8966a59c9c81601b2a128b6f963317b/tox.txt -- basically command runs with exit code 2 when run by tox but manually run using same virtualenv works, no idea what is causing it, especially as when it fails it only returns exit code 2.

[tusharsadhwani]

@ssbarnea can you check if the behaviour is the same on 4.11 and 4.12?

[webknjaz]

@gaborbernat I suppose my point is that this is one of the vars that was convenient to set globally to avoid repetition. I didn't notice that the output changed (and was missing from the expected place) months after this was released. I'm pretty sure many people have it set similarly, because it's pretty common to borrow ideas from other projects and copy a lot of env vars, especially the ones related prettifying the output since they tend to be useful globally.

This means that it'll be annoying and unobvious for many to (1) notice something changed and (2) connect the dots with something they did 5 years ago. Obviously, a proper solution is dependency pinning but even with that, it's not necessarily visible since this change doesn't cause failures, just silently hides a part of the output. People don't tend to check their successful CI runs and unfold green steps.

I suppose, I can accept the point that it's useful for some to run in parallel mode. But then, why would tox even switch to parallel with a single parallel env requested? And if it does, shouldn't it at least issue a warning?

@ssbarnea have you checked which env vars are set in both cases? Externally to tox and internally, on the testenv level. That's probably the source of your differences. The version bump might be related to hiding the output, but unlikely to the underlying failure, I think.

@tusharsadhwani what's your opinion on all this, having read my explanations above?

[tusharsadhwani]

I understand @webknjaz

I agree with raising a warning if no-spinner is set while parallel is not.

[webknjaz]

@tusharsadhwani maybe also update the docs/changelog to mention that setting no-spinner now forces parallel mode with a single env, suppressing the output? This is the most unobvious breaking change, it seems.

[webknjaz]

@gaborbernat mark this as a docs issue?

[ssbarnea]

Regarding missing output, I think I sorted the issue. When the command executed is missing, there is nothing else than exit 2 displayed. The real issue is that exit code 2 can also be produced by the command itself when it exists. I do think that tox itself should be wise enough to display a "command not found" message, just to avoid confusing the user.

I look that tox code to find a way to extract that info when we print the outcome, but apparently the run object does not have enough information in it:

p run
ToxEnvRunResult(name='docs', skipped=False, code=2, outcomes=[Outcome: exit 2 in 0.01 seconds for xxx], duration=0.01247899979352951, ignore_outcome=False)

[sparkiegeek]

To retain the functionality of #3158 whilst not introducing this regression, it seems to me that tox would need to behave differently between

tox --parallel-no-spinner -e env1,env2

and

env TOX_PARALLEL_NO_SPINNER=1 tox -e env1,env2

i.e. break the "environment variables are equivalent to command line flags" idea.

The rationale for doing so is that (as per #3158), it can be argued that writing --parallel-no-spinner should imply executing in parallel, but it's harder to argue that in e.g. a CI/CD environment where you want to ensure that all parallel runs should not use a spinner, setting TOX_PARALLEL_NO_SPINNER=1 is sufficient signal from "the user" that tox should run environments in parallel.

[jugmac00]

I think the key issue here is that parallel-no-spinner/ TOX_PARALLEL_NO_SPINNER is a bit of an ambiguous name.

Should it force parallel mode or just deactivate the spinner? We probably could have named it just no-spinner and then it would have been very clear, or no-spinner-in-parallel-mode, but this ship has sailed.

There are quite a couple of different ways forward now, but it looks like all have their pros and cons.

@sparkiegeek I am afraid, I definitely do not want to cancel our promise that configuration works the same for command line options and environment variables. Thinking of myself, I would never recall which one works the one or the other way.

From my past experience I know that tox users usually set up a pipeline, and leave it for years, and are highly reluctant to any change - if that wasn't the case, I would deprecate the current naming and create new ones which are very clear.

I currently think that either reverting #3158 or adding a warning if we have contrary settings is the way forward.

We could also create a new, dedicated "TOX_CI_xxx" option which would be a replacement for the old behavior.

And we definitely need to document the changes properly.

[jakkdl]

I have also been confused about this for some time on my local development machine, where I found the spinner obnoxious and had set TOX_PARALLEL_NO_SPINNER=1 in the env file of my shell.

One compromise between #3158 and wanting to set it globally is specifically giving a warning if passing --parallel-no-spinner on the command line without --parallel. That is technically a difference in behavior between options and env vars, but only in terms of command-line parsing.

Although, is there any effect (other than output-wise) between running a single environment in parallel vs not? That's perhaps an even more disruptive change, but I could see specifying --parallel with only one tox env ignoring the flag, and/or raising a warning/error.

I could also see reverting #3158 and introducing a new convenience flag that is --parallel + --parallel-no-spinner. With docs being clearer, and the convenience flag being listed next to the old --parallel-no-spinner, I think anybody that was bothered by #3158 should be happy.