base_python with full path fails with env name conflict

[0cjs]

Issue

I'm not sure if I'm doing this right; I asked on StackOverflow a couple of days ago and got no reply.

This may be related to #2838, though that has been closed as fixed by #2840.

I have a set of test environments that test my code with different versions of Python, using environment names with py* factors (e.g., py3.8-pytest7). When I run my tests without an explicit base_python setting, they work fine, finding the correct versions of Python only because I've explicitly added all of them to my PATH environment variable.

I would prefer to explicitly specify paths to my various versions of Python, which are built with pythonz, rather than adding all of them to my PATH. For example:

[tox]
skip_missing_interpreters = false

[testenv]
base_python =
    py3.7: /home/cjs/.pythonz/pythons/CPython-3.7.17/bin/python3.7
    py3.8: /home/cjs/.pythonz/pythons/CPython-3.8.18/bin/python3
    py3.9: /home/cjs/.pythonz/pythons/CPython-3.9.18/bin/python

I have confirmed that the versions of Python above are what they look like:

$ /home/cjs/.pythonz/pythons/CPython-3.7.17/bin/python3.7 --version
Python 3.7.17
$ /home/cjs/.pythonz/pythons/CPython-3.8.18/bin/python3 --version
Python 3.8.18
$ /home/cjs/.pythonz/pythons/CPython-3.9.18/bin/python --version
Python 3.9.18

But when I try this, tox fails:

$ tox -vvv -r -e py3.7-pytest7,py3.8-pytest7,py3.9-pytest7
ROOT: 139 D setup logging to NOTSET on pid 606655 [tox/report.py:221]
py3.7-pytest7: 206 W remove tox env folder /home/cjs/co/public/gh/cynic-net/pytest_pt/.tox/py3.7-pytest7 [tox/tox_env/api.py:325]
py3.7-pytest7: 242 E failed with env name py3.7-pytest7 conflicting with base python /home/cjs/.pythonz/pythons/CPython-3.7.17/bin/python3.7 [tox/session/cmd/run/single.py:57]
py3.7-pytest7: FAIL ✖ in 0.04 seconds
py3.8-pytest7: 242 W remove tox env folder /home/cjs/co/public/gh/cynic-net/pytest_pt/.tox/py3.8-pytest7 [tox/tox_env/api.py:325]
py3.8-pytest7: 243 E failed with env name py3.8-pytest7 conflicting with base python /home/cjs/.pythonz/pythons/CPython-3.8.18/bin/python3 [tox/session/cmd/run/single.py:57]
py3.8-pytest7: FAIL ✖ in 0 seconds
py3.9-pytest7: 244 W remove tox env folder /home/cjs/co/public/gh/cynic-net/pytest_pt/.tox/py3.9-pytest7 [tox/tox_env/api.py:325]
py3.9-pytest7: 245 E failed with env name py3.9-pytest7 conflicting with base python /home/cjs/.pythonz/pythons/CPython-3.9.18/bin/python [tox/session/cmd/run/single.py:57]
  py3.7-pytest7: FAIL code 1 (0.04 seconds)
  py3.8-pytest7: FAIL code 1 (0.00 seconds)
  py3.9-pytest7: FAIL code 1 (0.00 seconds)
  evaluation failed :( (0.11 seconds)

Am I somehow specifying these wrong, or is this a bug?

Environment

• tox version 4.12.1
• Python 3.11.2 (for tox)

The test cases above were run from my pytest_pt project; that doesn't have any other configuration that could be triggering the problem, as far as I can tell. However, I can probably cons up a smaller test case if necessary.

[gaborbernat]

PR welcome 👍

[0cjs]

PR welcome 👍

You're asking someone who's doesn't know tox well enough even to be sure if this is a buhttps://github.com/tox-dev/tox/issues/3191g or if he's using it wrong to write a PR? What on earth are you thinking? Or are you just trying to dismiss this issue as, "not the problem of tox developers"?

From the commits, it looks as if you may know tox fairly well; are you not even going to be helpful enough to answer my question about whether this is a bug or a misconfiguration?

(And if you didn't intend to come across as incredibly unhelpful, condescending and dismissive, you might want also to reconsider the communications style you use when talking to people who don't know you.)

[gaborbernat]

Let me rephrase. This is a community maintained OSS project. There's no such thing really as tox developers. If you run into an issue and want to fix it, you'll need to put in a PR yourself.

What you're doing is definitely not the expected way of using the tool, because of:

I would prefer to explicitly specify paths to my various versions of Python,

However, the configuration you set should still work.

[0cjs]

So let me suggest a different approach when receiving issues like this.

First, "PR welcome" not only can come across as condescending, but it's not helpful to anyone. Those who are capable of creating a useful PR can already easily see that this project accepts PRs; those who do not understand how community-maintained OSS projects work are very unlikely to be able to provide a PR anyway. So you can just leave that out.

Second, if you don't have time or desire to try to fix the issue, that's fair enough: that's how community-maintained projects work. (And I don't see that there's really any need to post a message stating that.) But if you're interested in seeing fixes come in, and you have a little bit of time, throwing out a few hints in comments on the issue can be really helpful, such as what tests cover this, what files and functions might be a good place to start looking for the problem, and so on.

Again, only if you have time. But with such information, I might be encouraged to have a quick look at what's going on. I've already put a several hours into learning a bit about tox, tracking down the problems I've been having, and writing up detailed issues for them, so another half hour look into starting a PR would be worthwhile for me. But as it is, knowing nothing about where to start looking in the code base, I don't feel it's worth it to spend an unknown amount of time to dig through it just to find the general location where the problem might be.

[0cjs]

I just noticed I was assigned to this issue. Assigning someone without even discussing it with them is passive-aggressive, at best. That's what unpleasant bosses do.

As I mentioned above, I am willing to consider looking at this, if I can get some guidance from a developer who knows the code. But if everybody here is taking the, "Hey buddy, you're on your own" attitude, I feel no special urge to try to be super-co-operative.

[gaborbernat]

Sadly, I don't know from the top of my head where exactly the issue is, a general area to look around would be https://github.com/tox-dev/tox/blob/main/src/tox/tox_env/python/api.py#L165; however, it can also end up being a bug in virtualenv. Currently, I don't have time to guide you more in-depth (as this is a side project, and I'm very busy with other life obligations). However, if you put in a PR to address the problem, I'll make some time to review it.

[0cjs]

Thanks; that's a good starting point.

[0cjs]

Ok, I chased this down a bit and it's definitely related to virtualenv's PythonSpec class, particularly the from_string_spec() method, though I'm not yet entirely clear on whether that class needs to be changed in some way to make this work or whether it's just not being used correctly.

I tried out a little test to characterise its behaviour with both just an interpreter filename python3.11 (intended to be found via $PATH) and absolute and relative paths to an interpreter /my/private/build/python3.11, and looked at the (major, minor, micro, path) tuples that were produced.

Just python3.11 produces (3, 11, None, None); I am unusure about the reasoning about it not setting path to python3.11. (Setting path to the spec would obviously be wrong for a spec of 3.11, of course; which seems to indicate that a PythonSpec isn't dealing in paths, but then again, it does have that path attribute.)

A relative or absolute path produces (None, None, None, 'relative/python3.11') or (None, None, None, '/my/private/build/python3.11'). I don't know if it's unable to identify these pretty unambiguous version specs by design (because the spec includes slashes and is thus clearly a path) or if this is some sort of error.

So is PythonSpec supposed to be dealing only in version specifications, in which case maybe it shouldn't have a path attribute at all and the client should be handling that?

Documenting the PythonSpec class and methods to clearly state the intent and purpose of this class, along with the problems it's intended to solve, would be a big help here. "Contains specification about a Python Interpreter" doesn't really say anything more to me than the class name itself does.

[gaborbernat]

PythonSpec is intended to work for spec and paths at the same time 👍

[0cjs]

Well that's rather vague. Can you describe in more detail what the path attribute is and how it's supposed to work? It appears to be a public interface, but there's no documentation for it, and there's very little test coverage for the path attribute. Are the tests I linked above incorrect, or are they showing a bug in PythonSpec?

[gaborbernat]

If a string does not parse as a spec, it falls back to be validated as an explicit path. Likely a bug in PythonSpec.

[0cjs]

I take it a "spec" is something like "312" or "3.12". (This does not seem to be documented anywhere; I think it should be.)

I still don't understand what the path attribute is supposed to be, exactly. Is that what the user passed in if it was determined not to be a spec? Then why does the path attribute sometimes not get set to None when a string failed to parse as a spec?

And again, are the tests I linked above showing misunderstanding of how PythonSpec is supposed to behave, or are they showing bugs in PythonSpec?

I think it might help a lot if you'd update the docstrings in that module to clearly describe the intent of the class and its methods, and the problems you intend it to solve.

[gaborbernat]

We don't redefine the spec definition, but we reuse virtualenv https://virtualenv.pypa.io/en/latest/user_guide.html#python-discovery. So information from there applies.

If a string does not parse as a spec, it falls back to be validated as an explicit path. Likely a bug in PythonSpec.

I don't have time/plan to do this, but I'll accept a PR if someone takes the effort.

[0cjs]

Ah, so that's where the docs are! It would be a big help to have the PythonSpec docstring link to that. Also, those docs seem to be slightly incorrect in that they say, "the version is a dot separated version number" but according to one of the tests (test_py_spec_first_digit_only_major) it looks as if version numbers without dots are also accepted.

Other than that, the docs seem pretty clear on the specifier, but not so much on the "a relative/absolute path to a Python interpreter" part. Is PythonSpec supposed to be able to interpret the implementation, version and architecture from relative or absolute paths in the same way as it does from specs? That would seem reasonable, given the way that tox is using it, but that clearly doesn't work. Perhaps better anyway is that, if it's given a path, it should just query the (alleged) interpreter itself for the spec (e.g., by running it with something along the lines of -c 'import sys; print(sys.version_info)').

Actually coding up a fix for this doesn't seem that hard, and I'm happy to work on it, but I really need to understand (in detail) the intent of PythonSpec first. Which is not only documentation of that would be cool, but also an answer to my question about the tests I linked above....

[gaborbernat]

The documentation might not be completely up-to-date 😊 PRs to improve it is welcome.

Perhaps better anyway is that, if it's given a path, it should just query the (alleged) interpreter itself for the spec (e.g., by running it with something along the lines of -c 'import sys; print(sys.version_info)').

That's done at a later stage as a validation step by https://github.com/pypa/virtualenv/blob/main/src/virtualenv/discovery/py_info.py in virtualenv from the path.

PythonSpec is defining the interpreter you wish to have, PythonInfo is enquiring what the interpreter actually is,.

[0cjs]

I would love to do a PR to improve the documentation, not to mention one to fix any bugs, but if you won't explain to me how you intend PythonSpec to work, I can't do that. A very minimal start on this would be to answer my question, already asked three times, about the failing tests I created and linked above.

[gaborbernat]

I had the impression that I already said that the test you shown should pass (minus the "python3.11" that will not generate a path).

[gaborbernat]

On a further thought, perhaps the relative and absolute paths should not generate any python implementation or version specification info, just path. So if someone symlinks 3.11 to 3.10 we don't end up in a confusing situation.

[0cjs]

No, I didn't get "the tests should pass" from what you said, much less "/my/private/build/python3.11 and relative/python3.11 should pass, but python3.11 should not."

So I think I (partially, anyway) see where you're going with this: anything with a / in the parameter should be considered a path and just be used, whereas anything without a / should be considered a spec from which the client (or maybe the library) will build its own interpreter name following specific rules. Except, of course, because of the way things are currently working (or not working) with paths, developers to date pretty much have to use interpreter names that are found through $PATH, rather than an actual path to the interpreter. So if someone really needs their "py3.11" tox target to use myspecificpyt3.11 as the interpreter, their only option is to specify it as "myspecificpy3.11" and make sure it's found via $PATH.

The "symlinking 3.11 to 3.10" issue is a good point which I feel is getting to the core of the problem: the current implementation of PythonSpec seems to be fine with that and even encouraging that sort of thing, at least in the way that tox uses it, because it's very concerned that the name of the interpreter looks good, and not at all concerned about what the interpreter actually reports its version as. That is to say, an interpreter of the correct version with a bad name will be rejected, and an interpreter with a "good" name that's on the path and can be interpreted as a spec will happily be accepted even if it's not the correct version.

Perhaps what tox should be doing is:

1. Given a "spec or interpreter," it should first just try to run it, and not consider it a spec if it's something that can be run but just use it.
2. Optionally, but ideally, if it's runnable it should be given a small Python program that reports the version (along the lines of my import sys; print(sys.version_spec) note above) and any other necessary information that could confirm it should reject instead of use that interpreter.
3. Only if what it gets is not a command that runs an interpreter should it consider it a spec, extract version information from that, and then try to construct a command that should run a Python matching that spec.

[gaborbernat]

Probably option 3. Option 2 and 1 would introduce a significant performance degradation.

[0cjs]

Option 3 requires at least option 1; otherwise how do you tell if mypython is a command that runs an interpreter or not?

And while of course actually trying first to run whatever you're given is a hit on performance, I did think about that and my first instinct is that it's probably closer to "minimal" than "significant," given that the client code using this is in most cases about to run some stuff that's way heavier than the python interpreter startup cost. (That said, I'm open to ideas about how to optimise things.)

[gaborbernat]

On Windows is significant. Speaking from experience. That's how version 3 used to work.

You can do file system checks to validate.

[0cjs]

Actually, trying to pull back a bit and see what's going on here, I'm feeling like there's two different things that tox wants: one is to be able to find a Python interpreter given a spec, and the other is to be able to check that an interpreter's version matches a spec derived from the tox environment name.

I should note that I find the latter extremely valuable if it works properly; I really don't want a tox environment that has py3.11 in the name runing anything other than some Python that claims to be 3.11, to the point where I'm willing to forbid others from being able to override that. But I don't think it's working properly if (to use your example) a symlink can break it, which is why I feel that actually running the interpreter and getting it to report its version is worth the performance penalty (which I do still think is pretty minimal). Also, it may be worth pointing out that this is not necessarily something we need to push into PythonSpec; it might be better to make tox can deal with this itself.

But either way, perhaps it might make sense to tweak tox (and/or PythonSpec to some degree, if that seems appropriate) to use specs to try to generate interpreter names, but not demand that things that are or seem intended to be interpreter names also be parsable as a spec?

Regarding the Windows thing, you've got me there: I don't do development on Windows. But you seem to be indicating that, even if it's too expensive to launch the intepreter, one could fairly cheaply check to see if the string one's been given matches to an executable file? That sounds like it might be a reasonable compromise.

[0cjs]

Oh, as we think about this, I'd also appreciate it if you can make clear for proposed solutions (to both tox and virtualenv issues) what needs to be changed in tox, and what needs to be changed in virtualenv. I've no issue with doing PRs for both, but of course I need to understand what behaviour should belong to one or the other.

[paveldikov]

Hello! We're experiencing this problem in our organisation as well. We tend to use our Python interpreters as absolute paths, as opposed to system-wide installations. We have tried various ways of side-stepping this for Linux but then this breaks Windows, and vice-versa.

Option 3 sounds great to me -- if base_python looks like an absolute path, it should be accepted verbatim without it being converted into a PythonSpec. I don't think there should be any conflicts/impact to compatibility as PythonSpec.from_string_spec()'s implementation uses regex matching and the pattern does not allow for the usual path separator characters (/ and \). I.e. absolute paths and string specs are mutually-exclusive categories of string. Checking whether a string is an absolute path is also a pretty cheap operation (does not even require file system access)

Assuming that we agree on this approach (@gaborbernat are you happy with this?), I'm more than happy to submit a PR.

[gaborbernat]

Sure, I can accept and live with something like that. So yeah, please go ahead and submit that pull request. However note, you are required to also add a test validating the change together with the changelog for it to be accepted. Thank you for your contribution.

[paveldikov]

PR #3311 opened.

On a related note, it transpires that I will need to make a similar change to tox-uv as well. The PR works fine with the default virtualenv-backed runner, but the tox-uv runner seems to make the same assumption.

P.S. also opened tox-dev/tox-uv#69

[paveldikov]

btw, the merged PR only fixes it for tox-uv. The vanilla virtualenv-backed runner still suffers from this issue. I had raised another PR (which is still open), but for consistency's sake that might need re-worked to use the same cached_py_info.from_exe() approach. Let me know if you want me to do that.