Re: [PATCH 2/2] devtool: implement build-image plugin

[Ed Bartosh <ed.bartosh@linux.intel.com>]

Hi Paul,

Thank you for review!

My answers are below.

On Thu, Aug 27, 2015 at 03:07:37PM +0100, Paul Eggleton wrote:
> Hi Ed,
> 
> On Monday 24 August 2015 10:33:31 Ed Bartosh wrote:
> > Implemented new plugin to build image from workspace packages.
> > 
> > Plugin creates <image>.bbappend file, adds
> > all workspace packages to the image using IMAGE_INSTALL_append
> > variable in bbappend file. After that it runs 'bitbake <image>'.
> > 
> > Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> > ---
> >  scripts/lib/devtool/build-image.py | 56
> > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)
> >  create mode 100644 scripts/lib/devtool/build-image.py
> > 
> > diff --git a/scripts/lib/devtool/build-image.py
> > b/scripts/lib/devtool/build-image.py new file mode 100644
> > index 0000000..d8e7b12
> > --- /dev/null
> > +++ b/scripts/lib/devtool/build-image.py
> > @@ -0,0 +1,56 @@
> > +# Development tool - build-image plugin
> > +#
> > +# Copyright (C) 2015 Intel Corporation
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License version 2 as
> > +# published by the Free Software Foundation.
> > +#
> > +# 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.
> > +
> > +"""Devtool plugin containing the build-image subcommand."""
> > +
> > +import os
> > +import logging
> > +
> > +from bb.process import ExecutionError
> > +from devtool import exec_build_env_command, add_md5
> > +
> > +LOG = logging.getLogger('devtool')
> 
> I recall you mentioning pylint had a problem with "logger" - we're using that 
> throughout the devtool code (and elsewhere). "LOG" just looks a bit weird. 
> What is it complaining about specifically?
> 

C: 27, 0: Invalid constant name "logger" (invalid-name)

It's not a big deal. I'll use "logger" in v2.

> > +def plugin_init(pluginlist):
> > +    """Plugin initialization"""
> > +    pass
> > +
> > +def build_image(args, config, basepath, workspace):
> > +    """Entry point for the devtool 'build-image' subcommand."""
> > +    image = args.recipe
> > +    appendfile = os.path.join(config.workspace_path, 'appends',
> > +                              '%s.bbappend' % image)
> > +    with open(appendfile, 'w') as afile:
> > +        afile.write('IMAGE_INSTALL_append = " %s"\n' % \
> > +                    ' '.join(workspace.keys()))
> 
> Some notes here:
> 
> 1) Recipes that aren't for the target somehow need to be filtered out (e.g. 
> native recipes). I'd imagine the way to do that would be to parse each one and 
> check if "class-target" is in d.getVar('OVERRIDES', True).split(':').
> 
> 2) We're making a direct mapping between recipe name and main package for the 
> recipe. That'll work for most recipes but there will be some where that's not 
> valid. I'm tempted to say that since we're parsing the recipe anyway, for now 
> we should do a quick check of PACKAGES to ensure we're not adding something 
> that'll cause the build to fail.
> 
> 3) It would be nice to add an option to add all the packages in PACKAGES for 
> each recipe to the image; and a separate option to just add the non-dev/-dbg/-
> staticdev packages. Perhaps we should leave this as a future enhancement 
> though.
> 
> 4) If there's no target recipes in the workspace then print something like "No 
> recipes in workspace, building image <image> unmodified". On the other hand if  
> there are we should report "Building image <image> with the following 
> additional packages: <packagelist>'
> 
> 5) If there are recipes can you add the following so that the build system 
> prints something every time the image recipe gets built:
> 
> do_rootfs[prefuncs] += "devtool_warn_image_extended"
> python devtool_warn_image_extended() {
>     bb.plain('NOTE: %s: building with additional packages due to "devtool 
> build-image", delete /path/to/bbappend to clear this' % d.getVar('PN', True))
> }
> 
> > +    add_md5(config, image, appendfile)
> 
> This isn't actually doing much if we can't use "devtool reset" is it? (Since 
> the recipe isn't officially in the workspace; we could make it so, but then we'd 
> need to ensure the other commands either behave themselves or error out as 
> appropriate if called with an image recipe that's in the workspace.)
> 

Fixed all of the above in v2. I'll send it for review soon.

> > +    try:
> > +        exec_build_env_command(config.init_path, basepath,
> > +                               'bitbake %s' % image, watch=True)
> > +    except ExecutionError as err:
> > +        return err.exitcode
> > +
> > +    LOG.info('Successfully built %s', image)
> > +
> > +def register_commands(subparsers, context):
> > +    """Register devtool subcommands from the build-image plugin"""
> > +    parser_package = subparsers.add_parser('build-image', help='Build
> > image') 
> > +    parser_package.add_argument('recipe', help='Image recipe to
> > build') 
> > +    parser_package.set_defaults(func=build_image)
> > +
> 
> Can you please change help for the command to 'Build image including workspace 
> recipe packages' and add description='Builds an image, extending it to include 
> packages from recipes in the workspace'.
>

Fixed in v2 too.

Regards,
Ed