-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
posargs with :
crashes virtualenv
#2860
Comments
PR welcome. |
Happy to give it a try next week if that is okay. |
Nevermind, I'm actually unable to reproduce this in my environment. It's possible that the issues are related, but less hopeful that it "just works". |
4.3.0 is out so please try out and see if it helped or not. |
@gaborbernat It is still there, could you please re-open this ticket? |
I have updated the issues with my findings. It seems that if |
Okay, the "I hope I can fix this quickly" was a famous last sentence, this bug is actually quite complicated, at least for a newbie in the First, all (both active and inactive) tox environments are created (and later the active ones are executed): tox/src/tox/session/env_select.py Line 365 in a2222e9
In this call, if the tox/src/tox/session/env_select.py Lines 197 to 202 in a2222e9
This requires the tox/src/tox/tox_env/python/package.py Line 114 in a2222e9
That, after a few indirections, calls the tox/src/tox/tox_env/python/package.py Lines 82 to 91 in a2222e9
This goes to tox/src/tox/tox_env/python/virtual_env/api.py Lines 102 to 136 in a2222e9
The Solution: I have pushed a draft PR #2875 to show one approach that solves the problem: the For example, we could skip the package environment generation for the inactive environments here: tox/src/tox/session/env_select.py Line 197 in a2222e9
This has bigger effect though that I am not sure whether it is expected for inactive environments. Happy to take further steps based on your inputs. |
I'm not a maintainer, but I've been going through the codebase recently. I personally think the |
We cannot do this. We need to discover all packaging environments for active or inactive envs because otherwise, we don't know if a target is valid or is not. Consider:
In this case, if someone does |
I see, it makes sense, and clarifies a lot of things I was not certain about. In this case, I believe the PoC PR is not a good fix either because it does not create the expected packiging environment. (My assumption was that an inactive environment is not used, but your example shows it was a wrong one.) So, the main challenge is that we must create all environments, but if some environment-related option is extended with Is there any clear UX specification in this regard? If not, we might consider starting with that one. For example:
|
There's no such thing as posargs is intended for active env only. It's passed on for all envs by design, active or inactive. So I'm tempted to close this issue as working as intended and the problem is with your configuration. |
I do agree that passing posargs for all environments is okay if no environment is selected with - e. But it not really expected when a specific environment is selected. Posargs can be used in many different contexts, in commands, in options, and it is not expected and even not possible that one posargs is meaningful and working by all environments. When a specific, isolated environment is called, and another one that has no connection with called one fails is not the expected user experience, in my opinion. |
We'll need to agree to disagree on this. The configuration does not change depending on active or inactive state of an env. That would be confusing. If for some envs the posargs does not make sense do not call it. |
One thing we could do here is to move the configuration error raised by virtualenv to a runtime error. A PR in that direction would be accepted. |
Okay, it makes sense to improve the error messaging. Do you mean that if an environment is inactive but fails the initialization due to posargs replacement, we throw a more meaningful error message what has happened? Or, do you have another UX idea here? |
I was thinking more to ignore errors during setup, but during the execution of that tox environment reraise it. So:
but
will still fail. |
This is a great idea. I will put a draft PR together soonish. |
@gaborbernat I gave a try to fix this issue with small intervention. I am not sure it is easy to hide the |
I don't follow. |
The issue is that The idea is that I would call |
Really not an issue, SystemExit is very much catch-able.
No, we don't. We only let it go if it is an inactive environment. We'd save the exception and reraise it when called at evaluation time of the tox env. |
Okay, that would also work. What is the call that you would use as "evaluation time"? Accessing the However, the error message should be hid which is not, |
No need, can just use https://docs.python.org/3.5/library/contextlib.html#contextlib.redirect_stderr |
Thank you for the great hint. Regarding the evaluation call, I thought that I would just let the |
Something like that. |
Issue
posargs
with:
are not properly passed,virtualenv
crashes.Environment
Provide at least:
pip list
of the host Python wheretox
is installed:Output of running tox
Provide the output of
tox -rvv
:Minimal example
If possible, provide a minimal reproducer for the issue:
UPDATE After doing the investigation, it turned out that one environment influences the other. Here is a minimum
tox.ini
to reproduce the issue:tox.int
:tox -e hello -- x:y
The text was updated successfully, but these errors were encountered: