[PATCH] Added scenario test for preventing cross-building

Lars Wirzenius lars.wirzenius at codethink.co.uk
Tue Oct 22 12:09:41 BST 2013


On Fri, Oct 18, 2013 at 03:47:35PM +0100, Richard Maw wrote:
> On Fri, Oct 18, 2013 at 03:20:28PM +0100, Daniel Firth wrote:
> > --- a/morphlib/plugins/branch_and_merge_plugin.py
> > +++ b/morphlib/plugins/branch_and_merge_plugin.py
> > @@ -1732,7 +1732,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
> >              env['GIT_COMMITTER_NAME'] = committer_name
> >              env['GIT_COMMITTER_EMAIL'] = committer_email
> >  
> > -            # Read tree from current HEAD into the morph index.
> > +            # Read tree from current HEAD into the morph index
> >              self.app.runcmd(['git', 'read-tree', system_branch_sha1],
> >                              cwd=repo_dir, env=env)
> >  
> 
> This is an irrelevant change, I'd rebase it out.

I agree.

> > +"morph cross-built-error" tests
> > +================================
> > +
> > +    SCENARIO building a system for a different architecture
> > +    GIVEN a workspace
> > +    AND a morph config file
> > +    AND a system branch
> > +    AND a system A that is not for our architecture
> > +    WHEN attempting to build the system A
> 
> I would include the base-system-testarch in the names, rather than hide
> it in the implementation.

I agree.

> 
> > +    THEN morph failed
> > +    AND error message includes the string "Are you trying to cross-build?"
> 
> This makes it fragile to language changes. Ideally we'd have morph return
> a set of distinct and well specified exit codes, but it's too much work
> right now.

I'm afraid exit codes won't work for this: exit codes are 7 bit
unsigned integers (or rather, the values the program exiting can
control are), so a value space of 127 error codes remains. In a
program as large as Morph, that's not going to be enough.

The better solution would be to have unique error codes for all error
messages. This is bit of pain to manage manually, but for a lot of
things we could manage by using the exception class name and have very
specific exception classes, instead of overloading cliapp.AppException
or morphlib.Error with meanings.

On the other hand, making those changes in this patch series is
inappropriate: let's try to keep patch series to the point, and not
fix unrelated things unless they impact the patch series directly. At
some point, I'd welcome a discussion of how error messages should be
handled, but for this patch, I'm willing to accept the error message
string matching Dan's done.

> > +    IMPLEMENTS GIVEN a system A that is not for our architecture
> > +    cat << EOF > "$DATADIR/workspace/system_branch/test:morphs/base-system-testarch.morph"
> > +    arch: testarch
> 
> Instead of adding an extra testarch to the list of valid architectures, I
> would default to using armv7b, and if we are armv7b then use armv7l. This
> also ties the test to the implementation, but we don't have a better
> way to do it at the moment anyway, so I don't mind adding a testarch.
> 
> In the future we may work out a way to not hard-code architectures,
> but until then this is fine IMO.

On the other hand I am in favour of having a test architecture name:
it keeps the tests clean, and doesn't involve us trying to meddle with
architecture names in test cases. For example, suppose we rename the
ARM architecture names (to, say, names such as armv7-le-sf and
armv7-be-hf), for more consistency. After that, the test cases would
need to be fixed, or else the old names will be misleading. Having a
clearly defined "this is not our architecture" test architecture name
is fine by me.

Having a way to add new architecture to Morph without having to change
the code would be even better.

> > +    cd "$DATADIR/workspace/system_branch"
> > +    attempt_morph build $SYSTEM_A 2> "$DATADIR/result"
> > +
> > +    IMPLEMENTS THEN error message includes the string "(.*)"
> > +    grep "$MATCH_1" $DATADIR/result
> 
> I think this would be generically useful, so I would alter the IMPLEMENTS
> to always save their output somewhere in $DATADIR.

I'm still feeling my way around good ways to structure scenario steps
like these. I am sure best-practice patterns will emerge, but until
then I'm very happy to see people try different approaches.

> > +    IMPLEMENTS GIVEN a system branch
> > +    cd "$DATADIR/workspace"
> > +    mkdir system_branch
> > +    mkdir system_branch/.morph-system-branch
> > +    cat << EOF > system_branch/.morph-system-branch/config
> > +    [branch]
> > +    name = master 
> > +    root = test:morphs
> > +    uuid = 0
> > +    EOF
> > +    mkdir system_branch/test:morphs
> > +    cd system_branch/test:morphs
> > +    git init .
> > +    echo "[morph]
> > +        repository = test:morphs
> > +        uuid = 0" >> .git/config
> 
> I would use morph checkout to set this up, rather than tie myself to
> the implementation.

Hmm. I think I agree with Richard: it'd be good to use morph to create
the system branches rather than encode in the test suite knowledge of
how they're implemented. Ideally the test suite would not worry about
such things.

> > +    IMPLEMENTS GIVEN a morph config file
> > +    cat << EOF > "$DATADIR/morph.conf"
> > +    [config]
> > +    log = $DATADIR/morph.log
> > +    cachedir = /src/cache
> > +    tempdir = /src/tmp
> > +    EOF
> 
> Why did you alter cachedir and tempdir? If it is because otherwise
> your test runs out of space, it is better to run the tests with
> TMPDIR=/src/tmp.
> 
> If you need to set it for other reasons, make sure cachedir and tempdir
> are inside $DATADIR. Otherwise you're polluting your real data with test
> data and vice versa.

Aye. The test suite MUST NOT assume it can write outside of $DATADIR,
though I now realise this is not documented anywhere (it's one of
those things I take for granted, from long experience, so I didn't
remember not everyone views things the same way I do).

Especially hardcoding paths to places such as /src/tmp, which are not
part of the Linux Filesystem Hierarchy Standard, and are merely one of
the common ways in which Baserock is deployed, is a no-no. Richard's
advice to have the person running the test suite set $TMPDIR is valid.

-- 
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/



More information about the baserock-dev mailing list