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

17.0.4: LLVM uses deprecated distutils python module #54337

Open
kloczek opened this issue Mar 11, 2022 · 24 comments
Open

17.0.4: LLVM uses deprecated distutils python module #54337

kloczek opened this issue Mar 11, 2022 · 24 comments
Labels

Comments

@kloczek
Copy link

kloczek commented Mar 11, 2022

distutils is already deprecated https://peps.python.org/pep-0632/ and should no longer be used.

[tkloczko@devel-g2v lldb-13.0.1.src]$ grep -r distutils
CMakeLists.txt:          -c "import distutils.sysconfig; print(distutils.sysconfig.get_python_lib(True, False, ''))"
packages/Python/lldbsuite/test/decorators.py:from distutils.version import LooseVersion
packages/Python/lldbsuite/test/lldbtest.py:from distutils.version import LooseVersion
packages/Python/lldbsuite/test/lldbtest.py:import distutils.spawn
packages/Python/lldbsuite/test/lldbtest.py:        path = distutils.spawn.find_executable("clang", lldb_dir)
test/API/tools/lldb-server/TestAppleSimulatorOSType.py:            from distutils.version import LooseVersion
test/Shell/helper/build.py:        from distutils.version import StrictVersion
test/Shell/helper/build.py:            from distutils.version import LooseVersion
test/Shell/lit.cfg.py:from distutils.spawn import find_executable
third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 11, 2022

@llvm/issue-subscribers-lldb

@kloczek
Copy link
Author

kloczek commented Apr 21, 2022

Just checked 14.0.0 and still I see used distutils

[tkloczko@devel-g2v lldb-14.0.0.src]$ grep -r distutils
packages/Python/lldbsuite/test/decorators.py:from distutils.version import LooseVersion
packages/Python/lldbsuite/test/lldbtest.py:from distutils.version import LooseVersion
packages/Python/lldbsuite/test/lldbtest.py:import distutils.spawn
packages/Python/lldbsuite/test/lldbtest.py:        path = distutils.spawn.find_executable("clang", lldb_dir)
test/API/tools/lldb-server/TestAppleSimulatorOSType.py:            from distutils.version import LooseVersion
test/Shell/helper/build.py:        from distutils.version import StrictVersion
test/Shell/helper/build.py:            from distutils.version import LooseVersion
test/Shell/lit.cfg.py:from distutils.spawn import find_executable
third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup

@kloczek kloczek changed the title 13.0.1 lldb: python module build uses distutils 14.0.0 lldb: python module build uses distutils Apr 21, 2022
@DavidSpickett
Copy link
Collaborator

To remove find_executable usage: https://reviews.llvm.org/D124601

DavidSpickett added a commit that referenced this issue Apr 29, 2022
distutils is deprecated and shutil.which is the suggested
replacement for this function.

https://peps.python.org/pep-0632/#migration-advice
https://docs.python.org/3/library/shutil.html#shutil.which

It was added in Python3.3 but given that we're already using
shutil.which elsewhere I think this is ok/no worse than before.

We do have our own find_executable in lldb/test/Shell/helper/build.py
but I'd rather leave that as is for now. Also we have our own versions
of which() but again, a change for another time.

This work is part of #54337.

Reviewed By: JDevlieghere

Differential Revision: https://reviews.llvm.org/D124601
@DavidSpickett
Copy link
Collaborator

https://reviews.llvm.org/D124670 is what switching to packaging for the version handling would look like. It would need us to make a copy of that in thirdparty/ (possible that we could pick out that one bit, haven't looked).

Needs testing on all platforms though, I'm not sure LegacyVersion matches LooseVersion in all the ways we expect.

Then we'd be left with:

$ grep -r distutils
third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup

Which could be updated if we want. I think it is used just for the test suite not any modules you would find in an install.

@kloczek
Copy link
Author

kloczek commented Apr 29, 2022

Only confirmation: lldb seems is no longer affected.
Feel free to close.

@DavidSpickett
Copy link
Collaborator

No longer affected how exactly. In that we've removed all the distutils usage that isn't related just to testing?

I'd keep this open just for removing distutils usage in general, testing or otherwise.

@kloczek
Copy link
Author

kloczek commented Apr 29, 2022

Current pexpect no longer uses distutils.
I think that the best would be drop bundling that module code.

@DavidSpickett
Copy link
Collaborator

Ok I understand now.

I think (in that I've never seen it written down) the policy for lldb is to include copies of third party python packages. So to be clear by "updated" I mean getting a copy of a newer version of pexpect, not editing the current one to remove distutils.

When I get a chance I'll find out what that process looks like. We do have to be careful about API changes there.

@labath
Copy link
Collaborator

labath commented May 4, 2022

It's not just lldb -- llvm in general likes to absorb things into its repository. Although I don't really agree with the practice, I can't deny the practicality of not having to install a bunch of random packages in order to run tests.

On a practical note, our copy of pexpect was updated fairly recently, and we don't make any modifications to it, so pulling in a new version should be fairly straight-forward.

@mgorny
Copy link
Member

mgorny commented Apr 17, 2023

@DavidSpickett, would you by any chance be interested in doing the find_executable() switch in other LLVM projects as well?

I'm wondering about the best replacement for LooseVersion/StrictVersion. One problem is that modern packaging no longer supplies LooseVersions, so unless we can strict to the strict variant (i.e. the one requiring conformance to PEP 440), we may end up having to parse the versions manually.

That said, I suppose most of LooseVersion uses are actually trivial.

@DavidSpickett
Copy link
Collaborator

I don't see why not. I see 22 instances of find_executable in llvm, a few are slightly different wrappers and one literally has a TODO to replace it with shutil.which.

Do you have the time to do patches or should I?

@mgorny
Copy link
Member

mgorny commented Apr 17, 2023

I would prefer if you did it. I have time to write patches but unfortunately insufficient time to test them properly ;-).

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Apr 17, 2023

https://reviews.llvm.org/D148522
https://reviews.llvm.org/D148527
https://reviews.llvm.org/D148528
https://reviews.llvm.org/D148529

After those the only find_executable are wrappers around shutil.which that add corner case handling.

@mgorny
Copy link
Member

mgorny commented Apr 17, 2023

Thank you!

@DavidSpickett
Copy link
Collaborator

All the above have landed, everything uses shutil.which now.

As for LooseVersion I see packaging.version.Version mentioned as an alternative but as I said above, we'd have to make a copy of packaging to use it.

Unfortunately I won't have time to look into this myself.

@mgorny
Copy link
Member

mgorny commented Apr 19, 2023

Note that packaging.version.Version is not equivalent to LooseVersion — the latter will accept pretty much anything, the former only valid Python-style versions.

@DavidSpickett
Copy link
Collaborator

Yeah I feared it might not be 100% the same.

It might be best to write up a small set of tests that mimic what the existing users in llvm-project are doing. Then see if any of the alternatives fit, otherwise we could make a small module to do just what we need.

@mgorny
Copy link
Member

mgorny commented Apr 19, 2023

Yeah, I'll try to find some time for it. No promises, though.

@kloczek
Copy link
Author

kloczek commented May 3, 2023

Just checked 16.x brancj and looks like distutils now is used even in more places 😞

[tkloczko@pers-jacek llvm-project]$ grep -r distutils | grep py:
clang/utils/check_cfc/setup.py:from distutils.core import setup
clang/utils/creduce-clang-crash.py:from distutils.spawn import find_executable
compiler-rt/lib/asan/scripts/asan_symbolize.py:from distutils.spawn import find_executable
compiler-rt/test/lit.common.cfg.py:    from distutils.version import LooseVersion
cross-project-tests/lit.cfg.py:from distutils.version import LooseVersion
libcxx/utils/libcxx/sym_check/extract.py:import distutils.spawn
libcxx/utils/libcxx/sym_check/extract.py:        return distutils.spawn.find_executable('nm')
libcxx/utils/libcxx/sym_check/extract.py:        return distutils.spawn.find_executable('readelf')
libcxx/utils/libcxx/sym_check/extract.py:         return distutils.spawn.find_executable('dump')
libcxx/utils/libcxx/sym_check/util.py:import distutils.spawn
libcxx/utils/libcxx/sym_check/util.py:_cppfilt_exe = distutils.spawn.find_executable('c++filt')
lldb/packages/Python/lldbsuite/test/decorators.py:from distutils.version import LooseVersion
lldb/packages/Python/lldbsuite/test/lldbtest.py:from distutils.version import LooseVersion
lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py:            from distutils.version import LooseVersion
lldb/test/Shell/helper/build.py:        from distutils.version import StrictVersion
lldb/test/Shell/helper/build.py:            from distutils.version import LooseVersion
lldb/third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup
llvm/utils/gn/build/write_vcsrevision.py:    # distutils.spawn.which() doesn't find .bat files,
llvm/utils/release/bump-version.py:    # parse the version string with distutils.
third-party/benchmark/setup.py:from distutils import sysconfig

@kloczek kloczek changed the title 14.0.0 lldb: python module build uses distutils 16.0.3: LLVM uses deprecated distutils python module May 3, 2023
@kloczek
Copy link
Author

kloczek commented Nov 6, 2023

Just FTR updated list of files which are still using distutils

clang/utils/check_cfc/setup.py:from distutils.core import setup
compiler-rt/test/lit.common.cfg.py:        from distutils.version import LooseVersion
cross-project-tests/lit.cfg.py:from distutils.version import LooseVersion
libcxx/utils/ci/Dockerfile:#RUN apt-get update && apt-get install -y ninja-build python3 python3-distutils python3-psutil git gdb ccache
libcxx/utils/ci/Dockerfile:RUN apt-get update && apt-get install -y python3 python3-distutils python3-psutil git gdb ccache
lldb/packages/Python/lldbsuite/test/decorators.py:from distutils.version import LooseVersion
lldb/packages/Python/lldbsuite/test/lldbtest.py:from distutils.version import LooseVersion
lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py:            from distutils.version import LooseVersion
lldb/test/Shell/helper/build.py:        from distutils.version import StrictVersion
lldb/test/Shell/helper/build.py:            from distutils.version import LooseVersion
lldb/third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup
llvm/utils/gn/build/write_vcsrevision.py:    # distutils.spawn.which() doesn't find .bat files,
llvm/utils/release/bump-version.py:    # parse the version string with distutils.
third-party/benchmark/setup.py:from distutils import sysconfig

@kloczek kloczek changed the title 16.0.3: LLVM uses deprecated distutils python module 17.0.4: LLVM uses deprecated distutils python module Nov 6, 2023
@kloczek
Copy link
Author

kloczek commented Feb 22, 2024

Any progress? 🤔
Almost two years after open this ticket issue still is not resolved.

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Feb 22, 2024

Surprisingly, yes :) 0c02329

As of now:

third_party/Python/module/pexpect-4.6/setup.py
2:from distutils.core import setup

test/Shell/helper/build.py
444:        from distutils.version import StrictVersion

@kloczek
Copy link
Author

kloczek commented Feb 22, 2024

From of the pan into the open fire .. pkg_resources now marked as DEPRECATED 😞
https://setuptools.pypa.io/en/latest/pkg_resources.html

@kloczek
Copy link
Author

kloczek commented Mar 24, 2024

Looks like use of distutils still is not removed

[tkloczko@pers-jacek llvm-project]$ git grep distutils
clang/utils/check_cfc/setup.py:from distutils.core import setup
compiler-rt/test/lit.common.cfg.py:        from distutils.version import LooseVersion
cross-project-tests/lit.cfg.py:from distutils.version import LooseVersion
libcxx/utils/ci/Dockerfile:        python3-distutils \
libcxx/utils/ci/Dockerfile:#RUN apt-get update && apt-get install -y ninja-build python3 python3-distutils python3-psutil git gdb ccache
lldb/test/Shell/helper/build.py:        from distutils.version import StrictVersion
lldb/third_party/Python/module/pexpect-4.6/setup.py:from distutils.core import setup
llvm/utils/gn/build/write_vcsrevision.py:    # distutils.spawn.which() doesn't find .bat files,
llvm/utils/release/bump-version.py:    # parse the version string with distutils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants