This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove uses of six module (NFC)
ClosedPublic

Authored by kastiglione on Aug 5 2022, 3:39 PM.

Details

Summary

With lldb (& llvm) requiring Python 3.6+, use of the six module can be removed.

Diff Detail

Event Timeline

kastiglione created this revision.Aug 5 2022, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 3:39 PM
kastiglione requested review of this revision.Aug 5 2022, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2022, 3:39 PM
kastiglione added inline comments.Aug 5 2022, 3:46 PM
lldb/packages/Python/lldbsuite/support/encoded_file.py
17–25

this function only deviated in the case of python 2, so I removed it.

lldb/packages/Python/lldbsuite/support/seven.py
12

this function was not used externally, so I inlined it into get_command_output.

27

this was also not used.

lldb/packages/Python/lldbsuite/test/lldbtest.py
2278–2281

this function had a parameter named str, which shadowed builtin.str. As a fix, in this file I renamed all variables named str to string.

I left third_party/Python/module/six, in case there are any lldb scripts that depend on the existence of six.

mib accepted this revision.Aug 5 2022, 4:02 PM

Very cool! Thanks for taking care of this! LGTM with the 2 comments and assuming the test suite runs fine :)

lldb/examples/python/scripted_process/scripted_process.py
5

nit: no need to specify metaclass=ABCMeta, it can just be ScriptedProcess(ABC)

194

ditto

lldb/packages/Python/lldbsuite/test/lldbtest.py
2278–2281

nit: If you feel like it, may be you can split everything related to the str-> string refactor into a separate since it's orthogonal to removing six

This revision is now accepted and ready to land.Aug 5 2022, 4:02 PM
mib added inline comments.Aug 5 2022, 4:04 PM
lldb/examples/python/scripted_process/scripted_process.py
5

disregard this comment, that's actually the way to do it ...

kastiglione added inline comments.Aug 5 2022, 4:06 PM
lldb/packages/Python/lldbsuite/test/lldbtest.py
2278–2281

I need to at least make the str -> string rename in the functions where the refactor involves isinstance(..., str). I will remove the other renames, for another diff.

Restore some "str" variable names

@mib the tests pass on my machine.

Missed a restore of str variable name

There is also a cmake var LLDB_USE_SYSTEM_SIX which can be removed.

remove LLDB_USE_SYSTEM_SIX

kastiglione added inline comments.Aug 8 2022, 9:34 AM
lldb/bindings/python/CMakeLists.txt
63–68 ↗(On Diff #450843)

@DavidSpickett I removed LLDB_USE_SYSTEM_SIX but left the code that copies vendored copy of six.py. Is this what you had in mind? My thinking is there might be lldb scripts that use six, and I don't want the removal of internal use of six to be a breaking change downstream, not yet anyway.

DavidSpickett added inline comments.Aug 9 2022, 1:50 AM
lldb/bindings/python/CMakeLists.txt
63–68 ↗(On Diff #450843)

I hadn't considered user scripts good point.

If you were using this option, you'd now get a warning from cmake that it's value was unused. Would it make more sense to have a proper deprecation warning and remove it and the copy of six at some later point?

Please land this change minus the LLDB_USE_SYSTEM_SIX stuff though. I don't want to block the rest on this.

Restore LLDB_USE_SYSTEM_SIX

This revision was landed with ongoing or failed builds.Aug 11 2022, 7:06 PM
This revision was automatically updated to reflect the committed changes.