pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.24k stars 1.12k forks source link

pre-commit hangs with v2.8.2 #4505

Closed Goldziher closed 3 years ago

Goldziher commented 3 years ago

Hi there,

When I upgrade my pre-commit config to use v2.8.2 from v2.8.1 it hangs infinitely in trying to initialize the repository. My config looks like this:

- repo: https://github.com/pycqa/pylint
    rev: "v2.8.2"
    hooks:
      - id: pylint
        exclude: "[a-zA-Z]*/(migrations|tests)/(.)*"
        args:
          [
            "--load-plugins=pylint_django",
            "--django-settings-module=consumer_portal.settings",
            "--unsafe-load-any-extension=y",
          ]
        additional_dependencies: [
          # pylint dependencies
          pylint_django,
          # runtime dependencies
          ...
        ]

This works without issue with v2.8.1.

Pierre-Sassoulas commented 3 years ago

One of the thing that changed in 2.8.2 is that we started to use setuptool_scm. It might be a problem with shallow copy done by pre-commit from the github repo that prevent scm_setuptool to get the proper tag. I'm using pylint as a system hook myself because it can't run in isolation, but it look like you have an additional_dependencies argument that take care of installing the dependencies in pre-commit venv ?

While waiting for a fix (probably in scm setuptools), installing pylint with pip should work with this kind of conf:

-   repo: local
    hooks:
    -   id: pylint
        name: pylint
        entry: pylint
        language: system
        types: [python]
cdce8p commented 3 years ago

I opened https://github.com/pre-commit/pre-commit/issues/1924 since I couldn't find an option to require a full git clone with pre-commit.

Pierre-Sassoulas commented 3 years ago

Look like pre-commit won't have an option for non shallow clone because setuptools_scm is considered and I quote "a solution looking for a problem" and we have to choose between reverting the scm_setuptools changes, forking our own pre-commit with non shallow clone and poker (😄), or recommend using system hooks for pre-commit. One of the solution is clearly easier than the other :D

In your case a system hook would be: pip install pylint==2.8.2 pylint_django + the project dependencies (probably pip install -e .) and then the pre-commit configuration would be:

-   repo: local
    hooks:
    -   id: pylint
        name: pylint
        entry: pylint
        language: system
        types: [python]
        exclude: "[a-zA-Z]*/(migrations|tests)/(.)*"
        args:
          [
            "--load-plugins=pylint_django",
            "--django-settings-module=consumer_portal.settings",
            "--unsafe-load-any-extension=y",
          ]

Main advantages for you, is you won't have to upgrade the dependencies in the pre-commit configuration each time there is a change in your packaging, but of course you'll have to be in a venv where everything is installed or the hooks will fail.

I think I want to wait a little before changing anything in the packaging or taking a decision. What do you think @cdce8p ?

Edit : I need to try it but it might be possible to do a system hook without having to do the local installation by using;

          additional_dependencies: [
              # pylint dependencies
              pylint=="2.8.2",
              pylint_django,
              # runtime dependencies
              ...
        ]
cdce8p commented 3 years ago

I mostly agree, a system hook is probably the best solution here. As for setuptools_scm, I'm personally not that big of a fan but it makes the release process easier and that might be worth it. We could look for a different solutions down the road, like bump2version, but it might be best to stick with it for now.

https://github.com/c4urself/bump2version

Goldziher commented 3 years ago

Thanks... Yhea the pre-commit author has a memorable attitude about him. He also strongly disagrees with pyproject.toml and wont support it for flake8, for reasons. I used to run pylint with a system hook, but I changed it to a similar config as mypy. Additional dependencies are annoying, but my config makes it easy to use precommit as a linter in CI, without relying on a venv. I guess no alternative to reverting this.

Unless, perhaps its possible to create a pre-commit hook mirror repo for this?

Pierre-Sassoulas commented 3 years ago

I'm going to experiment a bit, an easy default configuration for pre-commit that can be updated with auto-update directly usable from the pylint repo url would be great.

ssbarnea commented 3 years ago

While I am not encountering the same bug reported in the description I found that using pylint as a pre-commit hook seems to be a permanent source of problems, which often are not possible to replicate by running pylint outside pre-commit.

When I say weird, I mean getting error from cyclic-import from particular file, not being able to use comments to disable it and not being able to get the same error when calling pylint manually, same version of pylint.

Can someone post some links to setups that seem to be less problematic to maintain?

Pierre-Sassoulas commented 3 years ago

The way to use pylint with pre-commit is by using it outside of pre-commit and install it properly in a venv yourself, (pylint needs the depenency to be importable) like in the comment above, or in pylint itself:

  - repo: local
    hooks:
      - id: pylint
        name: pylint
        entry: pylint
        language: system
        types: [python]
        args:
          [
            "-rn",
            "-sn",
            "--rcfile=pylintrc",
            "--load-plugins=pylint.extensions.docparams",
          ]
Goldziher commented 3 years ago

Issue is resolved with newest version, 😃. Thanks!