From 6fb14d906bf480a2e6aafe682fcf5a800c3ff39c Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Tue, 27 Sep 2011 00:12:49 +0000 Subject: [PATCH] Revert r102783 "Support for |change| argument to |GetPreferredTrySlaves()|." Cause an infinite recursion in some context. TBR=asvitkine@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/8036046 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@102836 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 9 --- git_cl.py | 9 +-- git_try.py | 5 +- presubmit_support.py | 22 +++----- tests/presubmit_unittest.py | 45 ++++----------- tests/trychange_unittest.py | 8 +-- trychange.py | 110 ++++++++++++++---------------------- 7 files changed, 70 insertions(+), 138 deletions(-) diff --git a/gcl.py b/gcl.py index dffc566330..32286b23a6 100755 --- a/gcl.py +++ b/gcl.py @@ -961,20 +961,11 @@ def TryChange(change_info, args, swallow_exception): if change_info.patchset: trychange_args.extend(["--patchset", str(change_info.patchset)]) file_list = change_info.GetFileNames() - change = presubmit_support.SvnChange(change_info.name, - change_info.description, - change_info.GetLocalRoot(), - change_info.GetFiles(), - change_info.issue, - change_info.patchset, - None) else: file_list = [] - trychange_args.extend(args) return trychange.TryChange( trychange_args, - change=change, file_list=file_list, swallow_exception=swallow_exception, prog='gcl try', diff --git a/git_cl.py b/git_cl.py index 9946999c63..cb33488e4b 100755 --- a/git_cl.py +++ b/git_cl.py @@ -488,7 +488,8 @@ or verify this branch is set up to track another (via the --track argument to self.SetPatchset(0) self.has_issue = False - def GetChange(self, upstream_branch, author): + def RunHook(self, committing, upstream_branch, may_prompt, verbose, author): + """Calls sys.exit() if the hook fails; returns a HookResults otherwise.""" root = RunCommand(['git', 'rev-parse', '--show-cdup']).strip() or '.' absroot = os.path.abspath(root) @@ -510,7 +511,7 @@ or verify this branch is set up to track another (via the --track argument to if not author: author = RunGit(['config', 'user.email']).strip() or None - return presubmit_support.GitChange( + change = presubmit_support.GitChange( name, description, absroot, @@ -519,10 +520,6 @@ or verify this branch is set up to track another (via the --track argument to patchset, author) - def RunHook(self, committing, upstream_branch, may_prompt, verbose, author): - """Calls sys.exit() if the hook fails; returns a HookResults otherwise.""" - change = self.GetChange(upstream_branch, author) - # Apply watchlists on upload. if not committing: watchlist = watchlists.Watchlists(change.RepositoryRoot()) diff --git a/git_try.py b/git_try.py index 27eafba37e..06aaa6eb5f 100755 --- a/git_try.py +++ b/git_try.py @@ -13,7 +13,6 @@ from scm import GIT import subprocess2 import third_party.upload import trychange -import git_cl def GetRietveldIssueNumber(): @@ -54,10 +53,8 @@ if __name__ == '__main__': # Hack around a limitation in logging. logging.getLogger().handlers = [] try: - cl = git_cl.Changelist() - change = cl.GetChange(cl.GetUpstreamBranch(), None) sys.exit(trychange.TryChange( - args, change, file_list=[], swallow_exception=False, + args, file_list=[], swallow_exception=False, prog='git try', extra_epilog='\n' 'git try will diff against your tracked branch and will ' diff --git a/presubmit_support.py b/presubmit_support.py index 85b3a3f70c..5ac38ac3c6 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -16,7 +16,6 @@ import cPickle # Exposed through the API. import cStringIO # Exposed through the API. import fnmatch import glob -import inspect import logging import marshal # Exposed through the API. import optparse @@ -876,7 +875,7 @@ def ListRelevantPresubmitFiles(files, root): class GetTrySlavesExecuter(object): @staticmethod - def ExecPresubmitScript(script_text, presubmit_path, project, change): + def ExecPresubmitScript(script_text, presubmit_path, project): """Executes GetPreferredTrySlaves() from a single presubmit script. Args: @@ -895,14 +894,10 @@ class GetTrySlavesExecuter(object): function_name = 'GetPreferredTrySlaves' if function_name in context: - get_preferred_try_slaves = context[function_name] - function_info = inspect.getargspec(get_preferred_try_slaves) - if len(function_info[0]) == 1: - result = get_preferred_try_slaves(project) - elif len(function_info[0]) == 2: - result = get_preferred_try_slaves(project, change) - else: - result = get_preferred_try_slaves() + try: + result = eval(function_name + '(' + repr(project) + ')', context) + except TypeError: + result = eval(function_name + '()', context) if not isinstance(result, types.ListType): raise PresubmitFailure( 'Presubmit functions must return a list, got a %s instead: %s' % @@ -918,8 +913,7 @@ class GetTrySlavesExecuter(object): return result -def DoGetTrySlaves(change, - changed_files, +def DoGetTrySlaves(changed_files, repository_root, default_presubmit, project, @@ -948,7 +942,7 @@ def DoGetTrySlaves(change, output_stream.write("Running default presubmit script.\n") fake_path = os.path.join(repository_root, 'PRESUBMIT.py') results += executer.ExecPresubmitScript( - default_presubmit, fake_path, project, change) + default_presubmit, fake_path, project) for filename in presubmit_files: filename = os.path.abspath(filename) if verbose: @@ -956,7 +950,7 @@ def DoGetTrySlaves(change, # Accept CRLF presubmit script. presubmit_script = gclient_utils.FileRead(filename, 'rU') results += executer.ExecPresubmitScript( - presubmit_script, filename, project, change) + presubmit_script, filename, project) slaves = list(set(results)) if slaves and verbose: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 055e8b6fc3..466189e9c6 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -153,7 +153,7 @@ class PresubmitUnittest(PresubmitTestsBase): 'OutputApi', 'ParseFiles', 'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs', 'SvnAffectedFile', 'SvnChange', 'cPickle', 'cStringIO', - 'fix_encoding', 'fnmatch', 'gclient_utils', 'glob', 'inspect', 'json', + 'fix_encoding', 'fnmatch', 'gclient_utils', 'glob', 'json', 'load_files', 'logging', 'marshal', 'normpath', 'optparse', 'os', 'owners', 'pickle', 'presubmit_canned_checks', 'random', 're', 'rietveld', 'scm', @@ -669,18 +669,11 @@ def CheckChangeOnCommit(input_api, output_api): def testGetTrySlavesExecuter(self): self.mox.ReplayAll() - change = presubmit.Change( - 'foo', - 'Blah Blah\n\nSTORY=http://tracker.com/42\nBUG=boo\n', - self.fake_root_dir, - None, - 0, - 0, - None) + executer = presubmit.GetTrySlavesExecuter() - self.assertEqual([], executer.ExecPresubmitScript('', '', '', change)) - self.assertEqual([], - executer.ExecPresubmitScript('def foo():\n return\n', '', '', change)) + self.assertEqual([], executer.ExecPresubmitScript('', '', '')) + self.assertEqual( + [], executer.ExecPresubmitScript('def foo():\n return\n', '', '')) # bad results starts_with_space_result = [' starts_with_space'] @@ -689,7 +682,7 @@ def CheckChangeOnCommit(input_api, output_api): for result in starts_with_space_result, not_list_result1, not_list_result2: self.assertRaises(presubmit.PresubmitFailure, executer.ExecPresubmitScript, - self.presubmit_tryslave % result, '', '', change) + self.presubmit_tryslave % result, '', '') # good results expected_result = ['1', '2', '3'] @@ -699,31 +692,20 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual( result, executer.ExecPresubmitScript( - self.presubmit_tryslave % result, '', '', change)) + self.presubmit_tryslave % result, '', '')) def testGetTrySlavesExecuterWithProject(self): self.mox.ReplayAll() - change = presubmit.Change( - 'foo', - 'Blah Blah\n\nSTORY=http://tracker.com/42\nBUG=boo\n', - self.fake_root_dir, - None, - 0, - 0, - None) - executer = presubmit.GetTrySlavesExecuter() expected_result1 = ['1', '2'] expected_result2 = ['a', 'b', 'c'] script = self.presubmit_tryslave_project % ( repr('foo'), repr(expected_result1), repr(expected_result2)) self.assertEqual( - expected_result1, executer.ExecPresubmitScript(script, '', 'foo', - change)) + expected_result1, executer.ExecPresubmitScript(script, '', 'foo')) self.assertEqual( - expected_result2, executer.ExecPresubmitScript(script, '', 'bar', - change)) + expected_result2, executer.ExecPresubmitScript(script, '', 'bar')) def testDoGetTrySlaves(self): join = presubmit.os.path.join @@ -748,18 +730,13 @@ def CheckChangeOnCommit(input_api, output_api): self.presubmit_tryslave % '["linux"]') self.mox.ReplayAll() - change = presubmit.Change( - 'mychange', '', self.fake_root_dir, [], 0, 0, None) - output = StringIO.StringIO() self.assertEqual(['win'], - presubmit.DoGetTrySlaves(change, [filename], - self.fake_root_dir, + presubmit.DoGetTrySlaves([filename], self.fake_root_dir, None, None, False, output)) output = StringIO.StringIO() self.assertEqual(['win', 'linux'], - presubmit.DoGetTrySlaves(change, - [filename, filename_linux], + presubmit.DoGetTrySlaves([filename, filename_linux], self.fake_root_dir, None, None, False, output)) diff --git a/tests/trychange_unittest.py b/tests/trychange_unittest.py index f02c1bf21b..22c4e6a574 100755 --- a/tests/trychange_unittest.py +++ b/tests/trychange_unittest.py @@ -59,8 +59,8 @@ class SVNUnittest(TryChangeTestsBase): """trychange.SVN tests.""" def testMembersChanged(self): members = [ - 'AutomagicalSettings', 'CaptureStatus', 'GetCodeReviewSetting', - 'ReadRootFile', 'GenerateDiff', 'GetFileNames', 'files', 'file_tuples', + 'AutomagicalSettings', 'GetCodeReviewSetting', 'ReadRootFile', + 'GenerateDiff', 'GetFileNames', ] # If this test fails, you should add the relevant test. self.compareMembers(trychange.SVN, members) @@ -83,8 +83,8 @@ class GITUnittest(TryChangeTestsBase): """trychange.GIT tests.""" def testMembersChanged(self): members = [ - 'AutomagicalSettings', 'CaptureStatus', 'GetCodeReviewSetting', - 'ReadRootFile', 'GenerateDiff', 'GetFileNames', 'files', 'file_tuples', + 'AutomagicalSettings', 'GetCodeReviewSetting', 'ReadRootFile', + 'GenerateDiff', 'GetFileNames', ] # If this test fails, you should add the relevant test. self.compareMembers(trychange.GIT, members) diff --git a/trychange.py b/trychange.py index e322f1794b..9825b1e8b7 100755 --- a/trychange.py +++ b/trychange.py @@ -100,8 +100,7 @@ class SCM(object): items.append(None) self.diff_against = items[1] self.options = options - self._files = self.options.files - self._file_tuples = [('M', f) for f in self.files] + self.files = self.options.files self.options.files = None self.codereview_settings = None self.codereview_settings_file = 'codereview.settings' @@ -188,38 +187,6 @@ class SCM(object): logging.warning('Didn\'t find %s' % filename) return None - def _SetFileTuples(self, file_tuples): - excluded = ['!', '?', 'X', ' ', '~'] - def Excluded(f): - if f[0][0] in excluded: - return True - for r in self.options.exclude: - if re.search(r, f[1]): - logging.info('Ignoring "%s"' % f[1]) - return True - return False - - self._file_tuples = [f for f in file_tuples if not Excluded(f)] - self._files = [f[1] for f in self.file_tuples] - - def CaptureStatus(self): - """Returns the 'svn status' emulated output as an array of (status, file) - tuples.""" - raise NotImplementedError( - "abstract method -- subclass %s must override" % self.__class__) - - @property - def files(self): - if not self._files: - self._SetFileTuples(self.CaptureStatus()) - return self._files - - @property - def file_tuples(self): - if not self._file_tuples: - self._SetFileTuples(self.CaptureStatus()) - return self._file_tuples - class SVN(SCM): """Gathers the options and diff for a subversion checkout.""" @@ -243,19 +210,29 @@ class SVN(SCM): logging.debug('%s:\n%s' % (filename, data)) return data - def CaptureStatus(self): - previous_cwd = os.getcwd() - os.chdir(self.checkout_root) - result = scm.SVN.CaptureStatus(self.checkout_root) - os.chdir(previous_cwd) - return result - def GenerateDiff(self): """Returns a string containing the diff for the given file list. The files in the list should either be absolute paths or relative to the given root. """ + if not self.files: + previous_cwd = os.getcwd() + os.chdir(self.checkout_root) + + excluded = ['!', '?', 'X', ' ', '~'] + def Excluded(f): + if f[0][0] in excluded: + return True + for r in self.options.exclude: + if re.search(r, f[1]): + logging.info('Ignoring "%s"' % f[1]) + return True + return False + + self.files = [f[1] for f in scm.SVN.CaptureStatus(self.checkout_root) + if not Excluded(f)] + os.chdir(previous_cwd) return scm.SVN.GenerateDiff(self.files, self.checkout_root, full_move=True, revision=self.diff_against) @@ -278,10 +255,19 @@ class GIT(SCM): "(via the --track argument to \"git checkout -b ...\"") logging.info("GIT(%s)" % self.checkout_root) - def CaptureStatus(self): - return scm.GIT.CaptureStatus(self.checkout_root, self.diff_against) - def GenerateDiff(self): + if not self.files: + self.files = scm.GIT.GetDifferentFiles(self.checkout_root, + branch=self.diff_against) + + def NotExcluded(f): + for r in self.options.exclude: + if re.search(r, f): + logging.info('Ignoring "%s"' % f) + return False + return True + + self.files = filter(NotExcluded, self.files) return scm.GIT.GenerateDiff(self.checkout_root, files=self.files, full_move=True, branch=self.diff_against) @@ -478,7 +464,6 @@ def GetMungedDiff(path_diff, diff): def TryChange(argv, - change, file_list, swallow_exception, prog=None, @@ -716,18 +701,6 @@ def TryChange(argv, diffs.extend(GetMungedDiff(path_diff, diff)) options.diff = ''.join(diffs) - if not options.name: - if options.issue: - options.name = 'Issue %s' % options.issue - else: - options.name = 'Unnamed' - print('Note: use --name NAME to change the try job name.') - - if not options.email: - parser.error('Using an anonymous checkout. Please use --email or set ' - 'the TRYBOT_RESULTS_EMAIL_ADDRESS environment variable.') - print('Results will be emailed to: ' + options.email) - if not options.bot: # Get try slaves from PRESUBMIT.py files if not specified. # Even if the diff comes from options.url, use the local checkout for bot @@ -735,16 +708,7 @@ def TryChange(argv, try: import presubmit_support root_presubmit = checkouts[0].ReadRootFile('PRESUBMIT.py') - if change is None: - change = presubmit_support.Change(options.name, - '', - checkouts[0].checkout_root, - checkouts[0].file_tuples, - options.issue, - options.patchset, - options.email) options.bot = presubmit_support.DoGetTrySlaves( - change, checkouts[0].GetFileNames(), checkouts[0].checkout_root, root_presubmit, @@ -756,6 +720,18 @@ def TryChange(argv, # If no bot is specified, either the default pool will be selected or the # try server will refuse the job. Either case we don't need to interfere. + if options.name is None: + if options.issue: + options.name = 'Issue %s' % options.issue + else: + options.name = 'Unnamed' + print('Note: use --name NAME to change the try job name.') + if not options.email: + parser.error('Using an anonymous checkout. Please use --email or set ' + 'the TRYBOT_RESULTS_EMAIL_ADDRESS environment variable.') + else: + print('Results will be emailed to: ' + options.email) + # Prevent rietveld updates if we aren't running all the tests. if options.testfilter is not None: options.issue = None @@ -791,4 +767,4 @@ def TryChange(argv, if __name__ == "__main__": fix_encoding.fix_encoding() - sys.exit(TryChange(None, None, [], False)) + sys.exit(TryChange(None, [], False))