Bug 175279 – [Tools] Add script to download a GitHub release

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

Add a Python script for hitting the GitHub API and querying for the latest release of a project. Then the location should be passed in for downloading.

This will be important for WinCairoRequirements and for downloading something like vswhere for detection of Visual Studio instances.

[Ross Kirsling]

Created attachment 317863 [details]
Patch

[Don Olmstead]

The only thing I'm not sure of is how we prevent multiple downloads. Maybe just a file with the same extension and a .version so like WinCairoRequirements.zip.version.

Think we would then need a force flag as well where it'll do the logic no matter what.

[Ross Kirsling]

Comment on attachment 317863 [details]
Patch

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

> Tools/Scripts/get-latest-github-release.py:59
> +    data = json.loads(urllib2.urlopen(request).read())

Ugh, this must've been a Cmd-Z fail. Should be response.read(), of course.

[Ross Kirsling]

Created attachment 318091 [details]
Patch

[Ross Kirsling]

Comment on attachment 318091 [details]
Patch

- Add functionality to save / check a .version file of JSON format:

  {"tag_name": "2.1.3", "updated_at": "2017-07-29T03:53:05Z"}

- Add a --force/-f flag to skip this check.

[Per Arne Vollan]

(In reply to Ross Kirsling from comment #4)
> Created attachment 318091 [details]
> Patch

I am by no means a Python expert, but I think this looks good. Brent, is this something you would like to weigh in on?

[Per Arne Vollan]

Alexey or Alex, do you have a take on this? I think the patch looks good, but I thought I'd run it by you before approving. The script will be used to download 'vswhere' (detecting installation location of Visual Studio 2017), and to download WinCairo requirements (dlls and libs for building WinCairo).

[Alexey Proskuryakov]

Comment on attachment 318091 [details]
Patch

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

I'm not an expert either. My only comment is about usability - with a somewhat generic name, the tool should probably better explain what it does. It's too easy to assume that it gets the source code for the latest Gtk release or something like that. Maybe some example invocations would help.

Also, if it's actually downloading something, it should be called "download", not "get" I think.

> Tools/Scripts/get-latest-github-release.py:39
> +    parser.add_argument('--output', '-o', default='.', help='output directory')

It is a little annoying that curl's "-o" option has a different meaning, but I don't have a proposal for how to improve this.

> Tools/Scripts/get-latest-github-release.py:40
> +    parser.add_argument('--endpoint', '-e', default=PUBLIC_GITHUB_API_ENDPOINT, help='GitHub API endpoint (defaults to public)')

Do you envision a need to use a different endpoint? Will the users of the tool know how to make this choice?

> Tools/Scripts/get-latest-github-release.py:41
> +    parser.add_argument('--token', '-t', default=None, help='GitHub API OAuth token (where applicable)')

Again, will the users know if it's applicable?

> Tools/Scripts/get-latest-github-release.py:42
> +    parser.add_argument('--force', '-f', action='store_true', help='download latest release even if it already exists')

Exists where? Will this overwrite an existing file with the same name? Why is this option needed?

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #8)
> I'm not an expert either. My only comment is about usability - with a
> somewhat generic name, the tool should probably better explain what it does.
> It's too easy to assume that it gets the source code for the latest Gtk
> release or something like that. Maybe some example invocations would help.

This script can be used to download any binary asset included in the latest release of a GitHub repo to an arbitrary output directory, so it actually *is* extremely generic. I can certainly add a docblock with example usage though.

> Also, if it's actually downloading something, it should be called
> "download", not "get" I think.

Can do.

> > Tools/Scripts/get-latest-github-release.py:39
> > +    parser.add_argument('--output', '-o', default='.', help='output directory')
> 
> It is a little annoying that curl's "-o" option has a different meaning, but
> I don't have a proposal for how to improve this.

This is the same sort of "-o" as GCC though.

> > Tools/Scripts/get-latest-github-release.py:40
> > +    parser.add_argument('--endpoint', '-e', default=PUBLIC_GITHUB_API_ENDPOINT, help='GitHub API endpoint (defaults to public)')
> 
> Do you envision a need to use a different endpoint? Will the users of the
> tool know how to make this choice?
> 
> > Tools/Scripts/get-latest-github-release.py:41
> > +    parser.add_argument('--token', '-t', default=None, help='GitHub API OAuth token (where applicable)')
> 
> Again, will the users know if it's applicable?

These ensure that the script is able to target private repositories or private GitHub (Enterprise) instances. Whether or not these options are used upstream, they are necessary for completeness.

> > Tools/Scripts/get-latest-github-release.py:42
> > +    parser.add_argument('--force', '-f', action='store_true', help='download latest release even if it already exists')
> 
> Exists where? Will this overwrite an existing file with the same name? Why
> is this option needed?

The need is explained in the thread above (i.e. "downloads are time-consuming so avoid doing so unnecessarily by default"), but we can be explicit about it existing "in the specified output directory".

[Ross Kirsling]

> > > Tools/Scripts/get-latest-github-release.py:39
> > > +    parser.add_argument('--output', '-o', default='.', help='output directory')
> > 
> > It is a little annoying that curl's "-o" option has a different meaning, but
> > I don't have a proposal for how to improve this.
> 
> This is the same sort of "-o" as GCC though.

Well, I guess I take this back; I suppose your point was that you expected it to specify the output file and not just the output directory. Still, we already know the asset's filename, and I thought it would be most convenient to have an option that defaults to the working directory instead of a third positional argument.

[Ross Kirsling]

Created attachment 318446 [details]
Patch

[Ross Kirsling]

Comment on attachment 318446 [details]
Patch

- Change "get" to "download" in script name.

- Make help page more descriptive about the purpose and usage of this script.

- Remove -f/--force option after all. (Don and I discussed this further and decided that we're not specifically expecting a case for this to be used anyway.)

[Brent Fulgham]

Comment on attachment 318446 [details]
Patch

I assume this new script will be used in 'update-webkit' or something?

I think this patch looks fine, and seems like a good idea. We have a number of things this could be useful for retrieving.

[WebKit Commit Bot]

Comment on attachment 318446 [details]
Patch

Clearing flags on attachment: 318446

Committed r220918: <http://trac.webkit.org/changeset/220918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33963620>