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.

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

setenv PATH behavior change with tox 4 #2691

Open
dairiki opened this issue Dec 12, 2022 · 9 comments
Open

setenv PATH behavior change with tox 4 #2691

dairiki opened this issue Dec 12, 2022 · 9 comments
Labels
area:documentation enhancement help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@dairiki
Copy link

dairiki commented Dec 12, 2022

Issue

We have some python code that can use certain external binaries to provide extra functionality but is supposed to degrade gracefully if said binaries are not installed. To test that graceful degradation, we have been using

[testenv]
setenv =
    noutils: PATH=/dev/null
...

with the intention that running tox -e py311-noutils would run our tests with PATH set to a broken value so that our code under test could not find the external binaries.

This worked up until tox 4. With tox 4 tox -e py311-noutils reports

py311-noutils: skipped because could not find python interpreter with spec(s): py311

Apparently, the broken PATH setting in setenv now breaks tox's ability to find available python interpreters.

A minimal example is provided below.

Is this expected? (If so, is there another suggested approach for my use case?)

Minimal example

With this tox.ini

[tox]
isolated_build = true

[testenv]
setenv =
    noutils: PATH=/dev/null
commands =
    python -c "from shutil import which; exit(0 if which('ls') is None else 1)"

and this minimal pyproject.toml

[build-system]
requires = ["setuptools>=61"]
build-backend = "setuptools.build_meta"

[project]
name = "test-junk"
version = "0.1"

Using tox 4.0.5

$ type python3.11
$ tox -e py311-noutils
py311-noutils: skipped because could not find python interpreter with spec(s): py311
  py311-noutils: SKIP (0.01 seconds)
  evaluation failed :( (0.05 seconds)

Using tox 3.27.1

$ tox -e py311-noutils
.package create: /path/to/.tox/.package
.package installdeps: setuptools>=61
warning: sdist: standard file not found: should have one of README, README.rst, README.txt, README.md

py311-noutils create: /path/to/.tox/py311-noutils
py311-noutils inst: /path/to/.tox/.tmp/package/1/test-junk-0.1.tar.gz
py311-noutils installed: test-junk @ file:///path/to/.tox/.tmp/package/1/test-junk-0.1.tar.gz
py311-noutils run-test-pre: PYTHONHASHSEED='4193678857'
py311-noutils run-test: commands[0] | python -c 'from shutil import which; exit(0 if which('"'"'ls'"'"') is None else 1)'
___________________________________ summary ____________________________________
  py311-noutils: commands succeeded
  congratulations :)

Note that tox -e py311 works as expected (the test is run, but the test fails since ls is in PATH) under either version of tox.

@gaborbernat
Copy link
Member

PR to fix this is welcome.

@gaborbernat gaborbernat added bug:normal affects many people or has quite an impact help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Dec 12, 2022
@gaborbernat gaborbernat added this to the 4.0.x milestone Dec 12, 2022
dairiki added a commit to dairiki/tox that referenced this issue Dec 13, 2022
We want to resolve the python executable using PATH from the
environment rather than using any PATH which may be configured
set_env.  (See tox-dev#2691.)
@gaborbernat
Copy link
Member

gaborbernat commented Dec 13, 2022

On second thought, I'm not sure this is a valid use case. Because we use PATH to discover valid commands in commands, if you set the PATH to null, no command is valid. We also need to support the use case when people alter PATH via set_env and want the new paths to be included in command discovery. So we must use the PATH to discover new commands. There's no way we can satisfy both cases 🤔 so we must choose one or the other 🤔 It's almost what you're expecting here is some commands_set_env that overrides the set_env for the commands section...

@gaborbernat gaborbernat added enhancement area:documentation and removed bug:normal affects many people or has quite an impact labels Dec 13, 2022
@gaborbernat
Copy link
Member

We need to document this as a breaking change and provide the commands_override_env option as a better solution. PRs for both of these are welcome 👍

@dairiki
Copy link
Author

dairiki commented Dec 13, 2022

Because we use PATH to discover valid commands in commands, if you set the PATH to null, no command is valid.

That's not quite right.
From the docs on commands:

The virtual environment binary path (see env_bin_dir) is prepended to the PATH environment variable, meaning commands will first try to resolve to an executable from within the virtual environment, and only after that outside of it. Therefore python translates as the virtual environments python (having the same runtime version as the base_python), and pip translates as the virtual environments pip.

So, python, as well as any other commands installed in the virtual environment will always be found.

The issue is what PATH to use when constructing the virtualenv, where the PATH is used (solely, I think) to find candidate python executables for the environment. Personally, I don't see any reason that PATH has to be the same as the PATH used when running the commands (which only happens, clearly, after the virtual environment has been constructed.)


Adding command_override_env would work (but, as you note, would be a breaking change — there would be no way to construct a tox.ini that sets a null PATH and would work with tox 3 and tox 4.)
I'm not sure there are other uses for commands_override_env. Are there any other environment variables that one might want to set differently during commands execution than during virtualenv creation?

If commands_override_env would not be more generally useful than just for setting PATH, then to me it seems a better solution to special-case the set_env handling of PATH (to match tox 3 behavior) rather than adding a whole new config setting.

It's already the case that a different set of environment variables are used when constructing the virtual environment (VirtualEnv.virtualenv_env_vars) than when running commands (VirtualEnv.environment_variables). I don't really see why setting PATH differently in those two cases is an issue.

As noted above, I believe the only use of PATH during virtual environment creation is for locating the appropriate python executable. If that's the case, we lose nothing by not allowing a way to customize that PATH. If the user would like to use a python other than what would be found via os.environ["PATH"], they can explicitly set base_python.

@gaborbernat
Copy link
Member

The virtual environment binary path (see env_bin_dir) is prepended to the PATH environment variable, meaning commands will first try to resolve to an executable from within the virtual environment, and only after that outside of it. Therefore python translates as the virtual environments python (having the same runtime version as the base_python), and pip translates as the virtual environments pip.

So, python, as well as any other commands installed in the virtual environment will always be found.

If you take that ad litera it will imply that even if you set PATH to /dev/null, we'd still have to prepend the virtualenv PATH and pass that path on 🤔 The path handling is not separate for virtualenv creation, installation or command execution, they all use the same. And that's by design.

I don't see any reason that PATH has to be the same as the PATH used when running the commands (which only happens, clearly, after the virtual environment has been constructed.)

This kind of different behavior is confusing users because the boundaries where something applies or not are very confusing. For that reason, tox 4 no longer wants differing behaviors.

Adding command_override_env would work (but, as you note, would be a breaking change — there would be no way to construct a tox.ini that sets a null PATH and would work with tox 3 and tox 4.)

By design, as said above. This use case is rare enough that I'm happy to make that breaking change. tox 3 and 4 is intended to be mostly compatible, not under all circumstance. This is one of those.

If commands_override_env would not be more generally useful than just for setting PATH, then to me it seems a better solution to special-case the set_env handling of PATH (to match tox 3 behavior) rather than adding a whole new config setting.

Strongly opposed to any non-generic behavior. Such exceptions always just confuse the user and the maintainer alike. Might be handy for your specific use case, but in long term causes more issues than worth it.

It's already the case that a different set of environment variables are used when constructing the virtual environment (VirtualEnv.virtualenv_env_vars) than when running commands (VirtualEnv.environment_variables). I don't really see why setting PATH differently in those two cases is an issue.

The only differing environment variables are the ones that configure how virtualenv works. That's expected.

@dairiki
Copy link
Author

dairiki commented Dec 13, 2022

The only differing environment variables are the ones that configure how virtualenv works. That's expected.

I see PATH as one of those environment variables that configure how virtualenv works, since it determines how virtualenv chooses a python executable.

Agree to disagree, I guess...

@gaborbernat
Copy link
Member

gaborbernat commented Dec 13, 2022

I see PATH as one of those environment variables that configure how virtualenv works, since it determines how virtualenv chooses a python executable.

And it would be OK to append the PATH there, but not to overwrite what the user already asked in set_env. set_env must be respected. More importantly, the only env vars we set are to enforce our configuration. We only set env-vars for which we have configs there, and we have none for PATH.

dairiki added a commit to dairiki/lektor that referenced this issue Feb 24, 2023
Tox version 4 broke our scheme for hiding external
utilities (imagemagick, git, ffmpeg, etc) from Lektor.
(See tox-dev/tox#2691)
dairiki added a commit to dairiki/lektor that referenced this issue Feb 24, 2023
Tox version 4 broke our scheme for hiding external
utilities (imagemagick, git, ffmpeg, etc) from Lektor.
(See tox-dev/tox#2691)
dairiki added a commit to dairiki/lektor that referenced this issue Feb 25, 2023
Tox version 4 broke our scheme for hiding external
utilities (imagemagick, git, ffmpeg, etc) from Lektor.
(See tox-dev/tox#2691)
@cladmi
Copy link

cladmi commented May 15, 2023

I also relied on modifying PATH with tox 3 where an environment needed to have both python3 and python==python2 (used by an external tool). So executing in the python2 env, prepended with python3 env path.

PATH = {toxworkdir}/py3/bin:{env:PATH}

So it means my script currently has a maxversion requirement too.

And in my case, I need to use requires = virtualenv<20.22.0 so even with a correct tox version locally, the provisioned tox environment seem to be using version 4 (did not test much on this).

I can rework them to explicitly set the PATH before executing the command, but it is indeed a behavior change to not be able to modify the value.

@cladmi
Copy link

cladmi commented May 15, 2023

I also relied on modifying PATH with tox 3 where an environment needed to have both python3 and python==python2 (used by an external tool). So executing in the python2 env, prepended with python3 env path.

PATH = {toxworkdir}/py3/bin:{env:PATH}

So it means my script currently has a maxversion requirement too.

And in my case, I need to use requires = virtualenv<20.22.0 so even with a correct tox version locally, the provisioned tox environment seem to be using version 4 (did not test much on this).

I can rework them to explicitly set the PATH before executing the command, but it is indeed a behavior change to not be able to modify the value.

That's expected because the later runs at runtime, while the earlier is evaluated at startup, before any tox environments are created. That's by design and we'll not change it.

@gaborbernat gaborbernat removed this from the P-0 milestone Jun 17, 2023
dairiki added a commit to dairiki/lektor that referenced this issue Sep 11, 2023
Tox version 4 broke our scheme for hiding external
utilities (imagemagick, git, ffmpeg, etc) from Lektor.
(See tox-dev/tox#2691)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation enhancement help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Development

No branches or pull requests

3 participants