Skip to content

Fix using incorrect hidden property on ProgressBar instance #86

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

piiq
Copy link

@piiq piiq commented May 19, 2025

The ProgressBar does changed is_hidden to hidden a while back (pallets/click@fcd8503)

This results in tests/test_cliutils_progressbar.py failing and scancode not running unless a --verbose flag is specified when a vanilla pip install is used.
Related: aboutcode-org/scancode-toolkit#4369

This PR updates commoncode to use the latest API of click and updates the lowest supported version of click to be 8.2.0

@piiq
Copy link
Author

piiq commented Jun 2, 2025

Hey @AyanSinhaMahapatra I might need a hand here.

I've I tested a lot of approaches but I can't seem to get the tests to pass in Azure.

I cannot reproduce the error in any of the environments that I have (arm64 mac and amd64 linux container).
To pinpoint the bug I've been going line by line through the configure script doing and I'm not seeing what is happening in the pipelines.

If I understand correctly it fails to configure the environment with the dev dependencies. (basically the ./configure --clean && ./configure --dev is failing).
I have updated the requirements.txt and the requirements-dev.txt using the scripts in the etc/scripts. Nothing changes. The tests pass when I run them locally though.

I'm kinda lost. Can you help me out somehow?

@AyanSinhaMahapatra
Copy link
Member

AyanSinhaMahapatra commented Jun 10, 2025

TLDR click has dropped support for python3.9 even though the EOL for this version is in 6 months. See:

@piiq Thanks for looking into this intially.
I could reproduce the error locally, if you force the configure script to install using python3.9 instead of whatever is the default python3 executable.

The error is happening because https://github.com/pallets/click/blob/main/pyproject.toml#L16 where they are enforcing python3.10 and above, which is frankly problematic, since this python version is not EOL for another half a year and they are breaking core functionality simultaneously.

It took me quite some time to get to the cause of this issue since this only happens on python3.9 and the pip error message does not help at all in debugging this (opening an issue on pip for this: pypa/pip#13422):

INFO: pip is looking at multiple versions of commoncode to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install None because these package versions have conflicting dependencies.

The conflict is caused by:
    commoncode 0.0.0 depends on click>=8.2.0
    The user requested (constraint) click==8.2.1

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip to attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

Seems like we cannot possibly enforce click >= 8.2.0 and add this to scancode-toolkit because that would mean dropping support for installing scancode in python3.9 as this would bring the same dependency conflict there.

I'm suggesting the following instead for now as an alternative:

  • let's put an upper bound on click temporarily so that we only install click <= 8.1.8
  • let's keep this PR open, we can merge this after python3.9 is EOL in 6 months, and we can use click >= 8.2.0 exclusively
  • let the click maintainers know that this is hampering usage of click as a library

@piiq piiq mentioned this pull request Jun 10, 2025
piiq added 17 commits June 10, 2025 19:25
This reverts commit 15dcc61.

Signed-off-by: Theodore Aptekarev <[email protected]>
Signed-off-by: Theodore Aptekarev <[email protected]>
This ensures that the correct Click version is used in each test
and avoids dependency resolution conflicts caused by the editable
install in the configure script.

Signed-off-by: Theodore Aptekarev <[email protected]>
@piiq piiq force-pushed the bugfix/progressbar-hidden-propetry branch from 5c650de to 4b5d34b Compare June 10, 2025 16:26
Signed-off-by: Theodore Aptekarev <[email protected]>
@piiq
Copy link
Author

piiq commented Jun 10, 2025

Thanks a lot for the debugging @AyanSinhaMahapatra I should have tried doing this myself to be honest. If I would have tried launching it in 3.9 I would see the error fairly early on in my process. Well - I'll treat it as a learning opportunity.

I've updated this PR to remove python 3.9 from the test matrix and created an new PR that caps click to <8.2
#87

I can't convert this one to draft or add a "do not merge" label because I don't have permissions, but let me know if you'd like me to add anything here while this PR is on hold waiting for 3.9 EOL.

@AyanSinhaMahapatra
Copy link
Member

No worries, @piiq this was a head scratched indeed.

Thank you for your patience with this, much appreciated.

Click maintainers have actually responded at pallets/click#2975 (comment) pointing out that a better fix might be to extend the class and add a property which we can deprecate later, which probably better than dropping support for many older click versions, since people might put constraints/depend on those. Tbh, I missed this too, so lots of learning for me too there :P

Could you modify this PR to have something like:

class CompatProgressBar(click.ProgressBar):
    # TODO Remove when dropping support for Python 3.9 or Click 8.1.
    @property
    def is_hidden(self) -> bool:
        return self.hidden

    @is_hidden.setter
    def is_hidden(self, value: bool) -> None:
        self.hidden = value

as pointed out in the issue above and revert the tests/CI and click dependency constraints accordingly?

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.

2 participants