From a692e8f51fe375af7bd33c259ca7734c4f8f3272 Mon Sep 17 00:00:00 2001 From: "kustermann@google.com" Date: Thu, 18 Apr 2013 08:32:04 +0000 Subject: [PATCH] Changed the behaviour of '--transitive' in gclient.py to use revision instead of timestamp for identical repositories. Here's some background why we need this: We discovered that google code defines the timestamp of a revision to be the time when a commit was started rather than when it was finished (apache subversion takes the timestamp when the commit transaction is finished). This can result in a situation where revision R(i-1) has a higher timestamp than Ri. See bug: https://code.google.com/p/support/issues/detail?id=30419 When using 'gclient --transitive' we effectively do date-based checkouts. If a parent has a dependency (without a ...@revision) and that dependency lives in the same repository as the parent does we'd like to checkout the exact same revision as the parent (if we do a date-based checkout as we do now the google code bug can result in a situation where we don't get the same revision). Review URL: https://chromiumcodereview.appspot.com/13814012 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@194852 0039d316-1c4b-4281-b951-d872f2087c98 --- gclient.py | 87 ++++++++++++++++--------------- testing_support/fake_repos.py | 83 +++++++++++++++++++++++++----- tests/gclient_smoketest.py | 97 +++++++++++++++++++++-------------- 3 files changed, 175 insertions(+), 92 deletions(-) diff --git a/gclient.py b/gclient.py index 85f144c1f..3137093f5 100755 --- a/gclient.py +++ b/gclient.py @@ -298,6 +298,9 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): self._processed = False # This dependency had its hook run self._hooks_ran = False + # This is the scm used to checkout self.url. It may be used by dependencies + # to get the datetime of the revision we checked out. + self._used_scm = None if not self.name and self.parent: raise gclient_utils.Error('Dependency without name') @@ -538,42 +541,42 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): self.add_dependency(dep) self._mark_as_parsed(hooks) - @staticmethod def maybeGetParentRevision( - command, options, parsed_url, parent_name, revision_overrides): - """If we are performing an update and --transitive is set, set the - revision to the parent's revision. If we have an explicit revision - do nothing.""" + self, command, options, parsed_url, parent_name, revision_overrides): + """Uses revision/timestamp of parent if no explicit revision was specified. + + If we are performing an update and --transitive is set, use + - the parent's revision if 'self.url' is in the same repository + - the parent's timestamp otherwise + to update 'self.url'. The used revision/timestamp will be set in + 'options.revision'. + If we have an explicit revision do nothing. + """ if command == 'update' and options.transitive and not options.revision: _, revision = gclient_utils.SplitUrlRevision(parsed_url) if not revision: options.revision = revision_overrides.get(parent_name) - if options.verbose and options.revision: - print("Using parent's revision date: %s" % options.revision) - # If the parent has a revision override, then it must have been - # converted to date format. - assert (not options.revision or - gclient_utils.IsDateRevision(options.revision)) - - @staticmethod - def maybeConvertToDateRevision( - command, options, name, scm, revision_overrides): - """If we are performing an update and --transitive is set, convert the - revision to a date-revision (if necessary). Instead of having - -r 101 replace the revision with the time stamp of 101 (e.g. - "{2011-18-04}"). - This way dependencies are upgraded to the revision they had at the - check-in of revision 101.""" - if (command == 'update' and - options.transitive and - options.revision and - not gclient_utils.IsDateRevision(options.revision)): - revision_date = scm.GetRevisionDate(options.revision) - revision = gclient_utils.MakeDateRevision(revision_date) - if options.verbose: - print("Updating revision override from %s to %s." % - (options.revision, revision)) - revision_overrides[name] = revision + if (options.revision and + not gclient_utils.IsDateRevision(options.revision)): + assert self.parent and self.parent.used_scm + # If this dependency is in the same repository as parent it's url will + # start with a slash. If so we take the parent revision instead of + # it's timestamp. + # (The timestamps of commits in google code are broken -- which can + # result in dependencies to be checked out at the wrong revision) + if self.url.startswith('/'): + if options.verbose: + print('Using parent\'s revision %s since we are in the same ' + 'repository.' % options.revision) + else: + parent_revision_date = self.parent.used_scm.GetRevisionDate( + options.revision) + options.revision = gclient_utils.MakeDateRevision( + parent_revision_date) + if options.verbose: + print('Using parent\'s revision date %s since we are in a ' + 'different repository.' % options.revision) + revision_overrides[self.name] = options.revision # Arguments number differs from overridden method # pylint: disable=W0221 @@ -596,22 +599,19 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Sadly, pylint doesn't realize that parsed_url is of FileImpl. # pylint: disable=E1103 options.revision = parsed_url.GetRevision() - scm = gclient_scm.SVNWrapper(parsed_url.GetPath(), - self.root.root_dir, - self.name) - scm.RunCommand('updatesingle', options, - args + [parsed_url.GetFilename()], - file_list) + self._used_scm = gclient_scm.SVNWrapper( + parsed_url.GetPath(), self.root.root_dir, self.name) + self._used_scm.RunCommand('updatesingle', + options, args + [parsed_url.GetFilename()], file_list) else: # Create a shallow copy to mutate revision. options = copy.copy(options) options.revision = revision_overrides.get(self.name) self.maybeGetParentRevision( command, options, parsed_url, self.parent.name, revision_overrides) - scm = gclient_scm.CreateSCM(parsed_url, self.root.root_dir, self.name) - scm.RunCommand(command, options, args, file_list) - self.maybeConvertToDateRevision( - command, options, self.name, scm, revision_overrides) + self._used_scm = gclient_scm.CreateSCM( + parsed_url, self.root.root_dir, self.name) + self._used_scm.RunCommand(command, options, args, file_list) file_list = [os.path.join(self.name, f.strip()) for f in file_list] # TODO(phajdan.jr): We should know exactly when the paths are absolute. @@ -827,6 +827,11 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): def file_list(self): return tuple(self._file_list) + @property + def used_scm(self): + """SCMWrapper instance for this dependency or None if not processed yet.""" + return self._used_scm + @property def file_list_and_children(self): result = list(self.file_list) diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index 3c59b6e7e..c5e509133 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -161,8 +161,6 @@ def wait_for_port_to_free(host, port): assert False, '%d is still bound' % port -_FAKE_LOADED = False - class FakeReposBase(object): """Generate both svn and git repositories to test gclient functionality. @@ -181,11 +179,6 @@ class FakeReposBase(object): ] def __init__(self, host=None): - global _FAKE_LOADED - if _FAKE_LOADED: - raise Exception('You can only start one FakeRepos at a time.') - _FAKE_LOADED = True - self.trial = trial_dir.TrialDir('repos') self.host = host or '127.0.0.1' # Format is [ None, tree, tree, ...] @@ -321,6 +314,17 @@ class FakeReposBase(object): text += ''.join('%s = %s\n' % (usr, pwd) for usr, pwd in self.USERS) write(join(self.svn_repo, 'conf', 'passwd'), text) + # Necessary to be able to change revision properties + revprop_hook_filename = join(self.svn_repo, 'hooks', 'pre-revprop-change') + if sys.platform == 'win32': + # TODO(kustermann): Test on Windows one day. + write("%s.bat" % revprop_hook_filename, "") + else: + write(revprop_hook_filename, + '#!/bin/sh\n' + 'exit 0\n') + os.chmod(revprop_hook_filename, 0755) + # Mac 10.6 ships with a buggy subversion build and we need this line # to work around the bug. write(join(self.svn_repo, 'db', 'fsfs.conf'), @@ -392,6 +396,14 @@ class FakeReposBase(object): new_tree = tree.copy() self.svn_revs.append(new_tree) + def _set_svn_commit_date(self, revision, date): + subprocess2.check_output( + ['svn', 'propset', 'svn:date', '--revprop', '-r', revision, date, + self.svn_base, + '--username', self.USERS[0][0], + '--password', self.USERS[0][1], + '--non-interactive']) + def _commit_git(self, repo, tree): repo_root = join(self.git_root, repo) self._genTree(repo_root, tree) @@ -642,22 +654,67 @@ hooks = [ }) +class FakeRepoTransitive(FakeReposBase): + """Implements populateSvn()""" + + def populateSvn(self): + """Creates a few revisions of changes including a DEPS file.""" + # Repos + subprocess2.check_call( + ['svn', 'checkout', self.svn_base, self.svn_checkout, + '-q', '--non-interactive', '--no-auth-cache', + '--username', self.USERS[0][0], '--password', self.USERS[0][1]]) + assert os.path.isdir(join(self.svn_checkout, '.svn')) + + def file_system(rev): + DEPS = """deps = { + 'src/different_repo': '%(svn_base)strunk/third_party', + 'src/different_repo_fixed': '%(svn_base)strunk/third_party@1', + 'src/same_repo': '/trunk/third_party', + 'src/same_repo_fixed': '/trunk/third_party@1', + }""" % { 'svn_base': self.svn_base } + return { + 'trunk/src/DEPS': DEPS, + 'trunk/src/origin': 'svn/trunk/src@%(rev)d' % { 'rev': rev }, + 'trunk/third_party/origin': + 'svn/trunk/third_party@%(rev)d' % { 'rev': rev }, + } + + # We make three commits. We use always the same DEPS contents but + # - 'trunk/src/origin' contains 'svn/trunk/src/origin@rX' + # - 'trunk/third_party/origin' contains 'svn/trunk/third_party/origin@rX' + # where 'X' is the revision number. + # So the 'origin' files will change in every commit. + self._commit_svn(file_system(1)) + self._commit_svn(file_system(2)) + self._commit_svn(file_system(3)) + # We rewrite the timestamps so we can test that '--transitive' will take the + # parent timestamp on different repositories and the parent revision + # otherwise. + self._set_svn_commit_date('1', '2011-10-01T03:00:00.000000Z') + self._set_svn_commit_date('2', '2011-10-09T03:00:00.000000Z') + self._set_svn_commit_date('3', '2011-10-02T03:00:00.000000Z') + + def populateGit(self): + pass + + class FakeReposTestBase(trial_dir.TestCase): """This is vaguely inspired by twisted.""" - # static FakeRepos instance. Lazy loaded. - FAKE_REPOS = None + # Static FakeRepos instances. Lazy loaded. + CACHED_FAKE_REPOS = {} # Override if necessary. FAKE_REPOS_CLASS = FakeRepos def setUp(self): super(FakeReposTestBase, self).setUp() - if not FakeReposTestBase.FAKE_REPOS: - # Lazy create the global instance. - FakeReposTestBase.FAKE_REPOS = self.FAKE_REPOS_CLASS() + if not self.FAKE_REPOS_CLASS in self.CACHED_FAKE_REPOS: + self.CACHED_FAKE_REPOS[self.FAKE_REPOS_CLASS] = self.FAKE_REPOS_CLASS() + self.FAKE_REPOS = self.CACHED_FAKE_REPOS[self.FAKE_REPOS_CLASS] # No need to call self.FAKE_REPOS.setUp(), it will be called by the child # class. # Do not define tearDown(), since super's version does the right thing and - # FAKE_REPOS is kept across tests. + # self.FAKE_REPOS is kept across tests. @property def svn_base(self): diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index 4896e6633..afe838fe7 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -20,7 +20,9 @@ import unittest ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) -from testing_support.fake_repos import join, write, FakeReposTestBase +from testing_support.fake_repos import join, write +from testing_support.fake_repos import FakeReposTestBase, FakeRepoTransitive + import gclient_utils import subprocess2 @@ -348,43 +350,6 @@ class GClientSmokeSVN(GClientSmokeBase): tree['src/svn_hooked1'] = 'svn_hooked1' self.assertTree(tree) - def testSyncTransitive(self): - # TODO(maruel): safesync. - if not self.enabled: - return - self.gclient(['config', self.svn_base + 'trunk/src/']) - - # Make sure we can populate a new repository with --transitive. - self.parseGclient( - ['sync', '--transitive', '--revision', 'src@1', '--deps', 'mac', - '--jobs', '1'], - ['running', 'running', 'running', 'running']) - tree = self.mangle_svn_tree( - ('trunk/src@1', 'src'), - ('trunk/third_party/foo@1', 'src/third_party/fpp'), - ('trunk/other@1', 'src/other'), - ('trunk/third_party/foo@1', 'src/third_party/prout')) - - # Get up to date, so we can test synching back. - self.gclient(['sync', '--deps', 'mac', '--jobs', '1']) - - # Manually remove svn_hooked1 before synching to make sure it's not - # recreated. - os.remove(join(self.root_dir, 'src', 'svn_hooked1')) - - self.parseGclient( - ['sync', '--transitive', '--revision', 'src@1', '--deps', 'mac', - '--delete_unversioned_trees', '--jobs', '1'], - ['running', 'running', 'running', 'running', 'deleting']) - tree = self.mangle_svn_tree( - ('trunk/src@1', 'src'), - ('trunk/third_party/foo@1', 'src/third_party/fpp'), - ('trunk/other@1', 'src/other'), - ('trunk/third_party/foo@1', 'src/third_party/prout')) - tree['src/file/other/DEPS'] = ( - self.FAKE_REPOS.svn_revs[2]['trunk/other/DEPS']) - self.assertTree(tree) - def testSyncIgnoredSolutionName(self): """TODO(maruel): This will become an error soon.""" if not self.enabled: @@ -801,6 +766,62 @@ class GClientSmokeSVN(GClientSmokeBase): self.assertEquals(0, self.gclient(cmd)[-1]) +class GClientSmokeSVNTransitive(GClientSmokeBase): + FAKE_REPOS_CLASS = FakeRepoTransitive + + def setUp(self): + super(GClientSmokeSVNTransitive, self).setUp() + self.enabled = self.FAKE_REPOS.set_up_svn() + + def testSyncTransitive(self): + if not self.enabled: + return + + self.gclient(['config', self.svn_base + 'trunk/src/']) + + def test_case(parent, timestamp, fixed, output): + # We check out revision 'parent' and expect the following: + # - src/ is checked out at r'parent' + # - src/same_repo is checked out at r'parent' (due to --transitive) + # - src/same_repo_fixed is checked out at r'fixed' + # - src/different_repo is checked out at r'timestamp' + # (due to --transitive) + # - src/different_repo_fixed is checked out at r'fixed' + + revisions = self.FAKE_REPOS.svn_revs + self.parseGclient( + ['sync', '--transitive', '--revision', 'src@%d' % parent, + '--jobs', '1'], output) + self.assertTree({ + 'src/origin': revisions[parent]['trunk/src/origin'], + 'src/DEPS': revisions[parent]['trunk/src/DEPS'], + 'src/same_repo/origin': revisions[parent]['trunk/third_party/origin'], + 'src/same_repo_fixed/origin': + revisions[fixed]['trunk/third_party/origin'], + 'src/different_repo/origin': + revisions[timestamp]['trunk/third_party/origin'], + 'src/different_repo_fixed/origin': + revisions[fixed]['trunk/third_party/origin'], + }) + + # Here are the test cases for checking out 'trunk/src' at r1, r2 and r3 + # r1: Everything is normal + test_case(parent=1, timestamp=1, fixed=1, + output=['running', 'running', 'running', 'running', 'running']) + # r2: Svn will scan from r1 upwards until it finds a revision matching the + # given timestamp or it takes the next smallest one (which is r2 in this + # case). + test_case(parent=2, timestamp=2, fixed=1, + output=['running', 'running', 'running']) + # r3: Svn will scan from r1 upwards until it finds a revision matching the + # given timestamp or it takes the next smallest one. Since + # timestamp(r3) < timestamp(r2) svn will checkout r1. + # This happens often on http://googlecode.com but is unexpected to happen + # with svnserve (unless you manually change 'svn:date') + test_case(parent=3, timestamp=1, fixed=1, + output=['running', 'running', 'running']) + + class GClientSmokeGIT(GClientSmokeBase): def setUp(self): super(GClientSmokeGIT, self).setUp()