[Toaster] [review-request][PATCH] toaster: Fix import of custom layers from toasterjson.conf
sujith h
sujith.h at gmail.com
Mon Aug 24 06:19:08 PDT 2015
On Fri, Aug 21, 2015 at 8:07 PM, Damian, Alexandru <
alexandru.damian at intel.com> wrote:
>
>
> On Fri, Aug 21, 2015 at 1:52 PM, sujith h <sujith.h at gmail.com> wrote:
>
>>
>>
>> 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?
>>
>
> Would be inserted into the database as the file is read.
>
>
>>
>>> 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?
>>
>
>
> Yep, exactly my point.
>
> When inserting the Layer_Version objects for the "HEAD" branch, the
> Layer_Version.local_path should
>
> be set to the layer path.
>
> When checking out the layers in localhost bbcontroller, the checkout
> should be skipped at all times for "HEAD" branches; instead, the value in
> Layer_Version.local_path should be used.
>
Is there a way to access local_path of toasterconf.json file from
localhostbecontroller.py file?
>
>
>
>>
>>>> --
>>>> 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
>>
>
>
>
> --
> 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/20150824/11a0bf64/attachment-0001.html>
More information about the toaster
mailing list