[Webkit-unassigned] [Bug 185045] Enhanced Download Build Product Script with Additional Use Cases

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185045

--- Comment #18 from Amal Hussein <amal at bocoup.com> ---
Comment on attachment 340445
  --> https://bugs.webkit.org/attachment.cgi?id=340445
Patch

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

Youenn, thanks so much for the awesome feedback. Questions and comments below:

>> Tools/BuildSlaveSupport/download-built-product:62
>> +    build_binaries_fetcher = BuildBinariesFetcher(Host(), args.platform, args.architecture, args.configuration, args.full, args.revision, args.build_directory, args.delete_first, args.no_extract, args.url)
> 
> Why not passing args directly?
> This will make it easier if we ever add a new option.

Agreed - I passed args in an earlier iteration locally, but then changed it because of missing args in some use cases.  I was trying to avoid `if` statements inside `__init__`.  There may be a more elegant way to handle that.

>> Tools/Scripts/bisect-builds:194
>> +    revision_list = build_binaries_fetcher.get_sorted_revisions()
> 
> Maybe get_sorted_revisions could be a class function of BuildBinariesFetcher and would take platform, architecture and configuration as parameters?

good call - it's much better to avoid the initialization.

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:43
>> +    def __init__(self, host, platform, architecture='x86_64', configuration='release', full=False, revision=None, build_directory=None, delete_first=False, no_extract=False, url=None):
> 
> Do we need architecture and configuration defaults?
> It seems current call sites provide those values explicitly.

Good question - I left this in because I didn't want to rely on an external dependency to set the defaults.
This brings up a larger question on where validation should live. 
I considered moving the `supported_platforms` logic into its own common helper file, and methods for architecture and configuration can also live there and be imported here. 
Should this class have its own validation helpers which check that the  platform, architecture, and configuration are in the shared globals?

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:53
>> +        self.s3_zip_url = url
> 
> I am not sure all these values need to be public.

public api - platform, architecture,  revision, build_directory, s3_zip_url?

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:60
>> +            build_directory = ', '.join(self.build_directory.split('/')), self.platform + self.architecture + self.revision
> 
> Does this work in windows?
> Should we use Filesystem.sep?
> Should we add '-' separators between platform/architecture/revision? or maybe use s3_build_type for the final directory name.

yes, this works in windows - line 60 returns a tuple of all the directories, and that gets passed to WebKitFinder in line 62. 

I initially used the s3_build_type in a previous version of the patch but got feedback about the nested structure. 

Which option is preferable - both utilize s3_build_type:

A) DownloadedBinaries/ios-simulator-11-x86_64-release/231081
B) DownloadedBinaries/ios-simulator-11-x86_64-release-231081

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:85
>> +        return self.host.filesystem.join(self.local_downloaded_binaries_directory, self.configuration + '.zip')
> 
> Should the configuration be lowercased/capitalized or does it not matter?

I explicitly capitalize it when it does matter - see line 66. 

Its probably best sanitize all the string args to be lower case for consistency with logging and checking if a directory exists on Linux or mac. I will make that update.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180517/3248146e/attachment.html>