Make the try slave selection client side.

Make the try slave selection client side.

BUG=23071
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28155

[Lei Zhang, 2009-09-29 02:36:19]

FYI, I think msb broke a bunch of unit tests in r27380.

[M-A Ruel, 2009-09-29 13:43:25]

http://codereview.chromium.org/248029/diff/6001/7001
File presubmit_support.py (right):

http://codereview.chromium.org/248029/diff/6001/7001#newcode766
Line 766: 'Presubmit functions must return a list')
... return a list of strings; got a %s instead: %s' % (type(result),
str(result)))
and remove the 2 prints above.

http://codereview.chromium.org/248029/diff/6001/7001#newcode768
Line 768: if not isinstance(item, types.StringType):
I'd prefer if not isinstance(item, basestring):

http://codereview.chromium.org/248029/diff/6001/7001#newcode770
Line 770: item_stripped = item.strip()
It would be shorter to do or something similar:
if len(item.split()) != 1:
  raise ... No whitespaces')

http://codereview.chromium.org/248029/diff/6001/7001#newcode775
Line 775: raise exceptions.RuntimeError('Try slave names cannot contain'
I never enforced that.

http://codereview.chromium.org/248029/diff/6001/7001#newcode780
Line 780: return result
And you return None if function_name in context is False. Align -2

http://codereview.chromium.org/248029/diff/6001/7001#newcode1037
Line 1037: change = change_class(options.name,
uh?

http://codereview.chromium.org/248029/diff/6001/7002
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/248029/diff/6001/7002#newcode73
Line 73: 'GetTrySlavesExecuter', 'GitChange', 'GitAffectedFile', 'InputApi',
I know you can do better.

http://codereview.chromium.org/248029/diff/6001/7004
File trychange.py (left):

http://codereview.chromium.org/248029/diff/6001/7004#oldcode96
Line 96: settings['default_project'] =
gcl.GetCodeReviewSetting('TRYSERVER_PROJECT')
It's a tad too early to remove this, you'll break the NaCl try server.

http://codereview.chromium.org/248029/diff/6001/7004
File trychange.py (right):

http://codereview.chromium.org/248029/diff/6001/7004#newcode244
Line 244: values['bot'] = ','.join(options.bot)
I disagree with this change.

http://codereview.chromium.org/248029/diff/6001/7004#newcode518
Line 518: change = presubmit_support.SvnChange(change_info.name,
That's a regression. You cannot assume svn.

[Lei Zhang, 2009-09-30 06:46:15]

http://codereview.chromium.org/248029/diff/6001/7001
File presubmit_support.py (right):

http://codereview.chromium.org/248029/diff/6001/7001#newcode766
Line 766: 'Presubmit functions must return a list')
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> ... return a list of strings; got a %s instead: %s' % (type(result),
> str(result)))
> and remove the 2 prints above.

Done.

http://codereview.chromium.org/248029/diff/6001/7001#newcode768
Line 768: if not isinstance(item, types.StringType):
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> I'd prefer if not isinstance(item, basestring):

Done.

http://codereview.chromium.org/248029/diff/6001/7001#newcode770
Line 770: item_stripped = item.strip()
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> It would be shorter to do or something similar:
> if len(item.split()) != 1:
>   raise ... No whitespaces')

' foo '.split() returns ['foo'], which is why I added this check.

http://codereview.chromium.org/248029/diff/6001/7001#newcode775
Line 775: raise exceptions.RuntimeError('Try slave names cannot contain'
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> I never enforced that.

Ok, taking it out then.

http://codereview.chromium.org/248029/diff/6001/7001#newcode780
Line 780: return result
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> And you return None if function_name in context is False. Align -2

Err? This is aligned correctly. No?

http://codereview.chromium.org/248029/diff/6001/7001#newcode1037
Line 1037: change = change_class(options.name,
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> uh?

I was trying to do --list-try-slaves but eventually dropped it. Reverting this
part.

http://codereview.chromium.org/248029/diff/6001/7002
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/248029/diff/6001/7002#newcode73
Line 73: 'GetTrySlavesExecuter', 'GitChange', 'GitAffectedFile', 'InputApi',
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> I know you can do better.

It's true!

http://codereview.chromium.org/248029/diff/6001/7004
File trychange.py (left):

http://codereview.chromium.org/248029/diff/6001/7004#oldcode96
Line 96: settings['default_project'] =
gcl.GetCodeReviewSetting('TRYSERVER_PROJECT')
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> It's a tad too early to remove this, you'll break the NaCl try server.

Ok, I'll leave this alone.

http://codereview.chromium.org/248029/diff/6001/7004
File trychange.py (right):

http://codereview.chromium.org/248029/diff/6001/7004#newcode244
Line 244: values['bot'] = ','.join(options.bot)
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> I disagree with this change.

Yes, I suppose options.bot can be [].

http://codereview.chromium.org/248029/diff/6001/7004#newcode518
Line 518: change = presubmit_support.SvnChange(change_info.name,
On 2009/09/29 13:43:26, Marc-Antoine Ruel wrote:
> That's a regression. You cannot assume svn.

Ok, I removed the SvnChange object and just pass what's needed to
DoGetTrySlaves() instead.

[M-A Ruel, 2009-09-30 14:19:44]

http://codereview.chromium.org/248029/diff/2003/2007
File trychange.py (right):

http://codereview.chromium.org/248029/diff/2003/2007#newcode522
Line 522: change_info = gcl.LoadChangelistInfoForMultiple(options.name,
That still assumes svn and will need to be reworked. Add a todo at least or fix
git-try.

[Lei Zhang, 2009-10-01 07:26:33]

On 2009/09/30 14:19:44, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/248029/diff/2003/2007
> File trychange.py (right):
> 
> http://codereview.chromium.org/248029/diff/2003/2007#newcode522
> Line 522: change_info = gcl.LoadChangelistInfoForMultiple(options.name,
> That still assumes svn and will need to be reworked. Add a todo at least or
fix
> git-try.

Fixed git-try.

[M-A Ruel, 2009-10-01 16:37:25]

http://codereview.chromium.org/248029/diff/9001/10005
File git-try (right):

http://codereview.chromium.org/248029/diff/9001/10005#newcode117
Line 117: file_dict = {}
Why a dict? You should use a list. If you are afraid of dupes, use a set()

http://codereview.chromium.org/248029/diff/9001/10005#newcode121
Line 121: line = line[0:4] + os.path.join(prefix,filename)
add a space after comma.

http://codereview.chromium.org/248029/diff/9001/10001
File presubmit_support.py (right):

http://codereview.chromium.org/248029/diff/9001/10001#newcode9
Line 9: __version__ = '1.3.2'
please bump

http://codereview.chromium.org/248029/diff/9001/10004
File trychange.py (right):

http://codereview.chromium.org/248029/diff/9001/10004#newcode403
Line 403: git_checkout_root=None,
remove this

http://codereview.chromium.org/248029/diff/9001/10004#newcode404
Line 404: git_file_list=None,
I don't understand the necessity of this.

http://codereview.chromium.org/248029/diff/9001/10004#newcode547
Line 547: if isinstance(options.scm, SVN):
I'd rather have this code a member function of SVN/GIT classes.

[Lei Zhang, 2009-10-01 20:09:51]

http://codereview.chromium.org/248029/diff/9001/10005
File git-try (right):

http://codereview.chromium.org/248029/diff/9001/10005#newcode117
Line 117: file_dict = {}
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> Why a dict? You should use a list. If you are afraid of dupes, use a set()

Ya, I want to remove dupes. We're likely to get one for every set of '---' and
'+++'. Changed it to use a set instead.

http://codereview.chromium.org/248029/diff/9001/10005#newcode121
Line 121: line = line[0:4] + os.path.join(prefix,filename)
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> add a space after comma.

Done.

http://codereview.chromium.org/248029/diff/9001/10001
File presubmit_support.py (right):

http://codereview.chromium.org/248029/diff/9001/10001#newcode9
Line 9: __version__ = '1.3.2'
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> please bump

bumped to 1.3.3.

http://codereview.chromium.org/248029/diff/9001/10004
File trychange.py (right):

http://codereview.chromium.org/248029/diff/9001/10004#newcode403
Line 403: git_checkout_root=None,
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> remove this

because we want to have the SVN/GIT class figure out what this is for
themselves?

http://codereview.chromium.org/248029/diff/9001/10004#newcode404
Line 404: git_file_list=None,
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> I don't understand the necessity of this.

Git does not give a list of modified files to trychange.py. Rather, it does git
magic and generates a diff. We would need to parse the diff file and extract out
the file names, and then un-munge them. This requires trychange to understand a
lot of git-try internals, so instead, I just have git-try pass in a list of
files.

http://codereview.chromium.org/248029/diff/9001/10004#newcode547
Line 547: if isinstance(options.scm, SVN):
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> I'd rather have this code a member function of SVN/GIT classes.

ok, will work it.

[M-A Ruel, 2009-10-01 20:44:40]

I still think the change is odd but lgtm so we can get things going.

http://codereview.chromium.org/248029/diff/9003/9008
File git-try (right):

http://codereview.chromium.org/248029/diff/9003/9008#newcode129
Line 129: return (munged_diff, file_set)
return (munged_diff, list(file_set))
since set are a pain to play with.

[Lei Zhang, 2009-10-06 20:33:55]

http://codereview.chromium.org/248029/diff/9001/10004
File trychange.py (right):

http://codereview.chromium.org/248029/diff/9001/10004#newcode547
Line 547: if isinstance(options.scm, SVN):
On 2009/10/01 16:37:25, Marc-Antoine Ruel wrote:
> I'd rather have this code a member function of SVN/GIT classes.

Done.

http://codereview.chromium.org/248029/diff/9003/9008
File git-try (right):

http://codereview.chromium.org/248029/diff/9003/9008#newcode129
Line 129: return (munged_diff, file_set)
On 2009/10/01 20:44:40, Marc-Antoine Ruel wrote:
> return (munged_diff, list(file_set))
> since set are a pain to play with.

Ok, done.

[M-A Ruel, 2009-10-06 20:47:32]

lgtm

http://codereview.chromium.org/248029/diff/8005/8010
File tests/trychange_unittest.py (right):

http://codereview.chromium.org/248029/diff/8005/8010#newcode93
Line 93: upload.RunShell(['git', 'rev-parse',
'--show-cdup']).AndReturn(self.fake_root)
80 cols