Patchwork apply-patches.sh: detect missing patches

login
register
mail settings
Submitter Ralph Siemsen
Date Aug. 13, 2013, 4 p.m.
Message ID <20130813160017.GA2590@harvey.netwinder.org>
Download mbox | patch
Permalink /patch/266895/
State Superseded
Headers show

Comments

Ralph Siemsen - Aug. 13, 2013, 4 p.m.
apply-patches.sh: detect missing patches

The current patch logic does not detect missing patch files,
particularly when using a series file. If the series file
refers to non-existent patches, a minor warning appears,
but the build continues.

The root cause is the "cat <patchfile> | patch ..." pipleline.
When patchfile does not exist, the "cat" command prints a
warning, however, the pipeline status is determined by the
final "patch" command. And since patch has not received any
input, it returns success.

There are two possible solutions that I can see:
  1. set -o pipefail, then pipeline will return error from 'cat'
  2. check for patch existence explicitly
I've opted for 2nd choice, it seems less risky.

The following patch adds the check. It also fixes the indentation
of an adjacent line (TAB versus spaces) making it consistent.
Thomas De Schampheleire - Aug. 23, 2013, 10:31 a.m.
Hi Ralph,

On Tue, Aug 13, 2013 at 6:00 PM, Ralph Siemsen <ralphs@netwinder.org> wrote:
> apply-patches.sh: detect missing patches
>
> The current patch logic does not detect missing patch files,
> particularly when using a series file. If the series file
> refers to non-existent patches, a minor warning appears,
> but the build continues.
>
> The root cause is the "cat <patchfile> | patch ..." pipleline.
> When patchfile does not exist, the "cat" command prints a
> warning, however, the pipeline status is determined by the
> final "patch" command. And since patch has not received any
> input, it returns success.
>
> There are two possible solutions that I can see:
>   1. set -o pipefail, then pipeline will return error from 'cat'
>   2. check for patch existence explicitly
> I've opted for 2nd choice, it seems less risky.
>
> The following patch adds the check. It also fixes the indentation
> of an adjacent line (TAB versus spaces) making it consistent.
>
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index 7d5856c..a3f7153 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -76,7 +76,11 @@ function apply_patch {
>      esac
>      echo ""
>      echo "Applying $patch using ${type}: "
> -       echo $patch >> ${builddir}/.applied_patches_list
> +    if ! test -e "${path}/$patch" ; then

Any particular reason why your using the explicit 'test' and not [ -e ... ] ?

> +       echo "Error: missing patch file ${path}/$patch"
> +       return 1
> +    fi
> +    echo $patch >> ${builddir}/.applied_patches_list
>      ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}"
>      if [ $? != 0 ] ; then
>          echo "Patch failed!  Please fix ${patch}!"

Although I agree with the concept of checking for existence, I'm not
sure about the action to take.
Earlier in apply-patches.sh, if a patch is in an unsupported format,
it is simply skipped (and a message printed). I think the action in
that case should line up with the nonexisting patch case, so either
give a warning and continue, or an error and stop.

Best regards,
Thomas
Thomas De Schampheleire - Sept. 5, 2013, 7:03 a.m.
On Fri, Aug 23, 2013 at 12:31 PM, Thomas De Schampheleire
<patrickdepinguin+buildroot@gmail.com> wrote:
> Hi Ralph,
>
> On Tue, Aug 13, 2013 at 6:00 PM, Ralph Siemsen <ralphs@netwinder.org> wrote:
>> apply-patches.sh: detect missing patches
>>
>> The current patch logic does not detect missing patch files,
>> particularly when using a series file. If the series file
>> refers to non-existent patches, a minor warning appears,
>> but the build continues.
>>
>> The root cause is the "cat <patchfile> | patch ..." pipleline.
>> When patchfile does not exist, the "cat" command prints a
>> warning, however, the pipeline status is determined by the
>> final "patch" command. And since patch has not received any
>> input, it returns success.
>>
>> There are two possible solutions that I can see:
>>   1. set -o pipefail, then pipeline will return error from 'cat'
>>   2. check for patch existence explicitly
>> I've opted for 2nd choice, it seems less risky.
>>
>> The following patch adds the check. It also fixes the indentation
>> of an adjacent line (TAB versus spaces) making it consistent.
>>
>> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
>> index 7d5856c..a3f7153 100755
>> --- a/support/scripts/apply-patches.sh
>> +++ b/support/scripts/apply-patches.sh
>> @@ -76,7 +76,11 @@ function apply_patch {
>>      esac
>>      echo ""
>>      echo "Applying $patch using ${type}: "
>> -       echo $patch >> ${builddir}/.applied_patches_list
>> +    if ! test -e "${path}/$patch" ; then
>
> Any particular reason why your using the explicit 'test' and not [ -e ... ] ?
>
>> +       echo "Error: missing patch file ${path}/$patch"
>> +       return 1
>> +    fi
>> +    echo $patch >> ${builddir}/.applied_patches_list
>>      ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}"
>>      if [ $? != 0 ] ; then
>>          echo "Patch failed!  Please fix ${patch}!"
>
> Although I agree with the concept of checking for existence, I'm not
> sure about the action to take.
> Earlier in apply-patches.sh, if a patch is in an unsupported format,
> it is simply skipped (and a message printed). I think the action in
> that case should line up with the nonexisting patch case, so either
> give a warning and continue, or an error and stop.

Any input from others?

Thanks,
Thomas
Thomas Petazzoni - Sept. 5, 2013, 8:04 a.m.
Dear Thomas De Schampheleire,

On Thu, 5 Sep 2013 09:03:33 +0200, Thomas De Schampheleire wrote:

> > Although I agree with the concept of checking for existence, I'm not
> > sure about the action to take.
> > Earlier in apply-patches.sh, if a patch is in an unsupported format,
> > it is simply skipped (and a message printed). I think the action in
> > that case should line up with the nonexisting patch case, so either
> > give a warning and continue, or an error and stop.
> 
> Any input from others?

My general feeling is that when something goes wrong or looks wrong, we
should abort with an error, and not try to continue with something
more-or-less broken/incorrect.

Thomas
Luca Ceresoli - Sept. 5, 2013, 8:35 a.m.
Thomas Petazzoni wrote:
> Dear Thomas De Schampheleire,
>
> On Thu, 5 Sep 2013 09:03:33 +0200, Thomas De Schampheleire wrote:
>
>>> Although I agree with the concept of checking for existence, I'm not
>>> sure about the action to take.
>>> Earlier in apply-patches.sh, if a patch is in an unsupported format,
>>> it is simply skipped (and a message printed). I think the action in
>>> that case should line up with the nonexisting patch case, so either
>>> give a warning and continue, or an error and stop.
>> Any input from others?
> My general feeling is that when something goes wrong or looks wrong, we
> should abort with an error, and not try to continue with something
> more-or-less broken/incorrect.

I agree. Who reads a million lines of build outputwhen make exits returning
zero? Definitely BR should stop and shout out loud about missing or
incorrect patches.

Luca
Yann E. MORIN - Sept. 5, 2013, 2:01 p.m.
All,

On 2013-09-05 10:35 +0200, Luca Ceresoli spake thusly:
> Thomas Petazzoni wrote:
> >Dear Thomas De Schampheleire,
> >
> >On Thu, 5 Sep 2013 09:03:33 +0200, Thomas De Schampheleire wrote:
> >
> >>>Although I agree with the concept of checking for existence, I'm not
> >>>sure about the action to take.
> >>>Earlier in apply-patches.sh, if a patch is in an unsupported format,
> >>>it is simply skipped (and a message printed). I think the action in
> >>>that case should line up with the nonexisting patch case, so either
> >>>give a warning and continue, or an error and stop.
> >>Any input from others?
> >My general feeling is that when something goes wrong or looks wrong, we
> >should abort with an error, and not try to continue with something
> >more-or-less broken/incorrect.
> 
> I agree. Who reads a million lines of build outputwhen make exits returning
> zero? Definitely BR should stop and shout out loud about missing or
> incorrect patches.

I concur: we should abort on incorrect/missing/malformed patch.

Regards,
Yann E. MORIN.
Thomas De Schampheleire - Sept. 5, 2013, 7:20 p.m.
Hi Ralph,

On Thu, Sep 5, 2013 at 4:01 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> All,
>
> On 2013-09-05 10:35 +0200, Luca Ceresoli spake thusly:
>> Thomas Petazzoni wrote:
>> >Dear Thomas De Schampheleire,
>> >
>> >On Thu, 5 Sep 2013 09:03:33 +0200, Thomas De Schampheleire wrote:
>> >
>> >>>Although I agree with the concept of checking for existence, I'm not
>> >>>sure about the action to take.
>> >>>Earlier in apply-patches.sh, if a patch is in an unsupported format,
>> >>>it is simply skipped (and a message printed). I think the action in
>> >>>that case should line up with the nonexisting patch case, so either
>> >>>give a warning and continue, or an error and stop.
>> >>Any input from others?
>> >My general feeling is that when something goes wrong or looks wrong, we
>> >should abort with an error, and not try to continue with something
>> >more-or-less broken/incorrect.
>>
>> I agree. Who reads a million lines of build outputwhen make exits returning
>> zero? Definitely BR should stop and shout out loud about missing or
>> incorrect patches.
>
> I concur: we should abort on incorrect/missing/malformed patch.
>

Would you mind sending a second patch that changes the 'unknown patch
format' case so that an error is thrown?

Additionally, in your current patch, I would suggest changing the 'if
test ...' into 'if [ ... ], for the sake of lining up with the style
in the rest of the script.

Note that if you don't have time to send a second patch, just let me
know, then I will.

Best regards,
Thomas

Patch

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 7d5856c..a3f7153 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -76,7 +76,11 @@  function apply_patch {
     esac
     echo ""
     echo "Applying $patch using ${type}: "
-	echo $patch >> ${builddir}/.applied_patches_list
+    if ! test -e "${path}/$patch" ; then
+	echo "Error: missing patch file ${path}/$patch"
+	return 1
+    fi
+    echo $patch >> ${builddir}/.applied_patches_list
     ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}"
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix ${patch}!"