11:41 a.m.
On 10/18/2012 10:29 AM, Richard Maw wrote:
On Wed, Oct 17, 2012 at 01:48:14PM +0100, Sam Thursfield wrote:
> This change causes 'morph petrify' to avoid petrifying any chunk whose
> ref matches the current system branch, because it makes no sense to
> petrify something that is also being edited.
What about a release branch, where part of the release process is to
brand your component with the release name, and this change doesn't go
into the main line of development, so it's clear that any builds that
aren't from a release are development versions.
In this case it would be wasteful to have to create a branch for the
branding change, then create another branch for releasing with the
branding.
The best way to solve this would be to have a way of passing a parameter
to the chunk from the system or stratum morphology or some other
mechanism, so it didn't require manual editing on each release.
Lacking that, the procedure would be this:
morph branch baserock:morphs release/tracy-island master
morph edit base-system-x86_64-generic foundation foo
... edit branding ...
morph petrify
As long as nobody commits on the release/tracy-island branch in foo then
the integrity of the release is preserved.
However, maybe this isn't good enough - do you think we need a 'petrify
--full' or some such command?
> A non-obvious effect of this is that if you try to petrify
'master',
> many of the chunks won't get petrified because they are built from
> 'master'. However, petrifying master makes no sense so I'm not sure
> that we need to worry.
For full development, certainly, but it is common to hack on master for
a bit until you get an idea of what you're doing. I agree that it's not
a common problem though, so I'm happy with the change.
> diff --git a/morphlib/plugins/branch_and_merge_plugin.py
b/morphlib/plugins/branch_and_merge_plugin.py
> index 384f055..3f8ca91 100644
> --- a/morphlib/plugins/branch_and_merge_plugin.py
> +++ b/morphlib/plugins/branch_and_merge_plugin.py
> @@ -743,24 +746,42 @@ class BranchAndMergePlugin(cliapp.Plugin):
> + key = (stratum_info['repo'], stratum_info['morph'])
Keying strata by repository and morphology name worry me slightly, since
in other places we treat the repo, ref and name as the unique key, which
means we'd have a problem if something uses strata where their only
difference is the ref.
I do agree that this is a rare and stupid thing to do though, and
petrifying as a whole is likely to be problematic in that case, so we
can deal with that later.
> + if key in strata:
> + original_ref = strata[key]
> + if stratum_info['ref'] == branch:
> + strata[key] = branch
> + elif stratum_info['ref'] != original_ref:
> + if original_ref != branch:
> + self.app.output.write(
> + 'WARNING: not merging any differences from
'
> + 'ref %s into %s of stratum %s\n' %
> + (stratum_info['ref'], original_ref,
> + stratum_info['morph']))
> + stratum_info['ref'] = branch
Is this right? It appears to warn that it's not merging, but still
changes the ref of the stratum info.
What I mean here is that it's just ignoring one and taking the other, as
opposed to trying to merge the two. It's only triggered in the situation
you mention above where the same stratum is included twice at two
different refs in a system, which we should probably just disallow.
> + else:
> + strata[key] = stratum_info['ref']
> + stratum_info['ref'] = branch
> self.save_morphology(root_repo_dir, name, morphology)
>
> + for (repo, morph), ref in strata.iteritems():
> + repo_dir = self.edit_stratum(
> + branch, branch_path, root_repo, root_repo_dir,
> + { 'repo': repo, 'ref': ref, 'morph':
morph})
You could probably use locals() here instead of creating the dict, but
it's probably better like this.
My other thought was storing the stratum_info dict in 'stata' instead of
just the ref, so we could pass it ... but I was meant to be writing
documentation anyway :)
> +
> + stratum = self.load_morphology(repo_dir, stratum_info['morph'])
> +
> + for chunk_info in stratum['chunks']:
> + if chunk_info['ref'] != branch and \
> + 'unpetrify-ref' not in chunk_info:
PEP8 says to use parentheses rather than \ to handle line breaks in long
expressions, but if it turns out to be the only problem I'm happy to fix
that up in the merge.
Fine by me (I'm pretty sure I've made that mistake in a few other places)
Thanks for the review!
Sam