[OE-core] [PATCH v2 3/3] import: new plugin to import the devtool workspace
Leonardo Sandoval
leonardo.sandoval.gonzalez at linux.intel.com
Mon Jun 12 15:27:41 UTC 2017
On Mon, 2017-06-12 at 16:19 +0200, Paul Eggleton wrote:
> Hi Leo,
>
> A few comments below.
>
Thanks Paul. Too late to provided a v3 before M1, so I will work on the
changes during M2.
Leo
>
> On Friday, 2 June 2017 7:13:01 PM CEST leonardo.sandoval.gonzalez at linux.intel.com wrote:
> > From: Leonardo Sandoval <leonardo.sandoval.gonzalez at linux.intel.com>
> >
> > Takes a tar archive created by 'devtool export' and export it (untar) to
> > workspace. By default, the whole tar archive is imported, thus there is no
> > way to limit what is imported.
> >
> > https://bugzilla.yoctoproject.org/show_bug.cgi?id=10510
> >
> > [YOCTO #10510]
> >
> > Signed-off-by: Leonardo Sandoval <leonardo.sandoval.gonzalez at linux.intel.com>
> > ---
> > scripts/lib/devtool/import.py | 130 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 130 insertions(+)
> > create mode 100644 scripts/lib/devtool/import.py
> >
> > diff --git a/scripts/lib/devtool/import.py b/scripts/lib/devtool/import.py
> > new file mode 100644
> > index 0000000000..5b85571669
> > --- /dev/null
> > +++ b/scripts/lib/devtool/import.py
> > @@ -0,0 +1,130 @@
> > +# Development tool - import command plugin
> > +#
> > +# Copyright (C) 2014-2017 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 import plugin"""
> > +
> > +import os
> > +import tarfile
> > +import logging
> > +import re
> > +from devtool import standard, setup_tinfoil
> > +from devtool import export
> > +
> > +import oeqa.utils.ftools as ftools
>
> So, this is importing something from oeqa to use outside of the QA scripts.
> Aside from that structural oddity, I have to be honest and say I'd rather we
> did it the replacement in specific code here rather than creating and using
> a generic function (especially seeing as we don't already have one) - then
> we avoid the need to open the file and process all lines multiple times.
>
>
> > +import json
> > +
> > +logger = logging.getLogger('devtool')
> > +
> > +def devimport(args, config, basepath, workspace):
> > + """Entry point for the devtool 'import' subcommand"""
> > +
> > + def get_pn(name):
> > + dirpath, recipe = os.path.split(name)
> > + pn = ""
> > + for sep in "_ .".split():
> > + if sep in recipe:
> > + pn = recipe.split(sep)[0]
> > + break
> > + return pn
>
> This function is a little worrying. Recipe names can legally have dots
> in the PN part of the name. See below for a recommended alternative.
>
> > +
> > + # match exported workspace folders
> > + prog = re.compile('recipes|appends|sources')
> > +
> > + if not os.path.exists(args.file):
> > + logger.error('Tar archive %s does not exist. Export your workspace using "devtool export"')
> > + return 1
> > +
> > + # get current recipes
> > + current_recipes = []
> > + try:
> > + tinfoil = setup_tinfoil(config_only=False, basepath=basepath)
>
> The setup_tinfoil() here should be outside of the try...finally or if it fails
> to start it will still try to shut it down.
>
>
> > + current_recipes = [recipe[1] for recipe in tinfoil.cooker.recipecaches[''].pkg_fn.items()]
> > + finally:
> > + tinfoil.shutdown()
> > +
> > + imported = []
> > + with tarfile.open(args.file) as tar:
> > +
> > + # get exported metadata so values containing paths can be automatically replaced it
> > + export_workspace_path = export_workspace = None
> > + try:
> > + metadata = tar.getmember(export.metadata)
> > + tar.extract(metadata)
> > + with open(metadata.name) as fdm:
> > + export_workspace_path, export_workspace = json.load(fdm)
> > + os.unlink(metadata.name)
> > + except KeyError as ke:
> > + logger.warn('The export metadata file created by "devtool export" was not found')
> > + logger.warn('Manual editing is needed to correct paths on imported recipes/appends')
>
> We should just fail if the metadata file isn't there - we don't need to
> support users importing any random tarball; if the metadata file
> isn't there we can safely assume that it wasn't exported with
> devtool export.
>
> > +
> > + members = [member for member in tar.getmembers() if member.name != export.metadata]
> > + for member in members:
> > + # make sure imported bbappend has its corresponding recipe (bb)
> > + if member.name.startswith('appends'):
> > + bbappend = get_pn(member.name)
> > + if bbappend:
> > + if bbappend not in current_recipes:
> > + # check that the recipe is not in the tar archive being imported
> > + if bbappend not in export_workspace:
> > + logger.warn('No recipe to append %s, skipping' % bbappend)
> > + continue
> > + else:
> > + logger.warn('bbappend name %s could not be detected' % member.name)
> > + continue
>
> This isn't the right way to check this - PN isn't guaranteed to be the same
> as the name part of the filename, for one. It's much safer to iterate over
> tinfoil.cooker.recipecaches[''].pkg_fn (keys, i.e. filenames, not PN values)
> and see if the bbappend matches the item, breaking out on first match.
> Remember that a bbappend might be the exact same name as the bb file or
> it might use a % wildcard. (You could cheat and replace this % with a * and
> then use fnmatch.fnmatch() to make this a bit easier).
>
> Given the moderate expense of computing this we should probably do it
> once as part of the initial check and store it in a dict so we can use it later
> (I see get_pn() is called again later on).
>
> > + # extract file from tar
> > + path = os.path.join(config.workspace_path, member.name)
> > + if os.path.exists(path):
> > + if args.overwrite:
> > + try:
> > + tar.extract(member, path=config.workspace_path)
> > + except PermissionError as pe:
> > + logger.warn(pe)
>
> If a recipe is already in someone's workspace, is this going to leave the
> user with a mix of files extracted from the tarball and whatever happens to
> already be in the source tree? If so, that could be problematic. I think I
> would first check if the recipe's source tree exists at all and if so, either
> skip it with a warning or overwrite it entirely dependent on whether -o
> has been specified.
>
>
> > + else:
> > + logger.warn('File already present. Use --overwrite/-o to overwrite it: %s' % member.name)
> > + else:
> > + tar.extract(member, path=config.workspace_path)
> > +
> > + if member.name.startswith('appends'):
> > + recipe = get_pn(member.name)
> > +
> > + # Update EXTERNARLSRC
>
> Typo - EXTERNALSRC.
>
> > + if export_workspace_path:
> > + # appends created by 'devtool modify' just need to update the workspace
> > + ftools.replace_from_file(path, export_workspace_path, config.workspace_path)
> > +
> > + # appends created by 'devtool add' need replacement of exported source tree
> > + exported_srctree = export_workspace[recipe]['srctree']
> > + if exported_srctree:
> > + ftools.replace_from_file(path, exported_srctree, os.path.join(config.workspace_path, 'sources', recipe))
> > +
> > + # update .devtool_md5 file
> > + standard._add_md5(config, recipe, path)
> > + if recipe not in imported:
> > + imported.append(recipe)
> > +
> > + logger.info('Imported recipes into workspace %s: %s' % (config.workspace_path, ' '.join(imported)))
> > + return 0
> > +
> > +def register_commands(subparsers, context):
> > + """Register devtool import subcommands"""
> > + parser = subparsers.add_parser('import',
> > + help='Import tar archive into workspace',
> > + description='Import previously created tar archive into the workspace',
>
> We should be specific - 'Import exported tar archive into workspace' and
> 'Import tar archive previously created by "devtool export" into workspace'.
>
> > + group='advanced')
> > + parser.add_argument('file', metavar='FILE', help='Name of the tar archive to import')
> > + parser.add_argument('--overwrite', '-o', action="store_true", help='Overwrite previous export tar archive')
>
> We're not writing out a tar archive so this should be "Overwrite files when
> extracting" or similar.
>
> Cheers,
> Paul
>
More information about the Openembedded-core
mailing list