Skip to content

Bump click minimum version to 8.3.1#1556

Open
stratakis wants to merge 1 commit intodbcli:mainfrom
stratakis:click
Open

Bump click minimum version to 8.3.1#1556
stratakis wants to merge 1 commit intodbcli:mainfrom
stratakis:click

Conversation

@stratakis
Copy link

Click 8.1.8 through 8.3.0 have broken pager invocation for multi-argument PAGER values, which causes behave test failures.

Fixed on pallets/click@7db1f20

  • I've added my name to the AUTHORS file (or it's already there).

pyproject.toml Outdated
dependencies = [
"pgspecial>=2.0.0",
"click >= 4.1,<8.1.8",
"click >= 8.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try this?

Suggested change
"click >= 8.3.1",
"click >= 4.1,<8.3.2",

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will "allow" some configs with broken click versions to be used (8.1.8 to 8.3.0). I could exclude all the problematic ranges, which would be ugly to put in pyproject toml but doable. I could also explore fixing the compatibility issue in pgcli.

Would something like click >= 4.1, != 8.1.8, != 8.2.*, != 8.3.0, < 9 work? Noting that the next click version would be 9, not 8.3.2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great! Please commit the following so we can try.

Suggested change
"click >= 8.3.1",
# Click 8.1.8 through 8.3.0 have broken pager invocation for multi-argument PAGER values, which causes behave test failures.
"click >= 4.1, != 8.1.8, != 8.2.*, != 8.3.0, < 9",

@meeuw
Copy link
Contributor

meeuw commented Feb 19, 2026

@stratakis thanks for your PR, I approved the CI tests.

For anyone reading this, rationale can be found here:

Required for click's rebase to 8.3.1.

https://src.fedoraproject.org/rpms/pgcli/pull-request/14

DiegoDAF added a commit to DiegoDAF/pgcli.daf that referenced this pull request Feb 19, 2026
click 8.1.8-8.2.x had broken pager invocation for multi-argument PAGER
values. click 8.3.1 includes the fix (pallets/click@7db1f20).
Removes the defensive <8.1.8 upper bound.

Ref: dbcli#1556

Made with ❤️ and 🤖 Claude
Copy link

@DiegoDAF DiegoDAF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed working. We've been running with click 8.3.1 on our fork with the full test suite passing (2759 tests).

The old <8.1.8 upper bound was a workaround for the broken pager in 8.1.8-8.2.x. This is the proper fix.

@j-bennet
Copy link
Contributor

Looks like this fails in the 3.10 build.

@stratakis
Copy link
Author

Looks like this fails in the 3.10 build.

This is due to click 8.3.1 requiring python >= 3.10, as uv checks that in the CI run. In essence pgcli should move to requiring python >=3.10.

@meeuw
Copy link
Contributor

meeuw commented Feb 20, 2026

Python 3.10 hasn't reached EOL:
https://devguide.python.org/versions/

Maybe remove support end of this year?

Click 8.1.8 through 8.3.0 have broken pager invocation for
multi-argument PAGER values, which causes behave test failures.

Fixed on pallets/click@7db1f20
@stratakis
Copy link
Author

Python 3.10 hasn't reached EOL: https://devguide.python.org/versions/

Maybe remove support end of this year?

The build that is failing is not the 3.10 one but uv checking the whole dependency matrix, including Python 3.9. So Python 3.10 should be fine. Let's see with the latest changes with the exclusion of the bad click versions it should hopefully not use the incompatible 3.9.

@stratakis
Copy link
Author

And as I'm not familiar with pgcli's codebase I fed the relevant data into claude to see what it can come out with. Gave it pgcli's codebase including my latest commit, the integrations tests issue, click's codebase and asked for approaches for making it compatible with lower click versions as well. Maybe it's helpful, maybe not:

LLM output

Problem: Click 8.1.8 through 8.3.0 have a bug in _pipepager() where multi-argument PAGER strings are
not handled correctly. In 8.1.8, which() is called on the entire PAGER string instead of just the
command name. In 8.2.0-8.3.0, shlex.split() was added but Popen is called with shell=True + a list
argument, so only the first element is executed. Fixed in 8.3.1 with shell=False.

The behave tests set PAGER to python3 /path/to/wrappager.py ---boundary---, which triggers this bug.
The pager silently falls through to less, boundary markers are never emitted, and all pager-dependent
tests fail.

Approaches to make pgcli work with all click versions:

  1. Make wrappager.py a standalone executable with no arguments. Hardcode the boundary string (it's
    always ---boundary---), rely on the existing shebang, chmod +x, and set PAGER to just the absolute path
    of the script. A single-path PAGER works with all click versions. Smallest change.
  2. Pass the boundary via environment variable instead of argv. Set PAGER_BOUNDARY in environment.py,
    read it with os.environ in wrappager.py, and set PAGER to just the script path. Same single-path
    benefit, but keeps the boundary configurable from the test harness.
  3. Use the exclusion-based version constraint. "click >= 4.1, != 8.1.8, != 8.2.*, != 8.3.0" allows
    older click versions (which work) and 8.3.1+ (which is fixed), while excluding the broken range. No
    code changes needed but doesn't protect users who have PAGER="less -R" or similar multi-argument
    values.
  4. Bypass click.echo_via_pager in pgcli. Replace the click.echo_via_pager(text, color) call in output()
    with pgcli's own pager invocation using subprocess.Popen(shlex.split(...), shell=False). Fixes the
    issue for real users too (not just tests), but is a larger change.
  5. Monkey-patch click.echo_via_pager in the test harness. Override it in environment.py's before_all()
    with a corrected implementation. Contained to tests, but ugly.

Approach 1 or 2 combined with approach 3 covers both the test suite and real-world usage with minimal
changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments