Bug 197949 - [ews-build] Download archives from S3
Summary: [ews-build] Download archives from S3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-16 08:18 PDT by Aakash Jain
Modified: 2019-10-22 12:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.16 KB, patch)
2019-05-16 08:22 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2019-05-16 20:42 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-05-16 08:18:25 PDT
https://bugs.webkit.org/show_bug.cgi?id=197922 added support to upload archives to S3. We should start downloading these archives from S3 (instead of downloading them from build-master).
Comment 1 Aakash Jain 2019-05-16 08:22:46 PDT
Created attachment 370044 [details]
Patch
Comment 2 Build Bot 2019-05-16 08:28:12 PDT Comment hidden (obsolete)
Comment 3 Jonathan Bedard 2019-05-16 09:12:36 PDT
Comment on attachment 370044 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:796
> +    def getResultSummary(self):

Why didn't we have something like this previously? It looks like most of this change is just swapping out the url.

> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1017
> +                        command=['python', 'Tools/BuildSlaveSupport/download-built-product', '--platform=ios', '--release', 'https://s3-us-west-2.amazonaws.com/ews-archives.webkit.org/ios-simulator-12-x86_64-release/1234.zip'],

It feels like download-built-product is missing something. When I look at this script, it doesn't do anything with platform. It makes me think that the iOS simulator bit isn't actually going to work. I think we need to put the binaries into WebKitBuild/<configuration>-iphonesimulator.
Comment 4 Aakash Jain 2019-05-16 11:46:41 PDT
> Why didn't we have something like this previously?
I have been polishing these strings (e.g.: https://bugs.webkit.org/show_bug.cgi?id=195963, https://bugs.webkit.org/show_bug.cgi?id=195945, https://bugs.webkit.org/show_bug.cgi?id=195995), but this has been a low priority, especially the strings (like this one) which doesn't appear frequently.

> It looks like most of this change is just swapping out the url.
Right.
> > Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1017
> > +                        command=['python', 'Tools/BuildSlaveSupport/download-built-product', '--platform=ios', '--release', 'https://s3-us-west-2.amazonaws.com/ews-archives.webkit.org/ios-simulator-12-x86_64-release/1234.zip'],
> 
> It feels like download-built-product is missing something. When I look at this script, it doesn't do anything with platform.
Yeah, it only check if platform is windows or not. Do you prefer to remove this parameter? I kept it as it would be consistent with similar build.webkit.org step.

>  It makes me think that the iOS simulator bit isn't actually going to work. I think we need to put the binaries into WebKitBuild/<configuration>-iphonesimulator.
It actually works, tested in https://ews-build.webkit-uat.org/#/builders/20/builds/2636 . The uploaded archive itself has the subfolder "<configuration>-iphonesimulator.' You can verify by downloading and unzipping this: https://s3-us-west-2.amazonaws.com/ews-archives.webkit.org/ios-simulator-12-x86_64-release/369972.zip
Comment 5 Jonathan Bedard 2019-05-16 20:22:42 PDT
(In reply to Aakash Jain from comment #4)
> ...
> > It feels like download-built-product is missing something. When I look at this script, it doesn't do anything with platform.
> Yeah, it only check if platform is windows or not. Do you prefer to remove
> this parameter? I kept it as it would be consistent with similar
> build.webkit.org step.

I actually would prefer this variable be removed if it's not used.

> ... 
> >  It makes me think that the iOS simulator bit isn't actually going to work. I think we need to put the binaries into WebKitBuild/<configuration>-iphonesimulator.
> It actually works, tested in
> https://ews-build.webkit-uat.org/#/builders/20/builds/2636 . The uploaded
> archive itself has the subfolder "<configuration>-iphonesimulator.' You can
> verify by downloading and unzipping this:
> https://s3-us-west-2.amazonaws.com/ews-archives.webkit.org/ios-simulator-12-
> x86_64-release/369972.zip

This part is surprising to me. It may just be surprising because this isn't how our internal archiving system works, but that may also be indicative of an architectural flaw in the WebKit archiving system. But since it's working, that's not in the scope of this patch.
Comment 6 Aakash Jain 2019-05-16 20:42:50 PDT
Created attachment 370099 [details]
Patch
Comment 7 Build Bot 2019-05-16 20:44:50 PDT Comment hidden (obsolete)
Comment 8 Aakash Jain 2019-05-16 20:45:37 PDT
> I actually would prefer this variable be removed if it's not used.
Done
Comment 9 WebKit Commit Bot 2019-05-16 21:30:38 PDT
Comment on attachment 370099 [details]
Patch

Clearing flags on attachment: 370099

Committed r245433: <https://trac.webkit.org/changeset/245433>
Comment 10 WebKit Commit Bot 2019-05-16 21:30:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-05-16 21:31:21 PDT
<rdar://problem/50880970>