search

facebook / chisel

Chisel is a collection of LLDB commands to assist debugging iOS apps.
MIT License
9.13k stars 804 forks source link

Make chisel compatible with Python 3 #266

Closed liuliu closed 5 years ago

liuliu commented 5 years ago

Xcode 11 shipped with Python 3 now. Unfortunately, chisel was written in Python 2 style and will fail load for lldb. This PR fixed the Python 3 compatibility issues.

facebook-github-bot commented 5 years ago

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

facebook-github-bot commented 5 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

kastiglione commented 5 years ago

Do you want chisel to remain compatible with python 2 going forward? Or should there by a tagged or branch that people can checkout if the want Python 2 support?

liuliu commented 5 years ago

I believe the update itself is still compatible with Python 2 (I need to double check). Going forward, we will update our dev machines pretty aggressively to Xcode 11, thus, ongoing Python 2 support is not important to us. It is up to you guys to decide when you want to remove Python 2 support (considering macOS will stop shipping Python 2.7 in next major OS update and EOL is 2020).

kolinkrewinkel commented 5 years ago

Yeah, I think based on the changes we made we're still (temporarily) Python 2 compatible. I'm okay with us becoming non-compatible though eventually, since both Apple and Facebook are moving pretty aggressively towards 3.

kastiglione commented 5 years ago

we're still (temporarily) Python 2 compatible

I think the use of print(..., file=...) needs changes to make it python 2 compatible. For python 2, it would have include from __future__ import print_function.

liuliu commented 5 years ago

LMK if there is anything else you need to update.

liuliu commented 5 years ago

Doesn't seem like any conflicts, not sure if any other changes required after this merged in though given the latest tip. How to initiate merge from here?

kolinkrewinkel commented 5 years ago

Oh I see, it's because there were conflict resolutions across multiple commits – if I squash, which is what I wanted to do anyways, it's all good!