[PATCH 0/1] patch: don't strip GitApplyTree patches

[PATCH 0/1] patch: don't strip GitApplyTree patches

[bkylerussell]

I think that the GitApplyTree patch implementation may not be quite
performing as expected when dealing with a non-default ${S} and patch
striplevels.

I'm not quite sure there aren't scenarios where we *do* want to honor
striplevel in GitApplyTree, even though it always appears to apply
patches at the repository's root, so I wanted to send this out for
feedback.

The change does at least address the scenario I encountered, but there's
probably a legitimate use case I'm missing where we need striplevel here.

Kyle Russell (1):
  patch: don't strip GitApplyTree patches

 meta/lib/oe/patch.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/1] patch: don't strip GitApplyTree patches

[bkylerussell]

Don't pass a path removal parameter to git am/git apply since GitApplyTree
always applies patches from the root of the git repo, not from each
individual patchdir (like quilt).

GitApplyTree uses `git rev-parse --show-toplevel` to find the root of
the repo no matter which patchdir is passed into the GitApplyTree instance.
In most cases, this works fine when applying patches since the three-way
merge can figure out where the patch should be applied to existing files
based on the history, even if the file paths are excessively stripped.

An exception to this occurs if a patch adds a new file to the repo
and a custom ${S} has been set to a subdirectory of the repo such
that the patch requires a non-default striplevel parameter.

When processed by GitApplyTree, the patch will have its striplevel
parameter given to git even though it's applied at the root of the
tree.

For example, consider a patch that adds a new file src/foo to a repo.

  --- /dev/null
  +++ b/src/foo
  @@ -0,0 +1 @@
  +new file test

The recipe sets S = "${WORKDIR}/git/src" and the patch is added to SRC_URI
using "striplevel=2" since the default patch tool, quilt, applies each
patch at its patchdir, which defaults to ${S}.

When the GitApplyTree patch class is used (for example, through devtool),
the root of the repo is passed to git as the work tree, in addition to the
path removal paramter.  In the case above, GitApplyTree creates
${WORKDIR}/git/foo instead of ${WORKDIR}/git/src/foo.

Interestingly enough, you can take a more typical patch example that only
changes existing files (instead of adding a new file) and remove the -3
argument from the git am/git apply commands issued by GitApplyTree.  In
this scenario, the patch will also fail to apply because the path removal
arg is still being passed, and the resulting path does not exist.

Since GitApplyTree always applies from the root of the repo, it should
ignore any striplevel arguments.
---
 meta/lib/oe/patch.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 2b1eee1003..2e45a7ef89 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -517,7 +517,7 @@ class GitApplyTree(PatchTree):
             try:
                 shellcmd = [patchfilevar, "git", "--work-tree=%s" % reporoot]
                 self.gitCommandUserOptions(shellcmd, self.commituser, self.commitemail)
-                shellcmd += ["am", "-3", "--keep-cr", "-p%s" % patch['strippath']]
+                shellcmd += ["am", "-3", "--keep-cr"]
                 return _applypatchhelper(shellcmd, patch, force, reverse, run)
             except CmdError:
                 # Need to abort the git am, or we'll still be within it at the end
@@ -534,7 +534,7 @@ class GitApplyTree(PatchTree):
                 runcmd(["sh", "-c", " ".join(shellcmd)], self.dir)
 
                 # Fall back to git apply
-                shellcmd = ["git", "--git-dir=%s" % reporoot, "apply", "-p%s" % patch['strippath']]
+                shellcmd = ["git", "--git-dir=%s" % reporoot, "apply"]
                 try:
                     output = _applypatchhelper(shellcmd, patch, force, reverse, run)
                 except CmdError:
-- 
2.17.1

✗ patchtest: failure for patch: don't strip GitApplyTree patches

[Patchwork]

== Series Details ==

Series: patch: don't strip GitApplyTree patches
Revision: 1
URL   : https://patchwork.openembedded.org/series/23665/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Patch            [1/1] patch: don't strip GitApplyTree patches
 Issue             Patch is missing Signed-off-by [test_signed_off_by_presence] 
  Suggested fix    Sign off the patch (either manually or with "git commit --amend -s")



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches

[Richard Purdie]

On Thu, 2020-04-09 at 00:41 -0400, bkylerussell@gmail.com wrote:
> Don't pass a path removal parameter to git am/git apply since GitApplyTree
> always applies patches from the root of the git repo, not from each
> individual patchdir (like quilt).
> 
> GitApplyTree uses `git rev-parse --show-toplevel` to find the root of
> the repo no matter which patchdir is passed into the GitApplyTree instance.
> In most cases, this works fine when applying patches since the three-way
> merge can figure out where the patch should be applied to existing files
> based on the history, even if the file paths are excessively stripped.
> 
> An exception to this occurs if a patch adds a new file to the repo
> and a custom ${S} has been set to a subdirectory of the repo such
> that the patch requires a non-default striplevel parameter.
> 
> When processed by GitApplyTree, the patch will have its striplevel
> parameter given to git even though it's applied at the root of the
> tree.
> 
> For example, consider a patch that adds a new file src/foo to a repo.
> 
>   --- /dev/null
>   +++ b/src/foo
>   @@ -0,0 +1 @@
>   +new file test
> 
> The recipe sets S = "${WORKDIR}/git/src" and the patch is added to SRC_URI
> using "striplevel=2" since the default patch tool, quilt, applies each
> patch at its patchdir, which defaults to ${S}.
> 
> When the GitApplyTree patch class is used (for example, through devtool),
> the root of the repo is passed to git as the work tree, in addition to the
> path removal paramter.  In the case above, GitApplyTree creates
> ${WORKDIR}/git/foo instead of ${WORKDIR}/git/src/foo.
> 
> Interestingly enough, you can take a more typical patch example that only
> changes existing files (instead of adding a new file) and remove the -3
> argument from the git am/git apply commands issued by GitApplyTree.  In
> this scenario, the patch will also fail to apply because the path removal
> arg is still being passed, and the resulting path does not exist.
> 
> Since GitApplyTree always applies from the root of the repo, it should
> ignore any striplevel arguments.

I don't follow this argument. We have several recipes in OE-Core which
set striplevel in SRC_URI (e.g. bash, llvm and a pointless looking one
in build-compare). If you changed the patch tool to git for those
recipes, doesn't it fail with this patch applied?

I think you're right and there is an issue here since do_patch assumes
${S} is the root for patches whilst git assumes the root of the repo is
the patch root. If those are different, it will break.

Put another way, I think your patch fixes one case whilst breaking
others :(.

I'm not sure how we reconcile this. I certainly do not like hoping 'git
am -3' figures it out. We may need to teach GitApplyTree to calculate
the difference between the git repodir and patchdir and adjust the
offsets accordingly?

Cheers,

Richard

Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of bkylerussell@gmail.com
> Sent: den 9 april 2020 06:41
> To: openembedded-core@lists.openembedded.org
> Cc: wesley.lindauer@gmail.com; Kyle Russell <bkylerussell@gmail.com>
> Subject: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches

You want to change the prefix "patch:" to, e.g., "patch.py:" to 
differentiate it from a change to the patch recipe.

//Peter

Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches

[bkylerussell]

[-- Attachment #1: Type: text/plain, Size: 5047 bytes --]

Sorry, it has taken me a while to get back to this.

> If you changed the patch tool to git for those recipes, doesn't it fail
with this patch applied?

Even without my patch, I don't think bash works normally with PATCHTOOL =
"git" since we unpack it from a tgz.  I added PATCHTOOL = "git" to the bash
recipe, and it seems to trash my poky repo's git history instead of the
source in my bash workdir. :( There might be a separate case we should
protect against for that, but I'll have to look at it separately.

llvm looks like the perfect example though, and is much more interesting.
Initially, it actually fails the git am attempt with "fatal: empty ident
name (for <>) not allowed".  I think this might be because git doesn't find
a "From" line first, or something like that.  (If I move Upstream-Status
and Signed-off-by below "From" then git am is happy.). But by default git
am fails, and the first fallback, git apply, doesn't work with my patch as
I expected.  git apply appears to only focus on its current directory,
unlike git am which I think applies from the work-tree root.  I may have
only exercised the git am path initially, but I'll investigate the
difference here further before providing an updated patch.

> We may need to teach GitApplyTree to calculate the difference between the
git repodir and patchdir and adjust the offsets accordingly?

That was the approach I started with, but I couldn't come up with a
scenario where we would end up with an offset other than the default (1)
for commands that work from the repo root (git am).  So maybe the git am
case is the only one that needs to ignore the striplevel, if it truly
always expects to work from the repo root.  Maybe git apply is the case I
unintentionally broke.

When I post a revised patch, I'll also update the commit message subject as
Peter pointed out.

On Thu, Apr 9, 2020 at 6:07 AM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2020-04-09 at 00:41 -0400, bkylerussell@gmail.com wrote:
> > Don't pass a path removal parameter to git am/git apply since
> GitApplyTree
> > always applies patches from the root of the git repo, not from each
> > individual patchdir (like quilt).
> >
> > GitApplyTree uses `git rev-parse --show-toplevel` to find the root of
> > the repo no matter which patchdir is passed into the GitApplyTree
> instance.
> > In most cases, this works fine when applying patches since the three-way
> > merge can figure out where the patch should be applied to existing files
> > based on the history, even if the file paths are excessively stripped.
> >
> > An exception to this occurs if a patch adds a new file to the repo
> > and a custom ${S} has been set to a subdirectory of the repo such
> > that the patch requires a non-default striplevel parameter.
> >
> > When processed by GitApplyTree, the patch will have its striplevel
> > parameter given to git even though it's applied at the root of the
> > tree.
> >
> > For example, consider a patch that adds a new file src/foo to a repo.
> >
> >   --- /dev/null
> >   +++ b/src/foo
> >   @@ -0,0 +1 @@
> >   +new file test
> >
> > The recipe sets S = "${WORKDIR}/git/src" and the patch is added to
> SRC_URI
> > using "striplevel=2" since the default patch tool, quilt, applies each
> > patch at its patchdir, which defaults to ${S}.
> >
> > When the GitApplyTree patch class is used (for example, through devtool),
> > the root of the repo is passed to git as the work tree, in addition to
> the
> > path removal paramter.  In the case above, GitApplyTree creates
> > ${WORKDIR}/git/foo instead of ${WORKDIR}/git/src/foo.
> >
> > Interestingly enough, you can take a more typical patch example that only
> > changes existing files (instead of adding a new file) and remove the -3
> > argument from the git am/git apply commands issued by GitApplyTree.  In
> > this scenario, the patch will also fail to apply because the path removal
> > arg is still being passed, and the resulting path does not exist.
> >
> > Since GitApplyTree always applies from the root of the repo, it should
> > ignore any striplevel arguments.
>
> I don't follow this argument. We have several recipes in OE-Core which
> set striplevel in SRC_URI (e.g. bash, llvm and a pointless looking one
> in build-compare). If you changed the patch tool to git for those
> recipes, doesn't it fail with this patch applied?
>
> I think you're right and there is an issue here since do_patch assumes
> ${S} is the root for patches whilst git assumes the root of the repo is
> the patch root. If those are different, it will break.
>
> Put another way, I think your patch fixes one case whilst breaking
> others :(.
>
> I'm not sure how we reconcile this. I certainly do not like hoping 'git
> am -3' figures it out. We may need to teach GitApplyTree to calculate
> the difference between the git repodir and patchdir and adjust the
> offsets accordingly?
>
> Cheers,
>
> Richard
>
>
>

[-- Attachment #2: Type: text/html, Size: 5969 bytes --]

[PATCH v2 0/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

Here's an updated patch that I hope will be a little more clear.  I've dropped
the change to git apply since that wasn't behaving as I expected, then added
a selftest on llvm that demonstrates the issue I'm trying to resolve.  Without
the change in patch.py included here, the selftest that I'm proposing will fail.

One might argue, "Well, if you change the default PATCHTOOL, you could also
just change your patch SRC_URIs and drop the striplevel parameter."  While
that may be true, the same failure can also be demonstrated using devtool on
llvm without modifying PATCHTOOL at all, since by default devtool also uses
GitApplyTree.  So if you don't consider "modifying a recipe's PATCHTOOL without
also modify its SRC_URI patches" to be a valid workflow, devtool is another
common workflow where this behavior could be encountered.

Hope that helps,

Kyle

Kyle Russell (1):
  patch.py: don't apply striplevel to git am command

 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

-- 
2.17.1

[PATCH v2 1/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

Don't pass a path removal parameter to git am since it always applies
patches from the root of the git repo, not from each individual patchdir
(like quilt).

GitApplyTree uses `git rev-parse --show-toplevel` to find the root of
the repo no matter which patchdir is passed into the GitApplyTree instance.
But for recipes who set S to a subdirectory within their repo, patches
may be added to SRC_URI using a striplevel parameter to match the depth of S.

Currently when these patches are applied with git am in GitApplyTree instead
of quilt, the striplevel still takes effect even though git applies the
patch itself from the repo root, not from within S.

In most cases when a patch modifies existing files within a repo, git am
works fine and the desired outcome is still achieved because the three-way
merge figures out which file the patch was intended to modify based on the
repo history.

An exception to this occurs if the patch adds a new file to the repo.

For example, consider a patch that adds a new file src/foo to a repo.
Additionally, assume the recipe for this repo sets S = "${WORKDIR}/git/src",
and the patch is added to SRC_URI using "striplevel=2" since the default
patch tool, quilt, applies each patch at its patchdir, ${S}.

  --- /dev/null
  +++ b/src/foo
  @@ -0,0 +1 @@
  +new file test

When the GitApplyTree patch class is used (through either devtool or
PATCHTOOL = "git"), the root of the repo is passed to git as the work tree,
in addition to the path removal paramter.  In the case above, GitApplyTree
creates ${WORKDIR}/git/foo instead of ${WORKDIR}/git/src/foo.

This is demonstrated with the included selftest.  By not passing the striplevel
parameter to git am, the patch creates the new file at the intended depth in
the repo.
---
 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

diff --git a/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch b/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
new file mode 100644
index 0000000000..8af0afeb12
--- /dev/null
+++ b/meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
@@ -0,0 +1,17 @@
+From 057c3da44affb47c154ec1ef67bd11646a6429fa Mon Sep 17 00:00:00 2001
+From: Kyle Russell <bkylerussell@gmail.com>
+Date: Tue, 28 Apr 2020 15:18:43 -0400
+Subject: [PATCH] Test new file
+
+---
+ llvm/README.new | 1 +
+ 1 file changed, 1 insertion(+)
+ create mode 100644 llvm/README.new
+
+diff --git a/llvm/README.new b/llvm/README.new
+new file mode 100644
+index 00000000000..0527e6bd2d7
+--- /dev/null
++++ b/llvm/README.new
+@@ -0,0 +1 @@
++This is a test
diff --git a/meta-selftest/recipes-test/llvm/llvm_%.bbappend b/meta-selftest/recipes-test/llvm/llvm_%.bbappend
new file mode 100644
index 0000000000..205720982c
--- /dev/null
+++ b/meta-selftest/recipes-test/llvm/llvm_%.bbappend
@@ -0,0 +1,2 @@
+# This bbappend is used to alter the recipe using the test_recipe.inc file created by tests.
+include test_recipe.inc
diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 2b1eee1003..7b9fd67194 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -517,7 +517,7 @@ class GitApplyTree(PatchTree):
             try:
                 shellcmd = [patchfilevar, "git", "--work-tree=%s" % reporoot]
                 self.gitCommandUserOptions(shellcmd, self.commituser, self.commitemail)
-                shellcmd += ["am", "-3", "--keep-cr", "-p%s" % patch['strippath']]
+                shellcmd += ["am", "-3", "--keep-cr"]
                 return _applypatchhelper(shellcmd, patch, force, reverse, run)
             except CmdError:
                 # Need to abort the git am, or we'll still be within it at the end
diff --git a/meta/lib/oeqa/selftest/cases/bbtests.py b/meta/lib/oeqa/selftest/cases/bbtests.py
index dc423ec439..6d863b425a 100644
--- a/meta/lib/oeqa/selftest/cases/bbtests.py
+++ b/meta/lib/oeqa/selftest/cases/bbtests.py
@@ -4,6 +4,7 @@
 
 import os
 import re
+import tempfile
 
 import oeqa.utils.ftools as ftools
 from oeqa.utils.commands import runCmd, bitbake, get_bb_var, get_bb_vars
@@ -81,6 +82,23 @@ class BitbakeTests(OESelftestTestCase):
                 found = l
         self.assertTrue(found and found.startswith("ERROR:"), msg = "Incorrectly formed patch application didn't fail. bitbake output: %s" % result.output)
 
+    def test_patchtool_git(self):
+        test_recipe = 'llvm'
+        tmpdir = tempfile.mkdtemp(prefix='bitbakeqa')
+        bb_vars = get_bb_vars(['S'], test_recipe)
+        src_dir = bb_vars['S']
+
+        self.write_recipeinc('llvm', 'FILESEXTRAPATHS_prepend := "${THISDIR}/files:"\nSRC_URI += "file://0001-Test-new-file.patch;striplevel=2"')
+        bitbake('llvm -c patch')
+        self.assertExists('%s/README.new' % src_dir)
+
+        bitbake('%s -c clean' % test_recipe)
+        self.append_recipeinc('llvm', 'PATCHTOOL = "git"')
+        bitbake('llvm -c patch')
+        self.assertExists('%s/README.new' % src_dir)
+
+        self.delete_recipeinc('llvm')
+
     def test_force_task_1(self):
         # test 1 from bug 5875
         test_recipe = 'zlib'
-- 
2.17.1

[PATCH v2 0/1] patch.py: don't apply striplevel to git am command

[bkylerussell]

I thought I had sent this out a month ago, but I can't find it in patchwork,
so I'm resending.  This is an updated patch that addresses feedback on v1, and
should hopefully be more clear.

I've dropped the change to 'git apply' since that wasn't behaving as I originally
thought, then I added a selftest on llvm that demonstrates the issue I'm trying to
resolve.  Without the change in patch.py included here, the selftest that I'm
proposing will fail.

One might argue, "Well, if you change the default PATCHTOOL, you could also
just change your patch SRC_URIs and drop the striplevel parameter."  While
that may be true, the same failure can also be demonstrated using devtool on
llvm without modifying PATCHTOOL at all, since by default devtool also uses
GitApplyTree.  So if you don't consider "modifying a recipe's PATCHTOOL without
also modify its SRC_URI patches" to be a valid workflow, devtool is another
common workflow where this behavior could be encountered.

Hope that helps,
Kyle

Kyle Russell (1):
  patch.py: don't apply striplevel to git am command

 .../llvm/files/0001-Test-new-file.patch        | 17 +++++++++++++++++
 .../recipes-test/llvm/llvm_%.bbappend          |  2 ++
 meta/lib/oe/patch.py                           |  2 +-
 meta/lib/oeqa/selftest/cases/bbtests.py        | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 meta-selftest/recipes-test/llvm/files/0001-Test-new-file.patch
 create mode 100644 meta-selftest/recipes-test/llvm/llvm_%.bbappend

-- 
2.17.1