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

unimport crashes on py3.12 due to distutils removal #297

Closed
mxr opened this issue Oct 18, 2023 · 13 comments · Fixed by #303
Closed

unimport crashes on py3.12 due to distutils removal #297

mxr opened this issue Oct 18, 2023 · 13 comments · Fixed by #303
Labels
discussion Issue needs to be discussed and concluded.

Comments

@mxr
Copy link
Contributor

mxr commented Oct 18, 2023

Repro steps:

Environment (from here):

  • sys.version: '3.12.0 (main, Oct 2 2023, 12:03:24) [Clang 15.0.0 (clang-1500.0.40.1)]'
  • os.name: 'posix'
  • sys.platform: 'darwin'
  • platform.system(): 'Darwin'
  • platform.python_implementation(): 'CPython'
  • sys.implementation.name: 'cpython'
  • sysconfig.get_platform(): 'macosx-14-arm64'
$ cat example.py
import sys
print('hello world')
$ pip freeze | rg unimport
unimport==1.0.0
$ unimport example.py
Traceback (most recent call last):
  File "/Users/mxr/tmp/venv3.12/bin/unimport", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/__main__.py", line 4, in main
    from unimport.main import Main
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/main.py", line 6, in <module>
    from unimport import commands, utils
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/commands/__init__.py", line 1, in <module>
    from unimport.commands import options
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/commands/options.py", line 5, in <module>
    from unimport.config import Config
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/config.py", line 14, in <module>
    from unimport import constants as C
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/constants.py", line 2, in <module>
    import distutils.sysconfig
ModuleNotFoundError: No module named 'distutils'

See https://docs.python.org/3/whatsnew/3.10.html#distutils-deprecated for more info

This was referenced Oct 18, 2023
@hakancelikdev
Copy link
Owner

Hi thanks for the issue but

officially, unimport does not yet support versions after Python 3.10.
Wouldn't this fix be a bit pointless when no development has been made yet? It would be better to develop support for Python3.11 and Python3.12 first.

@hakancelikdev hakancelikdev added waiting for response discussion Issue needs to be discussed and concluded. labels Oct 25, 2023
@mxr
Copy link
Contributor Author

mxr commented Oct 25, 2023

Do you mind clarifying? I believe unimport already works on py3.11 (at least tox -e py3.11 passed and the wheel is not limited to <py3.10), so supporting it would simply require changing CI parameters. And if desired, supporting py3.12 can happen with the 2 PR's I have open.

@mxr
Copy link
Contributor Author

mxr commented Oct 26, 2023

Phrased another way - if we don't want to support py312 then the right thing is to configure that in python_requires (currently we allow an install but crash at runtime)

@hakancelikdev
Copy link
Owner

If all packages support Python 3.11 and 3.12, we can make the necessary changes in Unimport and provide support for these versions, there is no problem with this.

Do you want to do this job or should I do it?

@mxr
Copy link
Contributor Author

mxr commented Oct 31, 2023

I don't know what you mean by "if all packages". According to https://endoflife.date/python python3.11 and python3.12 are supported officially by Python maintainers

I can update this project to run in CI for python 3.11

@hakancelikdev
Copy link
Owner

When I say all packages, I mean other 3rd party packages that unimport uses.

@hakancelikdev
Copy link
Owner

You don't need to do anything additional for now, I will check your PR.

@mxr
Copy link
Contributor Author

mxr commented Nov 2, 2023

I think we are mixing up two things

A) Whether unimport runs under python3.11+?
B) Whether unimport can process syntax available in python3.11+?

(A) is enforced by setting python_requires. Since unimport sets python_requires = >=3.6, users expect that the tool runs under all of those versions. One way to resolve this bug report is to remove usage of distutils in favor of other libraries. Another way is to set a ceiling on python_requires and keep using distutils

(B) does not have a standard way of enforcement. At the time of the bug report in #291, the match statement (new to py310) could not be processed by libcst even though unimport does run on py310. One way to solve this class of issue is to add a new target_version argument to unimport in order to be explicit/implicit about which syntax you support

I opened this ticket in response to issue (A) but it sounds like you want to resolve both (A) and (B)? What I can do is check if the dependencies run on py311 and/or set a ceiling for python_requires if you feel this better guards against surprises

@hakancelikdev
Copy link
Owner

Yes, I understand you, you are right.

1 -) There was a mistake when determining python_requires, we should update it to include only versions that we guarantee to support.

2 -) If we want to support Python 3.11 or a higher Python version, we must check the packages we currently use, update our setup file, add new tests to ensure that we support new features, and fix any issues related to these versions, if exists.

There are two separate jobs, they must be done in order.

@mxr
Copy link
Contributor Author

mxr commented Nov 6, 2023

I think we are still not totally aligned, or at least the use of 'supports' is overloaded. I may be guilty of this too.

Let's use the terminology "runs under" and "language version" to delineate the two. For example

Now applied to unimport: unimport is configured to run under python 3.8. However it fails to run under 3.12 or higher - that's what this issue is about. Separately, it crashes on language version 3.10 or higher (#291).

It would be straightforward to fix unimport so it runs under python3.12 by removing distutils. I have a PR for that. It would take more work to support language versions 3.10+ and not something I can take on.

Let me know your thoughts

@hakancelikdev
Copy link
Owner

Ok I understand, thank you for your effort, and sorry for getting back late.

@hakancelikdev
Copy link
Owner

Now you can install and use version 1.0.1 -> https://pypi.org/project/unimport/1.0.1/

@mxr
Copy link
Contributor Author

mxr commented Nov 17, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issue needs to be discussed and concluded.
Projects
None yet
2 participants