tox-dev / tox

Command line driven CI frontend and development task automation tool.
https://tox.wiki
MIT License
3.69k stars 523 forks source link

tox4: The `usedevelop` ineffective with `skipsdist` / editable package not present in virtual environment #2730

Open sdatko opened 1 year ago

sdatko commented 1 year ago

Issue

So far in most of our tox environments, for example for linters, we do not need the package, so we set skipsdist to True in the main configuration and where the package is needed for functional tests, the option usedevelop installs the editable package. However, it is no longer true for new release of tox.

Apparently with release 4.0 the tox changed behavior for usedevelop option, but I did not find it mentioned in FAQ. Is this intentional behavior now? Maybe it is a side effect of modifications mentioned under packaging changes?

Environment

Package       Version
------------- -------
cachetools    5.2.0
chardet       5.1.0
colorama      0.4.6
distlib       0.3.6
filelock      3.8.2
packaging     22.0
pip           22.2.2
platformdirs  2.6.0
pluggy        1.0.0
pyproject_api 1.2.1
setuptools    63.2.0
tomli         2.0.1
tox           4.0.11
virtualenv    20.17.1

Output of running tox

Provide the output of tox -rvv:

[sdatko@polluks tox]$ tox3 -r
python recreate: /home/sdatko/Development/tox/.tox/python
python develop-inst: /home/sdatko/Development/tox
python installed: # Editable install with no version control (myproject==0.0.1),-e /home/sdatko/Development/tox
python run-test-pre: PYTHONHASHSEED='4231996547'
python run-test: commands[0] | myproject
123
____________________ summary ____________________
  python: commands succeeded
  congratulations :)
[sdatko@polluks tox]$ tox4 -r
py310: remove tox env folder /home/sdatko/Development/tox/.tox/py310
py310: commands[0]> myproject
py310: exit 2 (0.00 seconds) /home/sdatko/Development/tox> myproject
  py310: FAIL code 2 (0.15=setup[0.15]+cmd[0.00] seconds)
  evaluation failed :( (0.25 seconds)

Minimal example

myproject/__init__.py file

#!/usr/bin/env python3

def main():
    print(123)

if __name__ == '__main__':
    main()

pyproject.toml

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

[project]
name = "myproject"
version = "0.0.1"

[project.scripts]
myproject = "myproject:main"

tox.ini file

[tox]
env_list = py310
skipsdist = True

[testenv]
usedevelop = True
commands = myproject
gaborbernat commented 1 year ago

Huh, surprised this works. If you set skipsdist that's a global disable of packaging features. You're expected to set package=skip or skip_install = true for your linter packages... that's what we do too https://github.com/tox-dev/tox/blob/main/tox.ini#L44, granted we should document this change, PR welcome. But I'm surprised tox 3 worked as you've described it.

sdatko commented 1 year ago

TL;DR

skipsdist = True
usedevelop = True

tox 3 -> installs the editable package tox 4 -> does not install anything but deps

skipsdist = False
usedevelop = True

tox 3 -> installs the editable package tox 4 -> installs the editable package

skipsdist = True
usedevelop = False

tox 3 -> does not install anything but deps tox 4 -> does not install anything but deps

skipsdist = False
usedevelop = False

tox 3 -> installs the regular package (after adding isolated_build = True to tox.ini) tox 4 -> installs the regular package

sdatko commented 1 year ago

Huh, surprised this works. If you set skipsdist that's a global disable of packaging features. You're expected to set package=skip or skip_install = true for your linter packages... that's what we do too https://github.com/tox-dev/tox/blob/main/tox.ini#L44, granted we should document this change, PR welcome.

Got it, thanks for confirmation. I may propose a PR over the weekend, if no one comes first to that.

But I'm surprised tox 3 worked as you've described it.

To be honest, I learned that years ago in some OpenStack projects, such as here https://opendev.org/openstack/neutron/src/branch/master/tox.ini – so I thought this is the expected behavior ;-)

fungi commented 1 year ago

To be honest, I learned that years ago in some OpenStack projects, such as here https://opendev.org/openstack/neutron/src/branch/master/tox.ini – so I thought this is the expected behavior

Yes, there are many hundreds of projects in OpenStack alone impacted by this, but it's just one of many adjustments they'll need to make before they can remove their global tox<4 pin.

sivel commented 1 year ago

fwiw, just to link it here, using skipsdist=true and usedevelop=true was a recommendation to speed up testing in the tox.wiki examples page previous to 4.0:

https://tox.wiki/en/3.1.3/example/general.html#avoiding-expensive-sdist

(arbitrary version selected based off of a google search)

masenf commented 1 year ago

Have you tried with

package = editable

https://tox.wiki/en/latest/upgrading.html#editable-mode

gaborbernat commented 1 year ago

fwiw, just to link it here, using skipsdist=true and usedevelop=true was a recommendation to speed up testing in the tox.wiki examples page previous to 4.0:

I don't think this is a good practice, hence removing it. Using editable installation come with significant downsides.

sivel commented 1 year ago

I'm not necessarily asking for it to be restored, I just wanted to link to something that would describe why people may have had this configuration in place to begin with. We've been stuck using tox<4 due to this, and I hadn't determined this to be the cause until today, largely because I hadn't taken the time to investigate in depth.

fungi commented 1 year ago

I'm not necessarily asking for it to be restored, I just wanted to link to something that would describe why people may have had this configuration in place to begin with. We've been stuck using tox<4 due to this, and I hadn't determined this to be the cause until today, largely because I hadn't taken the time to investigate in depth.

Yes, it was fairly widespread. Communities I participate in had close to a thousand projects which needed their tox configuration troubleshot and adjusted to work with v4, and most of the time it turned out to be due to combining skipsdist with usedevelop which did install the project under v3 but stopped in v4. (Many of those projects are still pinning tox<4 too, others took it as an opportunity to migrate to nox since the configs needed a rewrite either way.)

hashar commented 1 year ago

Both skipsdist and usedevelop got introduced by Monty Taylor. In the original implementation skipsdist value defaulted to the same value as usedevelop (which itself defaulted to false). Or to say otherwise when using the package in editable mode, sdist was not applied.

Looking at the legacy branch (tox 3), the logic still holds:

        all_develop = all(
            name in config.envconfigs and config.envconfigs[name].usedevelop
            for name in config.envlist
        )
        config.skipsdist = reader.getbool("skipsdist", all_develop)

In main (tox 4) the settings got renamed (but the old one are still supported):

tox v3 tox v4
usedevelop use_develop
skipsdist no_package

What I have witnessed is that if I set both:

[tox]
skipsdist = true

[testenv:py3]
usedevelop = true

Tox states it is not using usedevelop:

$ tox -e py3 --showconf|egrep '(develop|no_package)'
# !!! unused: usedevelop

And if I understand it properly that comes from PythonRun._register_package_conf() in src/tox/tox_env/python/runner.py:

        if not super()._register_package_conf():
            self.conf.add_constant(["package"], desc, "skip")
            return False
        if getattr(self.options, "install_pkg", None) is not None:
            self.conf.add_constant(["package"], desc, "external")
        else:
            self.conf.add_config(
                keys=["use_develop", "usedevelop"],
                desc="use develop mode",
                default=False,
                of_type=bool,
            )

The parent super()_register_package_conf() returns false in which case package=skip is set and usedevelop is not even registered. That is why in --showconf it shows as being unused.

The parent is RunToxEnv in src/tox/tox_env/runner.py which manages the skipsdist / no_package configuration:

        """If this returns True package_env and package_tox_env_type configurations must be defined."""
        self.core.add_config(
            keys=["no_package", "skipsdist"],
            of_type=bool,
            default=False,
            desc="is there any packaging involved in this project",
        ) 
        core_no_package: bool = self.core["no_package"]
        if core_no_package is True:
            return False

So that when skipsdist is set it returns False which causes the child PythonRun to not recognize use_develop at all. A similar bug is that with skipsdist set, one can not use tox run --installpkg.

I believe the fix should be done in the child, it returns early on with package=skip but should instead process:

And after that if the package type is not determinated yet, set it to skip? Thus the call to the parent RunToxEnv._register_package_conf() should happen after and moved below. So that if the package is neither external nor editable.

hashar commented 1 year ago

Then I guess instead of messing up with no_package / use_develop it is probably easier to use the new semantic, respectively: package=skip and package=editable.

I have the use case of having large testenv that should skip the slow installation, but I still want one testenv triggering the sdist to ensure it is working. With tox 3 I had:

[tox]
envlist = check-sdist,<a lot of envs>

[testenv]
skipsdist = True
usedevelop = True

[testenv:check-sdist]
# So we at least try sdist once
skipsdist = False
usedevelop = False
commands = python setup.py --version

With tox v4 and the new semantic it seems I can do:

[tox]
envlist = check-sdist,<a lot of envs>

[testenv]
package = editable  # same as use_develop and implies skipping sdist

[testenv:check]
package = sdist
commands = python setup.py --version

I will give it a try on a live project and verify. It looks like eventually the skipsdist and use_develop might be subject for removal in a future version of tox in favor of solely using package which is unambiguous.