Bug 217717

Summary: [webkitscmpy] Use 'which' to find git and svn paths
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, dean_johnson, dewei_zhu, slewis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=215862
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Jonathan Bedard 2020-10-14 10:49:22 PDT
We should be more flexible about where to find git and svn binaries.
Comment 1 Radar WebKit Bug Importer 2020-10-14 10:49:43 PDT
<rdar://problem/70299192>
Comment 2 Jonathan Bedard 2020-10-14 11:22:08 PDT
Created attachment 411352 [details]
Patch
Comment 3 Dean Johnson 2020-10-15 11:09:11 PDT
Comment on attachment 411352 [details]
Patch

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

LGTM other than comment about default return value. Unofficial r+.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:37
> +    @decorators.Memoize()

Nit: `executable` seems more like an attribute (@property) than a function to me, but it doesn't need to change if you like this more.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:44
> +            return which.stdout.rstrip()

Can we call os.path.realpath on this return value? `which` can return a symlink, and it's probably better for us to work with real paths where possible.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:45
> +        return '/usr/bin/git'

If we can't find an executable, it seems like we should raise an exception to make it clear to the user that a git binary can't be found.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:52
> +        return '/usr/bin/svn'

Ditto to all of my comments in Git.

I'm not sure if it's desirable, but you could abstract '_executable' to store 'git' and 'svn' on their respective classes, then join those against base search paths (['/usr/bin', '/usr/local/bin']) so this `executable` function/property can live on the base Scm class.
Comment 4 Jonathan Bedard 2020-10-16 09:31:58 PDT
(In reply to Dean Johnson from comment #3)
> Comment on attachment 411352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411352&action=review
> 
> LGTM other than comment about default return value. Unofficial r+.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:37
> > +    @decorators.Memoize()
> 
> Nit: `executable` seems more like an attribute (@property) than a function
> to me, but it doesn't need to change if you like this more.

Oh, I agree, but you can't make a classmethod a property. To get that to work, we would need to use a metaclass, I thought that was more confusing

> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:44
> > +            return which.stdout.rstrip()
> 
> Can we call os.path.realpath on this return value? `which` can return a
> symlink, and it's probably better for us to work with real paths where
> possible.

Good idea

> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:45
> > +        return '/usr/bin/git'
> 
> If we can't find an executable, it seems like we should raise an exception
> to make it clear to the user that a git binary can't be found.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:52
> > +        return '/usr/bin/svn'
> 
> Ditto to all of my comments in Git.
> 
> I'm not sure if it's desirable, but you could abstract '_executable' to
> store 'git' and 'svn' on their respective classes, then join those against
> base search paths (['/usr/bin', '/usr/local/bin']) so this `executable`
> function/property can live on the base Scm class.

Didn't think of this!
Comment 5 Jonathan Bedard 2020-10-16 17:43:28 PDT
Created attachment 411636 [details]
Patch
Comment 6 Dean Johnson 2020-10-23 10:16:39 PDT
Comment on attachment 411636 [details]
Patch

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

Patch LGTM; unofficial r+.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/scm.py:46
> +        for candidate in ['/usr/bin/{}'.format(program), '/usr/local/bin/{}'.format(program)]:

Nit: It'd probably be slightly cleaner to join 'program' to a base path in the if statement, rather than doing it twice here.
Comment 7 Aakash Jain 2020-10-23 10:32:15 PDT
rs=me
Comment 8 Jonathan Bedard 2020-10-23 10:59:48 PDT
Created attachment 412198 [details]
Patch
Comment 9 EWS 2020-10-23 11:05:37 PDT
ChangeLog entry in Tools/ChangeLog contains OOPS!.
Comment 10 Jonathan Bedard 2020-10-23 11:37:29 PDT
Created attachment 412202 [details]
Patch for landing
Comment 11 EWS 2020-10-23 12:20:17 PDT
Committed r268936: <https://trac.webkit.org/changeset/268936>

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