New plugin version for tox 4 #42

frenzymadness · 2021-11-06T00:16:03Z

Initial implementation of the plugin for the new tox version 4. It works for me but I have tested it only for one simple project yet.

Notes, questions:

Is this the right time to remove the deprecated CLI option?
The recreation of the environment no longer needs to be handled manually, tox does it every time the environment changes.
The output is different for --print-* options because there is no summary and congratulations.

ToDo:

Enable testing and update tests to work with the new version
Manually test this with project actively using the %tox macro in Fedora

Fixes #41

src/tox_current_env/hooks3.py

src/tox_current_env/hooks.py

src/tox_current_env/hooks4.py

gaborbernat · 2021-11-06T09:12:11Z

src/tox_current_env/hooks4.py

+    parser.add_argument(
+        "--print-deps-to",
+        "--print-deps-to-file",
+        action="store",
+        type=argparse.FileType("w"),
+        metavar="FILE",
+        default=False,
+        help="Don't run tests, only print the dependencies to the given file "
+        + "(use `-` for stdout)",
+    )
+    parser.add_argument(
+        "--print-extras-to",
+        "--print-extras-to-file",
+        action="store",
+        type=argparse.FileType("w"),
+        metavar="FILE",
+        default=False,
+        help="Don't run tests, only print the  names of the required extras to the given file "
+        + "(use `-` for stdout)",


This sounds to me like would live better as a subcommand rather than a flag. You can still support this for backwards compatibility via the legacy shims, so you'd need to register these flags for the legacy parser too https://github.com/tox-dev/tox/blob/rewrite/src/tox/session/cmd/legacy.py

Do I understand it correctly that when registered for legacy parsers, you have to call tox le --print-deps-to - instead of the old tox --print-deps-to -? That's a way we can use to support the old API but it not 100% identical.

If you don't specify a sub-command the legacy endpoint defaults, so in this sense it's backwards compatible...

src/tox_current_env/hooks4.py

gaborbernat · 2021-11-06T09:28:24Z

src/tox_current_env/hooks4.py

+        super().__init__(create_args)
+
+        # As soon as this environment has enough info to do its job,
+        # do it and nothing more.
+
+        if self.options.print_deps_to:
+            print(
+                *self.core["requires"],
+                *self.conf["deps"].lines(),
+                sep="\n",
+                file=self.options.print_deps_to,
+            )
+            self.options.print_deps_to.flush()
+
+        if self.options.print_extras_to:
+            if "extras" not in self.conf:
+                # Unfortunately, if there is skipsdist/no_package or skip_install
+                # in the config, this section is not parsed at all so we have to
+                # do it here manually to be able to read its content.
+                self.conf.add_config(
+                    keys=["extras"],
+                    of_type=Set[str],
+                    default=set(),
+                    desc="extras to install of the target package",
+                )
+            print(
+                *self.conf["extras"],
+                sep="\n",
+                file=self.options.print_extras_to,
+            )
+            self.options.print_extras_to.flush()
+
+        # We are done
+        sys.exit(0)


This is incorrect. This should be a new sub-command. We don't support calling sys.exit, and this will likely be a no-op when called with -p.

If you don't want to make it a subcommand a better alternative would be to set commands to empty array via a memory loader and then do this within https://tox.wiki/en/rewrite/plugins.html#tox.plugin.spec.tox_before_run_commands

I'll probably do both things. We want to keep the stable API so I clear the list of commands and do this in the tox_before_run_commands and I'll also prepare new subcommand. We can then deprecate the old CLI options and stay with subcommands only.

frenzymadness · 2021-11-06T14:39:43Z

Thanks for the valuable feedback! We have to discuss some parts in the team because we use this plugin in %tox RPM macro so we basically decide how the final tox command looks like. But some aspects might definitely be improved especially if we want to support this plugin also for broader usage outside our specific use-case.

hroncok · 2021-12-06T09:58:59Z

Thing to double-check: If /usr/bin/python is not installed (i.e. only /usr/bin/python3 or even only /usr/bin/python3.X is) and somebody has python ... commands in their tox configuration, does it work?

frenzymadness · 2021-12-14T22:40:23Z

After a few more hours of testing, I think I have the final version. I'm testing it with all packages from Fedora where -t or -e is used for %pyproject_buildrequires or %tox is in use and all related packages from this folder. The results are in: https://copr.fedorainfracloud.org/coprs/lbalhar/tox4/builds/

Failed builds are mostly caused by broken or insufficient dependencies. setuptools_scm fails because expects a different output from tox.

I had to:

package pyproject-api which is a new dependency of tox4
change pyproject_buildrequires.py to call tox with -q -r -e instead of -qre because the parsing of arguments seems to be a little bit different in the new version.

I'd like to implement the same logic using subcommands which will be backward incompatible but ready to use when we'll have tox4 everywhere. But the core of the logic here is ready for review. I'm not sure it makes sense to invest time in making tests ready for the new version given the fact that the outputs might change and we are still in alpha.

gaborbernat · 2021-12-15T10:35:46Z

Let's circle back in a few weeks, I have one big refactor in the works, before cutting a beta release.

hroncok · 2021-12-15T11:18:13Z

change pyproject_buildrequires.py to call tox with -q -r -e instead of -qre because the parsing of arguments seems to be a little bit different in the new version.

Maybe this needs to be reported as a tox regression?

hroncok · 2021-12-15T11:19:50Z

setuptools_scm fails because expects a different output from tox.

Oh, not the real setuptools_scm from Fedora, but the one with internal checks for our macros. We can adapt that later. All is 🌈 🦄

hroncok · 2021-12-15T11:22:06Z

src/tox_current_env/hooks4.py

+            f"{sys.version_info.major}.{sys.version_info.minor}",
+        ):
+            os.symlink(sys.executable, Path(self.tempdir.name) / f"python{suffix}")
+        os.environ["PATH"] = ":".join((os.environ["PATH"], str(self.tempdir.name)))


Do we need to change the actual os.environ? Is there no tox internal environ/path to change?

There is an internal one but to change it it'd IMO need to override some more classes in the hierarchy and create our own executor which would change the path before it creates its instance. Do you think it's worth it?

I've accomplished this without altering os.environ. See the last commit. The problem, however, is that the new PATH is not propagated to all new processes so tox runs the correct Python (via the symlink in the temp directory) but if I have another python subprocesses in my tests, those will not see the altered PATH. Therefore I'd keep the old behavior and restore the original PATH in the _teardown method. What do you think?

I think that propagating that path to all-new subprocesses is a must, but setting it vis os.environ won't propagate it either, will it? It is a matter of how is the inner subprocess invoked. Or what am I not getting? How does request.env work? Could you give me an example where the inner subprocess does not respect request.env but respects the os.enviorn value set from our code?

And if we indeed end up manipulating os.environ I think we should maximally limit the scope of that by e.g. using a context manager in <ExecutorClass>.call() implementation rather than setting it globally when we build the instance.

Manipulating the os.environ is definitely not right, for one would break parallel mode.

My bad, it was a problem in my testing environment. The current way propagates the PATH to the subprocesses correctly.

Manipulating the os.environ is definitely not right, for one would break parallel mode.

Our plugin causes tests to run only in one environment. @hroncok why should we care about parallel mode?

If you only intend to use it for Fedora sure. But perhaps other people might want to use this for different use cases to use the host Python interpreter where this might be required. It really depends on how you want to scope this project.

Even in Fedora, we might be able to run multiple tox environments in parallel.

src/tox_current_env/hooks4.py

frenzymadness · 2022-02-07T09:15:31Z

There are some changes I need to accommodate here but the fixes should be pretty easy so I'm gonna start with testing soon. https://github.com/tox-dev/tox/blob/rewrite/docs/changelog.rst#v400b1-2022-02-05

frenzymadness · 2022-02-07T13:35:50Z

So, make the plugin compatible with the latest release was quite easy but now we have a strange bug. With this very simple config:

[tox]
envlist = py36,py37,py38,py39,py310

[testenv]
commands =
    pytest
deps =
    pytest
    py36: dataclasses

If I use current env to actually run tests, it runs only specified environment:

$ tox4 --current-env -e py310
py310: recreate env because env type changed from {'name': 'py310', 'type': 'PrintEnv'} to {'name': 'py310', 'type': 'CurrentEnv'}
py310: remove tox env folder /home/lbalhar/Projekty/python-tox-example/.tox/4/py310
py310: commands[0]> pytest
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.10.2, pytest-6.2.4, py-1.11.0, pluggy-0.13.1
cachedir: .tox/4/py310/.pytest_cache
rootdir: /home/lbalhar/Projekty/python-tox-example
plugins: hypothesis-6.14.3
collected 10 items

test_fac.py ..........                                                                                                                                                                 [100%]

===================================================================================== 10 passed in 0.02s =====================================================================================
  py310: OK (0.32=setup[0.01]+cmd[0.32] seconds)
  congratulations :) (0.38 seconds)

But when I want it to print out the dependencies, it runs all the environments which produces this:

tox4 --print-deps-to - -e py310
tox>=4.0.0b2.dev1
pytest
tox>=4.0.0b2.dev1
pytest
dataclasses
tox>=4.0.0b2.dev1
pytest
tox>=4.0.0b2.dev1
pytest
tox>=4.0.0b2.dev1
pytest
py310: recreate env because env type changed from {'name': 'py310', 'type': 'CurrentEnv'} to {'name': 'py310', 'type': 'PrintEnv'}
py310: remove tox env folder /home/lbalhar/Projekty/python-tox-example/.tox/4/py310
  py310: OK (0.00 seconds)
  congratulations :) (0.06 seconds)

I did a test and it seems that all environments are initialized even only one is specified to run. That's problem for our implementation not just because of the mentioned bug but also because our CurrentEnv creates a temp directory in its __init__ which means five created folders even only one is needed (in this specific example).

@gaborbernat is this correct and expected behavior and we should adapt to it or is it a bug?

gaborbernat · 2022-02-07T13:47:16Z

This is expected, but something I'm not happy with, and actually would love some input on it. However, could not find a better way to solve it. The problem at hand is that if someone specifies the following command tox -e alpha, how do I know if this is a valid target. It is valid if it's a runtime environment, however, it is not if it's a provision or packaging environment, e.g. by the user setting the following configs:

[tox]
wheel_build_env = alpha
[testenv]
package = wheel
wheel_build_env = alpha

The only way to know that alpha is a valid target is to build all environments and then check that was not defined something other than run environment. Just reading the configuration file is not enough because the packaging environment is generated dynamically by reading the config flags; therefore to find out if a tox environment is a valid target we must build all tox environments upfront.

This is what we do currently, but this does have the overhead of constructing a lot of environments we will end up not using (can make tox invocations slow when you have a lot of environments defined). Also does mean that all tox plugins environment build creation should be a NOOP. Busy work should be done inside the run environment section.

The only other alternative I could think of was to make everything lazy but end up with a non stable behaviour, as in consider the following configuration:

[testenv:magic]
package = wheel
wheel_build_env = alpha

tox -e alpha,magic will fail that can't run a packaging environment, while tox -e alpha will run happily as a run environment. This would allow the same environment to become of different types depending on the users' invocation. We fixed this odd behaviour with the beta release, at the cost of creating all environments all the time. If anyone has a better idea I'm all ears; or if you have a preference between the above two choices.

hroncok · 2022-12-13T16:02:10Z

I've squashed some. And I was hit by the xdist pip cache problem, so I added a workaround.

I like where this stands now. Let me check the following things:

check if all the tox3 tests are covered in tox4 tests
- Examine the tox4 plugin tests and make sure all tox3 tests are covered #60
update the README wrt passenv or adapt the behavior in tox4 to match tox3
bump the release number

hroncok · 2022-12-13T16:10:20Z

I see several options to handle the TOX_TESTENV_PASSENV situation:

we document this as a behavioral difference between tox3 and tox4 plugin
we read the $TOX_TESTENV_PASSENV environment variable in the tox4 plugin and set the value accordingly, documenting this anyway
we change the tox3 plugin to set passenv to * unconditionally and update the documentation

In Fedora's %tox, we set TOX_TESTENV_PASSENV='*' so either of the options works for us. I wonder if a change of behavior might not break others (or even running the tests in %check section of this package in Fedora). So I would probably read the $TOX_TESTENV_PASSENV environment variable in the tox4 plugin.

WDYT?

hroncok · 2022-12-13T16:12:16Z

I see several of the tests missing.

$ diff -u <(grep -E '^def test_' tests/test_integration_tox3.py | cut -d'(' -f1 | sort) <(grep -E '^def test_' tests/test_integration_tox4.py | cut -d'(' -f1 | sort)
--- /dev/fd/63	2022-12-13 17:11:34.777337501 +0100
+++ /dev/fd/62	2022-12-13 17:11:34.777337501 +0100
@@ -5,21 +5,10 @@
 def test_allenvs_print_extras
 def test_allenvs_print_extras_to_existing_file
 def test_allenvs_print_extras_to_file
-def test_all_toxenv_current_env
-def test_all_toxenv_current_env_skip_missing
-def test_current_after_print_deps
-def test_current_after_print_extras
-def test_current_after_regular_is_not_supported
-def test_current_recreate_after_regular
-def test_externals
-def test_missing_toxenv_current_env
 def test_native_toxenv_current_env
-def test_noquiet_installed_packages
 def test_print_deps
 def test_print_deps_extras_to_same_file_is_not_possible
 def test_print_deps_extras_to_stdout_is_not_possible
-def test_print_deps_only_deprecated
-def test_print_deps_only_print_deps_to_file_are_mutually_exclusive
 def test_print_deps_to_file
 def test_print_deps_with_commands_pre_post
 def test_print_deps_without_python_command
@@ -29,11 +18,7 @@
 def test_print_extras
 def test_print_extras_to_file
 def test_print_extras_with_commands_pre_post
-def test_regular_after_current_is_supported
-def test_regular_after_first_print_deps_is_supported
-def test_regular_after_killed_current_is_not_supported
-def test_regular_recreate_after_current
-def test_regular_recreate_after_print_deps
+def test_recreate_environment
 def test_regular_run
 def test_regular_run_native_toxenv
 def test_self_is_installed_with_regular_tox

tests/utils.py

gaborbernat

I have a few questions 🤔

gaborbernat · 2022-12-14T15:13:27Z

src/tox_current_env/hooks4.py

+
+    # No options used - switch back to the standard runner
+    # Workaround for: https://github.com/tox-dev/tox/issues/2264
+    opt.default_runner = "virtualenv"


This has been fixed upstream so can be removed?

The issue was closed but I don't see any indication it was fixed. I understood the close more or less as "this is how it works". Did I get that wrong?

What about tox-dev/tox#2305 that's linked in the issue?

I must have missed that one. We need to check.

gaborbernat · 2022-12-14T15:13:51Z

src/tox_current_env/hooks4.py

+        env_conf.loaders.insert(0, empty_commands)
+
+
+class Installer:


You should inherit from tox abc installer rather than define your own...

gaborbernat · 2022-12-14T15:14:57Z

src/tox_current_env/hooks4.py

+        out,
+        err,
+    ):
+        request.env["PATH"] = ":".join(


This seems like a bad approach to me, why not set this via set_env instead? This works but is ugly and not used as intended 🤔 Also does not work on Windows?

FWIW We don't support Windows. If Windows user supply PRs we gladly review them but that's it.

gaborbernat · 2022-12-14T15:16:04Z

src/tox_current_env/hooks4.py

+                # Unfortunately, if there is skipsdist/no_package or skip_install
+                # in the config, this section is not parsed at all so we have to
+                # do it here manually to be able to read its content.
+                self.conf.add_config(


Why do you need this if skipsdist/no_package?

Because we need to get the information if specified. I guess defining extras together with skipsdist/no_package is invalid -- does it raise an error?

Because we need to get the information if specified. I g

Why?

I guess defining extras together with skipsdist/no_package is invalid -- does it raise an error?

It's not invalid, but there's no package involved in these cases, so there's no target to find extras for?

Becasu the purpose of the --print-extras-to option is to get the list of extras so we can gather the requirements for tests and install them via dnf in rpm build environment.

Becasu the purpose of the --print-extras-to option is to get the list of extras so we can gather the requirements for tests and install them via dnf in rpm build environment.

It sounds like you want to execute a tox deps -e py310 that would print the dependencies, taking into consideration the extras specified for the env.

Pep-621 projects can be determined without building a wheel, but otherwise you'd need to build wheel or use the metadata hook to determine extras, no?

That is exactly why we just want the list of extras from tox. We are building the wheel/metadata elsewhere differently and will use the list of extras extracted from tox. I.e. we don't want tox to build the wheel/metadata and provide the specific dependencies, we just need the list of extras specified in the config.

For that, you could use:

tox c -k extras

and parse it within an INI parser?

That way, you would not miss changes to the extra parse we're adding, e.g. tox-dev/tox#2668; and is the more supported way of reading tox configuration.

Yes, that seems possible, unless the tox configuration changes. Whether or not the extras are normalized does not matter to us, we normalize them anyway.

gaborbernat · 2022-12-14T15:17:02Z

src/tox_current_env/hooks4.py

+        return sys.platform
+
+
+class PrintEnv(CurrentEnv):


What is this trying to achieve?

The environment or the inheritance?

This entire class 🤔 not sure what's its goal is.

The goal of this class is to provide an implementation for options that print information about deps and extras rather thane execute tests.

Perhaps it would be better served by a deps sub-command 😊 as part of the core.

Perhaps we could deprecate this option then, but for now, we need to maintain compatible behavior between the tox plugin for tox 3 and 4. (In the above thread I continue to discuss how to use this option at all.)

frenzymadness mentioned this pull request Nov 6, 2021

tox4: plugin migration use cases tox-dev/tox#1974

Closed

luzfcb reviewed Nov 6, 2021

View reviewed changes

src/tox_current_env/hooks3.py Show resolved Hide resolved