* [PATCH 0/1] patch: don't strip GitApplyTree patches @ 2020-04-09 4:41 bkylerussell 2020-04-09 4:41 ` [PATCH 1/1] " bkylerussell 2020-04-09 5:02 ` ✗ patchtest: failure for " Patchwork 0 siblings, 2 replies; 8+ messages in thread From: bkylerussell @ 2020-04-09 4:41 UTC (permalink / raw) To: openembedded-core; +Cc: wesley.lindauer, Kyle Russell 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] patch: don't strip GitApplyTree patches 2020-04-09 4:41 [PATCH 0/1] patch: don't strip GitApplyTree patches bkylerussell @ 2020-04-09 4:41 ` bkylerussell 2020-04-09 10:07 ` [OE-core] " Richard Purdie 2020-04-10 3:44 ` [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches Peter Kjellerstedt 2020-04-09 5:02 ` ✗ patchtest: failure for " Patchwork 1 sibling, 2 replies; 8+ messages in thread From: bkylerussell @ 2020-04-09 4:41 UTC (permalink / raw) To: openembedded-core; +Cc: wesley.lindauer, Kyle Russell 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches 2020-04-09 4:41 ` [PATCH 1/1] " bkylerussell @ 2020-04-09 10:07 ` Richard Purdie 2020-04-27 20:32 ` bkylerussell 2020-04-10 3:44 ` [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches Peter Kjellerstedt 1 sibling, 1 reply; 8+ messages in thread From: Richard Purdie @ 2020-04-09 10:07 UTC (permalink / raw) To: bkylerussell, openembedded-core; +Cc: wesley.lindauer 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches 2020-04-09 10:07 ` [OE-core] " Richard Purdie @ 2020-04-27 20:32 ` bkylerussell 2020-04-30 0:01 ` [PATCH v2 0/1] patch.py: don't apply striplevel to git am command bkylerussell 0 siblings, 1 reply; 8+ messages in thread From: bkylerussell @ 2020-04-27 20:32 UTC (permalink / raw) To: Richard Purdie; +Cc: OE-core, wesley.lindauer [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 0/1] patch.py: don't apply striplevel to git am command 2020-04-27 20:32 ` bkylerussell @ 2020-04-30 0:01 ` bkylerussell 2020-04-30 0:01 ` [PATCH v2 1/1] " bkylerussell 0 siblings, 1 reply; 8+ messages in thread From: bkylerussell @ 2020-04-30 0:01 UTC (permalink / raw) To: openembedded-core; +Cc: wesley.lindauer, richard.purdie, Kyle Russell 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] patch.py: don't apply striplevel to git am command 2020-04-30 0:01 ` [PATCH v2 0/1] patch.py: don't apply striplevel to git am command bkylerussell @ 2020-04-30 0:01 ` bkylerussell 0 siblings, 0 replies; 8+ messages in thread From: bkylerussell @ 2020-04-30 0:01 UTC (permalink / raw) To: openembedded-core; +Cc: wesley.lindauer, richard.purdie, Kyle Russell 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches 2020-04-09 4:41 ` [PATCH 1/1] " bkylerussell 2020-04-09 10:07 ` [OE-core] " Richard Purdie @ 2020-04-10 3:44 ` Peter Kjellerstedt 1 sibling, 0 replies; 8+ messages in thread From: Peter Kjellerstedt @ 2020-04-10 3:44 UTC (permalink / raw) To: bkylerussell, openembedded-core; +Cc: wesley.lindauer > -----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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* ✗ patchtest: failure for patch: don't strip GitApplyTree patches 2020-04-09 4:41 [PATCH 0/1] patch: don't strip GitApplyTree patches bkylerussell 2020-04-09 4:41 ` [PATCH 1/1] " bkylerussell @ 2020-04-09 5:02 ` Patchwork 1 sibling, 0 replies; 8+ messages in thread From: Patchwork @ 2020-04-09 5:02 UTC (permalink / raw) To: Kyle Russell; +Cc: openembedded-core == 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-30 0:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-09 4:41 [PATCH 0/1] patch: don't strip GitApplyTree patches bkylerussell 2020-04-09 4:41 ` [PATCH 1/1] " bkylerussell 2020-04-09 10:07 ` [OE-core] " Richard Purdie 2020-04-27 20:32 ` bkylerussell 2020-04-30 0:01 ` [PATCH v2 0/1] patch.py: don't apply striplevel to git am command bkylerussell 2020-04-30 0:01 ` [PATCH v2 1/1] " bkylerussell 2020-04-10 3:44 ` [OE-core] [PATCH 1/1] patch: don't strip GitApplyTree patches Peter Kjellerstedt 2020-04-09 5:02 ` ✗ patchtest: failure for " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.