pre-commit-ci / issues

public issues for https://pre-commit.ci
16 stars 3 forks source link

setuptools may be old in cached hook environment #111

Closed davidism closed 2 years ago

davidism commented 2 years ago

It seems like on pre-commit.ci, the cached reorder_python_imports@2.7.1 environment has an old setuptools. When I run pre-commit locally, it reorders MarkupSafe's setup.py to:

from distutils.errors import CCompilerError
from distutils.errors import DistutilsExecError
from distutils.errors import DistutilsPlatformError
from setuptools import Extension
from setuptools import setup
from setuptools.command.build_ext import build_ext

But when I push that to a PR, pre-commit.ci reorders it to

from distutils.errors import CCompilerError
from distutils.errors import DistutilsExecError
from distutils.errors import DistutilsPlatformError

from setuptools import Extension
from setuptools import setup
from setuptools.command.build_ext import build_ext

Notice the added empty line. You can see the commit here: https://github.com/pallets/markupsafe/pull/279

I previously identified this in https://github.com/asottile/reorder_python_imports/issues/210#issuecomment-1012523488 when I was upgrading another project, but in that one it was my local setuptools that was outdated.

asottile commented 2 years ago

the versions are listed here: https://github.com/pre-commit-ci/runner-image/blob/runner-image_2022-01-19-ec7da74/versions.md and sourced from here: https://github.com/pre-commit-ci/runner-image/blob/79a11f61bf2fdce6973fd96ba645ebb70c4b1773/requirements.txt#L12

setuptools was broken for reorder-python-imports so it was held back

asottile commented 2 years ago

(note for the jump here, pre-commit.ci produces reproducible artifacts by freezing and by preventing virtualenv from upgrading by forcing embedded things -- if you want to force a particular version of setuptools you can do so via additional_dependencies -- or you can wait for a future image upgrade)

asottile commented 2 years ago

if you want to force a particular version of setuptools you can do so via additional_dependencies

davidism commented 2 years ago

What you've said makes sense, but I'm not able to get it to work. It sounds like pre-commit, both locally and in CI, should already be forcing a specific setuptools version, so I'm not sure why I'm seeing different behavior between them. I tried setting additional_dependencies and it didn't fix the issue.

I added additional_dependencies: ["setuptools>60.9"] to the reorder_python_imports hook, and I still get the different behavior. Adding setuptools==60.2.0, the version from the CI image, caused the following exception:

Traceback (most recent call last):
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/bin/reorder-python-imports", line 8, in <module>
    sys.exit(main())
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/reorder_python_imports.py", line 912, in main
    retv |= _fix_file(filename, args)
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/reorder_python_imports.py", line 473, in _fix_file
    new_contents = fix_file_contents(
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/reorder_python_imports.py", line 454, in fix_file_contents
    partitioned = step(partitioned)
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/reorder_python_imports.py", line 364, in apply_import_sorting
    sorted_blocks = sort(import_obj_to_partition.keys(), **sort_kwargs)
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/aspy/refactor_imports/sort.py", line 98, in sort
    imports_partitioned[classify_func(import_obj)].append(import_obj)
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/aspy/refactor_imports/sort.py", line 72, in classify_func
    tp = classify_import(obj.import_statement.module, **kwargs)
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/aspy/refactor_imports/classify.py", line 135, in classify_import
    found, module_path, is_builtin = _get_module_info(
  File "/home/david/.cache/pre-commit/repo4dijr62u/py_env-python3.10/lib/python3.10/site-packages/aspy/refactor_imports/classify.py", line 103, in _get_module_info
    assert spec.submodule_search_locations is not None
AssertionError
asottile commented 2 years ago

the original version of setuptools comes from the virtualenv version -- not from the setuptools version. note also that virtualenv is not hermetic by default and will reach out to pypi to download the latest pip/setuptools/wheel after a few weeks.

can you show the run where you've set setuptools>60.9 in additional_dependencies? that should make it "consistent". 60.2 was broken which is why the version of virtualenv used does not allow that version

davidism commented 2 years ago

OK, I tried "setuptools>60.9" again this morning, and it seems to be consistent now, local pre-commit removed the line, then CI didn't add it back. https://results.pre-commit.ci/run/github/734244/1645026918.AXHgzdHiQEmEN5vP5cM6QA Thanks for the help.

davidism commented 2 years ago

Is there an issue I can track for when to unpin setuptools?

asottile commented 2 years ago

it'll just happen the next time I bump the versions in runner-image