Bug 214405 - [webkitcorepy] Add string_utils
Summary: [webkitcorepy] Add string_utils
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-16 07:46 PDT by Jonathan Bedard
Modified: 2020-07-28 07:00 PDT (History)
13 users (show)

See Also:


Attachments
Patch (113.10 KB, patch)
2020-07-16 07:51 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (113.74 KB, patch)
2020-07-17 07:52 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (114.35 KB, patch)
2020-07-21 06:50 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (13.58 KB, patch)
2020-07-22 09:45 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (101.34 KB, patch)
2020-07-22 11:53 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (101.32 KB, patch)
2020-07-22 11:58 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (101.54 KB, patch)
2020-07-27 14:25 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2020-07-27 15:35 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2020-07-16 07:46:00 PDT
Centralize handling of unicode encoding/decoding along with various tools for printing strings.
Comment 1 Jonathan Bedard 2020-07-16 07:51:12 PDT
Created attachment 404440 [details]
Patch
Comment 2 Jonathan Bedard 2020-07-17 07:52:04 PDT
Created attachment 404561 [details]
Patch
Comment 3 Sam Weinig 2020-07-17 08:52:43 PDT
Sorry for coming to this late, but what is the purpose/goal of webkitcorepy?
Comment 4 Jonathan Bedard 2020-07-17 09:07:17 PDT
(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
Comment 5 Sam Weinig 2020-07-17 09:33:28 PDT
(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.
Comment 6 Jonathan Bedard 2020-07-17 09:45:18 PDT
(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.
Comment 7 Jonathan Bedard 2020-07-21 06:50:39 PDT
Created attachment 404818 [details]
Patch
Comment 8 Jonathan Bedard 2020-07-22 09:45:37 PDT
Created attachment 404931 [details]
Patch
Comment 9 Jonathan Bedard 2020-07-22 09:48:18 PDT
(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.
Comment 10 EWS 2020-07-22 11:35:47 PDT
Committed r264715: <https://trac.webkit.org/changeset/264715>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404931 [details].
Comment 11 Radar WebKit Bug Importer 2020-07-22 11:38:01 PDT
<rdar://problem/65946874>
Comment 12 Jonathan Bedard 2020-07-22 11:53:47 PDT
Reopening to attach new patch.
Comment 13 Jonathan Bedard 2020-07-22 11:53:48 PDT
Created attachment 404943 [details]
Patch
Comment 14 Jonathan Bedard 2020-07-22 11:58:49 PDT
Created attachment 404944 [details]
Patch
Comment 15 Jonathan Bedard 2020-07-22 12:00:53 PDT
(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.
Comment 16 Aakash Jain 2020-07-27 09:47:18 PDT
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()?
Comment 17 Jonathan Bedard 2020-07-27 10:01:16 PDT
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.
Comment 18 Jonathan Bedard 2020-07-27 14:25:25 PDT
Created attachment 405312 [details]
Patch for landing
Comment 19 EWS 2020-07-27 15:11:32 PDT
Committed r264949: <https://trac.webkit.org/changeset/264949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405312 [details].
Comment 20 Jonathan Bedard 2020-07-27 15:35:32 PDT
Reopening to attach new patch.
Comment 21 Jonathan Bedard 2020-07-27 15:35:34 PDT
Created attachment 405318 [details]
Patch
Comment 22 Jonathan Bedard 2020-07-27 15:49:57 PDT
Sitting on this until tomorrow morning
Comment 23 EWS 2020-07-28 07:00:54 PDT
Committed r264981: <https://trac.webkit.org/changeset/264981>

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