Bug 197949

Summary: [ews-build] Download archives from S3
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, ews-watchlist, jbedard, lforschler, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197922
https://bugs.webkit.org/show_bug.cgi?id=203263
Attachments:
Description Flags
Patch
none
Patch none

Aakash Jain
Reported 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).
Attachments
Patch (4.16 KB, patch)
2019-05-16 08:22 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Patch (4.74 KB, patch)
2019-05-16 20:42 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Aakash Jain
Comment 1 2019-05-16 08:22:46 PDT
Created attachment 370044 [details] Patch
EWS Watchlist
Comment 2 2019-05-16 08:28:12 PDT Comment hidden (obsolete)
Jonathan Bedard
Comment 3 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.
Aakash Jain
Comment 4 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
Jonathan Bedard
Comment 5 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.
Aakash Jain
Comment 6 2019-05-16 20:42:50 PDT
Created attachment 370099 [details] Patch
EWS Watchlist
Comment 7 2019-05-16 20:44:50 PDT Comment hidden (obsolete)
Aakash Jain
Comment 8 2019-05-16 20:45:37 PDT
> I actually would prefer this variable be removed if it's not used. Done
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2019-05-16 21:30:39 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2019-05-16 21:31:21 PDT
<rdar://problem/50880970>
Note You need to log in before you can comment on or make changes to this bug.