Bug 201826

I've made some solid progress on this so far. Auto-migration tooling isn't going to be sufficient, our problems are mostly about unicode.

<rdar://problem/55452671>

Created attachment 379085 [details] Patch

Comment on attachment 379085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379085&action=review Looks good overall; IMO would be better to move the re-definitions and version checks to the top of the file. > Tools/Scripts/webkitpy/common/version.py:33 > @staticmethod Might be better just to subclass `operator` from the standard library: https://docs.python.org/3/library/operator.html > Tools/Scripts/webkitpy/common/version.py:-36 > - for i in xrange(len(val)): Nit: This should be a re-assignment up top: ```py if sys.version >= 3: xrange = range ``` > Tools/Scripts/webkitpy/common/checkout/changelog.py:36 > + Can we just re-assign unicode to str if sys.version >= 3? Or maybe unicode to bytes? > Tools/Scripts/webkitpy/common/system/abstractexecutive.py:104 > + if sys.version_info > (3, 0): Nit: It would be clearer to re-assign unicode at the top of the file to str if sys.version_info > (3, 0). > Tools/Scripts/webkitpy/common/system/abstractexecutive.py:115 > + if sys.version_info < (3, 0) and isinstance(arg, unicode): Ditto. > Tools/Scripts/webkitpy/common/system/executive.py:76 > + if sys.version_info > (3, 0): Ditto to previous comment on moving this up top. > Tools/Scripts/webkitpy/common/system/executive.py:470 > + string_args = map(str if sys.version_info > (3, 0) else unicode, args) Ditto to previous comments. > Tools/Scripts/webkitpy/common/system/filesystem.py:258 > + if (sys.version_info > (3, 0)): Nit: Can you just re-assign str to bytes if our version is > 3,0? Also, need_encodeing -> need_encoding. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:190 > + return [i for i in filter(path_filter, existing_files)] + [i for i in filter(path_filter, self.dirs)] Nit: Easier to cast these to list(). > Tools/Scripts/webkitpy/common/system/profiler.py:-44 > - profiler_class = next(itertools.ifilter(lambda profiler: profiler.name == profiler_name, profilers), None) Nit: itertools.ifilter in py2 is equivalent to filter in py3. > Tools/Scripts/webkitpy/common/system/user_mock.py:34 > +if sys.version_info > (3, 0): Nit: Might be more clear to just re-assign input or raw_input.

Comment on attachment 379085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379085&action=review > Tools/Scripts/webkitpy/common/checkout/changelog.py:440 > + result.write(unicode("\n Reviewed by %s.\n" % reviewer)) Let's add comments for cases when we need to make files python3 compatible. Also do we want to keep % notation? > Tools/Scripts/webkitpy/common/net/networktransaction.py:33 > +if sys.version_info > (3, 0): should we just make a common is_python_3 helper function? > Tools/Scripts/webkitpy/common/system/abstractexecutive.py:104 > + if sys.version_info > (3, 0): I like Dean's idea about reassigning unicode to str in python3 > Tools/Scripts/webkitpy/common/system/executive.py:76 > + if sys.version_info > (3, 0): unicode hack > Tools/Scripts/webkitpy/common/system/executive.py:95 > + continue This code feels extremely unsafe to me. We have no idea if Popen is depending on some of the __ functions you are not setting > Tools/Scripts/webkitpy/common/system/executive.py:-114 > - while True: keep comment? > Tools/Scripts/webkitpy/common/system/executive.py:132 > + teed_output.write(output_line.decode('utf-8')) maybe write a decode helper function? > Tools/Scripts/webkitpy/common/system/executive.py:374 > + if sys.version_info < (3, 0) and isinstance(input, unicode): rename unicode? > Tools/Scripts/webkitpy/common/system/executive.py:405 > + output = output = process.communicate()[0] bug > Tools/Scripts/webkitpy/common/system/executive.py:407 > + output = process.communicate(string_to_communicate.encode('utf-8'))[0] decide/encode cleanup > Tools/Scripts/webkitpy/common/system/executive.py:470 > + string_args = map(str if sys.version_info > (3, 0) else unicode, args) rename unicode? > Tools/Scripts/webkitpy/common/system/filesystem.py:258 > + if (sys.version_info > (3, 0)): encode/decode helper functions

Comment on attachment 379085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379085&action=review >> Tools/Scripts/webkitpy/common/version.py:33 >> @staticmethod > > Might be better just to subclass `operator` from the standard library: > https://docs.python.org/3/library/operator.html Not quite sure what you have in mind here. The operator module doesn't include anything to subclass >> Tools/Scripts/webkitpy/common/version.py:-36 >> - for i in xrange(len(val)): > > Nit: This should be a re-assignment up top: > ```py > if sys.version >= 3: > xrange = range > ``` In this case, we don't lose much by using range in Python 3, I'd rather not have the version check if we don 't need it. >> Tools/Scripts/webkitpy/common/net/networktransaction.py:33 >> +if sys.version_info > (3, 0): > > should we just make a common is_python_3 helper function? I don't know that an is_python_3 function is any shorter or more clear than this (and then we would have to import it) If we were going to do this, we would probably want to have a sort of 'webkitpy.common.python3' >> Tools/Scripts/webkitpy/common/system/abstractexecutive.py:115 >> + if sys.version_info < (3, 0) and isinstance(arg, unicode): > > Ditto. Still need the version check here, we want to use unicode in Python 3 >> Tools/Scripts/webkitpy/common/system/executive.py:132 >> + teed_output.write(output_line.decode('utf-8')) > > maybe write a decode helper function? What would you envision that function to be and where should it live? >> Tools/Scripts/webkitpy/common/system/executive.py:374 >> + if sys.version_info < (3, 0) and isinstance(input, unicode): > > rename unicode? I will, but we still need the version check because we want to prefer unicode in Python 3, but not Python 2 (string literals are unicode in Python 3) >>> Tools/Scripts/webkitpy/common/system/filesystem.py:258 >>> + if (sys.version_info > (3, 0)): >> >> Nit: Can you just re-assign str to bytes if our version is > 3,0? >> >> Also, need_encodeing -> need_encoding. > > encode/decode helper functions No, we definitely can't just re-assign str to bytes, because str is a valid identifier in Python 3. Also, we have had some systemic problems with unicode handling, I think forcing ourselves to be explicit is going to be a good thing for the codebase. It will force us to be clear about what we mean.

Created attachment 379382 [details] Patch

Comment on attachment 379085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379085&action=review Few more comments / code cleanup opportunities. >>> Tools/Scripts/webkitpy/common/version.py:33 >>> @staticmethod >> >> Might be better just to subclass `operator` from the standard library: >> https://docs.python.org/3/library/operator.html > > Not quite sure what you have in mind here. The operator module doesn't include anything to subclass I'm a bit confused -- reading the data model for Python 3.7 it isn't clear to me that the various comparator functions (__gt__, __lt__, __eq__, etc.) are removed from the base `object`. ➜ python3 Python 3.7.3 (...) ... >>> class test(object): ... pass ... >>> t = test() >>> dir(t) ['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__'] >>> g = test() >>> t == g False // NotImplementedError not raised >> Tools/Scripts/webkitpy/common/checkout/changelog.py:440 >> + result.write(unicode("\n Reviewed by %s.\n" % reviewer)) > > Let's add comments for cases when we need to make files python3 compatible. > > Also do we want to keep % notation? Won't this break python3 compatibility? ➜ python3 >>> unicode Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'unicode' is not defined >>> Tools/Scripts/webkitpy/common/net/networktransaction.py:33 >>> +if sys.version_info > (3, 0): >> >> should we just make a common is_python_3 helper function? > > I don't know that an is_python_3 function is any shorter or more clear than this (and then we would have to import it) > > If we were going to do this, we would probably want to have a sort of 'webkitpy.common.python3' IMO a version check here is fine vs a helper function given this is for specific imports. However, I do think it's useful to have a helper function for more frequent version-specific edge-cases, such as string vs unicode. > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:303 > + self.files[key] = value.encode('utf-8') This is a bit less clear, so I'm not necessarily advocating for it, but for other cases another way to write this could be: self.files = {key: value and value.encode('utf-8') for key, value in self.files.items()} > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:321 > + self.assertTrue(crash_log in [value.decode('utf-8') if value else None for value in self.files.values()]) Nit: Same to other comment, though maybe this would be a slight improvement in readability (your call): self.assertTrue(crash_log in [value and value.decode('utf-8') for value in self.files.values()]) >> Tools/Scripts/webkitpy/common/system/executive.py:95 >> + continue > > This code feels extremely unsafe to me. We have no idea if Popen is depending on some of the __ functions you are not setting I agree with this statement. Why don't we just subclass subprocess.Popen and write our own __enter__ and __exit__? >>> Tools/Scripts/webkitpy/common/system/executive.py:132 >>> + teed_output.write(output_line.decode('utf-8')) >> >> maybe write a decode helper function? > > What would you envision that function to be and where should it live? Something like webkitpy.common.py3_compatibility_hacks and a function named 'py23_decode_if_necessary`. Example page: .... output_line = child_process.stdout.readline() output_line = py23_decode_if_necessary(output_line) teed_output.write(output_line) .... > Tools/Scripts/webkitpy/common/system/executive.py:143 > + child_out_file = StringIO() if sys.version_info > (3, 0) else BytesIO() Can you please do this re-assignment at the top of the file? Doing all of these in-line makes it really difficult to understand which py3 hacks we've added and where. > Tools/Scripts/webkitpy/common/system/path.py:154 > + return urllib.parse.quote(path, safe='/+:') As part of the sys.version check, maybe we can just re-assign `urllib.quote = urllib.parse.quote`? Then we don't need anything special in our logic here.

(In reply to Dean Johnson from comment #8) > Comment on attachment 379085 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379085&action=review > > Few more comments / code cleanup opportunities. > > >>> Tools/Scripts/webkitpy/common/version.py:33 > >>> @staticmethod > >> > >> Might be better just to subclass `operator` from the standard library: > >> https://docs.python.org/3/library/operator.html > > > > Not quite sure what you have in mind here. The operator module doesn't include anything to subclass > > I'm a bit confused -- reading the data model for Python 3.7 it isn't clear > to me that the various comparator functions (__gt__, __lt__, __eq__, etc.) > are removed from the base `object`. > > ➜ python3 > Python 3.7.3 (...) > ... > >>> class test(object): > ... pass > ... > >>> t = test() > >>> dir(t) > ['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', > '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', > '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', > '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', > '__sizeof__', '__str__', '__subclasshook__', '__weakref__'] > >>> g = test() > >>> t == g > False // NotImplementedError not raised They removed the __cmp__ function. The suggested approach is basically exactly what I did <https://portingguide.readthedocs.io/en/latest/comparisons.html>. > > >> Tools/Scripts/webkitpy/common/checkout/changelog.py:440 > >> + result.write(unicode("\n Reviewed by %s.\n" % reviewer)) > > > > Let's add comments for cases when we need to make files python3 compatible. > > > > Also do we want to keep % notation? > > Won't this break python3 compatibility? Yes, hence the 'add comments for cases when we need to make files Python 3 compatible'. webkitpy is spaghetti code, I wasn't targeting common/checkout, just had to change the imports since webkitpy/port depends on it. > > ➜ python3 > >>> unicode > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > NameError: name 'unicode' is not defined > > >>> Tools/Scripts/webkitpy/common/net/networktransaction.py:33 > >>> +if sys.version_info > (3, 0): > >> > >> should we just make a common is_python_3 helper function? > > > > I don't know that an is_python_3 function is any shorter or more clear than this (and then we would have to import it) > > > > If we were going to do this, we would probably want to have a sort of 'webkitpy.common.python3' > > IMO a version check here is fine vs a helper function given this is for > specific imports. However, I do think it's useful to have a helper function > for more frequent version-specific edge-cases, such as string vs unicode. > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:303 > > + self.files[key] = value.encode('utf-8') > > This is a bit less clear, so I'm not necessarily advocating for it, but for > other cases another way to write this could be: > self.files = {key: value and value.encode('utf-8') for key, value in > self.files.items()} > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:321 > > + self.assertTrue(crash_log in [value.decode('utf-8') if value else None for value in self.files.values()]) > > Nit: Same to other comment, though maybe this would be a slight improvement > in readability (your call): > self.assertTrue(crash_log in [value and value.decode('utf-8') for value in > self.files.values()]) > > >> Tools/Scripts/webkitpy/common/system/executive.py:95 > >> + continue > > > > This code feels extremely unsafe to me. We have no idea if Popen is depending on some of the __ functions you are not setting > > I agree with this statement. Why don't we just subclass subprocess.Popen and > write our own __enter__ and __exit__? Because we aren't actually creating the Popen object, we're getting an already instantiated instance from subprocess. I'm open to other ideas, but as far as I can tell, this is the least disruptive and safest approach. The Python 3 context manger approach is definitely more correctly than Python 2's, and if we don't > > >>> Tools/Scripts/webkitpy/common/system/executive.py:132 > >>> + teed_output.write(output_line.decode('utf-8')) > >> > >> maybe write a decode helper function? > > > > What would you envision that function to be and where should it live? > > Something like webkitpy.common.py3_compatibility_hacks and a function named > 'py23_decode_if_necessary`. Example page: > .... > output_line = child_process.stdout.readline() > output_line = py23_decode_if_necessary(output_line) > teed_output.write(output_line) > .... > > > Tools/Scripts/webkitpy/common/system/executive.py:143 > > + child_out_file = StringIO() if sys.version_info > (3, 0) else BytesIO() > > Can you please do this re-assignment at the top of the file? Doing all of > these in-line makes it really difficult to understand which py3 hacks we've > added and where. I am very much against generalizing this hack because it's not always correct. In this case, it's only what we want because we're looking for an output file that is accessed as a text file instead of a binary file. The trouble is, a text file with the native string encoding will be StringIO in Python 3 and BytesIO in Python 2. It's possible, however, that we want a ByteIO (ie, binary) encoded file stream in Python 3 or a unicode encoded stream in Python 2. I think we need to force ourselves to think about what we want any time we use BytesIO and StringIO. If we wanted a 'generalized' hack, we should define BytesIO, StringIO (StringIO in Python 3, BytesIO in Python 2) and UnicodeIO. But that seems extremely confusing, it means that when developers used StringIO, they wouldn't actually be using StringIO. > > > Tools/Scripts/webkitpy/common/system/path.py:154 > > + return urllib.parse.quote(path, safe='/+:') > > As part of the sys.version check, maybe we can just re-assign `urllib.quote > = urllib.parse.quote`? Then we don't need anything special in our logic here.

(In reply to Jonathan Bedard from comment #9) > (In reply to Dean Johnson from comment #8) > ... > > I'm open to other ideas, but as far as I can tell, this is the least > disruptive and safest approach. The Python 3 context manger approach is > definitely more correctly than Python 2's, and if we don't use the context manager, we'll leave file handles open, so we would still need some sort of clean-up in all of our callers. > ...

(In reply to Jonathan Bedard from comment #9) > (In reply to Dean Johnson from comment #8) > > Comment on attachment 379085 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=379085&action=review > > > > Few more comments / code cleanup opportunities. > > > > >>> Tools/Scripts/webkitpy/common/version.py:33 > > >>> @staticmethod > > >> > > >> Might be better just to subclass `operator` from the standard library: > > >> https://docs.python.org/3/library/operator.html > > > > > > Not quite sure what you have in mind here. The operator module doesn't include anything to subclass > > > > I'm a bit confused -- reading the data model for Python 3.7 it isn't clear > > to me that the various comparator functions (__gt__, __lt__, __eq__, etc.) > > are removed from the base `object`. > > > > ➜ python3 > > Python 3.7.3 (...) > > ... > > >>> class test(object): > > ... pass > > ... > > >>> t = test() > > >>> dir(t) > > ['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', > > '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', > > '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', > > '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', > > '__sizeof__', '__str__', '__subclasshook__', '__weakref__'] > > >>> g = test() > > >>> t == g > > False // NotImplementedError not raised > > They removed the __cmp__ function. The suggested approach is basically > exactly what I did > <https://portingguide.readthedocs.io/en/latest/comparisons.html>. But why are you implementing all the other __* methods that aren't __cmp__? They should already use __cmp__ by default. > > > > > >> Tools/Scripts/webkitpy/common/checkout/changelog.py:440 > > >> + result.write(unicode("\n Reviewed by %s.\n" % reviewer)) > > > > > > Let's add comments for cases when we need to make files python3 compatible. > > > > > > Also do we want to keep % notation? > > > > Won't this break python3 compatibility? > > Yes, hence the 'add comments for cases when we need to make files Python 3 > compatible'. webkitpy is spaghetti code, I wasn't targeting common/checkout, > just had to change the imports since webkitpy/port depends on it. But can't this just be handled by putting something like the following at the top of the file?: if sys.version_info > (3, 2): unicode = str > > > > > ➜ python3 > > >>> unicode > > Traceback (most recent call last): > > File "<stdin>", line 1, in <module> > > NameError: name 'unicode' is not defined > > > > >>> Tools/Scripts/webkitpy/common/net/networktransaction.py:33 > > >>> +if sys.version_info > (3, 0): > > >> > > >> should we just make a common is_python_3 helper function? > > > > > > I don't know that an is_python_3 function is any shorter or more clear than this (and then we would have to import it) > > > > > > If we were going to do this, we would probably want to have a sort of 'webkitpy.common.python3' > > > > IMO a version check here is fine vs a helper function given this is for > > specific imports. However, I do think it's useful to have a helper function > > for more frequent version-specific edge-cases, such as string vs unicode. > > > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:303 > > > + self.files[key] = value.encode('utf-8') > > > > This is a bit less clear, so I'm not necessarily advocating for it, but for > > other cases another way to write this could be: > > self.files = {key: value and value.encode('utf-8') for key, value in > > self.files.items()} > > > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:321 > > > + self.assertTrue(crash_log in [value.decode('utf-8') if value else None for value in self.files.values()]) > > > > Nit: Same to other comment, though maybe this would be a slight improvement > > in readability (your call): > > self.assertTrue(crash_log in [value and value.decode('utf-8') for value in > > self.files.values()]) > > > > >> Tools/Scripts/webkitpy/common/system/executive.py:95 > > >> + continue > > > > > > This code feels extremely unsafe to me. We have no idea if Popen is depending on some of the __ functions you are not setting > > > > I agree with this statement. Why don't we just subclass subprocess.Popen and > > write our own __enter__ and __exit__? > > Because we aren't actually creating the Popen object, we're getting an > already instantiated instance from subprocess. > > I'm open to other ideas, but as far as I can tell, this is the least > disruptive and safest approach. The Python 3 context manger approach is > definitely more correctly than Python 2's, and if we don't > > > > > >>> Tools/Scripts/webkitpy/common/system/executive.py:132 > > >>> + teed_output.write(output_line.decode('utf-8')) > > >> > > >> maybe write a decode helper function? > > > > > > What would you envision that function to be and where should it live? > > > > Something like webkitpy.common.py3_compatibility_hacks and a function named > > 'py23_decode_if_necessary`. Example page: > > .... > > output_line = child_process.stdout.readline() > > output_line = py23_decode_if_necessary(output_line) > > teed_output.write(output_line) > > .... > > > > > Tools/Scripts/webkitpy/common/system/executive.py:143 > > > + child_out_file = StringIO() if sys.version_info > (3, 0) else BytesIO() > > > > Can you please do this re-assignment at the top of the file? Doing all of > > these in-line makes it really difficult to understand which py3 hacks we've > > added and where. > > I am very much against generalizing this hack because it's not always > correct. In this case, it's only what we want because we're looking for an > output file that is accessed as a text file instead of a binary file. The > trouble is, a text file with the native string encoding will be StringIO in > Python 3 and BytesIO in Python 2. It's possible, however, that we want a > ByteIO (ie, binary) encoded file stream in Python 3 or a unicode encoded > stream in Python 2. Generally, I'm against in-lining Python2/3 compatibility hacks. I'd rather add new variable names (like UnicodeIO) and use those instead where needed. For example: if sys.version_info > (3, 0): UnicodeIO = StringIO else: UnicodeIO = BytesIO Then use UnicodeIO in the places where it's needed. Taking this general approach makes it so any compatibility hacks that exist are immediately obvious when reading the file, and are generally in the same places. > > I think we need to force ourselves to think about what we want any time we > use BytesIO and StringIO. If we wanted a 'generalized' hack, we should > define BytesIO, StringIO (StringIO in Python 3, BytesIO in Python 2) and > UnicodeIO. But that seems extremely confusing, it means that when developers > used StringIO, they wouldn't actually be using StringIO. > > > > > > Tools/Scripts/webkitpy/common/system/path.py:154 > > > + return urllib.parse.quote(path, safe='/+:') > > > > As part of the sys.version check, maybe we can just re-assign `urllib.quote > > = urllib.parse.quote`? Then we don't need anything special in our logic here.

(In reply to Dean Johnson from comment #11) > ... > > > ... > > > >>> t = test() > > > >>> dir(t) > > > ['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', > > > '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', > > > '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', > > > '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', > > > '__sizeof__', '__str__', '__subclasshook__', '__weakref__'] > > > >>> g = test() > > > >>> t == g > > > False // NotImplementedError not raised > > > > They removed the __cmp__ function. The suggested approach is basically > > exactly what I did > > <https://portingguide.readthedocs.io/en/latest/comparisons.html>. > But why are you implementing all the other __* methods that aren't __cmp__? > They should already use __cmp__ by default. That doesn't work any more in Python 3. > ... > > > > > > > > >> Tools/Scripts/webkitpy/common/checkout/changelog.py:440 > > > >> + result.write(unicode("\n Reviewed by %s.\n" % reviewer)) > > > > > > > > Let's add comments for cases when we need to make files python3 compatible. > > > > > > > > Also do we want to keep % notation? > > > > > > Won't this break python3 compatibility? > > > > Yes, hence the 'add comments for cases when we need to make files Python 3 > > compatible'. webkitpy is spaghetti code, I wasn't targeting common/checkout, > > just had to change the imports since webkitpy/port depends on it. > But can't this just be handled by putting something like the following at > the top of the file?: > if sys.version_info > (3, 2): > unicode = str I have not audited this file. That will happen in another patch. We have spaghetti code in webkitpy, I'm trying to separate patches as much as possible. There are no doubt very simple solutions, but again, making webkitpy.common.checkout Python3 compatible is not the goal of this patch. > > > > > > > > ➜ python3 > > > >>> unicode > > > Traceback (most recent call last): > > > File "<stdin>", line 1, in <module> > > > NameError: name 'unicode' is not defined > > > > > > >>> Tools/Scripts/webkitpy/common/net/networktransaction.py:33 > > > >>> +if sys.version_info > (3, 0): > > > >> > > > >> should we just make a common is_python_3 helper function? > > > > > > > > I don't know that an is_python_3 function is any shorter or more clear than this (and then we would have to import it) > > > > > > > > If we were going to do this, we would probably want to have a sort of 'webkitpy.common.python3' > > > > > > IMO a version check here is fine vs a helper function given this is for > > > specific imports. However, I do think it's useful to have a helper function > > > for more frequent version-specific edge-cases, such as string vs unicode. > > > > > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:303 > > > > + self.files[key] = value.encode('utf-8') > > > > > > This is a bit less clear, so I'm not necessarily advocating for it, but for > > > other cases another way to write this could be: > > > self.files = {key: value and value.encode('utf-8') for key, value in > > > self.files.items()} > > > > > > > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:321 > > > > + self.assertTrue(crash_log in [value.decode('utf-8') if value else None for value in self.files.values()]) > > > > > > Nit: Same to other comment, though maybe this would be a slight improvement > > > in readability (your call): > > > self.assertTrue(crash_log in [value and value.decode('utf-8') for value in > > > self.files.values()]) > > > > > > >> Tools/Scripts/webkitpy/common/system/executive.py:95 > > > >> + continue > > > > > > > > This code feels extremely unsafe to me. We have no idea if Popen is depending on some of the __ functions you are not setting > > > > > > I agree with this statement. Why don't we just subclass subprocess.Popen and > > > write our own __enter__ and __exit__? > > > > Because we aren't actually creating the Popen object, we're getting an > > already instantiated instance from subprocess. > > > > I'm open to other ideas, but as far as I can tell, this is the least > > disruptive and safest approach. The Python 3 context manger approach is > > definitely more correctly than Python 2's, and if we don't > > > > > > > > >>> Tools/Scripts/webkitpy/common/system/executive.py:132 > > > >>> + teed_output.write(output_line.decode('utf-8')) > > > >> > > > >> maybe write a decode helper function? > > > > > > > > What would you envision that function to be and where should it live? > > > > > > Something like webkitpy.common.py3_compatibility_hacks and a function named > > > 'py23_decode_if_necessary`. Example page: > > > .... > > > output_line = child_process.stdout.readline() > > > output_line = py23_decode_if_necessary(output_line) > > > teed_output.write(output_line) > > > .... > > > > > > > Tools/Scripts/webkitpy/common/system/executive.py:143 > > > > + child_out_file = StringIO() if sys.version_info > (3, 0) else BytesIO() > > > > > > Can you please do this re-assignment at the top of the file? Doing all of > > > these in-line makes it really difficult to understand which py3 hacks we've > > > added and where. > > > > I am very much against generalizing this hack because it's not always > > correct. In this case, it's only what we want because we're looking for an > > output file that is accessed as a text file instead of a binary file. The > > trouble is, a text file with the native string encoding will be StringIO in > > Python 3 and BytesIO in Python 2. It's possible, however, that we want a > > ByteIO (ie, binary) encoded file stream in Python 3 or a unicode encoded > > stream in Python 2. > Generally, I'm against in-lining Python2/3 compatibility hacks. I'd rather > add new variable names (like UnicodeIO) and use those instead where needed. > For example: > if sys.version_info > (3, 0): > UnicodeIO = StringIO > else: > UnicodeIO = BytesIO > > Then use UnicodeIO in the places where it's needed. > > Taking this general approach makes it so any compatibility hacks that exist > are immediately obvious when reading the file, and are generally in the same > places. > I'll add the generalization, but I'd like to point out that your example code is wrong. UnicodeIO = io.StringIO in both Python2 and Python3, StringIO = UnicodeIO in Python3, but StringIO = BytesIO in Python2 (or at least, it should....the IO stream which accepts the String type in Python2 would be a bytes stream) TLDR, there are actually 2 versions of StringIO in Python 2, StringIO.StringIO (which is a byte stream) and io.StringIO (which is a unicode string stream).

Created attachment 379482 [details] Patch

Comment on attachment 379482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379482&action=review > Tools/Scripts/webkitpy/common/system/executive.py:90 > + if attribute.startswith('__'): To Stephanie's point here, if we are relying on __ functions from the popen we aren't picking up here, we'd end up with an exception like this: AttributeError: __<function>__ I think it's quite unlikely that we're relying on such functions, though.

Comment on attachment 379482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379482&action=review Unofficial r+. I really like the changes to put most of the py2/3 hacks into a separate module. I'd prefer if we named the module something like py23_compatibility though. > Tools/Scripts/webkitpy/common/unicode.py:54 > + return value Maybe it'd be better to name this file more for its purpose around python2/3 compatibility?

Comment on attachment 379482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379482&action=review The encode/decode functions make all of this way more readable. Echo Dean's comments about being more specific about purpose of the functions in naming but I'm happy with the patch. > Tools/Scripts/webkitpy/common/unicode.py:37 > +def encode(value, encoding='utf-8', errors='strict'): nit: I did like the name encode_if_necessary_for_python2 because it explains exactly what this function is doing. > Tools/Scripts/webkitpy/common/unicode.py:53 > + return decode(bytes(value)) maybe unicode_compatibility? >> Tools/Scripts/webkitpy/common/system/executive.py:90 >> + if attribute.startswith('__'): > > To Stephanie's point here, if we are relying on __ functions from the popen we aren't picking up here, we'd end up with an exception like this: > > AttributeError: __<function>__ > > I think it's quite unlikely that we're relying on such functions, though. As long as the errors are really obvious Ill be ok with this. I still think it might be better to add our own notimplemented message to make it clearer for someone who doesn't know about this hack

Created attachment 379602 [details] Patch for landing

Comment on attachment 379602 [details] Patch for landing Clearing flags on attachment: 379602 Committed r250375: <https://trac.webkit.org/changeset/250375>

All reviewed patches have been landed. Closing bug.