This is an archive of the discontinued LLVM Phabricator instance.

[Utils] Refactor update_cc_test_checks.py to use shutil
ClosedPublic

Authored by jmciver on Sep 15 2022, 10:56 PM.

Details

Summary

The package distutils is deprecated and removal is planned for Python 3.12. All calls to distutils.spawn.find_executable are replaced with local version of find_executable which makes use of shutils.which.

Diff Detail

Event Timeline

jmciver created this revision.Sep 15 2022, 10:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 10:56 PM
jmciver requested review of this revision.Sep 15 2022, 10:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 10:56 PM
jmciver edited the summary of this revision. (Show Details)Sep 15 2022, 11:00 PM
This revision is now accepted and ready to land.Sep 16 2022, 2:08 AM

@arichardson thanks for taking the time to review!

Can you please commit on my behalf?

Name: John McIver
Email: john.mciver.iii@gmail.com

This revision was landed with ongoing or failed builds.Sep 20 2022, 1:42 AM
This revision was automatically updated to reflect the committed changes.

Hi @jmciver and @nlopes, this change is causing failures on the PS5 Windows bot, can you take a look and revert if it will take a while to investigate?

https://lab.llvm.org/buildbot/#/builders/216/builds/10030

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "rm" "-rf" "Z:\test\build\tools\clang\test\utils\update_cc_test_checks\Output\check-globals.test.tmp"
$ "mkdir" "Z:\test\build\tools\clang\test\utils\update_cc_test_checks\Output\check-globals.test.tmp"
$ ":" "RUN: at line 5"
$ "cp" "Z:\test\llvm-project\clang\test\utils\update_cc_test_checks/Inputs/check-globals.c" "Z:\test\build\tools\clang\test\utils\update_cc_test_checks\Output\check-globals.test.tmp/norm.c"
$ ":" "RUN: at line 6"
$ "C:/Program Files/Python310/python.exe" "Z:\test\llvm-project\llvm\utils\update_cc_test_checks.py" "--clang" "Z:\test\build\bin\clang" "--opt" "Z:\test\build\bin\opt" "Z:\test\build\tools\clang\test\utils\update_cc_test_checks\Output\check-globals.test.tmp/norm.c" "--check-globals"
# command stderr:
Please specify --llvm-bin or --clang

I reverted this change as it breaks the tests on windows. It seems that distutils.spawn.find_executable is not exactly equivalent to shutil.which on Windows as the former adds ".exe" and the latter adds the extensions from PATHEXT. Furthermore we are invoking these functions on a full path e.g. "C:\p\15\vs2019\bin\clang" and looking at the docs I'm not sure what the behaviour of either function should be in that case.

thakis added a subscriber: thakis.Sep 20 2022, 10:30 AM

Also broke my windows bot: http://45.33.8.238/win/66470/step_7.txt

Thanks for the revert!

Thanks all for the feedback and the revert. I'll look into this more.

jmciver updated this revision to Diff 461792.Sep 20 2022, 10:42 PM
jmciver edited the summary of this revision. (Show Details)

Add a local find_executable function to update_cc_test_checks.py that takes the necessary functionality from distutils.spawn.find_executable and makes use of shutil.which.

The local find_executable:

  • Appends .exe to the executable name when the environment is win32
  • Test if executable is specified by an absolute or relative path and if iit is return the absolute or relative executable

I have tested in both Windows 11 and Linux using PATH, --clang (both absolute and relative), and --llvm-bin (both absolute and relative).

https://github.com/python/cpython/blob/main/Lib/distutils/spawn.py#L95
https://github.com/python/cpython/blob/main/Lib/shutil.py#L1441

jmciver reopened this revision.Sep 20 2022, 10:42 PM
This revision is now accepted and ready to land.Sep 20 2022, 10:42 PM
jmciver requested review of this revision.Sep 20 2022, 10:50 PM
jmciver updated this revision to Diff 461797.Sep 20 2022, 11:28 PM

Add a local function find_executable to update_cc_test_checks.py, which takes the necessary functionality from distutils.spawn.find_executable and makes use of shutil.which.

The local find_executable appends .exe to the executable name when the environment is win32. shutil.which correctly handles executables that are specified by absolute or relative path.

I have tested in both Windows 11 and Linux using PATH, --clang (both absolute and relative), and --llvm-bin (both absolute and relative).

https://github.com/python/cpython/blob/main/Lib/distutils/spawn.py#L95
https://github.com/python/cpython/blob/main/Lib/shutil.py#L1441

MaskRay accepted this revision.Sep 21 2022, 10:53 AM
This revision is now accepted and ready to land.Sep 21 2022, 10:53 AM
This revision was landed with ongoing or failed builds.Sep 21 2022, 10:56 AM
This revision was automatically updated to reflect the committed changes.