All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
@ 2019-01-10 20:30 Thomas Petazzoni
  2019-01-11  3:35 ` Baruch Siach
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Petazzoni @ 2019-01-10 20:30 UTC (permalink / raw)
  To: buildroot

We currently use plain old "patch" to apply patches. While this works
fine in most situations, it doesn't work well for patches generated
with git format-patch that:

 - Contain changes to binary files

 - Or contain changes to files in directories that were symlinks to
   other directories, with the symlink being removed as part of the
   same patch.

We encountered such issues with a large stack of patches to be applied
on top of arm-trusted-firmware. Such patches apply perfectly fine with
"git apply".

Switching everybody to unconditionally use "git apply" seems a bit
risky, so instead we take a different route: if applying the patch
with "patch" fails, then we try with "git apply".

A few implementation notes:

 - The script has "set -e" so for the "patch --dry-run" command, we
   have to take special precautions to not bail out of the script on
   error.

 - Also due to "set -e", the check on the return value of "patch" is
   no longer needed, and we also don't need to check the return value
   of "git apply".

 - We need to pass "--git-dir=/dev/null", otherwise "git apply"
   travels up the directory hierarchy until it finds a .git folder. If
   it finds one, but the files being patched are not under version
   control, it skips patching those files. Obviously, the files in
   output/build/foo/ are not under version control, so this behavior
   is not desirable. Simply making "git apply" believe it is not
   running from inside a Git repository disables this check.

Cc: Kostya Porotchkin <kostap@marvell.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/scripts/apply-patches.sh | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 66fef262ee..0fd336968e 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -119,11 +119,18 @@ function apply_patch {
         exit 1
     fi
     echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
-    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
-    if [ $? != 0 ] ; then
-        echo "Patch failed!  Please fix ${patch}!"
-        exit 1
+
+    # We don't want this to abort the script on failure (script is run
+    # with set -e)
+    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" --dry-run -t -N -s && retval=0 || retval=$?
+    if [ $retval -eq 0 ] ; then
+	    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
+	    # Due to set -e, if we reach here, applying the patch was successful
+	    return
     fi
+
+    [ -z "${silent}" ] && gitopts=-v
+    (cd ${builddir}; ${uncomp} "${path}/$patch" | git --git-dir=/dev/null apply -p1 ${gitopts})
 }
 
 function scan_patchdir {
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
  2019-01-10 20:30 [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches Thomas Petazzoni
@ 2019-01-11  3:35 ` Baruch Siach
  2019-01-11  8:15   ` Thomas Petazzoni
  0 siblings, 1 reply; 6+ messages in thread
From: Baruch Siach @ 2019-01-11  3:35 UTC (permalink / raw)
  To: buildroot

Hi Thomas,

On Thu, Jan 10 2019, Thomas Petazzoni wrote:

> We currently use plain old "patch" to apply patches. While this works
> fine in most situations, it doesn't work well for patches generated
> with git format-patch that:
>
>  - Contain changes to binary files
>
>  - Or contain changes to files in directories that were symlinks to
>    other directories, with the symlink being removed as part of the
>    same patch.
>
> We encountered such issues with a large stack of patches to be applied
> on top of arm-trusted-firmware. Such patches apply perfectly fine with
> "git apply".
>
> Switching everybody to unconditionally use "git apply" seems a bit
> risky, so instead we take a different route: if applying the patch
> with "patch" fails, then we try with "git apply".

This makes git a host dependency. Should we add host-git, or list git
with host requirements?

baruch

> A few implementation notes:
>
>  - The script has "set -e" so for the "patch --dry-run" command, we
>    have to take special precautions to not bail out of the script on
>    error.
>
>  - Also due to "set -e", the check on the return value of "patch" is
>    no longer needed, and we also don't need to check the return value
>    of "git apply".
>
>  - We need to pass "--git-dir=/dev/null", otherwise "git apply"
>    travels up the directory hierarchy until it finds a .git folder. If
>    it finds one, but the files being patched are not under version
>    control, it skips patching those files. Obviously, the files in
>    output/build/foo/ are not under version control, so this behavior
>    is not desirable. Simply making "git apply" believe it is not
>    running from inside a Git repository disables this check.
>
> Cc: Kostya Porotchkin <kostap@marvell.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  support/scripts/apply-patches.sh | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index 66fef262ee..0fd336968e 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -119,11 +119,18 @@ function apply_patch {
>          exit 1
>      fi
>      echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
> -    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
> -    if [ $? != 0 ] ; then
> -        echo "Patch failed!  Please fix ${patch}!"
> -        exit 1
> +
> +    # We don't want this to abort the script on failure (script is run
> +    # with set -e)
> +    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" --dry-run -t -N -s && retval=0 || retval=$?
> +    if [ $retval -eq 0 ] ; then
> +	    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
> +	    # Due to set -e, if we reach here, applying the patch was successful
> +	    return
>      fi
> +
> +    [ -z "${silent}" ] && gitopts=-v
> +    (cd ${builddir}; ${uncomp} "${path}/$patch" | git --git-dir=/dev/null apply -p1 ${gitopts})
>  }
>
>  function scan_patchdir {


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
  2019-01-11  3:35 ` Baruch Siach
@ 2019-01-11  8:15   ` Thomas Petazzoni
  2019-01-13  8:00     ` Baruch Siach
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Petazzoni @ 2019-01-11  8:15 UTC (permalink / raw)
  To: buildroot

Hello,

On Fri, 11 Jan 2019 05:35:13 +0200, Baruch Siach wrote:

> > Switching everybody to unconditionally use "git apply" seems a bit
> > risky, so instead we take a different route: if applying the patch
> > with "patch" fails, then we try with "git apply".  
> 
> This makes git a host dependency. Should we add host-git, or list git
> with host requirements?

It does not really make git a mandatory dependency. Indeed, "git apply"
is only tried if "patch" fails.

So, before my patch, if a patch failed to apply because "patch" failed,
then it aborted the build.

With my patch, if a patch fails to apply because "patch" failed, then
we will try "git apply". If "git apply" is not available, it will fail,
just like it used to be.

So my proposal doesn't *require* git, it only tries harder to apply
patches by using "git apply" if available.

Setups that used to work today without "git" installed will continue to
work with no change.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
  2019-01-11  8:15   ` Thomas Petazzoni
@ 2019-01-13  8:00     ` Baruch Siach
  2019-01-23 14:17       ` Arnout Vandecappelle
  0 siblings, 1 reply; 6+ messages in thread
From: Baruch Siach @ 2019-01-13  8:00 UTC (permalink / raw)
  To: buildroot


On Fri, Jan 11 2019, Thomas Petazzoni wrote:
> On Fri, 11 Jan 2019 05:35:13 +0200, Baruch Siach wrote:
>
>> > Switching everybody to unconditionally use "git apply" seems a bit
>> > risky, so instead we take a different route: if applying the patch
>> > with "patch" fails, then we try with "git apply".  
>> 
>> This makes git a host dependency. Should we add host-git, or list git
>> with host requirements?
>
> It does not really make git a mandatory dependency. Indeed, "git apply"
> is only tried if "patch" fails.
>
> So, before my patch, if a patch failed to apply because "patch" failed,
> then it aborted the build.
>
> With my patch, if a patch fails to apply because "patch" failed, then
> we will try "git apply". If "git apply" is not available, it will fail,
> just like it used to be.
>
> So my proposal doesn't *require* git, it only tries harder to apply
> patches by using "git apply" if available.
>
> Setups that used to work today without "git" installed will continue to
> work with no change.

With this patch a package selection change that works on the developer's
machine where git is installed, would fail on the production build
machine that is missing git. Is there a clear error message in case of
patch failure?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
  2019-01-13  8:00     ` Baruch Siach
@ 2019-01-23 14:17       ` Arnout Vandecappelle
  2019-01-23 20:34         ` Peter Korsgaard
  0 siblings, 1 reply; 6+ messages in thread
From: Arnout Vandecappelle @ 2019-01-23 14:17 UTC (permalink / raw)
  To: buildroot



On 13/01/2019 09:00, Baruch Siach wrote:
> 
> On Fri, Jan 11 2019, Thomas Petazzoni wrote:
>> On Fri, 11 Jan 2019 05:35:13 +0200, Baruch Siach wrote:
>>
>>>> Switching everybody to unconditionally use "git apply" seems a bit
>>>> risky, so instead we take a different route: if applying the patch
>>>> with "patch" fails, then we try with "git apply".  
>>>
>>> This makes git a host dependency. Should we add host-git, or list git
>>> with host requirements?
>>
>> It does not really make git a mandatory dependency. Indeed, "git apply"
>> is only tried if "patch" fails.
>>
>> So, before my patch, if a patch failed to apply because "patch" failed,
>> then it aborted the build.
>>
>> With my patch, if a patch fails to apply because "patch" failed, then
>> we will try "git apply". If "git apply" is not available, it will fail,
>> just like it used to be.
>>
>> So my proposal doesn't *require* git, it only tries harder to apply
>> patches by using "git apply" if available.
>>
>> Setups that used to work today without "git" installed will continue to
>> work with no change.
> 
> With this patch a package selection change that works on the developer's
> machine where git is installed, would fail on the production build
> machine that is missing git. Is there a clear error message in case of
> patch failure?

 I'm with Baruch here. The reason to have this feature is exactly to have the
possibility to have patches that modify binaries or that modify symlinks. Thus,
it becomes likely that someone will submit such a patch. It will work for the
developer, and it will also work in the autobuilders since all of them have git
installed (otherwise they wouldn't be able to clone the Buildroot repo). Thus,
any such patch will go unnoticed.

 I don't think it hurts to force git as a dependency.

 Regards,
 Arnout

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches
  2019-01-23 14:17       ` Arnout Vandecappelle
@ 2019-01-23 20:34         ` Peter Korsgaard
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Korsgaard @ 2019-01-23 20:34 UTC (permalink / raw)
  To: buildroot

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> With this patch a package selection change that works on the developer's
 >> machine where git is installed, would fail on the production build
 >> machine that is missing git. Is there a clear error message in case of
 >> patch failure?

 >  I'm with Baruch here. The reason to have this feature is exactly to have the
 > possibility to have patches that modify binaries or that modify symlinks. Thus,
 > it becomes likely that someone will submit such a patch. It will work for the
 > developer, and it will also work in the autobuilders since all of them have git
 > installed (otherwise they wouldn't be able to clone the Buildroot repo). Thus,
 > any such patch will go unnoticed.

I agree, that is not good.


 >  I don't think it hurts to force git as a dependency.

If we do that, should we then always use git apply instead of patch?
(and drop the patch dependency)

-- 
Bye, Peter Korsgaard

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-23 20:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-10 20:30 [Buildroot] [PATCH] support/scripts/apply-patches: use "git apply" as a fallback when applying patches Thomas Petazzoni
2019-01-11  3:35 ` Baruch Siach
2019-01-11  8:15   ` Thomas Petazzoni
2019-01-13  8:00     ` Baruch Siach
2019-01-23 14:17       ` Arnout Vandecappelle
2019-01-23 20:34         ` Peter Korsgaard

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.