Bug 218105 - [webkitpy] Use webkitcorepy's autoinstaller for buildbot and twisted
Summary: [webkitpy] Use webkitcorepy's autoinstaller for buildbot and twisted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-22 16:12 PDT by Jonathan Bedard
Modified: 2020-11-05 20:09 PST (History)
4 users (show)

See Also:


Attachments
Patch (48.92 KB, patch)
2020-10-22 17:33 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (19.13 KB, patch)
2020-10-23 09:14 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2020-10-23 10:03 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (19.90 KB, patch)
2020-10-23 10:43 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-10-22 16:12:59 PDT
The way we handle twisted and Buildbot are related, since buildbot requires specific versions of twisted.
Comment 1 Radar WebKit Bug Importer 2020-10-22 16:13:29 PDT
<rdar://problem/70593576>
Comment 2 Jonathan Bedard 2020-10-22 17:33:42 PDT
Created attachment 412148 [details]
Patch
Comment 3 Jonathan Bedard 2020-10-22 17:34:22 PDT
This is also the last change for https://bugs.webkit.org/show_bug.cgi?id=214950, so it removes all the auto-installer code.
Comment 4 Aakash Jain 2020-10-22 18:23:15 PDT
(In reply to Jonathan Bedard from comment #3)
> This is also the last change for
> https://bugs.webkit.org/show_bug.cgi?id=214950, so it removes all the auto-installer code.
Let’s remove the auto-installer code in separate patch, and keep this patch for migrating buildbot and twisted, so that it is easier to review.
Comment 5 Jonathan Bedard 2020-10-23 09:14:14 PDT
Created attachment 412188 [details]
Patch
Comment 6 Jonathan Bedard 2020-10-23 09:15:52 PDT
(In reply to Aakash Jain from comment #4)
> (In reply to Jonathan Bedard from comment #3)
> > This is also the last change for
> > https://bugs.webkit.org/show_bug.cgi?id=214950, so it removes all the auto-installer code.
> Let’s remove the auto-installer code in separate patch, and keep this patch
> for migrating buildbot and twisted, so that it is easier to review.

Uploaded a change that does that. Still need to remove the unit tests, though.
Comment 7 Jonathan Bedard 2020-10-23 10:03:32 PDT
Created attachment 412192 [details]
Patch
Comment 8 Aakash Jain 2020-10-23 10:20:17 PDT
Comment on attachment 412192 [details]
Patch

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

rs=me

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:157
> +                    if element.childNodes[0].data.endswith('.tar.gz') or element.childNodes[0].data.endswith('.tar.bz2'):

str.endswith also accepts a tuple https://docs.python.org/2/library/stdtypes.html#str.endswith , can rewrite this as:

if element.childNodes[0].data.endswith(('.tar.gz', '.tar.bz2'))

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:273
> +                location = os.path.join(AutoInstall.directory, self.name.split('.')[0])

Please add a comment mentioning the reason to add this logic.

> Tools/Scripts/webkitpy/autoinstalled/buildbot.py:27
> +if sys.version_info[0] > 2:

Maybe make it more explicit that we are referencing python3 here by changing it to: ">= 3"

> Tools/Scripts/webkitpy/autoinstalled/buildbot.py:45
> +    AutoInstall.register(Package('twisted', Version(12, 1, 0), pypi_name='Twisted'))

It's little confusing that we are installing twisted version 12.1 here and 15.5 in twisted.py, but it was probably like this earlier as well.
Comment 9 Jonathan Bedard 2020-10-23 10:26:30 PDT
Comment on attachment 412192 [details]
Patch

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

>> Tools/Scripts/webkitpy/autoinstalled/buildbot.py:45
>> +    AutoInstall.register(Package('twisted', Version(12, 1, 0), pypi_name='Twisted'))
> 
> It's little confusing that we are installing twisted version 12.1 here and 15.5 in twisted.py, but it was probably like this earlier as well.

Agreed. My hope is that moving forward, everything can be version 20.4.1...but we need to qualify performance infrastructure using that, and I'm pretty sure buildbot 0.8 needs a smaller version of twisted. That's the reason that these are in an autoinstalled directory instead of at the top level, by the way, it allows us to exclude certain packages.
Comment 10 Jonathan Bedard 2020-10-23 10:43:46 PDT
Created attachment 412196 [details]
Patch for landing
Comment 11 EWS 2020-10-23 11:15:50 PDT
Committed r268930: <https://trac.webkit.org/changeset/268930>

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