4:40 p.m.
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/richardmaw/S8847/branch-from-image-v3
SHA1: a2468159c731dbcf26dbb6483bcf50b5a81bccd8
Land: origin/master
Included in this series are the documentation changes requested in the
previous one.
The _initializing_system_branch() design has not been altered beyond
moving the _checkout_has_systems() check out to the callers.
Richard Maw (4):
b&m: Add system branch initializing context manager
b&m: checkout and branch use context manager
morphlib: Add SystemMetadataDir class
B&M: refactor branch-from-image
morphlib/__init__.py | 1 +
morphlib/plugins/branch_and_merge_new_plugin.py | 182 ++++++++++++++++++-----
morphlib/plugins/branch_and_merge_plugin.py | 12 +-
morphlib/systemmetadatadir.py | 87 +++++++++++
morphlib/systemmetadatadir_tests.py | 75 ++++++++++
tests.as-root/branch-from-image-works.script | 3 +-
6 files changed, 313 insertions(+), 47 deletions(-)
create mode 100644 morphlib/systemmetadatadir.py
create mode 100644 morphlib/systemmetadatadir_tests.py
--
1.7.10.4
4:41 p.m.
New subject: [PATCH 1/4] b&m: Add system branch initializing context manager
This creates an object that the with statement can use to handle the
context and clean up the workspace if the body raises an exception.
This is roughly equivalent to having a function that takes a callback of
what to do while the branch is being initialized, but with less
boilerplate at the call site.
contextlib is used to create a context manager from a generator
function. This is less verbose than defining a class with __enter__
and __exit__ methods.
---
morphlib/plugins/branch_and_merge_new_plugin.py | 35 +++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py
b/morphlib/plugins/branch_and_merge_new_plugin.py
index 39552ef..88ace70 100644
--- a/morphlib/plugins/branch_and_merge_new_plugin.py
+++ b/morphlib/plugins/branch_and_merge_new_plugin.py
@@ -15,6 +15,7 @@
import cliapp
+import contextlib
import glob
import logging
import os
@@ -97,6 +98,40 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
ws = morphlib.workspace.open('.')
self.app.output.write('%s\n' % ws.root)
+ # TODO: Move this somewhere nicer
+ @contextlib.contextmanager
+ def _initializing_system_branch(self, ws, root_url, system_branch,
+ cached_repo, base_ref):
+ '''A context manager for system branches under construction.
+
+ The purpose of this context manager is to factor out the branch
+ cleanup code for if an exception occurs while a branch is being
+ constructed.
+
+ This could be handled by a higher order function which takes
+ a function to initialize the branch as a parameter, but with
+ statements look nicer and are more obviously about resource
+ cleanup.
+
+ '''
+ root_dir = ws.get_default_system_branch_directory_name(system_branch)
+ try:
+ sb = morphlib.sysbranchdir.create(
+ root_dir, root_url, system_branch)
+ gd = sb.clone_cached_repo(cached_repo, base_ref)
+
+ yield (sb, gd)
+
+ gd.update_submodules(self.app)
+ gd.update_remotes()
+
+ except BaseException as e:
+ # Oops. Clean up.
+ logging.error('Caught exception: %s' % str(e))
+ logging.info('Removing half-finished branch %s' % system_branch)
+ self._remove_branch_dir_safe(ws.root, root_dir)
+ raise
+
def checkout(self, args):
'''Check out an existing system branch.
--
1.7.10.4
4:41 p.m.
New subject: [PATCH 2/4] b&m: checkout and branch use context manager
---
morphlib/plugins/branch_and_merge_new_plugin.py | 43 ++++-------------------
1 file changed, 7 insertions(+), 36 deletions(-)
diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py
b/morphlib/plugins/branch_and_merge_new_plugin.py
index 88ace70..7cd8489 100644
--- a/morphlib/plugins/branch_and_merge_new_plugin.py
+++ b/morphlib/plugins/branch_and_merge_new_plugin.py
@@ -159,6 +159,7 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
root_url = args[0]
system_branch = args[1]
+ base_ref = system_branch
self._require_git_user_config()
@@ -174,27 +175,12 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
# Check the git branch exists.
cached_repo.resolve_ref(system_branch)
- root_dir = ws.get_default_system_branch_directory_name(system_branch)
-
- try:
- # Create the system branch directory. This doesn't yet clone
- # the root repository there.
- sb = morphlib.sysbranchdir.create(
- root_dir, root_url, system_branch)
-
- gd = sb.clone_cached_repo(cached_repo, system_branch)
+ with self._initializing_system_branch(
+ ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
if not self._checkout_has_systems(gd):
- raise BranchRootHasNoSystemsError(root_url, system_branch)
+ raise BranchRootHasNoSystemsError(base_ref)
- gd.update_submodules(self.app)
- gd.update_remotes()
- except BaseException as e:
- # Oops. Clean up.
- logging.error('Caught exception: %s' % str(e))
- logging.info('Removing half-finished branch %s' % system_branch)
- self._remove_branch_dir_safe(ws.root, root_dir)
- raise
def branch(self, args):
'''Create a new system branch.
@@ -247,29 +233,14 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
# Make sure the base_ref exists.
cached_repo.resolve_ref(base_ref)
- root_dir = ws.get_default_system_branch_directory_name(system_branch)
+ with self._initializing_system_branch(
+ ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
- try:
- # Create the system branch directory. This doesn't yet clone
- # the root repository there.
- sb = morphlib.sysbranchdir.create(
- root_dir, root_url, system_branch)
-
- gd = sb.clone_cached_repo(cached_repo, base_ref)
gd.branch(system_branch, base_ref)
gd.checkout(system_branch)
if not self._checkout_has_systems(gd):
- raise BranchRootHasNoSystemsError(root_url, base_ref)
-
- gd.update_submodules(self.app)
- gd.update_remotes()
- except BaseException as e:
- # Oops. Clean up.
- logging.error('Caught exception: %s' % str(e))
- logging.info('Removing half-finished branch %s' % system_branch)
- self._remove_branch_dir_safe(ws.root, root_dir)
- raise
+ raise BranchRootHasNoSystemsError(base_ref)
def _save_dirty_morphologies(self, loader, sb, morphs):
logging.debug('Saving dirty morphologies: start')
--
1.7.10.4
4:41 p.m.
New subject: [PATCH 3/4] morphlib: Add SystemMetadataDir class
This provides access to the /baserock directory as if it were a dict,
abstracting away the details of how to get data out of it.
The abstraction is useful since it is easier to use than accessing
/baserock yourself, and allows the storage format to be changed more
easily.
Keys with / in may be supported in the future. since there have been
discussions about allowing morphologies to be placed in subdirectories.
Adding this support would require creating and removing directory
components when values are set and deleted respectively.
Iterating would require using os.walk instead of glob.iglob, since
python doesn't support ** in globs.
---
morphlib/__init__.py | 1 +
morphlib/systemmetadatadir.py | 87 +++++++++++++++++++++++++++++++++++
morphlib/systemmetadatadir_tests.py | 75 ++++++++++++++++++++++++++++++
3 files changed, 163 insertions(+)
create mode 100644 morphlib/systemmetadatadir.py
create mode 100644 morphlib/systemmetadatadir_tests.py
diff --git a/morphlib/__init__.py b/morphlib/__init__.py
index b1e3c7c..7eb3f97 100644
--- a/morphlib/__init__.py
+++ b/morphlib/__init__.py
@@ -78,6 +78,7 @@ import sourcepool
import stagingarea
import stopwatch
import sysbranchdir
+import systemmetadatadir
import tempdir
import util
import workspace
diff --git a/morphlib/systemmetadatadir.py b/morphlib/systemmetadatadir.py
new file mode 100644
index 0000000..eac5b44
--- /dev/null
+++ b/morphlib/systemmetadatadir.py
@@ -0,0 +1,87 @@
+# Copyright (C) 2013 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
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# =*= License: GPL-2 =*=
+
+
+import collections
+import glob
+import json
+import os
+
+
+class SystemMetadataDir(collections.MutableMapping):
+
+ '''An abstraction over the /baserock metadata directory.
+
+ This allows methods of iterating over it, and accessing it like
+ a dict.
+
+ The /baserock metadata directory contains information about all of
+ the chunks in a built system. It exists to provide traceability from
+ the input sources to the output.
+
+ If you create the object with smd = SystemMetadataDir('/baserock')
+ data = smd['key'] will read /baserock/key.meta and return its JSON
+ encoded contents as native python objects.
+
+ smd['key'] = data will write data to /baserock/key.meta as JSON
+
+ The key may not have '\0' characters in it since the underlying
+ system calls don't support embedded NUL bytes.
+
+ The key may not have '/' characters in it since we do not support
+ morphologies with slashes in their names.
+
+ '''
+
+ def __init__(self, metadata_path):
+ collections.MutableMapping.__init__(self)
+ self._metadata_path = metadata_path
+
+ def _join_path(self, *args):
+ return os.path.join(self._metadata_path, *args)
+
+ def _raw_path_iter(self):
+ return glob.iglob(self._join_path('*.meta'))
+
+ @staticmethod
+ def _check_key(key):
+ if any(c in key for c in "\0/"):
+ raise KeyError(key)
+
+ def __getitem__(self, key):
+ self._check_key(key)
+ try:
+ with open(self._join_path('%s.meta' % key), 'r') as f:
+ return json.load(f)
+ except IOError:
+ raise KeyError(key)
+
+ def __setitem__(self, key, value):
+ self._check_key(key)
+ with open(self._join_path('%s.meta' % key), 'w') as f:
+ json.dump(value, f, indent=4, sort_keys=True)
+
+ def __delitem__(self, key):
+ self._check_key(key)
+ os.unlink(self._join_path('%s.meta' % key))
+
+ def __iter__(self):
+ return (os.path.basename(fn)[:-len('.meta')]
+ for fn in self._raw_path_iter())
+
+ def __len__(self):
+ return len(list(self._raw_path_iter()))
diff --git a/morphlib/systemmetadatadir_tests.py b/morphlib/systemmetadatadir_tests.py
new file mode 100644
index 0000000..0126f86
--- /dev/null
+++ b/morphlib/systemmetadatadir_tests.py
@@ -0,0 +1,75 @@
+# Copyright (C) 2013 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
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# =*= License: GPL-2 =*=
+
+
+import operator
+import os
+import shutil
+import tempfile
+import unittest
+
+import morphlib
+
+
+class SystemMetadataDirTests(unittest.TestCase):
+
+ def setUp(self):
+ self.tempdir = tempfile.mkdtemp()
+ self.metadatadir = os.path.join(self.tempdir, 'baserock')
+ os.mkdir(self.metadatadir)
+ self.smd = morphlib.systemmetadatadir.SystemMetadataDir(
+ self.metadatadir)
+
+ def tearDown(self):
+ shutil.rmtree(self.tempdir)
+
+ def test_add_new(self):
+ self.smd['key'] = {'foo': 'bar'}
+ self.assertEqual(self.smd['key']['foo'], 'bar')
+
+ def test_replace(self):
+ self.smd['key'] = {'foo': 'bar'}
+ self.smd['key'] = {'foo': 'baz'}
+ self.assertEqual(self.smd['key']['foo'], 'baz')
+
+ def test_remove(self):
+ self.smd['key'] = {'foo': 'bar'}
+ del self.smd['key']
+ self.assertTrue('key' not in self.smd)
+
+ def test_iterate(self):
+ self.smd['build-essential'] = "Some data"
+ self.smd['core'] = "More data"
+ self.smd['foundation'] = "Yet more data"
+ self.assertEqual(sorted(self.smd.keys()),
+ ['build-essential', 'core',
'foundation'])
+ self.assertEqual(dict(self.smd.iteritems()),
+ {
+ 'build-essential': "Some data",
+ 'core': "More data",
+ 'foundation': "Yet more data",
+ })
+
+ def test_raises_KeyError(self):
+ self.assertRaises(KeyError, operator.getitem, self.smd, 'key')
+
+ def test_validates_keys(self):
+ for key in ('foo/bar', 'baz\0quux'):
+ self.assertRaises(KeyError, operator.getitem, self.smd, key)
+ self.assertRaises(KeyError, operator.setitem,
+ self.smd, key, 'value')
+ self.assertRaises(KeyError, operator.delitem, self.smd, key)
--
1.7.10.4
4:41 p.m.
New subject: [PATCH 4/4] B&M: refactor branch-from-image
This changes the interface of branch-from-image to only take 1 parameter,
the name of the new system branch, as the root repository is loaded from
the metadata.
This was also what the previous version of branch-from-image did, but
that silently ignored the parameter.
Given there are not many users of branch-from-image, I felt it was a
reasonable change.
---
morphlib/plugins/branch_and_merge_new_plugin.py | 104 +++++++++++++++++++++++
morphlib/plugins/branch_and_merge_plugin.py | 12 +--
tests.as-root/branch-from-image-works.script | 3 +-
3 files changed, 108 insertions(+), 11 deletions(-)
diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py
b/morphlib/plugins/branch_and_merge_new_plugin.py
index 7cd8489..809699e 100644
--- a/morphlib/plugins/branch_and_merge_new_plugin.py
+++ b/morphlib/plugins/branch_and_merge_new_plugin.py
@@ -56,6 +56,15 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
arg_synopsis='-- COMMAND [ARGS...]')
self.app.add_subcommand('status', self.status,
arg_synopsis='')
+ self.app.add_subcommand('branch-from-image', self.branch_from_image,
+ arg_synopsis='BRANCH')
+ group_branch = 'Branching Options'
+ self.app.settings.string(['metadata-dir'],
+ 'Set metadata location for branch-from-image'
+ ' (default: /baserock)',
+ metavar='DIR',
+ default='/baserock',
+ group=group_branch)
def disable(self):
pass
@@ -790,3 +799,98 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
if not has_uncommitted_changes:
self.app.output.write("\nNo repos have outstanding changes.\n")
+
+ def branch_from_image(self, args):
+ '''Produce a branch of an existing system image.
+
+ Given the metadata specified by --metadata-dir, create a new
+ branch then petrify it to the state of the commits at the time
+ the system was built.
+
+ If --metadata-dir is not specified, it defaults to your currently
+ running system.
+
+ '''
+ if len(args) != 1:
+ raise cliapp.AppException(
+ "branch-from-image needs exactly 1 argument "
+ "of the new system branch's name")
+ system_branch = args[0]
+ metadata_path = self.app.settings['metadata-dir']
+ alias_resolver = morphlib.repoaliasresolver.RepoAliasResolver(
+ self.app.settings['repo-alias'])
+
+ self._require_git_user_config()
+
+ ws = morphlib.workspace.open('.')
+
+ system, metadata = self._load_system_metadata(metadata_path)
+ resolved_refs = dict(self._resolve_refs_from_metadata(alias_resolver,
+ metadata))
+ logging.debug('Resolved refs: %r' % resolved_refs)
+ base_ref = system['sha1']
+ # The previous version would fall back to deducing this from the repo
+ # url and the repo alias resolver, but this does not always work, and
+ # new systems always have repo-alias in the metadata
+ root_url = system['repo-alias']
+
+ lrc, rrc = morphlib.util.new_repo_caches(self.app)
+ cached_repo = lrc.get_updated_repo(root_url)
+
+
+ with self._initializing_system_branch(
+ ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
+
+ # TODO: It's nasty to clone to a sha1 then create a branch
+ # of that sha1 then check it out, a nicer API may be the
+ # initial clone not checking out a branch at all, then
+ # the user creates and checks out their own branches
+ gd.branch(system_branch, base_ref)
+ gd.checkout(system_branch)
+
+ loader = morphlib.morphloader.MorphologyLoader()
+ morphs = self._load_all_sysbranch_morphologies(sb, loader)
+
+ morphs.repoint_refs(sb.root_repository_url,
+ sb.system_branch_name)
+
+ morphs.petrify_chunks(resolved_refs)
+
+ self._save_dirty_morphologies(loader, sb, morphs.morphologies)
+
+ @staticmethod
+ def _load_system_metadata(path):
+ '''Load all metadata in `path` corresponding to a single System.
+ '''
+
+ smd = morphlib.systemmetadatadir.SystemMetadataDir(path)
+ metadata = smd.values()
+ systems = [md for md in metadata
+ if 'kind' in md and md['kind'] == 'system']
+
+ if not systems:
+ raise cliapp.AppException(
+ 'Metadata directory does not contain any systems.')
+ if len(systems) > 1:
+ raise cliapp.AppException(
+ 'Metadata directory contains multiple systems.')
+ system_metadatum = systems[0]
+
+ metadata_cache_id_lookup = dict((md['cache-key'], md)
+ for md in metadata)
+
+ return system_metadatum, metadata_cache_id_lookup
+
+ @staticmethod
+ def _resolve_refs_from_metadata(alias_resolver, metadata):
+ '''Pre-resolve a set of refs from existing metadata.
+
+ Given the metadata, generate a mapping of all the (repo, ref)
+ pairs defined in the metadata and the commit id they resolved to.
+
+ '''
+ for md in metadata.itervalues():
+ repourls = set((md['repo-alias'], md['repo']))
+ repourls.update(alias_resolver.aliases_from_url(md['repo']))
+ for repourl in repourls:
+ yield ((repourl, md['original_ref']), md['sha1'])
diff --git a/morphlib/plugins/branch_and_merge_plugin.py
b/morphlib/plugins/branch_and_merge_plugin.py
index 37d5e40..2f8560d 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -68,15 +68,9 @@ class BranchAndMergePlugin(cliapp.Plugin):
self.app.add_subcommand('build', self.build,
arg_synopsis='SYSTEM')
self.app.add_subcommand('old-status', self.status)
- self.app.add_subcommand('branch-from-image', self.branch_from_image,
- arg_synopsis='REPO BRANCH')
- group_branch = 'Branching Options'
- self.app.settings.string(['metadata-dir'],
- 'Set metadata location for branch-from-image'
- ' (default: /baserock)',
- metavar='DIR',
- default='/baserock',
- group=group_branch)
+ self.app.add_subcommand('old-branch-from-image',
+ self.branch_from_image,
+ arg_synopsis='REPO BRANCH')
# Advanced commands
self.app.add_subcommand('old-foreach', self.foreach,
diff --git a/tests.as-root/branch-from-image-works.script
b/tests.as-root/branch-from-image-works.script
index 9f82f62..942301e 100755
--- a/tests.as-root/branch-from-image-works.script
+++ b/tests.as-root/branch-from-image-works.script
@@ -48,8 +48,7 @@ git commit --quiet -m 'Make hello say goodbye'
workspace="$DATADIR/workspace"
"$SRCDIR/scripts/test-morph" init "$workspace"
cd "$workspace"
-"$SRCDIR/scripts/test-morph" branch-from-image \
- test:morphs mybranch \
+"$SRCDIR/scripts/test-morph" branch-from-image mybranch \
--metadata-dir="$extracted/baserock"
cd mybranch/test:morphs
grep -qFe "$hello_chunk_commit" hello-stratum.morph
--
1.7.10.4
5:12 p.m.
New subject: [RFC 0/2] Get rid of _initializing_system_branch
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/richardmaw/S8847/branch-from-image-v4
SHA1: 0b2c51289735a172267764129914a0dabd097609
Base: origin/baserock/richardmaw/S8847/branch-from-image-v3
Land: origin/master
This is an extension to the previous patch series, which gets rid of the
context manager for creating a whole system branch and replaces it with
one for the subdirectory.
The caller is expected to initialize the system branch themselves.
Richard Maw (2):
sysbranchdir: Allow creating in an empty directory
FIXME: b&m: Nuke _initializing_system_branch()
morphlib/plugins/branch_and_merge_new_plugin.py | 88 ++++++++++-------------
morphlib/sysbranchdir.py | 7 +-
morphlib/sysbranchdir_tests.py | 9 ++-
3 files changed, 49 insertions(+), 55 deletions(-)
--
1.7.10.4
5:12 p.m.
New subject: [RFC 1/2] sysbranchdir: Allow creating in an empty directory
This is convenient as it allows something else to manage the directory's
lifetime.
---
morphlib/sysbranchdir.py | 7 ++++---
morphlib/sysbranchdir_tests.py | 9 ++++++++-
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/morphlib/sysbranchdir.py b/morphlib/sysbranchdir.py
index a05ca52..7dc3011 100644
--- a/morphlib/sysbranchdir.py
+++ b/morphlib/sysbranchdir.py
@@ -166,7 +166,7 @@ def create(root_directory, root_repository_url, system_branch_name):
Return a SystemBranchDirectory object that represents the directory.
- The directory MUST NOT exist already. If it does,
+ The directory MUST NOT exist already or must be empty. If it is not,
SystemBranchDirectoryAlreadyExists is raised.
Note that this does NOT check out the root repository, or do any
@@ -174,11 +174,12 @@ def create(root_directory, root_repository_url,
system_branch_name):
'''
- if os.path.exists(root_directory):
+ if not os.path.exists(root_directory):
+ os.makedirs(root_directory)
+ elif os.listdir(root_directory):
raise SystemBranchDirectoryAlreadyExists(root_directory)
magic_dir = os.path.join(root_directory, '.morph-system-branch')
- os.makedirs(root_directory)
os.mkdir(magic_dir)
sb = SystemBranchDirectory(
diff --git a/morphlib/sysbranchdir_tests.py b/morphlib/sysbranchdir_tests.py
index 7ec8ef5..0a7f578 100644
--- a/morphlib/sysbranchdir_tests.py
+++ b/morphlib/sysbranchdir_tests.py
@@ -123,8 +123,15 @@ class SystemBranchDirectoryTests(unittest.TestCase):
self.assertEqual(sb.root_repository_url, self.root_repository_url)
self.assertEqual(sb.system_branch_name, self.system_branch_name)
- def test_fails_to_create_if_directory_already_exists(self):
+ def test_creates_if_directory_empty(self):
os.mkdir(self.root_directory)
+ morphlib.sysbranchdir.create(self.root_directory,
+ self.root_repository_url,
+ self.system_branch_name)
+
+ def test_fails_to_create_if_directory_non_empty(self):
+ os.mkdir(self.root_directory)
+ os.mkdir(os.path.join(self.root_directory, 'foo'))
self.assertRaises(
morphlib.sysbranchdir.SystemBranchDirectoryAlreadyExists,
morphlib.sysbranchdir.create,
--
1.7.10.4
5:12 p.m.
New subject: [RFC 2/2] b&m: Nuke _initializing_system_branch()
In its place is _atomic_subdirectory, which handles cleaning up the
system branch if an exception is raised.
Its api is designed such that it could create temporary directories in
the future and rename them into place, but this is not implemented.
Now, subcommands that create a system branch use _atomic_subdirectory to
make the required path appear, then create the branch in that, instead
of that being handled by a helper function, since the details between
creating and being finished varies too much to be worth attempting to
find common logic to factor out.
---
morphlib/plugins/branch_and_merge_new_plugin.py | 96 ++++++++++++-----------
1 file changed, 49 insertions(+), 47 deletions(-)
diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py
b/morphlib/plugins/branch_and_merge_new_plugin.py
index 809699e..96d6829 100644
--- a/morphlib/plugins/branch_and_merge_new_plugin.py
+++ b/morphlib/plugins/branch_and_merge_new_plugin.py
@@ -108,37 +108,44 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
self.app.output.write('%s\n' % ws.root)
# TODO: Move this somewhere nicer
+ @staticmethod
@contextlib.contextmanager
- def _initializing_system_branch(self, ws, root_url, system_branch,
- cached_repo, base_ref):
- '''A context manager for system branches under construction.
+ def _atomic_subdirectory(root, subdir):
+ '''Context manager for creating a directory and its contents.
+
+ It takes a root directory and the subdirectory which will have
+ been created after the context manager exits.
+
+ The __enter__ method returns a path to where the subdirectory
+ can be initialized.
- The purpose of this context manager is to factor out the branch
- cleanup code for if an exception occurs while a branch is being
- constructed.
+ The separation between the subdirectory passed and the
+ subdirectory to initialize is to allow the manager to use a
+ temporary directory and rename it into place after it has been
+ initialized.
- This could be handled by a higher order function which takes
- a function to initialize the branch as a parameter, but with
- statements look nicer and are more obviously about resource
- cleanup.
+ Renaming a tempdir into place is not yet implemented.
'''
- root_dir = ws.get_default_system_branch_directory_name(system_branch)
+ os.makedirs(subdir)
try:
- sb = morphlib.sysbranchdir.create(
- root_dir, root_url, system_branch)
- gd = sb.clone_cached_repo(cached_repo, base_ref)
+ yield subdir
+ except BaseException as e:
+ logging.error('Caught exception: %s' % str(e))
- yield (sb, gd)
+ def handle_error(function, path, excinfo):
+ logging.warning ("Error while trying to clean up %s: %s" %
+ (path, excinfo))
+ shutil.rmtree(subdir, onerror=handle_error)
- gd.update_submodules(self.app)
- gd.update_remotes()
+ # Remove parent directories that are empty too, avoiding exceptions
+ parent = os.path.dirname(subdir)
+ while parent != os.path.abspath(root):
+ if len(os.listdir(parent)) > 0 or os.path.islink(parent):
+ break
+ os.rmdir(parent)
+ parent = os.path.dirname(parent)
- except BaseException as e:
- # Oops. Clean up.
- logging.error('Caught exception: %s' % str(e))
- logging.info('Removing half-finished branch %s' % system_branch)
- self._remove_branch_dir_safe(ws.root, root_dir)
raise
def checkout(self, args):
@@ -184,13 +191,17 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
# Check the git branch exists.
cached_repo.resolve_ref(system_branch)
- with self._initializing_system_branch(
- ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
+ with self._atomic_subdirectory(
+ ws.root,
+ ws.get_default_system_branch_directory_name(system_branch)) \
+ as path:
+
+ sb = morphlib.sysbranchdir.create(path, root_url, system_branch)
+ gd = sb.clone_cached_repo(cached_repo, base_ref)
if not self._checkout_has_systems(gd):
raise BranchRootHasNoSystemsError(base_ref)
-
def branch(self, args):
'''Create a new system branch.
@@ -242,8 +253,13 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
# Make sure the base_ref exists.
cached_repo.resolve_ref(base_ref)
- with self._initializing_system_branch(
- ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
+ with self._atomic_subdirectory(
+ ws.root,
+ ws.get_default_system_branch_directory_name(system_branch)) \
+ as path:
+
+ sb = morphlib.sysbranchdir.create(path, root_url, system_branch)
+ gd = sb.clone_cached_repo(cached_repo, base_ref)
gd.branch(system_branch, base_ref)
gd.checkout(system_branch)
@@ -527,24 +543,6 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
sb = morphlib.sysbranchdir.open_from_within('.')
self.app.output.write('%s\n' % sb.get_config('branch.root'))
- def _remove_branch_dir_safe(self, workspace_root, system_branch_root):
- # This function avoids throwing any exceptions, so it is safe to call
- # inside an 'except' block without altering the backtrace.
-
- def handle_error(function, path, excinfo):
- logging.warning ("Error while trying to clean up %s: %s" %
- (path, excinfo))
-
- shutil.rmtree(system_branch_root, onerror=handle_error)
-
- # Remove parent directories that are empty too, avoiding exceptions
- parent = os.path.dirname(system_branch_root)
- while parent != os.path.abspath(workspace_root):
- if len(os.listdir(parent)) > 0 or os.path.islink(parent):
- break
- os.rmdir(parent)
- parent = os.path.dirname(parent)
-
def _require_git_user_config(self):
'''Warn if the git user.name and user.email variables are not
set.'''
@@ -837,9 +835,13 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
lrc, rrc = morphlib.util.new_repo_caches(self.app)
cached_repo = lrc.get_updated_repo(root_url)
+ with self._atomic_subdirectory(
+ ws.root,
+ ws.get_default_system_branch_directory_name(system_branch)) \
+ as path:
- with self._initializing_system_branch(
- ws, root_url, system_branch, cached_repo, base_ref) as (sb, gd):
+ sb = morphlib.sysbranchdir.create(path, root_url, system_branch)
+ gd = sb.clone_cached_repo(cached_repo, base_ref)
# TODO: It's nasty to clone to a sha1 then create a branch
# of that sha1 then check it out, a nicer API may be the
--
1.7.10.4
4:03 p.m.
On Wed, Sep 25, 2013 at 03:40:59PM +0100, Richard Maw wrote:
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/richardmaw/S8847/branch-from-image-v3
SHA1: a2468159c731dbcf26dbb6483bcf50b5a81bccd8
Land: origin/master
+1 (passes "./check --full" as well).
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
4:16 p.m.
New subject: [RFC 0/2] Get rid of _initializing_system_branch
On Wed, Sep 25, 2013 at 04:12:24PM +0100, Richard Maw wrote:
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/richardmaw/S8847/branch-from-image-v4
SHA1: 0b2c51289735a172267764129914a0dabd097609
Base: origin/baserock/richardmaw/S8847/branch-from-image-v3
Land: origin/master
This is an extension to the previous patch series, which gets rid of the
context manager for creating a whole system branch and replaces it with
one for the subdirectory.
The caller is expected to initialize the system branch themselves.
I'm not happy with the API in this variant either. I'm going to -1
this, on the grounds that the API in the version I already +1'd is
good enough, and not significantly worse than this API, and I think we
can look at this later and come up with something much better.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
4:41 p.m.
On Wed, Sep 25, 2013 at 03:40:59PM +0100, Richard Maw wrote:
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/richardmaw/S8847/branch-from-image-v3
SHA1: a2468159c731dbcf26dbb6483bcf50b5a81bccd8
Land: origin/master
Included in this series are the documentation changes requested in the
previous one.
The _initializing_system_branch() design has not been altered beyond
moving the _checkout_has_systems() check out to the callers.
I have tested this by:
* ./check --full passes with an attempted merge with master.
* The code changes look sane to my knowledge (I'll defer to others
who know the code better)
* It does the job it's expected to - branch-from-image on a baserock 10
system has an identical tree-sha1 to the baserock/release/baserock-10
branch of morphs.
Consider it a +1
4:49 p.m.
On Mon, Sep 30, 2013 at 03:41:36PM +0100, Jonathan Maw wrote:
I have tested this by:
* ./check --full passes with an attempted merge with master.
* The code changes look sane to my knowledge (I'll defer to others
who know the code better)
* It does the job it's expected to - branch-from-image on a baserock 10
system has an identical tree-sha1 to the baserock/release/baserock-10
branch of morphs.
Consider it a +1
On instruction from Lars Wirzenius, I have merged this branch.
3458
days inactive
3463
days old
11 comments
3 participants
participants (3)
-
Jonathan Maw -
Lars Wirzenius -
Richard Maw