Bug 217717 – [webkitscmpy] Use 'which' to find git and svn paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We should be more flexible about where to find git and svn binaries.

[Radar WebKit Bug Importer]

<rdar://problem/70299192>

[Jonathan Bedard]

Created attachment 411352 [details]
Patch

[Dean Johnson]

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.

[Jonathan Bedard]

(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!

[Jonathan Bedard]

Created attachment 411636 [details]
Patch

[Dean Johnson]

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.

[Aakash Jain]

rs=me

[Jonathan Bedard]

Created attachment 412198 [details]
Patch

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Jonathan Bedard]

Created attachment 412202 [details]
Patch for landing

[EWS]

Committed r268936: <https://trac.webkit.org/changeset/268936>

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