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

Make chisel compatible with Python 3 #266

Merged
merged 7 commits into from
Oct 11, 2019
Merged

Conversation

liuliu
Copy link
Contributor

@liuliu liuliu commented Sep 25, 2019

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.

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
Copy link
Contributor

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.

@@ -423,9 +423,9 @@ def isHeap(addr):

allocations = (addr for addr in pointers if isHeap(addr))
for addr in allocations:
print >>self.result, '0x{addr:x} {path}'.format(addr=addr, path=pointers[addr])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some advice on this.

Copy link
Contributor

@natansh natansh Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might help.

https://stackoverflow.com/questions/34385674/what-does-print-do-in-python

I suspect this means we should write it as

print('0x{addr:x} {path}'.format(addr=addr, path=pointers[addr]), file=self.result)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@facebook-github-bot
Copy link
Contributor

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

@natansh natansh requested a review from kastiglione September 25, 2019 05:10
Copy link
Contributor

@kolinkrewinkel kolinkrewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for fixing this up, really appreciate it! Will give Dave a chance to look, but looks good to me!

commands/FBDebugCommands.py Show resolved Hide resolved
@kastiglione
Copy link
Contributor

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?

@kastiglione kastiglione mentioned this pull request Sep 25, 2019
@liuliu
Copy link
Contributor Author

liuliu commented Sep 25, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

liuliu commented Sep 27, 2019

LMK if there is anything else you need to update.

@CyberMew CyberMew mentioned this pull request Oct 10, 2019
Copy link
Contributor

@kolinkrewinkel kolinkrewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry for the delay – thought I'd already approved. Looks great, just looks like there are some conflicts that've accrued! Thanks again for doing this!

@liuliu
Copy link
Contributor Author

liuliu commented Oct 11, 2019

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
Copy link
Contributor

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!

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

Successfully merging this pull request may close these issues.

7 participants