Merge branch 'sam/sourceresolver-fixes'

Reviewed-By: Paul Martin <paul.martin@codethink.co.uk>
Reviewed-By: Francisco Redondo Marchena <francisco.marchena@codethink.co.uk>

4 files changed, 138 insertions, 87 deletions

diff --git a/morphlib/localrepocache.py b/morphlib/localrepocache.py
index 9bccb20b..39fbd200 100644
--- a/morphlib/localrepocache.py
+++ b/morphlib/localrepocache.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2012-2014  Codethink Limited
+# Copyright (C) 2012-2015  Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -204,7 +204,9 @@ class LocalRepoCache(object):
         if self._tarball_base_url:
             ok, error = self._clone_with_tarball(repourl, path)
             if ok:
-                return self.get_repo(reponame)
+                repo = self.get_repo(reponame)
+                repo.update()
+                return repo
             else:
                 errors.append(error)
                 self._app.status(
diff --git a/morphlib/localrepocache_tests.py b/morphlib/localrepocache_tests.py
index ab6e71fd..aeb32961 100644
--- a/morphlib/localrepocache_tests.py
+++ b/morphlib/localrepocache_tests.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2012-2014  Codethink Limited
+# Copyright (C) 2012-2015 Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -139,7 +139,11 @@ class LocalRepoCacheTests(unittest.TestCase):
         self.lrc._fetch = lambda url, path: self.fetched.append(url)
         self.unpacked_tar = ""
         self.mkdir_path = ""
-        self.lrc.cache_repo(self.repourl)
+
+        with morphlib.gitdir_tests.monkeypatch(
+                morphlib.cachedrepo.CachedRepo, 'update', lambda self: None):
+            self.lrc.cache_repo(self.repourl)
+
         self.assertEqual(self.fetched, [self.tarball_url])
         self.assertFalse(self.lrc.fs.exists(self.cache_path + '.tar'))
         self.assertEqual(self.remotes['origin']['url'], self.repourl)
diff --git a/morphlib/sourceresolver.py b/morphlib/sourceresolver.py
index 29069d7d..22e643d2 100644
--- a/morphlib/sourceresolver.py
+++ b/morphlib/sourceresolver.py
@@ -90,13 +90,6 @@ class MorphologyNotFoundError(SourceResolverError): # pragma: no cover
             self, "Couldn't find morphology: %s" % filename)
 
 
-class NotcachedError(SourceResolverError):
-    def __init__(self, repo_name):
-        SourceResolverError.__init__(
-            self, "Repository %s is not cached locally and there is no "
-                  "remote cache specified" % repo_name)
-
-
 class SourceResolver(object):
     '''Provides a way of resolving the set of sources for a given system.
 
@@ -155,6 +148,18 @@ class SourceResolver(object):
 
         self._definitions_checkout_dir = None
 
+    def cache_repo_locally(self, reponame):
+        if self.update:
+            self.status(msg='Caching git repository %(reponame)s',
+                        reponame=reponame)
+            repo = self.lrc.cache_repo(reponame)
+        else:  # pragma: no cover
+            # This is likely to raise a morphlib.localrepocache.NotCached
+            # exception, because the caller should have checked if the
+            # localrepocache already had the repo. But we may as well try.
+            repo = self.lrc.get_repo(reponame)
+        return repo
+
     def _resolve_ref(self, reponame, ref): # pragma: no cover
         '''Resolves commit and tree sha1s of the ref in a repo and returns it.
 
@@ -195,16 +200,9 @@ class SourceResolver(object):
                                 chatty=True)
             except BaseException, e:
                 logging.warning('Caught (and ignored) exception: %s' % str(e))
+
         if absref is None:
-            if self.update:
-                self.status(msg='Caching git repository %(reponame)s',
-                            reponame=reponame)
-                repo = self.lrc.cache_repo(reponame)
-                repo.update()
-            else:
-                # This is likely to raise an exception, because if the local
-                # repo cache had the repo we'd have already resolved the ref.
-                repo = self.lrc.get_repo(reponame)
+            repo = self.cache_repo_locally(reponame)
             absref = repo.resolve_ref_to_commit(ref)
             tree = repo.resolve_ref_to_tree(absref)
 
@@ -214,28 +212,15 @@ class SourceResolver(object):
 
         return absref, tree
 
-    def _get_morphology(self, reponame, sha1, filename):
-        '''Read the morphology at the specified location.
-
-        Returns None if the file does not exist in the specified commit.
-
-        '''
-        key = (reponame, sha1, filename)
-        if key in self._resolved_morphologies:
-            return self._resolved_morphologies[key]
-
-        if reponame == self._definitions_repo and \
-                sha1 == self._definitions_absref: # pragma: no cover
-            defs_filename = os.path.join(self._definitions_checkout_dir,
-                                         filename)
+    def _get_morphology_from_definitions(self, loader,
+                                         filename):  # pragma: no cover
+        if os.path.exists(filename):
+            return loader.load_from_file(filename)
         else:
-            defs_filename = None
-
+            return None
 
-        loader = morphlib.morphloader.MorphologyLoader()
-        if defs_filename and os.path.exists(defs_filename): # pragma: no cover
-            morph = loader.load_from_file(defs_filename)
-        elif self.lrc.has_repo(reponame):
+    def _get_morphology_from_repo(self, loader, reponame, sha1, filename):
+        if self.lrc.has_repo(reponame):
             self.status(msg="Looking for %(reponame)s:%(filename)s in the "
                             "local repo cache.",
                         reponame=reponame, filename=filename, chatty=True)
@@ -245,7 +230,6 @@ class SourceResolver(object):
                 morph = loader.load_from_string(text)
             except IOError:
                 morph = None
-                file_list = repo.list_files(ref=sha1, recurse=False)
         elif self.rrc is not None:
             self.status(msg="Looking for %(reponame)s:%(filename)s in the "
                             "remote repo cache.",
@@ -255,11 +239,37 @@ class SourceResolver(object):
                 morph = loader.load_from_string(text)
             except morphlib.remoterepocache.CatFileError:
                 morph = None
+        else:  # pragma: no cover
+            repo = self.cache_repo_locally(reponame)
+            text = repo.read_file(filename, sha1)
+            morph = loader.load_from_string(text)
+
+        return morph
+
+    def _get_morphology(self, reponame, sha1, filename):
+        '''Read the morphology at the specified location.
+
+        Returns None if the file does not exist in the specified commit.
+
+        '''
+        key = (reponame, sha1, filename)
+        if key in self._resolved_morphologies:
+            return self._resolved_morphologies[key]
+
+        loader = morphlib.morphloader.MorphologyLoader()
+        morph = None
+
+        if reponame == self._definitions_repo and \
+                sha1 == self._definitions_absref: # pragma: no cover
+            # There is a temporary local checkout of the definitions repo which
+            # we can quickly read definitions files from.
+            defs_filename = os.path.join(self._definitions_checkout_dir,
+                                         filename)
+            morph = self._get_morphology_from_definitions(loader,
+                                                          defs_filename)
         else:
-            # We assume that _resolve_ref() must have already been called and
-            # so the repo in question would have been made available already
-            # if it had been possible.
-            raise NotcachedError(reponame)
+            morph = self._get_morphology_from_repo(loader, reponame, sha1,
+                                                   filename)
 
         if morph is None:
             return None
@@ -280,16 +290,26 @@ class SourceResolver(object):
                         "chunk morph from repo's build system" %
                     expected_filename, chatty=True)
 
+        file_list = None
+
         if self.lrc.has_repo(reponame):
             repo = self.lrc.get_repo(reponame)
-            file_list = repo.list_files(ref=sha1, recurse=False)
+            try:
+                file_list = repo.list_files(ref=sha1, recurse=False)
+            except morphlib.gitdir.InvalidRefError:  # pragma: no cover
+                pass
         elif self.rrc is not None:
-            file_list = self.rrc.ls_tree(reponame, sha1)
-        else:
-            # We assume that _resolve_ref() must have already been called and
-            # so the repo in question would have been made available already
-            # if it had been possible.
-            raise NotcachedError(reponame)
+            try:
+                # This may or may not succeed; if the is repo not
+                # hosted on the same Git server as the cache server then
+                # it'll definitely fail.
+                file_list = self.rrc.ls_tree(reponame, sha1)
+            except morphlib.remoterepocache.LsTreeError:
+                pass
+
+        if not file_list:
+            repo = self.cache_repo_locally(reponame)
+            file_list = repo.list_files(ref=sha1, recurse=False)
 
         buildsystem = morphlib.buildsystem.detect_build_system(file_list)
 
@@ -320,8 +340,7 @@ class SourceResolver(object):
                                            definitions_tree,
                                            visit): # pragma: no cover
         definitions_queue = collections.deque(system_filenames)
-        chunk_in_definitions_repo_queue = set()
-        chunk_in_source_repo_queue = set()
+        chunk_queue = set()
 
         while definitions_queue:
             filename = definitions_queue.popleft()
@@ -349,35 +368,45 @@ class SourceResolver(object):
                         for s in morphology['build-depends'])
                 for c in morphology['chunks']:
                     if 'morph' not in c:
+                        # Autodetect a path if one is not given. This is to
+                        # support the deprecated approach of putting the chunk
+                        # .morph file in the toplevel directory of the chunk
+                        # repo, instead of putting it in the definitions.git
+                        # repo.
+                        #
+                        # All users should be specifying a full path to the
+                        # chunk morph file, using the 'morph' field, and this
+                        # code path should be removed.
                         path = morphlib.util.sanitise_morphology_path(
                             c.get('morph', c['name']))
-                        chunk_in_source_repo_queue.add(
-                            (c['repo'], c['ref'], path))
-                        continue
-                    chunk_in_definitions_repo_queue.add(
-                        (c['repo'], c['ref'], c['morph']))
+                        chunk_queue.add((c['repo'], c['ref'], path))
+                    else:
+                        chunk_queue.add((c['repo'], c['ref'], c['morph']))
 
-        return chunk_in_definitions_repo_queue, chunk_in_source_repo_queue
+        return chunk_queue
 
     def process_chunk(self, definition_repo, definition_ref, chunk_repo,
                       chunk_ref, filename, visit): # pragma: no cover
+        absref = None
+        tree = None
+
         definition_key = (definition_repo, definition_ref, filename)
-        chunk_key = (chunk_repo, chunk_ref, filename)
+        chunk_key = None
 
         morph_name = os.path.splitext(os.path.basename(filename))[0]
 
-        morphology = None
+        morphology = self._get_morphology(*definition_key)
         buildsystem = None
 
         if chunk_key in self._resolved_buildsystems:
             buildsystem = self._resolved_buildsystems[chunk_key]
 
-        if buildsystem is None:
-            # The morphologies aren't locally cached, so a morphology
-            # for a chunk kept in the chunk repo will be read every time.
-            # So, always keep your chunk morphs in your definitions repo,
-            # not in the chunk repo!
-            morphology = self._get_morphology(*definition_key)
+        if morphology is None and buildsystem is None:
+            # This is a slow operation (looking for a file in Git repo may
+            # potentially require cloning the whole thing).
+            absref, tree = self._resolve_ref(chunk_repo, chunk_ref)
+            chunk_key = (chunk_repo, absref, filename)
+            morphology = self._get_morphology(*chunk_key)
 
         if morphology is None:
             if buildsystem is None:
@@ -390,7 +419,9 @@ class SourceResolver(object):
                     buildsystem, morph_name)
                 self._resolved_morphologies[definition_key] = morphology
 
-        absref, tree = self._resolve_ref(chunk_repo, chunk_ref)
+        if not absref or not tree:
+            absref, tree = self._resolve_ref(chunk_repo, chunk_ref)
+
         visit(chunk_repo, chunk_ref, filename, absref, tree, morphology)
 
     def traverse_morphs(self, definitions_repo, definitions_ref,
@@ -417,28 +448,22 @@ class SourceResolver(object):
             try:
                 definitions_cached_repo = self.lrc.get_repo(definitions_repo)
             except morphlib.localrepocache.NotCached:
-                definitions_cached_repo = self.lrc.cache_repo(definitions_repo)
+                definitions_cached_repo = self.cache_repo_locally(
+                    definitions_repo)
             definitions_cached_repo.extract_commit(
                 definitions_absref, self._definitions_checkout_dir)
 
             # First, process the system and its stratum morphologies. These
             # will all live in the same Git repository, and will point to
             # various chunk morphologies.
-            chunk_in_definitions_repo_queue, chunk_in_source_repo_queue = \
-                self._process_definitions_with_children(
+            chunk_queue = self._process_definitions_with_children(
                     system_filenames, definitions_repo, definitions_ref,
                     definitions_absref, definitions_tree, visit)
 
-            # Now process all the chunks involved in the build. First those
-            # with morphologies in definitions.git, and then (for compatibility
-            # reasons only) those with the morphology in the chunk's source
-            # repository.
-            for repo, ref, filename in chunk_in_definitions_repo_queue:
+            # Now process all the chunks involved in the build.
+            for repo, ref, filename in chunk_queue:
                 self.process_chunk(definitions_repo, definitions_absref, repo,
                                    ref, filename, visit)
-
-            for repo, ref, filename in chunk_in_source_repo_queue:
-                self.process_chunk(repo, ref, repo, ref, filename, visit)
         finally:
             shutil.rmtree(self._definitions_checkout_dir)
             self._definitions_checkout_dir = None
diff --git a/morphlib/sourceresolver_tests.py b/morphlib/sourceresolver_tests.py
index 2410218a..638f593f 100644
--- a/morphlib/sourceresolver_tests.py
+++ b/morphlib/sourceresolver_tests.py
@@ -22,9 +22,8 @@ import unittest
 import morphlib
 from morphlib.sourceresolver import (SourceResolver,
                                      PickleCacheManager,
-                                     MorphologyNotFoundError,
-                                     NotcachedError)
-from morphlib.remoterepocache import CatFileError
+                                     MorphologyNotFoundError)
+from morphlib.remoterepocache import CatFileError, LsTreeError
 
 
 class FakeRemoteRepoCache(object):
@@ -135,6 +134,9 @@ class FakeLocalRepo(object):
     def list_files(self, ref, recurse):
         return self.morphologies.keys()
 
+    def update(self):
+        pass
+
 
 class FakeLocalRepoCache(object):
 
@@ -147,6 +149,9 @@ class FakeLocalRepoCache(object):
     def get_repo(self, reponame):
         return self.lr
 
+    def cache_repo(self, reponame):
+        return self.lr
+
 
 class SourceResolverTests(unittest.TestCase):
 
@@ -188,6 +193,9 @@ class SourceResolverTests(unittest.TestCase):
     def noremotefile(self, *args):
         raise CatFileError('reponame', 'ref', 'filename')
 
+    def noremoterepo(self, *args):
+        raise LsTreeError('reponame', 'ref')
+
     def localmorph(self, *args):
         return ['chunk.morph']
 
@@ -241,6 +249,15 @@ class SourceResolverTests(unittest.TestCase):
                                             'assumed-local.morph')
         self.assertEqual('autotools', name)
 
+    def test_cache_repo_if_not_in_either_cache(self):
+        self.lrc.has_repo = self.doesnothaverepo
+        self.lr.read_file = self.nolocalmorph
+        self.lr.list_files = self.autotoolsbuildsystem
+        self.rrc.ls_tree = self.noremoterepo
+        name = self.sr._detect_build_system('reponame', 'sha1',
+                                            'assumed-local.morph')
+        self.assertEqual('autotools', name)
+
     def test_autodetects_remote_morphology(self):
         self.lrc.has_repo = self.doesnothaverepo
         self.rrc.cat_file = self.noremotemorph
@@ -262,7 +279,8 @@ class SourceResolverTests(unittest.TestCase):
 
     def test_raises_error_when_repo_does_not_exist(self):
         self.lrc.has_repo = self.doesnothaverepo
-        self.assertRaises(NotcachedError, self.lsr._detect_build_system,
+        self.assertRaises(MorphologyNotFoundError,
+                          self.lsr._detect_build_system,
                           'reponame', 'sha1', 'non-existent.morph')
 
     def test_raises_error_when_failed_to_detect_build_system(self):
@@ -289,10 +307,12 @@ class SourceResolverTests(unittest.TestCase):
                                             'assumed-local.morph')
         self.assertEqual('autotools', name)
 
-    def test_fails_when_local_not_cached_and_no_remote(self):
+    def test_succeeds_when_local_not_cached_and_no_remote(self):
         self.lrc.has_repo = self.doesnothaverepo
-        self.assertRaises(NotcachedError, self.lsr._get_morphology,
-                          'reponame', 'sha1', 'unreached.morph')
+        self.lr.list_files = self.localmorph
+        morph = self.sr._get_morphology('reponame', 'sha1',
+                                       'chunk.morph')
+        self.assertEqual('chunk', morph['name'])
 
     def test_arch_is_validated(self):
         self.lr.arch = 'unknown'