[PATCH 0/4] devtool fixes

[PATCH 0/4] devtool fixes

[Paul Eggleton]

A few fixes for devtool, mostly relating to recipes that fetch from git
where S points to a subdirectory of the repo.

Note: I wasn't 100% sure if it was appropriate to be referencing WORKDIR
in patch.py; let me know if I should try to rework that.


The following changes since commit db28cd0e1540e44db963108430205c8c0c817774:

  gperf: Switch to upstream patch (2022-07-09 20:58:31 +0100)

are available in the git repository at:

  git://git.openembedded.org/openembedded-core-contrib paule/devtool37-oe
  http://cgit.openembedded.org/openembedded-core-contrib/log/?h=paule/devtool37-oe

Paul Eggleton (4):
  patch: handle if S points to a subdirectory of a git repo
  devtool: finish: handle patching when S points to subdir of a git repo
  devtool: ignore pn- overrides when determining SRC_URI overrides
  oe-selftest: devtool: test modify git recipe building from a subdir

 meta/lib/oe/patch.py                    |  8 +++---
 meta/lib/oe/recipeutils.py              |  9 ++++--
 meta/lib/oeqa/selftest/cases/devtool.py | 51 +++++++++++++++++++++++++++++++++
 scripts/lib/devtool/standard.py         | 29 ++++++++++++++-----
 4 files changed, 84 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] patch: handle if S points to a subdirectory of a git repo

[Paul Eggleton]

From: Paul Eggleton <paul.eggleton@microsoft.com>

If PATCHTOOL = "git", SRC_URI fetches from a git repo and S points to
a subdirectory of the checked out sources, then we were erroneously
initialising the subdirectory as its own git repo. Check if the returned
top-level repo directory is a subdirectory of WORKDIR and do not
run initialise the source directory if that is the case.

(This was a regression introduced with OE-Core revision
6184b56a7a0fc6f5d19fdfb81e7453667f7da940, however we didn't have a test
that verified the behaviour.)

Signed-off-by: Paul Eggleton <paul.eggleton@microsoft.com>
---
 meta/lib/oe/patch.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 95b915a..4ec9cae 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -299,10 +299,10 @@ class GitApplyTree(PatchTree):
         PatchTree.__init__(self, dir, d)
         self.commituser = d.getVar('PATCH_GIT_USER_NAME')
         self.commitemail = d.getVar('PATCH_GIT_USER_EMAIL')
-        if not self._isInitialized():
+        if not self._isInitialized(d):
             self._initRepo()
 
-    def _isInitialized(self):
+    def _isInitialized(self, d):
         cmd = "git rev-parse --show-toplevel"
         try:
             output = runcmd(cmd.split(), self.dir).strip()
@@ -310,8 +310,8 @@ class GitApplyTree(PatchTree):
             ## runcmd returned non-zero which most likely means 128
             ## Not a git directory
             return False
-        ## Make sure repo is in builddir to not break top-level git repos
-        return os.path.samefile(output, self.dir)
+        ## Make sure repo is in builddir to not break top-level git repos, or under workdir
+        return os.path.samefile(output, self.dir) or oe.path.is_path_parent(d.getVar('WORKDIR'), output)
 
     def _initRepo(self):
         runcmd("git init".split(), self.dir)
-- 
1.8.3.1

[PATCH 2/4] devtool: finish: handle patching when S points to subdir of a git repo

[Paul Eggleton]

From: Paul Eggleton <paul.eggleton@microsoft.com>

If devtool finish needs to create a patch and have it applied to the
sources for a recipe where S points to a subdirectory of the sources,
then the patch needs to be applied at the root of the repo i.e. we need
to add a patchdir= parameter to the SRC_URI entry.

Signed-off-by: Paul Eggleton <paul.eggleton@microsoft.com>
---
 meta/lib/oe/recipeutils.py      |  9 +++++++--
 scripts/lib/devtool/standard.py | 25 +++++++++++++++++++------
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/meta/lib/oe/recipeutils.py b/meta/lib/oe/recipeutils.py
index 872ff97..6755ae8 100644
--- a/meta/lib/oe/recipeutils.py
+++ b/meta/lib/oe/recipeutils.py
@@ -666,7 +666,7 @@ def get_bbappend_path(d, destlayerdir, wildcardver=False):
     return (appendpath, pathok)
 
 
-def bbappend_recipe(rd, destlayerdir, srcfiles, install=None, wildcardver=False, machine=None, extralines=None, removevalues=None, redirect_output=None):
+def bbappend_recipe(rd, destlayerdir, srcfiles, install=None, wildcardver=False, machine=None, extralines=None, removevalues=None, redirect_output=None, params=None):
     """
     Writes a bbappend file for a recipe
     Parameters:
@@ -696,6 +696,9 @@ def bbappend_recipe(rd, destlayerdir, srcfiles, install=None, wildcardver=False,
         redirect_output:
             If specified, redirects writing the output file to the
             specified directory (for dry-run purposes)
+        params:
+            Parameters to use when adding entries to SRC_URI. If specified,
+            should be a list of dicts with the same length as srcfiles.
     """
 
     if not removevalues:
@@ -762,12 +765,14 @@ def bbappend_recipe(rd, destlayerdir, srcfiles, install=None, wildcardver=False,
     copyfiles = {}
     if srcfiles:
         instfunclines = []
-        for newfile, origsrcfile in srcfiles.items():
+        for i, (newfile, origsrcfile) in enumerate(srcfiles.items()):
             srcfile = origsrcfile
             srcurientry = None
             if not srcfile:
                 srcfile = os.path.basename(newfile)
                 srcurientry = 'file://%s' % srcfile
+                if params:
+                    srcurientry = '%s;%s' % (srcurientry, ';'.join('%s=%s' % (k,v) for k,v in params[i].items()))
                 # Double-check it's not there already
                 # FIXME do we care if the entry is added by another bbappend that might go away?
                 if not srcurientry in rd.getVar('SRC_URI').split():
diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index 4b50e3c..a942ef3 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -1604,6 +1604,19 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
     if not os.path.exists(append):
         raise DevtoolError('unable to find workspace bbappend for recipe %s' %
                            recipename)
+    srctreebase = workspace[recipename]['srctreebase']
+    relpatchdir = os.path.relpath(srctreebase, srctree)
+    if relpatchdir == '.':
+        patchdir_params = {}
+    else:
+        patchdir_params = {'patchdir': relpatchdir}
+
+    def srcuri_entry(fname):
+        if patchdir_params:
+            paramstr = ';'.join('%s=%s' % (k,v) for k,v in patchdir_params.items())
+        else:
+            paramstr = ''
+        return 'file://%s%s' % (basepath, paramstr)
 
     initial_rev, update_rev, changed_revs, filter_patches = _get_patchset_revs(srctree, append, initial_rev, force_patch_refresh)
     if not initial_rev:
@@ -1625,7 +1638,6 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
             new_f = {}
             del_f = {}
         else:
-            srctreebase = workspace[recipename]['srctreebase']
             upd_f, new_f, del_f = _export_local_files(srctree, rd, local_files_dir, srctreebase)
 
         remove_files = []
@@ -1661,14 +1673,15 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
                     removedentries, remaining = _remove_file_entries(
                                                     srcuri, remove_files)
                     if removedentries or remaining:
-                        remaining = ['file://' + os.path.basename(item) for
+                        remaining = [srcuri_entry(os.path.basename(item)) for
                                      item in remaining]
                         removevalues = {'SRC_URI': removedentries + remaining}
                 appendfile, destpath = oe.recipeutils.bbappend_recipe(
                                 rd, appendlayerdir, files,
                                 wildcardver=wildcard_version,
                                 removevalues=removevalues,
-                                redirect_output=dry_run_outdir)
+                                redirect_output=dry_run_outdir,
+                                params=[patchdir_params] * len(files))
             else:
                 logger.info('No patches or local source files needed updating')
         else:
@@ -1692,7 +1705,7 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
                     # replace the entry in SRC_URI with our local version
                     logger.info('Replacing remote patch %s with updated local version' % basepath)
                     path = os.path.join(files_dir, basepath)
-                    _replace_srcuri_entry(srcuri, basepath, 'file://%s' % basepath)
+                    _replace_srcuri_entry(srcuri, basepath, srcuri_entry(basepath))
                     updaterecipe = True
                 else:
                     logger.info('Updating patch %s%s' % (basepath, dry_run_suffix))
@@ -1706,7 +1719,7 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
                            os.path.join(files_dir, basepath),
                            dry_run_outdir=dry_run_outdir,
                            base_outdir=recipedir)
-                srcuri.append('file://%s' % basepath)
+                srcuri.append(srcuri_entry(basepath))
                 updaterecipe = True
             for basepath, path in new_p.items():
                 logger.info('Adding new patch %s%s' % (basepath, dry_run_suffix))
@@ -1714,7 +1727,7 @@ def _update_recipe_patch(recipename, workspace, srctree, rd, appendlayerdir, wil
                            os.path.join(files_dir, basepath),
                            dry_run_outdir=dry_run_outdir,
                            base_outdir=recipedir)
-                srcuri.append('file://%s' % basepath)
+                srcuri.append(srcuri_entry(basepath))
                 updaterecipe = True
             # Update recipe, if needed
             if _remove_file_entries(srcuri, remove_files)[0]:
-- 
1.8.3.1

[PATCH 3/4] devtool: ignore pn- overrides when determining SRC_URI overrides

[Paul Eggleton]

From: Paul Eggleton <paul.eggleton@microsoft.com>

If (perhaps foolishly) at your configuration level you have e.g.

  SRC_URI_append_pn-recipename = " file://patchname.patch"

and then run devtool modify on a different recipe, an error occurs:

  INFO: SRC_URI contains some conditional appends/prepends - will create branches to represent these
  ...
  ERROR: [Errno 2] No such file or directory: '/path/to/downloads/patchname.patch'

pn- overrides would not constitute an alternative configuration that we
should handle in this context, so just ignore them to avoid the issue.

Signed-off-by: Paul Eggleton <paul.eggleton@microsoft.com>
---
 scripts/lib/devtool/standard.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index a942ef3..b0b484e 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -520,7 +520,9 @@ def _extract_source(srctree, keep_temp, devbranch, sync, config, basepath, works
         for event in history:
             if not 'flag' in event:
                 if event['op'].startswith((':append[', ':prepend[')):
-                    extra_overrides.append(event['op'].split('[')[1].split(']')[0])
+                    override = event['op'].split('[')[1].split(']')[0]
+                    if not override.startswith('pn-'):
+                        extra_overrides.append(override)
         # We want to remove duplicate overrides. If a recipe had multiple
         # SRC_URI_override += values it would cause mulitple instances of
         # overrides. This doesn't play nicely with things like creating a
-- 
1.8.3.1

[PATCH 4/4] oe-selftest: devtool: test modify git recipe building from a subdir

[Paul Eggleton]

From: Paul Eggleton <paul.eggleton@microsoft.com>

Add a test that verifies that devtool modify + devtool finish do the
right thing on a recipe that fetches from git and sets S to point to
a subdirectory of the source tree. We have a few examples among the core
recipes, dos2unix is a convenient one so let's use that. (The test first
verifies that that is still true in case the recipe is changed in
future.)

Signed-off-by: Paul Eggleton <paul.eggleton@microsoft.com>
---
 meta/lib/oeqa/selftest/cases/devtool.py | 51 +++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/meta/lib/oeqa/selftest/cases/devtool.py b/meta/lib/oeqa/selftest/cases/devtool.py
index ddf6c0c..ddab150 100644
--- a/meta/lib/oeqa/selftest/cases/devtool.py
+++ b/meta/lib/oeqa/selftest/cases/devtool.py
@@ -718,6 +718,7 @@ class DevtoolModifyTests(DevtoolBase):
 
         self.assertTrue(bbclassextended, 'None of these recipes are BBCLASSEXTENDed to native - need to adjust testrecipes list: %s' % ', '.join(testrecipes))
         self.assertTrue(inheritnative, 'None of these recipes do "inherit native" - need to adjust testrecipes list: %s' % ', '.join(testrecipes))
+
     def test_devtool_modify_localfiles_only(self):
         # Check preconditions
         testrecipe = 'base-files'
@@ -1316,6 +1317,56 @@ class DevtoolUpdateTests(DevtoolBase):
         expected_status = []
         self._check_repo_status(os.path.dirname(recipefile), expected_status)
 
+    def test_devtool_finish_modify_git_subdir(self):
+        # Check preconditions
+        testrecipe = 'dos2unix'
+        bb_vars = get_bb_vars(['SRC_URI', 'S', 'WORKDIR', 'FILE'], testrecipe)
+        self.assertIn('git://', bb_vars['SRC_URI'], 'This test expects the %s recipe to be a git recipe' % testrecipe)
+        workdir_git = '%s/git/' % bb_vars['WORKDIR']
+        if not bb_vars['S'].startswith(workdir_git):
+            self.fail('This test expects the %s recipe to be building from a subdirectory of the git repo' % testrecipe)
+        subdir = bb_vars['S'].split(workdir_git, 1)[1]
+        # Clean up anything in the workdir/sysroot/sstate cache
+        bitbake('%s -c cleansstate' % testrecipe)
+        # Try modifying a recipe
+        tempdir = tempfile.mkdtemp(prefix='devtoolqa')
+        self.track_for_cleanup(tempdir)
+        self.track_for_cleanup(self.workspacedir)
+        self.add_command_to_tearDown('bitbake -c clean %s' % testrecipe)
+        self.add_command_to_tearDown('bitbake-layers remove-layer */workspace')
+        result = runCmd('devtool modify %s -x %s' % (testrecipe, tempdir))
+        testsrcfile = os.path.join(tempdir, subdir, 'dos2unix.c')
+        self.assertExists(testsrcfile, 'Extracted source could not be found')
+        self.assertExists(os.path.join(self.workspacedir, 'conf', 'layer.conf'), 'Workspace directory not created. devtool output: %s' % result.output)
+        self.assertNotExists(os.path.join(tempdir, subdir, '.git'), 'Subdirectory has been initialised as a git repo')
+        # Check git repo
+        self._check_src_repo(tempdir)
+        # Modify file
+        runCmd("sed -i '1s:^:/* Add a comment */\\n:' %s" % testsrcfile)
+        result = runCmd('git commit -a -m "Add a comment"', cwd=tempdir)
+        # Run devtool finish
+        recipefile = bb_vars['FILE']
+        recipedir = os.path.dirname(recipefile)
+        res = re.search('recipes-.*', recipedir)
+        self.assertTrue(res, 'Unable to find recipe subdirectory')
+        recipesubdir = res[0]
+        self.add_command_to_tearDown('rm -rf %s' % os.path.join(self.testlayer_path, recipesubdir))
+        result = runCmd('devtool finish %s meta-selftest' % testrecipe)
+        # Check bbappend file contents
+        appendfn = os.path.join(self.testlayer_path, recipesubdir, '%s_%%.bbappend' % testrecipe)
+        with open(appendfn, 'r') as f:
+            appendlines = f.readlines()
+        expected_appendlines = [
+            'FILESEXTRAPATHS:prepend := "${THISDIR}/${PN}:"\n',
+            '\n',
+            'SRC_URI += "file://0001-Add-a-comment.patch;patchdir=.."\n',
+            '\n'
+        ]
+        self.assertEqual(appendlines, expected_appendlines)
+        self.assertExists(os.path.join(os.path.dirname(appendfn), testrecipe, '0001-Add-a-comment.patch'))
+        # Try building
+        bitbake('%s -c patch' % testrecipe)
+
 class DevtoolExtractTests(DevtoolBase):
 
     def test_devtool_extract(self):
-- 
1.8.3.1

Re: [OE-core] [PATCH 0/4] devtool fixes

[Alexander Kanavin]

Thanks Paul, this was one of the most annoying shortcomings of devtool.

Alex

On Tue, 12 Jul 2022 at 06:07, Paul Eggleton
<paul.eggleton@linux.microsoft.com> wrote:
>
> A few fixes for devtool, mostly relating to recipes that fetch from git
> where S points to a subdirectory of the repo.
>
> Note: I wasn't 100% sure if it was appropriate to be referencing WORKDIR
> in patch.py; let me know if I should try to rework that.
>
>
> The following changes since commit db28cd0e1540e44db963108430205c8c0c817774:
>
>   gperf: Switch to upstream patch (2022-07-09 20:58:31 +0100)
>
> are available in the git repository at:
>
>   git://git.openembedded.org/openembedded-core-contrib paule/devtool37-oe
>   http://cgit.openembedded.org/openembedded-core-contrib/log/?h=paule/devtool37-oe
>
> Paul Eggleton (4):
>   patch: handle if S points to a subdirectory of a git repo
>   devtool: finish: handle patching when S points to subdir of a git repo
>   devtool: ignore pn- overrides when determining SRC_URI overrides
>   oe-selftest: devtool: test modify git recipe building from a subdir
>
>  meta/lib/oe/patch.py                    |  8 +++---
>  meta/lib/oe/recipeutils.py              |  9 ++++--
>  meta/lib/oeqa/selftest/cases/devtool.py | 51 +++++++++++++++++++++++++++++++++
>  scripts/lib/devtool/standard.py         | 29 ++++++++++++++-----
>  4 files changed, 84 insertions(+), 13 deletions(-)
>
> --
> 1.8.3.1
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#167889): https://lists.openembedded.org/g/openembedded-core/message/167889
> Mute This Topic: https://lists.openembedded.org/mt/92326549/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

Re: [OE-core] [PATCH 0/4] devtool fixes

[Richard Purdie]

On Mon, 2022-07-11 at 21:06 -0700, Paul Eggleton wrote:
> A few fixes for devtool, mostly relating to recipes that fetch from git
> where S points to a subdirectory of the repo.
> 
> Note: I wasn't 100% sure if it was appropriate to be referencing WORKDIR
> in patch.py; let me know if I should try to rework that.

It is probably ok. The code won't pick up on the variable usage from a
hash perspective there but it would be referenced elsewhere anyway.

There was a failure which is probably from this series though:

https://autobuilder.yoctoproject.org/typhoon/#/builders/87/builds/3817/steps/14/logs/stdio


(I've not confirmed it was the series, I'm just guessing)

Cheers,

Richard

Re: [OE-core] [PATCH 0/4] devtool fixes

[Paul Eggleton]

On Tue, Jul 12, 2022 at 04:59:38PM +0100, Richard Purdie wrote:
> On Mon, 2022-07-11 at 21:06 -0700, Paul Eggleton wrote:
> > A few fixes for devtool, mostly relating to recipes that fetch from git
> > where S points to a subdirectory of the repo.
> > 
> > Note: I wasn't 100% sure if it was appropriate to be referencing WORKDIR
> > in patch.py; let me know if I should try to rework that.
> 
> It is probably ok. The code won't pick up on the variable usage from a
> hash perspective there but it would be referenced elsewhere anyway.
> 
> There was a failure which is probably from this series though:
> 
> https://autobuilder.yoctoproject.org/typhoon/#/builders/87/builds/3817/steps/14/logs/stdio
> 
> 
> (I've not confirmed it was the series, I'm just guessing)

Hmm, yes, it probably is. I did run the tests but somehow I didn't see this
failure. I'll investigate further.

Paul