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

rfe: Deprecate distutils module #3920

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PaulYuuu
Copy link
Contributor

@PaulYuuu PaulYuuu commented Jun 3, 2024

distutils will be formally marked as deprecated since python3.10, and will no longer work from python3.12. To enable python3.12 in avocado framework, this patch will replace distutils to use other modules.

ref: https://peps.python.org/pep-0632

@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Jun 3, 2024

https://github.com/avocado-framework/avocado-vt/blob/master/virttest/shared/scripts/virtio_console_guest.py
this file is used in guest, even though it also uses distutils module, but I didn't modify it together, cause I am not sure about the guest python version.
cc @nanliu-r

@PaulYuuu PaulYuuu force-pushed the deprecate-distutils branch 2 times, most recently from fb14fb0 to 1cafcb4 Compare June 3, 2024 09:22
@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Jun 3, 2024

Hello @clebergnu @luckyh, please help to review and raise your concerns.

Another one is I noticed setup.py would also be deprecated, do we plan to migrate to the modern pyproject.toml? https://packaging.python.org/en/latest/guides/modernize-setup-py-project/#modernize-setup-py-project

Besides pyproject.toml, I tried using pip install -e . to install avocado-vt, and this works, and with pip install but not python setup.py install/develop, the CleanCommand in setup.py is not a must.

cmdclass={"clean": Clean},
install_requires=[
"netifaces",
"packaging",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be in avocado rather than avocado-vt?

Copy link
Contributor

@luckyh luckyh left a comment

Choose a reason for hiding this comment

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

Hi @PaulYuuu , thanks for raising this up! I think the overall idea is good, however, I also have the following concerns.

@@ -910,7 +909,7 @@ def bootstrap(options, interactive=False):
LOG.info("%d - Updating test providers repo configuration from local copy", step)
tp_base_dir = data_dir.get_base_test_providers_dir()
tp_local_dir = data_dir.get_test_providers_dir()
dir_util.copy_tree(tp_base_dir, tp_local_dir)
shutil.copytree(tp_base_dir, tp_local_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

shutil.copytree will raise FileExistsError against existing target dirs by default,
(there is an argument dirs_exist_ok added since Python 3.8 in favor to that)

>>> shutil.copytree('./a', './b')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.12/shutil.py", line 600, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/shutil.py", line 498, in _copytree
    os.makedirs(dst, exist_ok=dirs_exist_ok)
  File "<frozen os>", line 225, in makedirs
FileExistsError: [Errno 17] File exists: './b'

while distutil.dir_util.copy_tree don't.

>>> dir_util.copy_tree('./a', './b')
['./b/foo']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, as we also support python3.6, I will check the dst first, if it exists, then remove it.


VERSION = open("VERSION", "r").read().strip()


class Clean(clean):
"""Our custom command to get rid of scratch files after build."""
class CleanCommand(Command):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incompatible with the original use of the clean command.

[before]

$ python setup.py clean --help
Common commands: (see '--help-commands' for more)

  setup.py build      will build the package underneath 'build/'
  setup.py install    will install the package

Global options:
  --verbose (-v)  run verbosely (default)
  --quiet (-q)    run quietly (turns verbosity off)
  --dry-run (-n)  don't actually do anything
  --help (-h)     show detailed help message
  --no-user-cfg   ignore pydistutils.cfg in your home directory

Options for 'Clean' command:
  --build-base (-b)  base build directory [default: 'build.build-base']
  --build-lib        build directory for all modules [default: 'build.build-
                     lib']
  --build-temp (-t)  temporary build directory [default: 'build.build-temp']
  --build-scripts    build directory for scripts [default: 'build.build-
                     scripts']
  --bdist-base       temporary directory for built distributions
  --all (-a)         remove all build output, not just temporary by-products

usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

[after]

$ python setup.py clean --help                                            141 ↵
Common commands: (see '--help-commands' for more)

  setup.py build      will build the package underneath 'build/'
  setup.py install    will install the package

Global options:
  --verbose (-v)  run verbosely (default)
  --quiet (-q)    run quietly (turns verbosity off)
  --dry-run (-n)  don't actually do anything
  --help (-h)     show detailed help message
  --no-user-cfg   ignore pydistutils.cfg in your home directory

Options for 'CleanCommand' command:

usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

FYI, see https://github.com/pypa/setuptools/blob/main/setuptools/_distutils/command/clean.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All clean options are wanna clean sub dir under build, however, our custom cleaning_list will clean the entire build dir, which means these options are not used for us.

avocado-vt/setup.py

Lines 33 to 43 in 4cfe8c9

cleaning_list = [
"MANIFEST",
"BUILD",
"BUILDROOT",
"SPECS",
"RPMS",
"SRPMS",
"SOURCES",
"PYPI_UPLOAD",
"./build",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And, after we switch to pip install in the future, we don't need to care about build dir, as it will use the tmp during build.

@@ -31,8 +30,8 @@ def __init__(self, interval):
raise ValueError("Invalid string representation of an interval")
self.opening, lower, upper, self.closing = match.groups()

self.lower_bound = LooseVersion(lower) if lower else None
self.upper_bound = LooseVersion(upper) if upper else None
self.lower_bound = parse(lower) if lower else None
Copy link
Contributor

Choose a reason for hiding this comment

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

As the implementation of packaging.version.Version is quite different to distutils.version.LooseVersion's, I'm afraid we could not do the simple replacement like this, in order to keeping the compatibility.

distutils will be formally marked as deprecated since python3.10,
and will no longer work from python3.12. To enable python3.12 in
avocado framework, this patch will replace distutils to use other
modules.

Signed-off-by: Yihuang Yu <yihyu@redhat.com>
@PaulYuuu PaulYuuu marked this pull request as draft September 18, 2024 09:20
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