[Toaster] [review-request][PATCH] toaster: Fix import of custom layers from toasterjson.conf
Damian, Alexandru
alexandru.damian at intel.com
Fri Aug 21 04:15:14 PDT 2015
Hi,
I think we need a better patch description. If I understand correctly, the
issue you were facing is:
- even if we have layers at HEAD that are not under the poky/ tree, the
code would always return the
checkout path in the poky/ tree; I recognize that this is a valid issue.
I feel that this issue can be handled a bit differently if we frame it as
such:
- if we have a HEAD branch specified for a layer, we need to find out the
current checkout directory for that layer, since no checkout would be
performed.
As such, when we import a toasterconf.json file (they only way to get HEAD
branches for a layer) we need to store the actual layer checkout path in
the database (in the localpath, perhaps?), and reuse that value, instead of
employing special logic to search for it. What I mean, the core of the
problem is solved by replacing the whole gitrepos checkout logic with a
database value for all "HEAD" layers, and eliminate the magic in
getGitCloneDirectory().
I have a couple more comments on the patch below.
Cheers,
Alex
On Fri, Aug 21, 2015 at 9:41 AM, Sujith H <sujith.h at gmail.com> wrote:
> This patch fixes import of custom layers from toasterjson.conf
> file. For example it was verified using layers like meta-mentor,
> meta-fsl-arm, meta-sourcery etc.
>
> Signed-off-by: Sujith Haridasan <Sujith_Haridasan at mentor.com>
> ---
> .../toaster/bldcontrol/localhostbecontroller.py | 49
> ++++++++++++++++++++--
> .../bldcontrol/management/commands/loadconf.py | 12 ++++--
> 2 files changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/bitbake/lib/toaster/bldcontrol/localhostbecontroller.py
> b/bitbake/lib/toaster/bldcontrol/localhostbecontroller.py
> index 231a7d3..c43f33f 100644
> --- a/bitbake/lib/toaster/bldcontrol/localhostbecontroller.py
> +++ b/bitbake/lib/toaster/bldcontrol/localhostbecontroller.py
> @@ -178,7 +178,7 @@ class
> LocalhostBEController(BuildEnvironmentController):
> self.be.save()
> logger.debug("localhostbecontroller: Stopped bitbake server")
>
> - def getGitCloneDirectory(self, url, branch):
> + def
>
> getGitCloneDirectory(self, url, branch, cached_layers=None):
> """ Utility that returns the last component of a git path as
> directory
> """
> import re
> @@ -191,6 +191,17 @@ class
> LocalhostBEController(BuildEnvironmentController):
>
> # word of attention; this is a localhost-specific issue; only on
> the localhost we expect to have "HEAD" releases
> # which _ALWAYS_ means the current poky checkout
> + if cached_layers:
>
Using cached_layers here works only by coincidence - if the desired layer
is not manually checked out under be.sourcedir, it will not be found by
cached_layers code, and will not appear here.
I think a completely new piece of code is needed, which will track where
any layer that has a HEAD branch is actually checked out. This path would
be discovered and stored by importing a toasterconf.json file (the only
place that can generate a HEAD branch), and verified to actually exist at
build time in this function, getGitCloneDirectory(). This patch touches
loadconf, but I think we might be able to get a better handling of that.
+ if not url.endswith('.git'):
>
I am not sure why the actual GIT url format is relevant for "HEAD"
branches; we don't need to figure out where the layer would get checked
out, but where it actually exists.
+ from os.path import dirname
> + extractDir = dirname(url)
> + for key, val in cached_layers.iteritems():
> + if val == extractDir:
> + local_checkout_path = key
> + return cached_layers[local_checkout_path]
> + else:
> + local_checkout_path = cached_layers[url]
> + return local_checkout_path
> from os.path import dirname as DN
> local_checkout_path =
> DN(DN(DN(DN(DN(os.path.abspath(__file__))))))
> #logger.debug("localhostbecontroller: using HEAD checkout in %s"
> % local_checkout_path)
> @@ -245,7 +256,14 @@ class
> LocalhostBEController(BuildEnvironmentController):
>
> # 3. checkout the repositories
> for giturl, commit in gitrepos.keys():
> - localdirname = os.path.join(self.be.sourcedir,
> self.getGitCloneDirectory(giturl, commit))
> + if not giturl.endswith('.git'):
> + for key, val in cached_layers.iteritems():
> + if os.path.dirname(giturl) == val:
> + tmpKey = key
> + tmpVal = gitrepos[(giturl, commit)]
> + gitrepos[(tmpKey, commit)] += tmpVal
> + giturl = tmpKey
> + localdirname = os.path.join(self.be.sourcedir,
> self.getGitCloneDirectory(giturl, commit, cached_layers))
> logger.debug("localhostbecontroller: giturl %s:%s checking
> out in current directory %s" % (giturl, commit, localdirname))
>
> # make sure our directory is a git repository
> @@ -280,13 +298,36 @@ class
> LocalhostBEController(BuildEnvironmentController):
>
> # verify our repositories
> for name, dirpath in gitrepos[(giturl, commit)]:
> - localdirpath = os.path.join(localdirname, dirpath)
>
I am not sure what this change tries to do; dirpath is actually a path in
the git repo where the layer lives; it is not immediate that the all the
layers live in the top directory of a git repo. This change invalidates the
check below, where we verify that the layer path actually exists after
checkout.
> + localdirpath = localdirname
>
logger.debug("localhostbecontroller: localdirpath expected
> '%s'" % localdirpath)
> if not os.path.exists(localdirpath):
> raise BuildSetupException("Cannot find layer git path
> '%s' in checked out repository '%s:%s'. Aborting." % (localdirpath, giturl,
> commit))
>
> + layerlisturl, layerlistdir = "", ""
> + layerlisturl = [localurl for localurl, localpath in
> cached_layers.iteritems() if localpath == localdirpath]
>
I suspect this logic will go away if we're not going to use cached_layers
to discover the local checkout directories for HEAD urls.
> + if len(layerlisturl) != 0:
> + layerlisturl = layerlisturl[0]
> + if len(layerlisturl) == 0:
> + if os.path.basename(localdirpath).startswith("_"):
> + layerlisturl = localdirpath
> if name != "bitbake":
> - layerlist.append(localdirpath.rstrip("/"))
> + for gitlocal, localpath in gitrepos.iteritems():
> + if layerlisturl in gitlocal:
> + if len(localpath) > 2:
> + for paths in localpath:
> + if "bitbake" not in paths[1]:
> + layerlistdir = localdirpath +"/"
> + str(paths[1])
> + if layerlistdir not in layerlist:
> +
> layerlist.append(layerlistdir.rstrip("/"))
> + else:
> + layerlist.append(localdirpath.rstrip("/"))
> + break
> + if len(layerlist) == 0:
> + for gitlocal, localpath in gitrepos.iteritems():
> + for paths in localpath:
> + if "bitbake" not in paths:
> + layerlist.append(layerlisturl + "/" +
> str(paths[1]))
> +
>
> logger.debug("localhostbecontroller: current layer list %s " %
> pformat(layerlist))
>
> diff --git a/
>
> bitbake/lib/toaster/bldcontrol/management/commands/loadconf.py
> b/bitbake/lib/toaster/bldcontrol/management/commands/loadconf.py
> index 5022b59..a22f744 100644
> --- a/bitbake/lib/toaster/bldcontrol/management/commands/loadconf.py
> +++ b/bitbake/lib/toaster/bldcontrol/management/commands/loadconf.py
> @@ -45,12 +45,14 @@ class Command(BaseCommand):
> for i in ['bitbake', 'releases', 'defaultrelease', 'config',
> 'layersources']:
> assert i in data
>
> - def _read_git_url_from_local_repository(address):
> + def _read_git_url_from_local_repository(address, path = None):
>
If you want to pass in the path, please move the whole function outside
_import_layer_config, as it's no longer a closure.
> url = None
> + if not path:
> + path = os.path.dirname(filepath)
> # we detect the remote name at runtime
> import subprocess
> (remote, remote_name) = address.split(":", 1)
> - cmd = subprocess.Popen("git remote -v", shell=True, cwd =
> os.path.dirname(filepath), stdout=subprocess.PIPE, stderr = subprocess.PIPE)
> + cmd = subprocess.Popen("git remote -v", shell=True, cwd =
> path, stdout=subprocess.PIPE, stderr = subprocess.PIPE)
> (out,err) = cmd.communicate()
> if cmd.returncode != 0:
> logging.warning("Error while importing layer vcs_url: git
> error: %s" % err)
> @@ -121,7 +123,11 @@ class Command(BaseCommand):
> logger.error("Local layer path %s must exists.
> Are you trying to import a layer that does not exist ? Check your local
> toasterconf.json" % lo.local_path)
>
> if layerinfo['vcs_url'].startswith("remote:"):
> - lo.vcs_url =
> _read_git_url_from_local_repository(layerinfo['vcs_url'])
> + if layerinfo['local_path'] not in ["meta",
> "meta-yocto", "meta-yocto-bsp"]:
>
Please don't use hardcoded layer names anywhere in the code.
We do not want special handling for some layers, all layers need to be
treated in the same, uniform, way.
+ repo_path = layerinfo['local_path']
> + else:
> + repo_path = None
> + lo.vcs_url =
> _read_git_url_from_local_repository(layerinfo['vcs_url'], repo_path)
>
I think this code needs
> if lo.vcs_url is None:
> logger.error("The toaster config file
> references the local git repo, but Toaster cannot detect it.\nYour local
> configuration for layer %s is invalid. Make sure that the toasterconf.json
> file is correct." % layerinfo['name'])
>
> --
> 1.9.1
>
> --
> _______________________________________________
> toaster mailing list
> toaster at yoctoproject.org
> https://lists.yoctoproject.org/listinfo/toaster
>
--
Alex Damian
Yocto Project
SSG / OTC
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150821/84fb855a/attachment-0001.html>
More information about the toaster
mailing list