This is an archive of the discontinued LLVM Phabricator instance.

[Python] Update checkDsymForUUIDIsOn to be compatible with Python 3.
ClosedPublic

Authored by davide on Jan 9 2019, 3:14 PM.

Details

Summary

In python 2, strings and bytes are the same, but they're not in
python 3, hence the return of read() needs an explicit conversion.
While I'm around, rename the return of Popen() from pipe to
process, as that's what Popen returns.

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Jan 9 2019, 3:14 PM

Another way of dealing with this is to pass universal_newlines=True to subprocess.Popen. Despite the silly name, all this really means is "redirected pipes are text mode". As long as you expect the command being run to always return text rather than binary, it's safe to use. The name universal_newlines is actually so poor that in Python 3.7 they made a new alias that is identical, but just called text, so in 3.7 and higher you can just say text=True. We can't do that here because we probably want to support 3.5 and higher, but the point is just that universal_newlines isn't really anything fancy, it just means text.

If you do that, then process.stdout.read() will just return a string, and you don't need to manually decode it.

davide added a comment.Jan 9 2019, 4:53 PM

Given the name is really bizarre, I would stick with what I have now, which at least clarifies the intent (unless you feel strongly about it)

It's probably fine the way it is. The biggest advantage of using universal_newlines=True is that the result will always be a string in both Python 2 and Python 3. This makes it easier to reason about subsequent code , but if you're not doing much with the subsequent code, it doesn't matter. In Python 2, the return type is a Unicode object, which is different than a string. For this code it's fine, it's just something to keep in mind in future code.

(BTW, if for whatever reason in the future we decide that we require a minimum of Python 3.7, we can change this to text=True and then it will "just work"

zturner accepted this revision.Jan 9 2019, 5:01 PM
This revision is now accepted and ready to land.Jan 9 2019, 5:01 PM
This revision was automatically updated to reflect the committed changes.