Hi Leo, A few comments below.
On Friday, 2 June 2017 7:13:01 PM CEST leonardo.sandoval.gonza...@linux.intel.com wrote: > From: Leonardo Sandoval <leonardo.sandoval.gonza...@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.gonza...@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 -- Paul Eggleton Intel Open Source Technology Centre -- _______________________________________________ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
