[Toaster] [review-request][PATCH] toaster: Fix import of custom layers from toasterjson.conf
sujith h
sujith.h at gmail.com
Fri Aug 21 05:52:42 PDT 2015
On Fri, Aug 21, 2015 at 4:45 PM, Damian, Alexandru <
alexandru.damian at intel.com> wrote:
> 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.
>
Ah, so would it be good to read from the database? Because I was giving
full path of layers in the toasterconf.json file. I assume that layer
information from the toasterconf.json file would be updated in the database?
>
> 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'])
>>
>
A bit more explanation would help me move further to improvise the patch.
If you don't mind please? So if I understand correctly a separate function
should be there to handle HEAD branches provided by toasterconf.json file,
right?
>
>> --
>> 1.9.1
>>
>> --
>> _______________________________________________
>> toaster mailing list
>> toaster at yoctoproject.org
>> https://lists.yoctoproject.org/listinfo/toaster
>>
>
>
>
> --
> Alex Damian
> Yocto Project
> SSG / OTC
>
--
സുജിത് ഹരിദാസന്
Bangalore
<Project>Contributor to KDE project
http://fci.wikia.com/wiki/Anti-DRM-Campaign
<Blog> http://sujithh.info
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.yoctoproject.org/pipermail/toaster/attachments/20150821/c9ec99be/attachment-0001.html>
More information about the toaster
mailing list