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

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

Open
sdatko opened this issue Dec 15, 2022 · 11 comments
Labels
area:documentation help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@sdatko
Copy link

sdatko commented Dec 15, 2022

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

  • OS: Arch Linux (up to date)
  • pip list
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
@sdatko sdatko changed the title tox4: The usedevelop ineffective with skipsdist / no editable package not present in virtual environment tox4: The usedevelop ineffective with skipsdist / editable package not present in virtual environment Dec 15, 2022
@gaborbernat
Copy link
Member

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.

@gaborbernat gaborbernat added area:documentation help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Dec 15, 2022
@gaborbernat gaborbernat added this to the 4.0.x milestone Dec 15, 2022
@sdatko
Copy link
Author

sdatko commented Dec 15, 2022

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
Copy link
Author

sdatko commented Dec 15, 2022

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
Copy link

fungi commented Dec 15, 2022

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.

openstack-mirroring pushed a commit to openstack/python-novaclient that referenced this issue Dec 25, 2022
* removed skipsdist=True to make sure novaclient is available in the virtual
  env. The usedevelop and skipsdist does not work together any more
  tox-dev/tox#2730. For bindep we still don't
  need the current repo to be installed in the env so skipsdist added
  there.

Depends-On: https://review.opendev.org/c/zuul/zuul-jobs/+/866943
Change-Id: I979b91570c7b60273f35fbdf8464f6a9ee2007d6
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Dec 25, 2022
* Update python-novaclient from branch 'master'
  to c7cb02f1f7d508863c5e33f9d4b8e26b1a7ab6c0
  - Make tox.ini tox 4.0.0 compatible
    
    * removed skipsdist=True to make sure novaclient is available in the virtual
      env. The usedevelop and skipsdist does not work together any more
      tox-dev/tox#2730. For bindep we still don't
      need the current repo to be installed in the env so skipsdist added
      there.
    
    Depends-On: https://review.opendev.org/c/zuul/zuul-jobs/+/866943
    Change-Id: I979b91570c7b60273f35fbdf8464f6a9ee2007d6
openstack-mirroring pushed a commit to openstack/ovn-octavia-provider that referenced this issue Jan 2, 2023
Following the recommendations provided in [1], this patch disables the
"skipsdist" flag. Also format passenv values due to cannot contain
whitespace, and add allowlist_externals where necessary because looks
more strictly checked.

[1]tox-dev/tox#2730

Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Jan 2, 2023
* Update ovn-octavia-provider from branch 'master'
  to ef019ed63b7be9b2ff2eff5a1530d0870529da72
  - Fix jobs after tox4 upgrade
    
    Following the recommendations provided in [1], this patch disables the
    "skipsdist" flag. Also format passenv values due to cannot contain
    whitespace, and add allowlist_externals where necessary because looks
    more strictly checked.
    
    [1]tox-dev/tox#2730
    
    Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
openstack-mirroring pushed a commit to openstack/ovn-octavia-provider that referenced this issue Jan 2, 2023
Following the recommendations provided in [1], this patch disables the
"skipsdist" flag. Also format passenv values due to cannot contain
whitespace, and add allowlist_externals where necessary because looks
more strictly checked.

[1]tox-dev/tox#2730

Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
(cherry picked from commit ef019ed)
openstack-mirroring pushed a commit to openstack/ovn-octavia-provider that referenced this issue Jan 3, 2023
Following the recommendations provided in [1], this patch disables the
"skipsdist" flag. Also format passenv values due to cannot contain
whitespace, and add allowlist_externals where necessary because looks
more strictly checked.

[1]tox-dev/tox#2730

Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
(cherry picked from commit ef019ed)
openstack-mirroring pushed a commit to openstack/ovn-octavia-provider that referenced this issue Jan 3, 2023
Following the recommendations provided in [1], this patch disables the
"skipsdist" flag. Also format passenv values due to cannot contain
whitespace, and add allowlist_externals where necessary because looks
more strictly checked.

[1]tox-dev/tox#2730

Conflicts:
       tox.ini

Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
(cherry picked from commit ef019ed)
openstack-mirroring pushed a commit to openstack/ovn-octavia-provider that referenced this issue Jan 3, 2023
Following the recommendations provided in [1], this patch disables the
"skipsdist" flag. Also format passenv values due to cannot contain
whitespace, and add allowlist_externals where necessary because looks
more strictly checked.

[1]tox-dev/tox#2730

Change-Id: Iea8e355cd18c51a00bdbe5225239965cfc1704d7
(cherry picked from commit ef019ed)
fnordahl added a commit to openstack-charmers/release-tools that referenced this issue Jan 4, 2023
The use of this global setting may have some undesired side
effects, and I see no documentation as to why we have it in the
first place.

Let's drop it on the back of what some other OpenStack projects
are doing.

References:
https://review.opendev.org/c/x/charm-ovn-central/+/869165/comments/9e2cf4c4_acea15b6
tox-dev/tox#2730
fnordahl added a commit to openstack-charmers/release-tools that referenced this issue Jan 4, 2023
The use of the `skipsdist` global setting may have some undesired side
effects, let's drop it in favor of the upstream recommendations.

References:
https://tox.wiki/en/latest/config.html#skip_install
tox-dev/tox#2730
https://review.opendev.org/c/x/charm-ovn-central/+/869165/comments/9e2cf4c4_acea15b6
stephenfin added a commit to vicamo/patchwork that referenced this issue Jan 13, 2023
'[tox] skipsdist' behaves differently in tox 4 [1]. In addition, setting
'[testenv] basepython' with '[tox] ignore_basepython_conflict' has been
the cause of a few tox 4 bugs (most since fixed, thankfully) and might
be deprecated [2]. Remove it since we don't need it in any of our
environments.

[1] tox-dev/tox#2730
[2] tox-dev/tox#2846

Signed-off-by: Stephen Finucane <stephen@that.guru>
stephenfin added a commit to getpatchwork/patchwork that referenced this issue Jan 13, 2023
'[tox] skipsdist' behaves differently in tox 4 [1]. In addition, setting
'[testenv] basepython' with '[tox] ignore_basepython_conflict' has been
the cause of a few tox 4 bugs (most since fixed, thankfully) and might
be deprecated [2]. Remove it since we don't need it in any of our
environments.

[1] tox-dev/tox#2730
[2] tox-dev/tox#2846

Signed-off-by: Stephen Finucane <stephen@that.guru>
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 8, 2023
* Update neutron from branch 'master'
  to 61f1b31e6fd39512cd2a8362a8f39ae63578727a
  - Merge "Adapt deploy_rootwrap filters path for tox4"
  - Adapt deploy_rootwrap filters path for tox4
    
    This patch also fixes the issue of the deploy_rootwrap.sh script related
    to the rootwrap filters path. This path checks if the path is absolute
    (tox3) or relative (tox4) and makes it absolute in the second case.
    
    [1]tox-dev/tox#2730
    
    Depends-On: https://review.opendev.org/c/zuul/zuul-jobs/+/866943
    
    Closes-Bug: #1999558
    Change-Id: I7a4a30268cb352f25bad7983b94690c0b681e5fa
openstack-mirroring pushed a commit to openstack/neutron that referenced this issue Feb 8, 2023
This patch also fixes the issue of the deploy_rootwrap.sh script related
to the rootwrap filters path. This path checks if the path is absolute
(tox3) or relative (tox4) and makes it absolute in the second case.

[1]tox-dev/tox#2730

Depends-On: https://review.opendev.org/c/zuul/zuul-jobs/+/866943

Closes-Bug: #1999558
Change-Id: I7a4a30268cb352f25bad7983b94690c0b681e5fa
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 16, 2023
* Update python-magnumclient from branch 'master'
  to 4d74c31c9a8d0677996d32ee7f7057e23bba4683
  - Support tox4
    
    Renamed deprecated option whitelist_externals to allowlist_externals.
    
    Bumped tox minversion as allowlist_externals is only supported in
    3.18 [1].
    
    Also set skipsdist=False (default) due to [2].
    
    [1] https://tox.wiki/en/3.24.5/config.html#conf-allowlist_externals
    [2] tox-dev/tox#2730
    
    Change-Id: Iee2f00418804c21589bd8abc19a8b25aff458e77
openstack-mirroring pushed a commit to openstack/python-magnumclient that referenced this issue Feb 16, 2023
Renamed deprecated option whitelist_externals to allowlist_externals.

Bumped tox minversion as allowlist_externals is only supported in
3.18 [1].

Also set skipsdist=False (default) due to [2].

[1] https://tox.wiki/en/3.24.5/config.html#conf-allowlist_externals
[2] tox-dev/tox#2730

Change-Id: Iee2f00418804c21589bd8abc19a8b25aff458e77
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 20, 2023
* Update magnum-ui from branch 'master'
  to 452ca7da23f4489bc0dd25cfd031d4f1db04dfa6
  - Fix tox
    
    tox4 errors if basepython and python in other envs are different. [1]
    
    Bumped tox minversion as allowlist_externals is only supported in 3.18
    [2]
    
    [1] tox-dev/tox#2838
    [2] tox-dev/tox#2730
    
    Change-Id: I9d24395a7cc5d8423a58ec1e3ed8468ca6984e77
openstack-mirroring pushed a commit to openstack/magnum-ui that referenced this issue Feb 20, 2023
tox4 errors if basepython and python in other envs are different. [1]

Bumped tox minversion as allowlist_externals is only supported in 3.18
[2]

[1] tox-dev/tox#2838
[2] tox-dev/tox#2730

Change-Id: I9d24395a7cc5d8423a58ec1e3ed8468ca6984e77
kytta added a commit to jazzband/django-simple-menu that referenced this issue Mar 11, 2023
In tox v4, "skipdist" together with "usedevelop" do not work:
tox-dev/tox#2730

I do not understand why we use both options, since we do not lint the
code with tox (or similar), so we actually need to install the package
every time.

Closes #123
kytta added a commit to jazzband/django-simple-menu that referenced this issue Mar 11, 2023
@sivel
Copy link

sivel commented Apr 25, 2023

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
Copy link
Collaborator

masenf commented Apr 25, 2023

Have you tried with

package = editable

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

@gaborbernat
Copy link
Member

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
Copy link

sivel commented Apr 25, 2023

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
Copy link

fungi commented Apr 25, 2023

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.)

sebageek added a commit to sapcc/networking-ccloud that referenced this issue May 30, 2023
sebageek added a commit to sapcc/networking-ccloud that referenced this issue May 30, 2023
@gaborbernat gaborbernat removed this from the P-0 milestone Jun 17, 2023
stephenfin added a commit to getpatchwork/patchwork that referenced this issue Aug 1, 2023
'[tox] skipsdist' behaves differently in tox 4 [1]. In addition, setting
'[testenv] basepython' with '[tox] ignore_basepython_conflict' has been
the cause of a few tox 4 bugs (most since fixed, thankfully) and might
be deprecated [2]. Remove it since we don't need it in any of our
environments.

[1] tox-dev/tox#2730
[2] tox-dev/tox#2846

Conflicts:
	tox.ini

NOTE(stephenfin): Conflicts are due to lack of support for Django 4.1.

Signed-off-by: Stephen Finucane <stephen@that.guru>
(cherry picked from commit a03a0a5)
@hashar
Copy link
Contributor

hashar commented Sep 14, 2023

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:

  • install_pkg to set package = external
  • register the use_develop config in order to be able to set package = editable

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
Copy link
Contributor

hashar commented Sep 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation 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

6 participants