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

Tox/Poetry issue when running with pre-commit #2859

Closed
gaborbernat opened this issue Jan 13, 2023 Discussed in #2857 · 5 comments
Closed

Tox/Poetry issue when running with pre-commit #2859

gaborbernat opened this issue Jan 13, 2023 Discussed in #2857 · 5 comments
Assignees
Labels
area:commands-execution bug:minor does not affect many people or has no big impact bug:upstream something does not behave as it should, but can't or shouldn't be fixed in tox, but in a dependency help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@gaborbernat
Copy link
Member

Discussed in #2857

Originally posted by razy69 January 12, 2023

Issue

Hello everyone,

I encounter a strange issue while using pre-commit with tox/poetry.
I have to run pre-commit with PRE_COMMIT_COLOR=never to make it works.
The issue appears with tox 4.0.12 and still not resolved in 4.2.8.

Environment

Provide at least:

  • OS: darwin
  • pip list of the host Python where tox is installed:
$ pip list
Package              Version
-------------------- -----------
attrs                22.2.0
CacheControl         0.12.11
cachetools           5.2.1
certifi              2022.12.7
cffi                 1.15.1
cfgv                 3.3.1
chardet              5.1.0
charset-normalizer   2.1.1
cleo                 2.0.1
colorama             0.4.6
crashtest            0.4.1
distlib              0.3.6
dulwich              0.20.50
filelock             3.9.0
html5lib             1.1
identify             2.5.12
idna                 3.4
importlib-metadata   6.0.0
jaraco.classes       3.2.3
jsonschema           4.17.3
keyring              23.13.1
lockfile             0.12.2
more-itertools       9.0.0
msgpack              1.0.4
nodeenv              1.7.0
packaging            23.0
pexpect              4.8.0
pip                  22.3.1
pkginfo              1.9.6
platformdirs         2.6.2
pluggy               1.0.0
poetry               1.3.2
poetry-core          1.4.0
poetry-plugin-export 1.2.0
pre-commit           2.21.0
ptyprocess           0.7.0
pycparser            2.21
pyproject_api        1.4.0
pyrsistent           0.19.3
PyYAML               6.0
rapidfuzz            2.13.7
requests             2.28.1
requests-toolbelt    0.10.1
setuptools           58.1.0
shellingham          1.5.0.post1
six                  1.16.0
tomli                2.0.1
tomlkit              0.11.6
tox                  4.2.8
trove-classifiers    2022.12.22
urllib3              1.26.13
virtualenv           20.17.1
webencodings         0.5.1
xattr                0.10.1
zipp                 3.11.0

python >= 3.10
tox > 4.0.11
poetry > 1.3.0
pre-commit >= 2.20.0

tox.ini

[tox]
isolated_build = true
system_site_packages = false
no_package = true
env_list = {tests}

[testenv]
always_copy = true
download = true
base_python = python3.10
skip_install = true

[testenv:tests]
description = Run unit tests with pytest
allowlist_externals =
    poetry
commands_pre =
    poetry install -vvv --sync --with tests
commands =
    poetry run pytest -v {posargs}

.pre-commit-config.yaml

fail_fast: true
default_install_hook_types: [pre-commit]
repos:
- repo: local
  hooks:
  - id: tox-code-checks
    name: Run tox targets -- tests
    stages: [commit]
    language: system
    types: [python]
    pass_filenames: false
    verbose: true
    entry: tox -vvv -e tests

Minimal example

Running tox directly:

$ tox -e tests
tests: commands_pre[0]> poetry install -vvv --sync --with tests
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/config.toml
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/auth.toml
Adding repository XXX (https://XXX/simple)
Using virtualenv: /Users/jdacunha/Dev/project/.tox/tests
Installing dependencies from lock file

Finding the necessary packages for the current system

Package operations: 0 installs, 0 updates, 0 removals, 71 skipped

  • Installing anyio (3.6.2): Pending...
  • Installing anyio (3.6.2): Skipped for the following reason: Already installed
  • Installing dogpile-cache (1.1.8): Pending...
  • Installing dogpile-cache (1.1.8): Skipped for the following reason: Already installed
  • Installing cffi (1.15.1): Pending...

  ...

tests: commands[0]> poetry run pytest -v
================================================= test session starts =================================================
platform darwin -- Python 3.10.5, pytest-7.2.0, pluggy-1.0.0 -- /Users/jdacunha/Dev/project/.tox/tests/bin/python
cachedir: .tox/tests/.pytest_cache
rootdir: /Users/jdacunha/Dev/project, configfile: pyproject.toml
plugins: recording-0.12.1, cov-4.0.0, anyio-3.6.2
collected 381 items
  ...

================================================= slowest 5 durations =================================================
1.01s call     tests/unit/common/a.py::TestA::a
1.01s call     tests/unit/common/b.py::TestB::b
0.13s call     tests/unit/common/c.py::TestC::c
0.11s call     tests/unit/common/d.py::TestD::d
0.11s call     tests/unit/common/e.py::TestE::e
========================================== 381 passed, 19 warnings in 10.19s ==========================================
  tests: OK (13.13=setup[0.06]+cmd[1.41,11.65] seconds)
  congratulations :) (13.21 seconds)

Running pre-commit:

$ pre-commit run tox-code-checks
Run tox targets -- tests.......................Failed
- hook id: tox-code-checks
- duration: 1.71s
- exit code: 1

ROOT: 193 D setup logging to NOTSET on pid 96007 [tox/report.py:221]
tests: 258 I find interpreter for spec PythonSpec(major=3, minor=10) [virtualenv/discovery/builtin.py:56]
tests: 258 D discover exe for PythonInfo(spec=CPython3.10.5.final.0-64, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding_fs_io=utf-8-utf-8) in /Users/jdacunha/.pyenv/versions/3.10.5 [virtualenv/discovery/py_info.py:437]
tests: 259 D filesystem is not case-sensitive [virtualenv/info.py:24]
tests: 260 D got python info of /Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10 from /Users/jdacunha/Library/Application Support/virtualenv/py_info/1/ec7cb05918b9d40294b520615642683600e9927f06d12b706d7bab5356c36c9c.json [virtualenv/app_data/via_disk_folder.py:129]
tests: 260 I proposed PythonInfo(spec=CPython3.10.5.final.0-64, system=/Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
tests: 260 D accepted PythonInfo(spec=CPython3.10.5.final.0-64, system=/Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
tests: 286 W commands_pre[0]> poetry install -vvv --sync --with tests [tox/tox_env/api.py:427]
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/config.toml
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/auth.toml
Adding repository XXX (https://XXX/simple)
Using virtualenv: /Users/jdacunha/Dev/project/.tox/tests
Installing dependencies from lock file

Finding the necessary packages for the current system

Package operations: 0 installs, 0 updates, 0 removals, 71 skipped

tests: 1669 C exit 1 (1.38 seconds) /Users/jdacunha/Dev/project> poetry install -vvv --sync --with tests pid=96008 [tox/execute/api.py:275]
  tests: FAIL code 1 (1.42=setup[0.04]+cmd[1.38] seconds)
  evaluation failed :( (1.48 seconds)

Running pre-commit with PRE_COMMIT_COLOR=never

$ PRE_COMMIT_COLOR=never pre-commit run tox-code-checks
Run tox targets -- tests.......................Passed
- hook id: tox-code-checks
- duration: 13.32s

ROOT: 192 D setup logging to NOTSET on pid 97155 [tox/report.py:221]
tests: 257 I find interpreter for spec PythonSpec(major=3, minor=10) [virtualenv/discovery/builtin.py:56]
tests: 257 D discover exe for PythonInfo(spec=CPython3.10.5.final.0-64, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding
_fs_io=utf-8-utf-8) in /Users/jdacunha/.pyenv/versions/3.10.5 [virtualenv/discovery/py_info.py:437]
tests: 258 D filesystem is not case-sensitive [virtualenv/info.py:24]
tests: 259 D got python info of /Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10 from /Users/jdacunha/Library/Application Support/virtualenv/py_info/1/ec7cb05918b9d40294b520615642683600e9927f06d12b706d7bab5356c36c9c.json [virtualen
v/app_data/via_disk_folder.py:129]
tests: 259 I proposed PythonInfo(spec=CPython3.10.5.final.0-64, system=/Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:
36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:63]
tests: 260 D accepted PythonInfo(spec=CPython3.10.5.final.0-64, system=/Users/jdacunha/.pyenv/versions/3.10.5/bin/python3.10, exe=/Users/jdacunha/Dev/project/.venv/bin/python3, platform=darwin, version='3.10.5 (main, Jan 10 2023, 15:
36:10) [Clang 14.0.0 (clang-1400.0.29.102)]', encoding_fs_io=utf-8-utf-8) [virtualenv/discovery/builtin.py:65]
tests: 285 W commands_pre[0]> poetry install -vvv --sync --with tests [tox/tox_env/api.py:427]
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/config.toml
Loading configuration file /Users/jdacunha/Library/Preferences/pypoetry/auth.toml
Adding repository XXX (https://XXX/simple)
Using virtualenv: /Users/jdacunha/Dev/project/.tox/tests
Installing dependencies from lock file

Finding the necessary packages for the current system

Package operations: 0 installs, 0 updates, 0 removals, 71 skipped

  • Installing anyio (3.6.2): Skipped for the following reason: Already installed
  • Installing appdirs (1.4.4): Skipped for the following reason: Already installed
  ...

tests: 13277 I exit 0 (11.62 seconds) /Users/jdacunha/Dev/project> poetry run pytest -v pid=97161 [tox/execute/api.py:275]
  tests: OK (13.03=setup[0.05]+cmd[1.35,11.62] seconds)
  congratulations :) (13.09 seconds)

Does anyone succeed to make them work together ?
Thanks.

@gaborbernat gaborbernat added needs:reproducer ideally a failing test marked as xfail. If that is not possible exact instructions to reproduce needs:more-info labels Jan 13, 2023
@gaborbernat
Copy link
Member Author

@razy69 seems we need more info on why this broke, we'll accept a PR if you can put it in, but note we don't plan to work on this ourselves.

@ssbarnea
Copy link
Member

ssbarnea commented Jan 14, 2023

While I do not use poetry in any of my projects, I did spot that https://github.com/Textualize/rich project tox file is not working at all and the reasons are related to its use of poetry. I am not sure if that is their misconfiguration of tox or a tox issue, especially as they do not even use tox in their GHA pipelines, which is another questionable thing as drifting would be guaranteed to happen.

I posted this because, I am quite curious if there is any project using poetry + tox4 in a working status.

@masenf
Copy link
Collaborator

masenf commented Jan 18, 2023

There's perhaps a smoking gun in the log file

less .tox/tests/log/1-commands_pre\[0\].log

...

env COLUMNS: 0
env LINES: 0

If i run instead with COLUMNS= pre-commit run tox-code-checks, then there is no error (and the output has color!).

After peeling the layers back a bit, I find this trace

Traceback (most recent call last):
  File "/git/repro/flufl.lock/.venv/lib/python3.8/site-packages/poetry/installation/executor.py", line 246, in _execute_operation
    self._sections[id(operation)].write_line(
  File "/git/repro/flufl.lock/.venv/lib/python3.8/site-packages/cleo/io/outputs/output.py", line 88, in write_line
    self.write(messages, new_line=True, verbosity=verbosity, type=type)
  File "/git/repro/flufl.lock/.venv/lib/python3.8/site-packages/cleo/io/outputs/output.py", line 109, in write
    self._write(message, new_line=new_line)
  File "/git/repro/flufl.lock/.venv/lib/python3.8/site-packages/cleo/io/outputs/section_output.py", line 83, in _write
    self.add_content(message)
  File "/git/repro/flufl.lock/.venv/lib/python3.8/site-packages/cleo/io/outputs/section_output.py", line 69, in add_content
    len(self.remove_format(line_content).replace("\t", "        "))
ZeroDivisionError: division by zero

  <error>•</error> <fg=default>Removing <c1>cleo</c1> (<error>2.0.1</>)</>: <error>Failed</error>

There are at least 3 legit bugs scattered in various projects:

  1. when poetry hits an exception while formatting command output, it swallows it and doesn't tell the user the actual traceback Poetry 1.3 dies when run with a TTY reporting size 0/0 python-poetry/poetry#7184
  2. when cleo (cli library used by poetry) attempts to format a section, it performs an unchecked divide by shutil.get_terminal_size().columns (which can be Zero!) fix: avoid zerodivisionerrors in no-tty usecase python-poetry/cleo#298
  3. when pre-commit runs a command in a pty, it doesn't pass through the window size, so tox is unable to determine the size of the pty to create

As a workaround, tox could assume that if the terminal size is less than 1x1, you actually want 1x1, which in itself is sufficient to hide the previous 3 deficiencies exposed by your workflow.


Here's an actual minimal repro of this issue:

repro.py

#!/usr/bin/env python
import os
import shutil

from cleo.application import Application
from cleo.commands.command import Command
from cleo.helpers import argument, option


class Repro(Command):
    name = "repro"

    def handle(self):
        self._io.write_line(
            "shutil.get_terminal_size() = {}".format(shutil.get_terminal_size()),
        )
        self._io.write_line(
            "COLUMNS={}   LINES={}".format(
                os.environ.get("COLUMNS", ""),
                os.environ.get("LINES", ""),
            )
        )
        section = self._io.section()
        section.write_line(
            "<info>This causes division by zero on cleo >= 1.0.0 when executed via pre-commit</>",
        )


application = Application()
application.add(Repro())

if __name__ == "__main__":
    application.run()

tox.ini

[tox]
envlist = py,pre

[testenv]
deps =
    pre-commit
    cleo < 2
commands =
    python repro.py repro

[testenv:pre]
deps =
    cleo < 1

.pre-commit-config.yaml

repos:
- repo: local
  hooks:
  - id: repro1
    name: Run repro via tox
    stages: [commit]
    language: system
    types: [python]
    pass_filenames: false
    verbose: true
    entry: tox
  - id: repro2
    name: Run repro via cleo app
    stages: [commit]
    language: system
    types: [python]
    pass_filenames: false
    verbose: true
    entry: .tox/py/bin/python repro.py repro
  - id: repro3
    name: Run repro via cleo <1 app
    stages: [commit]
    language: system
    types: [python]
    pass_filenames: false
    verbose: true
    entry: .tox/pre/bin/python repro.py repro

Steps

1. tox to create the environments. Everything should pass.

py: install_deps> python -I -m pip install 'cleo<2' pre-commit
py: commands[0]> python repro.py repro
shutil.get_terminal_size() = os.terminal_size(columns=120, lines=45)
COLUMNS=120   LINES=45
This causes division by zero on cleo >= 1.0.0 when executed via pre-commit
py: OK ✔ in 5.34 seconds
pre: install_deps> python -I -m pip install 'cleo<1'
pre: commands[0]> python repro.py repro
shutil.get_terminal_size() = os.terminal_size(columns=120, lines=45)
COLUMNS=120   LINES=45
This causes division by zero on cleo >= 1.0.0 when executed via pre-commit
  py: OK (5.34=setup[5.24]+cmd[0.10] seconds)
  pre: OK (2.03=setup[1.92]+cmd[0.11] seconds)
  congratulations :) (7.45 seconds)

2. ./.tox/py/bin/pre-commit

The environments using cleo >= 1.0.0 should fail with "division by zero" when the bug is present.

Additionally running with cleo < 1.0.0 via tox will also fail with "division by zero" since tox is setting COLUMNS=0 to match the pty window size.

Run repro via tox........................................................Failed
- hook id: repro1
- duration: 1.54s
- exit code: 255

py: commands[0]> python repro.py repro
shutil.get_terminal_size() = os.terminal_size(columns=0, lines=0)
COLUMNS=0   LINES=0

division by zero
py: exit 1 (0.10 seconds) /git/repro/tox-2859> python repro.py repro pid=1236258
py: FAIL ✖ in 0.5 seconds
pre: commands[0]> python repro.py repro
shutil.get_terminal_size() = os.terminal_size(columns=0, lines=0)
COLUMNS=0   LINES=0

  ZeroDivisionError

  division by zero

  at .tox/pre/lib/python3.8/site-packages/clikit/api/io/section_output.py:62 in add_content
       58│     def add_content(self, content):  # type: (str) -> None
       59│         for line_content in content.split("\n"):
       60│             self._lines += (
       61│                 math.ceil(
    →  62│                     len(self.remove_format(line_content).replace("\t", "        "))
       63│                     / self._terminal.width
       64│                 )
       65│                 or 1
       66│             )
pre: exit 1 (0.15 seconds) /git/repro/tox-2859> python repro.py repro pid=1236267
  py: FAIL code 1 (0.50=setup[0.41]+cmd[0.10] seconds)
  pre: FAIL code 1 (0.16=setup[0.01]+cmd[0.15] seconds)
  evaluation failed :( (1.30 seconds)


Run repro via cleo app...................................................Failed
- hook id: repro2
- duration: 0.09s
- exit code: 1

shutil.get_terminal_size() = os.terminal_size(columns=0, lines=0)
COLUMNS=   LINES=

division by zero

Run repro via cleo <1 app................................................Passed
- hook id: repro3
- duration: 0.11s

shutil.get_terminal_size() = os.terminal_size(columns=0, lines=0)
COLUMNS=   LINES=
This causes division by zero on cleo >= 1.0.0 when executed via pre-commit

Fix

Jotting an idea down here before cleaning it up into a real PR

diff --git a/src/tox/execute/local_sub_process/__init__.py b/src/tox/execute/local_sub_process/__init__.py
index 478fb6b0..c8a7b0de 100644
--- a/src/tox/execute/local_sub_process/__init__.py
+++ b/src/tox/execute/local_sub_process/__init__.py
@@ -191,9 +191,9 @@ class LocalSubProcessExecuteInstance(ExecuteInstance):
     def __enter__(self) -> ExecuteStatus:
         # adjust sub-process terminal size
         columns, lines = shutil.get_terminal_size(fallback=(-1, -1))
-        if columns != -1:  # pragma: no branch
+        if columns > 0:  # pragma: no branch
             self.request.env.setdefault("COLUMNS", str(columns))
-        if lines != -1:  # pragma: no branch
+        if lines > 0:  # pragma: no branch
             self.request.env.setdefault("LINES", str(lines))

         stdout, stderr = self.get_stream_file_no("stdout"), self.get_stream_file_no("stderr")
@@ -306,10 +306,9 @@ def _pty(key: str) -> tuple[int, int] | None:
         return None  # pragma: no cover

     # adjust sub-process terminal size
-    columns, lines = shutil.get_terminal_size(fallback=(-1, -1))
-    if columns != -1 and lines != -1:
-        size = struct.pack("HHHH", columns, lines, 0, 0)
-        fcntl.ioctl(child, termios.TIOCSWINSZ, size)
+    columns, lines = shutil.get_terminal_size(fallback=(0, 0))
+    size = struct.pack("HHHH", columns or 1, lines or 1, 0, 0)
+    fcntl.ioctl(child, termios.TIOCSWINSZ, size)

     return main, child

With this change, the repro no longer hits the division by zero (except of course for the direct execution via pre-commit that doesn't involve tox at all)

@masenf masenf self-assigned this Jan 19, 2023
@masenf masenf added bug:upstream something does not behave as it should, but can't or shouldn't be fixed in tox, but in a dependency bug:minor does not affect many people or has no big impact area:commands-execution and removed needs:reproducer ideally a failing test marked as xfail. If that is not possible exact instructions to reproduce needs:more-info labels Jan 19, 2023
@gaborbernat gaborbernat added the help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. label Jun 16, 2023
@gaborbernat
Copy link
Member Author

@masenf did you made this PR in the end?

@masenf
Copy link
Collaborator

masenf commented Jun 20, 2023

I did not put in a PR, but I did notice last week that the underlying issue was fixed upstream. Definitely worth a retest.

python-poetry/cleo#299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:commands-execution bug:minor does not affect many people or has no big impact bug:upstream something does not behave as it should, but can't or shouldn't be fixed in tox, but in a dependency 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