From 1811135923f955bae5b0d71ab27ba83e39d7d102 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Sun, 20 Dec 2009 17:21:28 +0000 Subject: [PATCH] Remove gclient-specific hacks from trychange into gcl. Remove upload dependency. This is towards making trychange.py work mostly standalone for the webkit try server. TEST=unit tests and tested both gcl try and git-try. Review URL: http://codereview.chromium.org/501143 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@35071 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 34 +++++++++- gclient_utils.py | 15 ++-- tests/gclient_utils_test.py | 4 +- tests/trychange_unittest.py | 96 +++++++++++++------------- trychange.py | 132 +++++++++++++----------------------- 5 files changed, 138 insertions(+), 143 deletions(-) diff --git a/gcl.py b/gcl.py index 4258f400e..61efad566 100755 --- a/gcl.py +++ b/gcl.py @@ -24,7 +24,7 @@ import breakpad from scm import SVN import gclient_utils -__version__ = '1.1.2' +__version__ = '1.1.3' CODEREVIEW_SETTINGS = { @@ -858,8 +858,35 @@ def TryChange(change_info, args, swallow_exception): return ErrorExit("You need to install trychange.py to use the try server.") + def PathDifference(root, subpath): + """Returns the difference subpath minus root.""" + root = os.path.realpath(root) + subpath = os.path.realpath(subpath) + if not subpath.startswith(root): + return None + # If the root does not have a trailing \ or /, we add it so the returned + # path starts immediately after the seperator regardless of whether it is + # provided. + root = os.path.join(root, '') + return subpath[len(root):] + + # Try to find the gclient root. + def FindGclientRootDir(from_dir): + path = os.path.realpath(from_dir) + while not os.path.exists(os.path.join(path, '.gclient')): + next = os.path.split(path) + if not next[1]: + return None + path = next[0] + return path + + trychange_args = [] + gclient_root = FindGclientRootDir(GetRepositoryRoot()) + if gclient_root: + trychange_args.extend(['--root', PathDifference(gclient_root, + GetRepositoryRoot())]) if change_info: - trychange_args = ['--name', change_info.name] + trychange_args.extend(['--name', change_info.name]) if change_info.issue: trychange_args.extend(["--issue", str(change_info.issue)]) if change_info.patchset: @@ -870,7 +897,8 @@ def TryChange(change_info, args, swallow_exception): swallow_exception=swallow_exception, prog='gcl try') else: - trychange.TryChange(args, + trychange_args.extend(args) + trychange.TryChange(trychange_args, file_list=None, swallow_exception=swallow_exception, prog='gcl try') diff --git a/gclient_utils.py b/gclient_utils.py index 81379c30e..6ea091c12 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -40,12 +40,15 @@ def CheckCall(command, cwd=None): Works on python 2.4 """ - process = subprocess.Popen(command, cwd=cwd, - shell=sys.platform.startswith('win'), - stdout=subprocess.PIPE) - output = process.communicate()[0] - if process.retcode: - raise CheckCallError(command, cwd, process.retcode, output) + try: + process = subprocess.Popen(command, cwd=cwd, + shell=sys.platform.startswith('win'), + stdout=subprocess.PIPE) + output = process.communicate()[0] + except OSError, e: + raise CheckCallError(command, cwd, errno, None) + if process.returncode: + raise CheckCallError(command, cwd, process.returncode, output) return output diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index ca5b14a9e..cbe91fa88 100644 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -29,7 +29,7 @@ class CheckCallTestCase(SuperMoxTestBase): def testCheckCallSuccess(self): command = ['boo', 'foo', 'bar'] process = self.mox.CreateMockAnything() - process.retcode = 0 + process.returncode = 0 gclient_utils.subprocess.Popen( command, cwd=None, stdout=gclient_utils.subprocess.PIPE, @@ -41,7 +41,7 @@ class CheckCallTestCase(SuperMoxTestBase): def testCheckCallFailure(self): command = ['boo', 'foo', 'bar'] process = self.mox.CreateMockAnything() - process.retcode = 42 + process.returncode = 42 gclient_utils.subprocess.Popen( command, cwd=None, stdout=gclient_utils.subprocess.PIPE, diff --git a/tests/trychange_unittest.py b/tests/trychange_unittest.py index a47d65f61..5c6612268 100644 --- a/tests/trychange_unittest.py +++ b/tests/trychange_unittest.py @@ -14,20 +14,33 @@ from super_mox import mox, SuperMoxTestBase class TryChangeTestsBase(SuperMoxTestBase): """Setups and tear downs the mocks but doesn't test anything as-is.""" - pass + def setUp(self): + SuperMoxTestBase.setUp(self) + self.mox.StubOutWithMock(trychange.gclient_utils, 'CheckCall') + self.mox.StubOutWithMock(trychange.scm.GIT, 'Capture') + self.mox.StubOutWithMock(trychange.scm.GIT, 'GetEmail') + self.mox.StubOutWithMock(trychange.scm.SVN, 'DiffItem') + self.mox.StubOutWithMock(trychange.scm.SVN, 'GetCheckoutRoot') + self.mox.StubOutWithMock(trychange.scm.SVN, 'GetEmail') + self.fake_root = self.Dir() + self.expected_files = ['foo.txt', 'bar.txt'] + self.options = optparse.Values() + self.options.files = self.expected_files + self.options.diff = None + self.options.name = None + self.options.email = None class TryChangeUnittest(TryChangeTestsBase): """General trychange.py tests.""" def testMembersChanged(self): members = [ - 'EscapeDot', 'GIT', 'GetSourceRoot', - 'GetTryServerSettings', 'GuessVCS', - 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'PathDifference', + 'EscapeDot', 'GIT', 'GetTryServerSettings', 'GuessVCS', + 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'SCM', 'SVN', 'TryChange', 'USAGE', 'breakpad', 'datetime', 'gcl', 'gclient_utils', 'getpass', 'logging', 'optparse', 'os', 'presubmit_support', 'scm', 'shutil', 'socket', - 'subprocess', 'sys', 'tempfile', 'upload', 'urllib', + 'subprocess', 'sys', 'tempfile', 'urllib', ] # If this test fails, you should add the relevant test. self.compareMembers(trychange, members) @@ -35,64 +48,53 @@ class TryChangeUnittest(TryChangeTestsBase): class SVNUnittest(TryChangeTestsBase): """trychange.SVN tests.""" - def setUp(self): - SuperMoxTestBase.setUp(self) - self.fake_root = '/fake_root' - self.expected_files = ['foo.txt', 'bar.txt'] - change_info = trychange.gcl.ChangeInfo( - 'test_change', 0, 0, 'desc', - [('M', f) for f in self.expected_files], - self.fake_root) - self.svn = trychange.SVN(None) - self.svn.change_info = change_info - def testMembersChanged(self): members = [ - 'GenerateDiff', 'GetFileNames', 'GetLocalRoot', 'ProcessOptions', - 'options' + 'GetFileNames', 'GetLocalRoot', ] # If this test fails, you should add the relevant test. - self.compareMembers(trychange.SVN(None), members) - - def testGetFileNames(self): + self.compareMembers(trychange.SVN, members) + + def testBasic(self): + trychange.os.getcwd().AndReturn(self.fake_root) + trychange.scm.SVN.GetCheckoutRoot(self.fake_root).AndReturn(self.fake_root) + trychange.os.getcwd().AndReturn('pro') + trychange.os.chdir(self.fake_root) + trychange.scm.SVN.DiffItem(self.expected_files[0]).AndReturn('bleh') + trychange.scm.SVN.DiffItem(self.expected_files[1]).AndReturn('blew') + trychange.os.chdir('pro') + trychange.scm.SVN.GetEmail(self.fake_root).AndReturn('georges@example.com') self.mox.ReplayAll() - self.assertEqual(self.svn.GetFileNames(), self.expected_files) - - def testGetLocalRoot(self): - self.mox.ReplayAll() - self.assertEqual(self.svn.GetLocalRoot(), self.fake_root) + svn = trychange.SVN(self.options) + self.assertEqual(svn.GetFileNames(), self.expected_files) + self.assertEqual(svn.GetLocalRoot(), self.fake_root) class GITUnittest(TryChangeTestsBase): """trychange.GIT tests.""" - def setUp(self): - self.fake_root = trychange.os.path.join( - trychange.os.path.dirname(__file__), 'fake_root') - self.expected_files = ['foo.txt', 'bar.txt'] - options = optparse.Values() - options.files = self.expected_files - self.git = trychange.GIT(options) - SuperMoxTestBase.setUp(self) - def testMembersChanged(self): members = [ - 'GenerateDiff', 'GetFileNames', 'GetLocalRoot', - 'GetPatchName', 'ProcessOptions', 'options' + 'GetFileNames', 'GetLocalRoot', ] # If this test fails, you should add the relevant test. - self.compareMembers(trychange.GIT(None), members) - - def testGetFileNames(self): - self.mox.ReplayAll() - self.assertEqual(self.git.GetFileNames(), self.expected_files) + self.compareMembers(trychange.GIT, members) - def testGetLocalRoot(self): - self.mox.StubOutWithMock(trychange.upload, 'RunShell') - trychange.upload.RunShell(['git', 'rev-parse', '--show-cdup']).AndReturn( - self.fake_root) + def testBasic(self): + trychange.gclient_utils.CheckCall( + ['git', 'rev-parse', '--show-cdup']).AndReturn(self.fake_root) trychange.os.path.abspath(self.fake_root).AndReturn(self.fake_root) + trychange.gclient_utils.CheckCall( + ['git', 'cl', 'upstream']).AndReturn('random branch') + trychange.gclient_utils.CheckCall( + ['git', 'diff-tree', '-p', '--no-prefix', 'random branch', 'HEAD'] + ).AndReturn('This is a dummy diff\n+3\n-4\n') + trychange.gclient_utils.CheckCall( + ['git', 'symbolic-ref', 'HEAD']).AndReturn('refs/heads/another branch') + trychange.scm.GIT.GetEmail('.').AndReturn('georges@example.com') self.mox.ReplayAll() - self.assertEqual(self.git.GetLocalRoot(), self.fake_root) + git = trychange.GIT(self.options) + self.assertEqual(git.GetFileNames(), self.expected_files) + self.assertEqual(git.GetLocalRoot(), self.fake_root) if __name__ == '__main__': diff --git a/trychange.py b/trychange.py index 691c67de2..2389bb093 100755 --- a/trychange.py +++ b/trychange.py @@ -25,9 +25,8 @@ import gcl import gclient_utils import scm import presubmit_support -import upload -__version__ = '1.1.2' +__version__ = '1.2' # Constants @@ -66,23 +65,6 @@ class NoTryServerAccess(Exception): return self.args[0] + '\n' + HELP_STRING -def PathDifference(root, subpath): - """Returns the difference subpath minus root.""" - if subpath.find(root) != 0: - return None - # If the root does not have a trailing \ or /, we add it so the returned path - # starts immediately after the seperator regardless of whether it is provided. - if not root.endswith(os.sep): - root += os.sep - return subpath[len(root):] - - -def GetSourceRoot(): - """Returns the absolute directory one level up from the repository root.""" - # TODO(maruel): This is odd to assume that '..' is the source root. - return os.path.abspath(os.path.join(gcl.GetRepositoryRoot(), '..')) - - def GetTryServerSettings(): """Grab try server settings local to the repository.""" def _SafeResolve(host): @@ -124,62 +106,62 @@ class SCM(object): def __init__(self, options): self.options = options - def ProcessOptions(self): - raise NotImplementedError + def GetFileNames(self): + """Return the list of files in the diff.""" + return self.options.files class SVN(SCM): """Gathers the options and diff for a subversion checkout.""" - def GenerateDiff(self, files, root): + def __init__(self, *args, **kwargs): + SCM.__init__(self, *args, **kwargs) + self.checkout_root = scm.SVN.GetCheckoutRoot(os.getcwd()) + self.options.files + if not self.options.diff: + # Generate the diff from the scm. + self.options.diff = self._GenerateDiff() + if not self.options.email: + # Assumes the svn credential is an email address. + self.options.email = scm.SVN.GetEmail(self.checkout_root) + + 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 no root directory is provided, the repository root will be - used. + given root. """ previous_cwd = os.getcwd() - os.chdir(root or scm.SVN.GetCheckoutRoot(previous_cwd)) - + os.chdir(self.checkout_root) + if not self.options.files: + self.options.files = [f[1] for f in scm.SVN.CaptureStatus(None)] # Directories will return None so filter them out. - diff = filter(None, [scm.SVN.DiffItem(f) for f in files]) + diff = filter(None, [scm.SVN.DiffItem(f) for f in self.options.files]) os.chdir(previous_cwd) return "".join(diff) - def GetFileNames(self): - """Return the list of files in the diff.""" - return self.change_info.GetFileNames() - def GetLocalRoot(self): """Return the path of the repository root.""" - return self.change_info.GetLocalRoot() - - def ProcessOptions(self): - checkout_root = None - if not self.options.diff: - # Generate the diff with svn and write it to the submit queue path. The - # files are relative to the repository root, but we need patches relative - # to one level up from there (i.e., 'src'), so adjust both the file - # paths and the root of the diff. - # TODO(maruel): Remove this hack. - source_root = GetSourceRoot() - checkout_root = scm.SVN.GetCheckoutRoot(os.getcwd()) - prefix = PathDifference(source_root, checkout_root) - adjusted_paths = [os.path.join(prefix, x) for x in self.options.files] - self.options.diff = self.GenerateDiff(adjusted_paths, root=source_root) - self.change_info = gcl.LoadChangelistInfoForMultiple(self.options.name, - gcl.GetRepositoryRoot(), True, True) - if not self.options.email: - checkout_root = checkout_root or scm.SVN.GetCheckoutRoot(os.getcwd()) - self.options.email = scm.SVN.GetEmail(checkout_root) + return self.checkout_root class GIT(SCM): """Gathers the options and diff for a git checkout.""" - def GenerateDiff(self): + def __init__(self, *args, **kwargs): + SCM.__init__(self, *args, **kwargs) + self.checkout_root = os.path.abspath( + gclient_utils.CheckCall(['git', 'rev-parse', '--show-cdup']).strip()) + if not self.options.diff: + self.options.diff = self._GenerateDiff() + if not self.options.name: + self.options.name = self._GetPatchName() + if not self.options.email: + self.options.email = scm.GIT.GetEmail('.') + + def _GenerateDiff(self): """Get the diff we'll send to the try server. We ignore the files list.""" - branch = upload.RunShell(['git', 'cl', 'upstream']).strip() - diff = upload.RunShell(['git', 'diff-tree', '-p', '--no-prefix', - branch, 'HEAD']).splitlines(True) + branch = gclient_utils.CheckCall(['git', 'cl', 'upstream']).strip() + diff = gclient_utils.CheckCall(['git', 'diff-tree', '-p', '--no-prefix', + branch, 'HEAD']).splitlines(True) for i in range(len(diff)): # In the case of added files, replace /dev/null with the path to the # file being added. @@ -187,34 +169,20 @@ class GIT(SCM): diff[i] = '--- %s' % diff[i+1][4:] return ''.join(diff) - def GetFileNames(self): - """Return the list of files in the diff.""" - return self.options.files - - def GetLocalRoot(self): - """Return the path of the repository root.""" - # TODO: check for errors here? - root = upload.RunShell(['git', 'rev-parse', '--show-cdup']).strip() - return os.path.abspath(root) - - def GetPatchName(self): + def _GetPatchName(self): """Construct a name for this patch.""" # TODO: perhaps include the hash of the current commit, to distinguish # patches? - branch = upload.RunShell(['git', 'symbolic-ref', 'HEAD']).strip() + branch = gclient_utils.CheckCall(['git', 'symbolic-ref', 'HEAD']).strip() if not branch.startswith('refs/heads/'): # TODO(maruel): Find a better type. raise NoTryServerAccess("Couldn't figure out branch name") branch = branch[len('refs/heads/'):] return branch - def ProcessOptions(self): - if not self.options.diff: - self.options.diff = self.GenerateDiff() - if not self.options.name: - self.options.name = self.GetPatchName() - if not self.options.email: - self.options.email = scm.GIT.GetEmail('.') + def GetLocalRoot(self): + """Return the path of the repository root.""" + return self.checkout_root def _ParseSendChangeOptions(options): @@ -365,17 +333,12 @@ def GuessVCS(options): # Git has a command to test if you're in a git tree. # Try running it, but don't die if we don't have git installed. try: - out, returncode = subprocess.Popen( - ["git", "rev-parse", "--is-inside-work-tree"], - shell=sys.platform.startswith('win'), - stdout=subprocess.PIPE).communicate()[0] - if returncode == 0: - logging.info("Guessed VCS = Git") - return GIT(options) - except OSError, (errno, message): - if errno != 2: # ENOENT -- they don't have git installed. + gclient_utils.CheckCall(["git", "rev-parse", "--is-inside-work-tree"]) + logging.info("Guessed VCS = Git") + return GIT(options) + except gclient_utils.CheckCallError, e: + if e.retcode != 2: # ENOENT -- they don't have git installed. raise - raise NoTryServerAccess("Could not guess version control system. " "Are you in a working copy directory?") @@ -520,7 +483,6 @@ def TryChange(argv, # Process the VCS in any case at least to retrieve the email address. try: options.scm = GuessVCS(options) - options.scm.ProcessOptions() except NoTryServerAccess, e: # If we got the diff, we don't care. if not options.diff: