--- morphlib/morphologyfactory.py | 5 +++++ morphlib/morphologyfactory_tests.py | 13 +++++++++++++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/morphlib/morphologyfactory.py b/morphlib/morphologyfactory.py index de9e03e..5a049d0 100644 --- a/morphlib/morphologyfactory.py +++ b/morphlib/morphologyfactory.py @@ -18,22 +18,27 @@ import morphlib import cliapp + class MorphologyFactoryError(cliapp.AppException): pass + class AutodetectError(MorphologyFactoryError): def __init__(self, repo_name, ref): MorphologyFactoryError.__init__(self, "Failed to determine the build system of repo %s at " "ref %s" % (repo_name, ref)) + class NotcachedError(MorphologyFactoryError): def __init__(self, repo_name): MorphologyFactoryError.__init__(self, "Repository %s is not cached locally and there is no " "remote cache specified" % repo_name) + class MorphologyFactory(object): + '''An way of creating morphologies which will provide a default''' def __init__(self, local_repo_cache, remote_repo_cache=None): diff --git a/morphlib/morphologyfactory_tests.py b/morphlib/morphologyfactory_tests.py index 0cd9e4b..11d53a1 100644 --- a/morphlib/morphologyfactory_tests.py +++ b/morphlib/morphologyfactory_tests.py @@ -22,7 +22,9 @@ from morphlib.morphologyfactory import (MorphologyFactory, NotcachedError) from morphlib.remoterepocache import CatFileError + class FakeRemoteRepoCache(object): + def cat_file(self, reponame, sha1, filename): if filename.endswith('.morph'): return '''{ @@ -32,7 +34,9 @@ class FakeRemoteRepoCache(object): }''' return 'text' + class FakeLocalRepo(object): + def cat(self, sha1, filename): if filename.endswith('.morph'): return '''{ @@ -42,15 +46,21 @@ class FakeLocalRepo(object): }''' return 'text' + class FakeLocalRepoCache(object): + def __init__(self, lr): self.lr = lr + def has_repo(self, reponame): return True + def get_repo(self, reponame): return self.lr + class MorphologyFactoryTests(unittest.TestCase): + def setUp(self): self.lr = FakeLocalRepo() self.lrc = FakeLocalRepoCache(self.lr) @@ -60,12 +70,15 @@ class MorphologyFactoryTests(unittest.TestCase): def nolocalfile(self, *args): raise IOError('File not found') + def noremotefile(self, *args): raise CatFileError('reponame', 'ref', 'filename') + def nolocalmorph(self, *args): if args[-1].endswith('.morph'): raise IOError('File not found') return 'text' + def noremotemorph(self, *args): if args[-1].endswith('.morph'): raise CatFileError('reponame', 'ref', 'filename') -- 1.7.2.5

This way we can have one place in the code where we determine what artifacts get built from a specific morphology, rather than spreading the information around the code base. to get the list of artifacts. ArtifactResolver has been changed to do that. Some of the tests are now a bit messier, and should really be changed to create Morphology objects using MorphologyFactory, but that's a change for another day. --- morphlib/artifactresolver.py | 24 ++++--------- morphlib/artifactresolver_tests.py | 14 +++++++- morphlib/cachekeycomputer_tests.py | 8 ++++ morphlib/morphologyfactory.py | 59 +++++++++++++++++++++++++-------- morphlib/morphologyfactory_tests.py | 61 ++++++++++++++++++++++++++++++++++- 5 files changed, 134 insertions(+), 32 deletions(-) diff --git a/morphlib/artifactresolver.py b/morphlib/artifactresolver.py index 9a589ad..1b47a72 100644 --- a/morphlib/artifactresolver.py +++ b/morphlib/artifactresolver.py @@ -93,13 +93,9 @@ class ArtifactResolver(object): source = queue.popleft() if source.morphology['kind'] == 'system': - if source.morphology['arch'] == 'arm': - systems = [self._get_artifact( - source, source.morphology['name'] + name) - for name in ('-rootfs', '-kernel')] - else: - systems = [self._get_artifact( - source, source.morphology['name']+'-rootfs')] + systems = [self._get_artifact(source, a) + for a in source.morphology.builds_artifacts] + if any(a not in self._added_artifacts for a in systems): artifacts.extend(systems) @@ -113,8 +109,9 @@ class ArtifactResolver(object): artifacts.append(artifact) self._added_artifacts.add(artifact) elif source.morphology['kind'] == 'stratum': - artifact = self._get_artifact( - source, source.morphology['name']) + assert len(source.morphology.builds_artifacts) == 1 + artifact = self._get_artifact(source, + source.morphology.builds_artifacts[0]) if not artifact in self._added_artifacts: artifacts.append(artifact) @@ -128,7 +125,7 @@ class ArtifactResolver(object): artifacts.append(artifact) self._added_artifacts.add(artifact) elif source.morphology['kind'] == 'chunk': - names = self._chunk_artifact_names(source) + names = source.morphology.builds_artifacts for name in names: artifact = self._get_artifact(source, name) if not artifact in self._added_artifacts: @@ -207,7 +204,7 @@ class ArtifactResolver(object): info['ref'], '%s.morph' % info['morph']) - possible_names = self._chunk_artifact_names(chunk_source) + possible_names = chunk_source.morphology.builds_artifacts if not info['name'] in possible_names: raise UndefinedChunkArtifactError(stratum.source, info['name']) @@ -248,8 +245,3 @@ class ArtifactResolver(object): return artifacts - def _chunk_artifact_names(self, source): - if len(source.morphology['chunks']) > 0: - return sorted(source.morphology['chunks'].keys()) - else: - return [source.morphology['name']] diff --git a/morphlib/artifactresolver_tests.py b/morphlib/artifactresolver_tests.py index 5dc7f29..055343d 100644 --- a/morphlib/artifactresolver_tests.py +++ b/morphlib/artifactresolver_tests.py @@ -37,6 +37,7 @@ class FakeChunkMorphology(morphlib.morph2.Morphology): "chunks": %s } ''' % (name, json.dumps(artifacts))) + self.builds_artifacts = artifact_names else: text = (''' { @@ -44,8 +45,9 @@ class FakeChunkMorphology(morphlib.morph2.Morphology): "kind": "chunk" } ''' % name) + self.builds_artifacts = [name] morphlib.morph2.Morphology.__init__(self, text) - + class FakeStratumMorphology(morphlib.morph2.Morphology): @@ -81,6 +83,7 @@ class FakeStratumMorphology(morphlib.morph2.Morphology): } ''' % (name, json.dumps(build_depends))) + self.builds_artifacts = [name] morphlib.morph2.Morphology.__init__(self, text) @@ -136,6 +139,7 @@ class ArtifactResolverTests(unittest.TestCase): pool.add(source) artifacts = self.resolver.resolve_artifacts(pool) + artifacts.sort(key=lambda a: a.name) self.assertEqual(len(artifacts), 2) @@ -159,6 +163,7 @@ class ArtifactResolverTests(unittest.TestCase): "kind": "stratum" } ''') + morph.builds_artifacts = ['foo'] stratum = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'foo.morph') pool.add(stratum) @@ -180,6 +185,7 @@ class ArtifactResolverTests(unittest.TestCase): "kind": "system" } ''') + morph.builds_artifacts = ['foo-rootfs'] system = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'foo.morph') pool.add(system) @@ -202,6 +208,7 @@ class ArtifactResolverTests(unittest.TestCase): "arch": "arm" } ''') + morph.builds_artifacts = ['foo-rootfs', 'foo-kernel'] system = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'foo.morph') pool.add(system) @@ -452,6 +459,7 @@ class ArtifactResolverTests(unittest.TestCase): ] } ''') + morph.builds_artifacts = ['system-rootfs'] system = morphlib.source.Source( 'repo', 'ref', 'sha1', morph, 'system.morph') pool.add(system) @@ -514,6 +522,7 @@ class ArtifactResolverTests(unittest.TestCase): ] } ''') + morph.builds_artifacts = ['stratum'] stratum = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'stratum.morph') pool.add(stratum) @@ -660,6 +669,7 @@ class ArtifactResolverTests(unittest.TestCase): ] } ''') + morph.builds_artifacts = ['stratum'] stratum = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'stratum.morph') pool.add(stratum) @@ -708,6 +718,7 @@ class ArtifactResolverTests(unittest.TestCase): ] } ''') + morph.builds_artifacts = ['stratum'] stratum = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'stratum.morph') pool.add(stratum) @@ -743,6 +754,7 @@ class ArtifactResolverTests(unittest.TestCase): ] } ''') + morph.builds_artifacts = ['stratum'] stratum = morphlib.source.Source( 'repo', 'original/ref', 'sha1', morph, 'stratum.morph') pool.add(stratum) diff --git a/morphlib/cachekeycomputer_tests.py b/morphlib/cachekeycomputer_tests.py index 82d3e6e..4847218 100644 --- a/morphlib/cachekeycomputer_tests.py +++ b/morphlib/cachekeycomputer_tests.py @@ -85,6 +85,14 @@ class CacheKeyComputerTests(unittest.TestCase): source = morphlib.source.Source('repo', 'original/ref', 'sha', morphlib.morph2.Morphology(text), name) self.source_pool.add(source) + # FIXME: This should use MorphologyFactory + m = source.morphology + if m['kind'] == 'system': + m.builds_artifacts = [m['name'] + '-rootfs'] + elif m['kind'] == 'stratum': + m.builds_artifacts = [m['name']] + elif m['kind'] == 'chunk': + m.builds_artifacts = [m['name']] self.build_env = DummyBuildEnvironment({ "USER": "foouser", "USERNAME": "foouser", diff --git a/morphlib/morphologyfactory.py b/morphlib/morphologyfactory.py index b28f2a4..c53d247 100644 --- a/morphlib/morphologyfactory.py +++ b/morphlib/morphologyfactory.py @@ -50,20 +50,12 @@ class MorphologyFactory(object): text = self._autodetect_text(reponame, sha1, filename) morphology = morphlib.morph2.Morphology(text) - if morphology['kind'] == 'system' and \ - morphology['arch'] is None: #pragma: no cover - raise morphlib.Error('No arch specified in system %s ' - '(arch is a mandatory field)' % - filename) - if morphology['kind'] == 'stratum': #pragma: no cover - for source in morphology['sources']: - if source.get('build-depends', None) is None: - name = source.get('name', source.get('repo', 'unknown')) - raise morphlib.Error('No build dependencies ' - 'stratum %s for chunk %s ' - '(build-depends is a mandatory ' - 'field)' % - (filename, name)) + + method_name = '_check_and_tweak_%s' % morphology['kind'] + if hasattr(self, method_name): + method = getattr(self, method_name) + method(morphology, reponame, sha1, filename) + return morphology def _cat_text(self, reponame, sha1, filename): @@ -106,3 +98,42 @@ class MorphologyFactory(object): morph_name = filename[:-len('.morph')] morph_text = bs.get_morphology_text(morph_name) return morph_text + + def _check_and_tweak_system(self, morphology, reponame, sha1, filename): + '''Check and tweak a system morphology.''' + + if morphology['arch'] is None: #pragma: no cover + raise morphlib.Error('No arch specified in system %s ' + '(arch is a mandatory field)' % + filename) + + name = morphology['name'] + if morphology['arch'] == 'arm': + morphology.builds_artifacts = [name + '-kernel', name + '-rootfs'] + else: + # FIXME: -rootfs is a misnomer, should be -disk, but can't + # change this during refactoring. + morphology.builds_artifacts = [name + '-rootfs'] + + def _check_and_tweak_stratum(self, morphology, reponame, sha1, filename): + '''Check and tweak a stratum morphology.''' + + for source in morphology['sources']: # pragma: no cover + if source.get('build-depends', None) is None: + name = source.get('name', source.get('repo', 'unknown')) + raise morphlib.Error('No build dependencies ' + 'stratum %s for chunk %s ' + '(build-depends is a mandatory ' + 'field)' % + (filename, name)) + + morphology.builds_artifacts = [morphology['name']] + + def _check_and_tweak_chunk(self, morphology, reponame, sha1, filename): + '''Check and tweak a chunk morphology.''' + + if 'chunks' in morphology and len(morphology['chunks']) > 1: + morphology.builds_artifacts = morphology['chunks'].keys() + else: + morphology.builds_artifacts = [morphology['name']] + diff --git a/morphlib/morphologyfactory_tests.py b/morphlib/morphologyfactory_tests.py index 11d53a1..89b66d0 100644 --- a/morphlib/morphologyfactory_tests.py +++ b/morphlib/morphologyfactory_tests.py @@ -37,8 +37,42 @@ class FakeRemoteRepoCache(object): class FakeLocalRepo(object): + morphologies = { + 'chunk.morph': '''{ + "name": "local-foo", + "kind": "chunk", + "build-system": "bar" + }''', + 'chunk-split.morph': '''{ + "name": "local-foo", + "kind": "chunk", + "build-system": "bar", + "chunks": { + "local-foo-runtime": [], + "local-foo-devel": [] + } + }''', + 'stratum.morph': '''{ + "name": "foo-stratum", + "kind": "stratum" + }''', + 'system.morph': '''{ + "name": "foo-system", + "kind": "system", + "arch": "%(arch)s" + }''', + } + + def __init__(self): + self.arch = 'unknown' + def cat(self, sha1, filename): - if filename.endswith('.morph'): + if filename in self.morphologies: + values = { + 'arch': self.arch, + } + return self.morphologies[filename] % values + elif filename.endswith('.morph'): return '''{ "name": "local-foo", "kind": "chunk", @@ -138,3 +172,28 @@ class MorphologyFactoryTests(unittest.TestCase): self.lrc.has_repo = self.doesnothaverepo self.assertRaises(NotcachedError, self.lmf.get_morphology, 'reponame', 'sha1', 'unreached.morph') + + def test_sets_builds_artifacts_for_simple_chunk(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'chunk.morph') + self.assertEqual(morph.builds_artifacts, ['local-foo']) + + def test_sets_builds_artifacts_for_split_chunk(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'chunk-split.morph') + self.assertEqual(morph.builds_artifacts, + ['local-foo-runtime', 'local-foo-devel']) + + def test_sets_builds_artifacts_for_artifact(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'stratum.morph') + self.assertEqual(morph.builds_artifacts, ['foo-stratum']) + + def test_sets_builds_artifacts_for_x86_64_system(self): + self.lr.arch = 'x86_64' + morph = self.mf.get_morphology('reponame', 'sha1', 'system.morph') + self.assertEqual(morph.builds_artifacts, ['foo-system-rootfs']) + + def test_sets_builds_artifacts_for_arm_system(self): + self.lr.arch = 'arm' + morph = self.mf.get_morphology('reponame', 'sha1', 'system.morph') + self.assertEqual(sorted(morph.builds_artifacts), + sorted(['foo-system-rootfs', 'foo-system-kernel'])) + -- 1.7.2.5

On Fri, Jul 20, 2012 at 11:12:46AM +0100, Richard Maw wrote: Good catch. Fixed. That would be a change to what we're currently doing, so I'm not going to do that right now. I'm not sure I want to force everyone to write a boilerplate chunks listing for the simple cases (where everything is in the same chunk artifact, which is named the as the source chunk). But we can discuss that separately some day. Since I've made minimal changes, and Richard isn't available right now for further review, and I need this for other work to happen, I'll go ahead and push. -- http://www.codethink.co.uk/

--- morphlib/app.py | 4 +--- morphlib/morphologyfactory.py | 3 +++ morphlib/morphologyfactory_tests.py | 12 ++++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/morphlib/app.py b/morphlib/app.py index 96c98cf..b478989 100755 --- a/morphlib/app.py +++ b/morphlib/app.py @@ -288,9 +288,7 @@ class BuildCommand(object): name=artifact.name) copy(self.rac.get(artifact), self.lac.put(artifact)) - # For strata we also need the metadata in the local cache. - # FIXME: why do we need it and can we fix things so we don't? - if artifact.source.morphology['kind'] == 'stratum': + if artifact.source.morphology.needs_artifact_metadata_cached: if not self.lac.has_artifact_metadata(artifact, 'meta'): self.app.status(msg='Fetching to local cache: ' 'artifact metadata %(name)s', diff --git a/morphlib/morphologyfactory.py b/morphlib/morphologyfactory.py index ac743f7..4f9f752 100644 --- a/morphlib/morphologyfactory.py +++ b/morphlib/morphologyfactory.py @@ -116,6 +116,7 @@ class MorphologyFactory(object): morphology.builds_artifacts = [name + '-rootfs'] morphology.needs_staging_area = False + morphology.needs_artifact_metadata_cached = False def _check_and_tweak_stratum(self, morphology, reponame, sha1, filename): '''Check and tweak a stratum morphology.''' @@ -131,6 +132,7 @@ class MorphologyFactory(object): morphology.builds_artifacts = [morphology['name']] morphology.needs_staging_area = False + morphology.needs_artifact_metadata_cached = True def _check_and_tweak_chunk(self, morphology, reponame, sha1, filename): '''Check and tweak a chunk morphology.''' @@ -141,4 +143,5 @@ class MorphologyFactory(object): morphology.builds_artifacts = [morphology['name']] morphology.needs_staging_area = True + morphology.needs_artifact_metadata_cached = False diff --git a/morphlib/morphologyfactory_tests.py b/morphlib/morphologyfactory_tests.py index 4aa3c01..9176884 100644 --- a/morphlib/morphologyfactory_tests.py +++ b/morphlib/morphologyfactory_tests.py @@ -209,3 +209,15 @@ class MorphologyFactoryTests(unittest.TestCase): morph = self.mf.get_morphology('reponame', 'sha1', 'system.morph') self.assertEqual(morph.needs_staging_area, False) + def test_does_not_set_needs_artifact_metadata_cached_for_chunk(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'chunk.morph') + self.assertEqual(morph.needs_artifact_metadata_cached, False) + + def test_sets_artifact_metadata_cached_for_stratum(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'stratum.morph') + self.assertEqual(morph.needs_artifact_metadata_cached, True) + + def test_does_not_set_artifact_metadata_cached_for_system(self): + morph = self.mf.get_morphology('reponame', 'sha1', 'system.morph') + self.assertEqual(morph.needs_artifact_metadata_cached, False) + -- 1.7.2.5