support/scripts/apply-patches: use "git apply" as a fallback when applying patches
diff mbox series

Message ID 20190110203004.9812-1-thomas.petazzoni@bootlin.com
State Changes Requested
Headers show
Series
  • support/scripts/apply-patches: use "git apply" as a fallback when applying patches
Related show

Commit Message

Thomas Petazzoni Jan. 10, 2019, 8:30 p.m. UTC
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(-)

Comments

Baruch Siach Jan. 11, 2019, 3:35 a.m. UTC | #1
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@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Thomas Petazzoni Jan. 11, 2019, 8:15 a.m. UTC | #2
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
Baruch Siach Jan. 13, 2019, 8 a.m. UTC | #3
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
Arnout Vandecappelle Jan. 23, 2019, 2:17 p.m. UTC | #4
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
Peter Korsgaard Jan. 23, 2019, 8:34 p.m. UTC | #5
>>>>> "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)

Patch
diff mbox series

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 {