Bug 214405 – [webkitcorepy] Add string_utils

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Centralize handling of unicode encoding/decoding along with various tools for printing strings.

[Jonathan Bedard]

Created attachment 404440 [details]
Patch

[Jonathan Bedard]

Created attachment 404561 [details]
Patch

[Sam Weinig]

Sorry for coming to this late, but what is the purpose/goal of webkitcorepy?

[Jonathan Bedard]

(In reply to Sam Weinig from comment #3)
> Sorry for coming to this late, but what is the purpose/goal of webkitcorepy?

Let us use our Python tooling in services and share tooling (at the moment, git tooling is of the most interest) with more projects. And to de-duplicate many Internal Python tools that have no reason to be Internal (https://bugs.webkit.org/show_bug.cgi?id=214475 is an example of this)

We could do this with webkitpy, but webkitpy uses many bad Python 2 idioms and has a lot of unnecessary coupling. webkitcorepy (and any other libraries we may write) would be generic tooling that happens to be used by WebKit instead of tooling specifically built around the eccentricities of WebKit development.

There are a few more ideas of how we want to use such a library (auto-installer, shared logging idioms, modern multi-processes harness), but I'm trying to keep those discussions with the code that actually implements those bits

[Sam Weinig]

(In reply to Jonathan Bedard from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Sorry for coming to this late, but what is the purpose/goal of webkitcorepy?
> 
> Let us use our Python tooling in services and share tooling (at the moment,
> git tooling is of the most interest) with more projects. And to de-duplicate
> many Internal Python tools that have no reason to be Internal
> (https://bugs.webkit.org/show_bug.cgi?id=214475 is an example of this)

Internal here meaning internally at Apple?

> 
> We could do this with webkitpy, but webkitpy uses many bad Python 2 idioms
> and has a lot of unnecessary coupling. webkitcorepy (and any other libraries
> we may write) would be generic tooling that happens to be used by WebKit
> instead of tooling specifically built around the eccentricities of WebKit
> development.
> 
> There are a few more ideas of how we want to use such a library
> (auto-installer, shared logging idioms, modern multi-processes harness), but
> I'm trying to keep those discussions with the code that actually implements
> those bits

This implies that the WebKit tools will now have use outside of the WebKit tree (or perhaps that is already true). While we have a clear understanding that breaking API and ABI of our WebKit c/c++/objective-c needs to be considered carefully, I am not sure if we have historically had the same level of care for scripts. 

Regardless of whether this dependency is new or not, it might be a good idea to document in some form what level of care people need to take when changing scripts in WebKit so others can be aware that their changes might have effects outside of the tree.

Thanks for the explanation.

[Jonathan Bedard]

(In reply to Sam Weinig from comment #5)
> (In reply to Jonathan Bedard from comment #4)
> > (In reply to Sam Weinig from comment #3)
> > > Sorry for coming to this late, but what is the purpose/goal of webkitcorepy?
> > 
> > Let us use our Python tooling in services and share tooling (at the moment,
> > git tooling is of the most interest) with more projects. And to de-duplicate
> > many Internal Python tools that have no reason to be Internal
> > (https://bugs.webkit.org/show_bug.cgi?id=214475 is an example of this)
> 
> Internal here meaning internally at Apple?

Yes, sorry for not being specific. Apple has Internal tooling that leverages webkitpy, mostly used by Safari at the moment, but there has been intention to more widely use some of Safari's tools.

> 
> > 
> > We could do this with webkitpy, but webkitpy uses many bad Python 2 idioms
> > and has a lot of unnecessary coupling. webkitcorepy (and any other libraries
> > we may write) would be generic tooling that happens to be used by WebKit
> > instead of tooling specifically built around the eccentricities of WebKit
> > development.
> > 
> > There are a few more ideas of how we want to use such a library
> > (auto-installer, shared logging idioms, modern multi-processes harness), but
> > I'm trying to keep those discussions with the code that actually implements
> > those bits
> 
> This implies that the WebKit tools will now have use outside of the WebKit
> tree (or perhaps that is already true). While we have a clear understanding
> that breaking API and ABI of our WebKit c/c++/objective-c needs to be
> considered carefully, I am not sure if we have historically had the same
> level of care for scripts. 
> 
> Regardless of whether this dependency is new or not, it might be a good idea
> to document in some form what level of care people need to take when
> changing scripts in WebKit so others can be aware that their changes might
> have effects outside of the tree.
> 
> Thanks for the explanation.

WebKit tools already has use outside of WebKit's tree. And it's true that we haven't historically had the same level of care for scripts, which has been a source of pain, particularly for OpenSource contributors, when they start modifying tooling code that ends up breaking something they had no way of knowing about.

I think a document is a great idea, and I can put one together in another patch. One of the goals of breaking off parts of webkitpy into libraries is that we can may the guarantee that webkitpy will not be used outside of the WebKit tree, only the libraries will. I think that will make hacking easier, since it will be clear when you are working in an area that may have clients you aren't aware of.

[Jonathan Bedard]

Created attachment 404818 [details]
Patch

[Jonathan Bedard]

Created attachment 404931 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #8)
> Created attachment 404931 [details]
> Patch

Aakash asked me to split up the patch. The first part of the patch is the actual code change, the second part will consist of the refactor to adopt the webkitcorepy version of unicode tooling instead of the webkitpy version.

[EWS]

Committed r264715: <https://trac.webkit.org/changeset/264715>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404931 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65946874>

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 404943 [details]
Patch

[Jonathan Bedard]

Created attachment 404944 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #14)
> Created attachment 404944 [details]
> Patch

This change simply adopt webkitcorepy.string_utils, and should not change the function on any scripts.

[Aakash Jain]

Comment on attachment 404944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404944&action=review

rs=me

> Tools/Scripts/webkitpy/common/checkout/changelog_unittest.py:-33
> -from webkitpy.common.unicode_compatibility import StringIO

Is this removal (without new addition) intentional?

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:-32
> -import tempfile

I would have preferred removal of all these stray imports in separate patch (since there are many). But here is fine as well for now.

> Tools/Scripts/webkitpy/common/system/executive.py:39
> +from webkitcorepy import StringIO, string_utils, unicode

Doesn't the unicode in webkitcorepy hides the built-in unicode()?

[Jonathan Bedard]

Comment on attachment 404944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404944&action=review

>> Tools/Scripts/webkitpy/common/checkout/changelog_unittest.py:-33
>> -from webkitpy.common.unicode_compatibility import StringIO
> 
> Is this removal (without new addition) intentional?

Yes, I mention it in the changelog for the file, StringIO was not used in this file, so I just removed it.

I've configured PyCharm for this project, so it's warning me when an import is unused, whenever I come across one, I just delete it.

>> Tools/Scripts/webkitpy/common/system/executive.py:39
>> +from webkitcorepy import StringIO, string_utils, unicode
> 
> Doesn't the unicode in webkitcorepy hides the built-in unicode()?

It does, but Python 3 doesn't actually have a built-in unicode. That's one of the features of string_utils. When a program in Python 3 says "unicode", what they really want is "str". So string_utils sets unicode to unicode for Python 2, but unicode to str for Python 3.

[Jonathan Bedard]

Created attachment 405312 [details]
Patch for landing

[EWS]

Committed r264949: <https://trac.webkit.org/changeset/264949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405312 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 405318 [details]
Patch

[Jonathan Bedard]

Sitting on this until tomorrow morning

[EWS]

Committed r264981: <https://trac.webkit.org/changeset/264981>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405318 [details].