yeandy commented on code in PR #21968: URL: https://github.com/apache/beam/pull/21968#discussion_r905435086
########## sdks/python/setup.py: ########## @@ -30,9 +28,15 @@ from pkg_resources import DistributionNotFound from pkg_resources import get_distribution from pkg_resources import normalize_path +from pkg_resources import parse_version from pkg_resources import to_filename from setuptools import Command +# distutils from stdlib is deprecated from python 3.10. +# setuptools has been maintaining a copy of distutils and importing +# setuptools will replace stdlib/distutils with setuptools/distutils. +from distutils.errors import DistutilsError # pylint: disable=wrong-import-order Review Comment: So basically, as long as setuptools is always imported before distutils, we can still use `DistutilsError` because it's maintained separately inside `setuptools`? ########## sdks/python/apache_beam/runners/portability/stager.py: ########## @@ -54,14 +54,14 @@ import shutil import sys import tempfile -from distutils.version import StrictVersion from typing import Callable from typing import List from typing import Optional from typing import Tuple from urllib.parse import urlparse import pkg_resources +from pkg_resources import parse_version Review Comment: Am I understanding this correctly: Since `setuptools` has its own copy of `distutils`, this means we can still use the original `distutils` logic (provided we make your changes), right? And you said we can replace `StrictVersion` with the alternative `parse_version`. Besides minimizing usage of `distutils`, are there any other benefits of using `parse_version`? Maybe in the future, when don't want to use `setuptools'` copy of `distutils` anymore, it would be easier to refactor any remaining usage of `distutils`? Finally, I saw setuptools' [warning](https://github.com/pypa/setuptools/blob/main/_distutils_hack/__init__.py) that "To avoid these issues, avoid "using distutils directly, ensure that setuptools is installed in the traditional way (e.g. not an editable install), and/or make sure that setuptools is always imported before distutils." To me, it feels safer (and easier) to manger to eliminate using `distutils` entirely than to maintain our codebase such that `setuptools` is always imported before `distutils`. ########## sdks/python/setup.py: ########## @@ -30,9 +28,15 @@ from pkg_resources import DistributionNotFound from pkg_resources import get_distribution from pkg_resources import normalize_path +from pkg_resources import parse_version from pkg_resources import to_filename from setuptools import Command +# distutils from stdlib is deprecated from python 3.10. +# setuptools has been maintaining a copy of distutils and importing +# setuptools will replace stdlib/distutils with setuptools/distutils. +from distutils.errors import DistutilsError # pylint: disable=wrong-import-order Review Comment: And we would rather keep this because `DistutilsError` is more meaningful than your previously proposed usage of `setuptools.errors.BaseError`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
