[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Ralph Siemsen]

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
+	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}!"

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Thomas De Schampheleire]

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

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Thomas De Schampheleire]

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

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Luca Ceresoli]

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

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Yann E. MORIN]

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Thomas De Schampheleire]

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

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Ralph Siemsen]

Hi Thomas,

Sorry for delayed response.

On Thu, Sep 05, 2013 at 09:20:36PM +0200, Thomas De Schampheleire wrote:
> 
> Would you mind sending a second patch that changes the 'unknown patch
> format' case so that an error is thrown?

I've added it. Note that it is possible to fool this check, by using
for example a name like mydiff.patch.zzz. This matches "*.patch*" and
therefore is allowed, even though we don't know how to deal with ".zzz".

> 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.

Also done. Revised patch follows.

Cheers,
-Ralph

The return status of the "cat <patchfile> | patch ..." pipeline
is zero (success) even if the patchfile does not exist. This is
because patch receives no input, which is not an error condition.
Therefore, explicitly check that patch file exists.

Based on feedback on buildroot mailing list, also changed the
check for unsupported file format. The build will now error out,
rather than continuing on silently.
---
 support/scripts/apply-patches.sh |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 7d5856c..d6e8983 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -70,13 +70,17 @@ function apply_patch {
 	*.patch*)
 	type="patch"; uncomp="cat"; ;;
 	*)
-	echo "Unsupported format file for ${patch}, skip it";
-	return 0;
+	echo "Unsupported format file for ${path}/${patch}";
+	return 1;
 	;;
     esac
     echo ""
     echo "Applying $patch using ${type}: "
-	echo $patch >> ${builddir}/.applied_patches_list
+    if [ ! -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}" -t
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix ${patch}!"
-- 
1.7.7.6

[Buildroot] [PATCH] apply-patches.sh: detect missing patches

[Thomas De Schampheleire]

Hi Ralph,

On Wed, Sep 11, 2013 at 2:06 PM, Ralph Siemsen <ralphs@netwinder.org> wrote:
> Hi Thomas,
>
> Sorry for delayed response.
>
> On Thu, Sep 05, 2013 at 09:20:36PM +0200, Thomas De Schampheleire wrote:
>>
>> Would you mind sending a second patch that changes the 'unknown patch
>> format' case so that an error is thrown?
>
> I've added it. Note that it is possible to fool this check, by using
> for example a name like mydiff.patch.zzz. This matches "*.patch*" and
> therefore is allowed, even though we don't know how to deal with ".zzz".

True, I think this is something we cannot fix.
Some patches in the tree are named .patch.avr32, or
.patch.conditional, but they are in the regular patch format so should
be accepted by apply_patches.sh.

>
>> 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.
>
> Also done. Revised patch follows.

If you send a patch, the general format is:

commit message
--- (three dashes)
any other comments _not_ in the commit message

actual patch (diff --git ...)


The mail you sent is in the format:
other comments
commit message
actual patch

and this cannot be applied as such. Also, it makes the patch less
visible than in a separate mail.
So, please send it again, and put 'PATCH v2' in the subject line, so
it's easy to keep track of the different versions.

Thanks a lot,
Thomas

[Buildroot] [PATCH v2] apply-patches.sh: detect missing patches

[Ralph Siemsen]

The return status of the "cat <patchfile> | patch ..." pipeline
is zero (success) even if the patchfile does not exist. This is
because patch receives no input, which is not an error condition.
Therefore, explicitly check that patch file exists.

Based on feedback on buildroot mailing list, also changed the
check for unsupported file format. The build will now error out,
rather than continuing on silently.
---

Hi Thomas,
Hopefully the formatting is now correct.
-Ralph

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 7d5856c..d6e8983 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -70,13 +70,17 @@ function apply_patch {
 	*.patch*)
 	type="patch"; uncomp="cat"; ;;
 	*)
-	echo "Unsupported format file for ${patch}, skip it";
-	return 0;
+	echo "Unsupported format file for ${path}/${patch}";
+	return 1;
 	;;
     esac
     echo ""
     echo "Applying $patch using ${type}: "
-	echo $patch >> ${builddir}/.applied_patches_list
+    if [ ! -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}!"
-- 
1.7.7.6

[Buildroot] [PATCH v2] apply-patches.sh: detect missing patches

[Yann E. MORIN]

Ralph, All,

On 2013-09-13 14:15 -0400, Ralph Siemsen spake thusly:
> The return status of the "cat <patchfile> | patch ..." pipeline
> is zero (success) even if the patchfile does not exist. This is
> because patch receives no input, which is not an error condition.
> Therefore, explicitly check that patch file exists.
> 
> Based on feedback on buildroot mailing list, also changed the
> check for unsupported file format. The build will now error out,
> rather than continuing on silently.
> ---
> 
> Hi Thomas,
> Hopefully the formatting is now correct.
> -Ralph
> 
> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> index 7d5856c..d6e8983 100755
> --- a/support/scripts/apply-patches.sh
> +++ b/support/scripts/apply-patches.sh
> @@ -70,13 +70,17 @@ function apply_patch {
>  	*.patch*)
>  	type="patch"; uncomp="cat"; ;;
>  	*)
> -	echo "Unsupported format file for ${patch}, skip it";
> -	return 0;
> +	echo "Unsupported format file for ${path}/${patch}";
> +	return 1;

I was a bit surprised to read that you used "return 1" here, rather than
a more explicit "exit 1".

For example, if the patch failed to apply cleanly, the function does not
"return 1" but does "exit 1" (just the line after your patch ends). So I
would prefer this behaviour to be homogeneous across the different
code-paths.

For the records, "return 1" does work since we call apply_patch thus:
    apply_patch "${patchfile}" || exit 1

This is not really straightforward. Just remove the "|| exit 1" and
change all the "return 1" into "exit 1", so the script really errors out
right away.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2] apply-patches.sh: detect missing patches

[Ralph Siemsen]

On Fri, Sep 13, 2013 at 08:27:02PM +0200, Yann E. MORIN wrote:
> > 
> > diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
> > index 7d5856c..d6e8983 100755
> > --- a/support/scripts/apply-patches.sh
> > +++ b/support/scripts/apply-patches.sh
> > @@ -70,13 +70,17 @@ function apply_patch {
> >  	*.patch*)
> >  	type="patch"; uncomp="cat"; ;;
> >  	*)
> > -	echo "Unsupported format file for ${patch}, skip it";
> > -	return 0;
> > +	echo "Unsupported format file for ${path}/${patch}";
> > +	return 1;
> 
> I was a bit surprised to read that you used "return 1" here, rather than
> a more explicit "exit 1".

As you mentioned, both "return" and "exit" work here, so I opted for the
lesser change, figuring it would be less controversial ;)

> This is not really straightforward. Just remove the "|| exit 1" and
> change all the "return 1" into "exit 1", so the script really errors out
> right away.

I'll submit v3 with that fixed.

-R

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Ralph Siemsen]

The "patch" command returns an error code only if patches fail
to apply. Therefore the pipleline "cat <patchfile> | patch ..."
does not fail, even if <patchfile> is missing. Fix this by
adding an explicit check for patch file existence.

Based on feedback from buildroot mailing list, also change the
existing check for unsupported patch format into a fatal error.
---

v1. original
v2. error on unsupported patch formats
v3. explicit exit calls

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index e9c6869..656aa71 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -73,13 +73,17 @@ function apply_patch {
 	*.patch*)
 	type="patch"; uncomp="cat"; ;;
 	*)
-	echo "Unsupported format file for ${patch}, skip it";
-	return 0;
+	echo "Unsupported format file for ${path}/${patch}";
+	exit 1;
 	;;
     esac
     echo ""
     echo "Applying $patch using ${type}: "
-	echo $patch >> ${builddir}/.applied_patches_list
+    if [ ! -e "${path}/$patch" ] ; then
+	echo "Error: missing patch file ${path}/$patch"
+	exit 1
+    fi
+    echo $patch >> ${builddir}/.applied_patches_list
     ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix ${patch}!"
@@ -96,7 +100,7 @@ function scan_patchdir {
     # to apply patches. Skip line starting with a dash.
     if [ -e "${path}/series" ] ; then
         for i in `grep -Ev "^#" ${path}/series 2> /dev/null` ; do
-            apply_patch "$path" "$i" || exit 1
+            apply_patch "$path" "$i"
         done
     else
         for i in `cd $path; ls -d $patches 2> /dev/null` ; do
@@ -109,7 +113,7 @@ function scan_patchdir {
                 tar -C "$unpackedarchivedir" -xaf "${path}/$i"
                 scan_patchdir "$unpackedarchivedir"
             else
-                apply_patch "$path" "$i" || exit 1
+                apply_patch "$path" "$i"
             fi
         done
     fi
-- 
1.8.3.1

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Ralph" == Ralph Siemsen <ralphs@netwinder.org> writes:

 Ralph> The "patch" command returns an error code only if patches fail
 Ralph> to apply. Therefore the pipleline "cat <patchfile> | patch ..."
 Ralph> does not fail, even if <patchfile> is missing. Fix this by
 Ralph> adding an explicit check for patch file existence.

 Ralph> Based on feedback from buildroot mailing list, also change the
 Ralph> existing check for unsupported patch format into a fatal error.
 Ralph> ---

 Ralph> v1. original
 Ralph> v2. error on unsupported patch formats
 Ralph> v3. explicit exit calls

Committed, thanks.

You forgot to add your 'Signed-off-by' on the patch. I've added it for
you, hopefully that is OK?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <jacmet@uclibc.org> writes:

>>>>> "Ralph" == Ralph Siemsen <ralphs@netwinder.org> writes:
 Ralph> The "patch" command returns an error code only if patches fail
 Ralph> to apply. Therefore the pipleline "cat <patchfile> | patch ..."
 Ralph> does not fail, even if <patchfile> is missing. Fix this by
 Ralph> adding an explicit check for patch file existence.

 Ralph> Based on feedback from buildroot mailing list, also change the
 Ralph> existing check for unsupported patch format into a fatal error.
 Ralph> ---

 Ralph> v1. original
 Ralph> v2. error on unsupported patch formats
 Ralph> v3. explicit exit calls

 Peter> Committed, thanks.

 Peter> You forgot to add your 'Signed-off-by' on the patch. I've added it for
 Peter> you, hopefully that is OK?

It unfortunately for the projects using tarballs containing patches,
E.G:

http://autobuild.buildroot.net/results/3d5/3d5f2c960ff2cc2003c9cc68d3a5a48009f5602f/build-end.log

The tarball contains:

debian/
debian/control
debian/postinst
debian/postinst.nfs
debian/source/
debian/source/options
debian/source/format
debian/shlibs
debian/prerm.nfs
debian/rules
debian/postrm
debian/shlibs.nfslock
debian/patches/
debian/patches/07-493462-dotlockfile.c.patch
debian/patches/series
debian/patches/04-505851-remove-debug-code.patch
debian/patches/02-COPYRIGHT.patch
debian/patches/06-493462-dotlockfile.1.patch
debian/patches/09-562937-make-ar-overwrittable.patch
debian/changelog

So a bunch of non-patch files. I guess our best option is to degrade the
"Unsupported format" check to a warning again?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Ralph Siemsen]

On Sun, Sep 15, 2013 at 10:09:45PM +0200, Peter Korsgaard wrote:
> 
> Committed, thanks.
> 
> You forgot to add your 'Signed-off-by' on the patch. I've added it for
> you, hopefully that is OK?

Yes, that is fine. What is the exact syntax/capitalization for this? The
current buildroot manual lists a couple of slight variations.

-Ralph

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Ralph Siemsen]

On Mon, Sep 16, 2013 at 09:11:49AM +0200, Peter Korsgaard wrote:
> 
> The tarball contains:
> 
> debian/
> debian/control
> debian/postinst
> debian/postinst.nfs
> debian/source/
> debian/source/options
> debian/source/format
> debian/shlibs
> debian/prerm.nfs
> debian/rules
> debian/postrm
> debian/shlibs.nfslock
> debian/patches/
> debian/patches/07-493462-dotlockfile.c.patch
> debian/patches/series
> debian/patches/04-505851-remove-debug-code.patch
> debian/patches/02-COPYRIGHT.patch
> debian/patches/06-493462-dotlockfile.1.patch
> debian/patches/09-562937-make-ar-overwrittable.patch
> debian/changelog
> 
> So a bunch of non-patch files. I guess our best option is to degrade the
> "Unsupported format" check to a warning again?

Hmm, none of the packages I regularly build have used this feature.

Going back to a warning would be a simple fix. The other option would
be to expand the tarballs and extract only the patches directory.
I'm not sure which option is better...

-Ralph

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Ralph" == Ralph Siemsen <ralphs@netwinder.org> writes:

 Ralph> On Sun, Sep 15, 2013 at 10:09:45PM +0200, Peter Korsgaard wrote:
 >> 
 >> Committed, thanks.
 >> 
 >> You forgot to add your 'Signed-off-by' on the patch. I've added it for
 >> you, hopefully that is OK?

 Ralph> Yes, that is fine. What is the exact syntax/capitalization for this? The
 Ralph> current buildroot manual lists a couple of slight variations.

The format used by git format-patch -s / the Linux kernel - E.G:

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

I'll fix the manual to use this everywhere, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Thomas Petazzoni]

Dear Peter Korsgaard,

On Mon, 16 Sep 2013 09:11:49 +0200, Peter Korsgaard wrote:

> So a bunch of non-patch files. I guess our best option is to degrade the
> "Unsupported format" check to a warning again?

Another way of seeing that is that using:

<pkg>_PATCH = some-debian-tarball

is not a good idea, and we should have a better way of handling this,
no?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Ralph Siemsen]

On Mon, Sep 16, 2013 at 05:24:44PM +0200, Thomas Petazzoni wrote:
> Dear Peter Korsgaard,
> 
> On Mon, 16 Sep 2013 09:11:49 +0200, Peter Korsgaard wrote:
> 
> > So a bunch of non-patch files. I guess our best option is to degrade the
> > "Unsupported format" check to a warning again?
> 
> Another way of seeing that is that using:
> 
> <pkg>_PATCH = some-debian-tarball
> 
> is not a good idea, and we should have a better way of handling this,
> no?

It looks like only a small number of packages might be affected:

$ grep -ri _PATCH.*DEBIAN .
./package/cvs/cvs.mk:CVS_POST_PATCH_HOOKS += CVS_DEBIAN_PATCHES
./package/thttpd/thttpd.mk:THTTPD_POST_PATCH_HOOKS = THTTPD_DEBIAN_PATCHES
./package/input-tools/input-tools.mk:INPUT_TOOLS_POST_PATCH_HOOKS = INPUT_TOOLS_DEBIAN_PATCHES
./package/sysvinit/sysvinit.mk:SYSVINIT_POST_PATCH_HOOKS = SYSVINIT_DEBIAN_PATCHES
./package/sysklogd/sysklogd.mk:SYSKLOGD_POST_PATCH_HOOKS = SYSKLOGD_DEBIAN_PATCHES
./package/mii-diag/mii-diag.mk:MII_DIAG_POST_PATCH_HOOKS = MII_DIAG_DEBIAN_PATCHES
./package/argus/argus.mk:ARGUS_POST_PATCH_HOOKS += ARGUS_DEBIAN_PATCH_APPLY
./package/liblockfile/liblockfile.mk:LIBLOCKFILE_PATCH = liblockfile_$(LIBLOCKFILE_VERSION)-4.debian.tar.bz2
./package/setserial/setserial.mk:SETSERIAL_POST_PATCH_HOOKS += SETSERIAL_APPLY_DEBIAN_PATCHES
./boot/grub/grub.mk:GRUB_POST_PATCH_HOOKS += GRUB_DEBIAN_PATCHES
$

Many of those packages actually build fine, because they explicitly apply only
the contents of the debian/patches directory. For example from thttpd.mk:

define THTTPD_DEBIAN_PATCHES
        if [ -d $(@D)/debian/patches ]; then \
                support/scripts/apply-patches.sh $(@D) $(@D)/debian/patches \*.patch; \
        fi
endef

The only broken packages are: cvs sysvinit liblockfile setserial

So we have a couple of options:
 1) revert the original change back to a warning, or
 2) fixup the four broken packges using the same method.
 3) something else.

I'm not sure which way to go, please give me your thoughts.

-Ralph

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Arnout Vandecappelle]

On 16/09/13 21:45, Ralph Siemsen wrote:
> Many of those packages actually build fine, because they explicitly apply only
> the contents of the debian/patches directory. For example from thttpd.mk:
>
> define THTTPD_DEBIAN_PATCHES
>          if [ -d $(@D)/debian/patches ]; then \
>                  support/scripts/apply-patches.sh $(@D) $(@D)/debian/patches \*.patch; \
>          fi
> endef
>
> The only broken packages are: cvs sysvinit liblockfile setserial
>
> So we have a couple of options:
>   1) revert the original change back to a warning, or
>   2) fixup the four broken packges using the same method.
>   3) something else.
>
> I'm not sure which way to go, please give me your thoughts.

  The issue with e.g. cvs is not that the patch format is wrong, but 
rather that the patch files don't have the .patch extension:

$ ls build/cvs-1.12.13/debian/patches/
10_rsc2log_fix                  67_date_format_option 

11_check_method_crash           68_DSA_external_passwd_file
12_rcs2log_POSIX_sort           69_ext_allowroot
14_ext_expansion                71_cvsbug_tmpfix
15_PATH_MAX_check               80_cvs-repouid-0.1
20_readdir_errno                81_fix_-l
21_getcwd_chroot                85_normalize_correct_roots
25_import-n-X                   86_server_wrapper
30_quieten_syslog_errors        89_history_val-tag_world_writeable
31_ipv6                         90zlib-read-compressed.diff
51_newlines_in_commit_template  93_homedir
55_keyword_alphanumerics        94_parseopts
56_extra_tags                   95_flag_conflicted_copies
60_PAM_support                  96_manpage_fixes
61_remove_-R_warning            97_cvs.info.typo
62_cvsrc_whitespace             98_fix_sparc_sigbus.diff
65_login_cvspass_message        99_copyright
66_64bit_crashfix

  Of course, before your commit, all these patches (except the few ending 
with .diff) were silently ignored, so we can wonder if these debian 
patches are actually even necessary...


  Regards,
  Arnout
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 Thomas> Dear Peter Korsgaard,
 Thomas> On Mon, 16 Sep 2013 09:11:49 +0200, Peter Korsgaard wrote:

 >> So a bunch of non-patch files. I guess our best option is to degrade the
 >> "Unsupported format" check to a warning again?

 Thomas> Another way of seeing that is that using:

 Thomas> <pkg>_PATCH = some-debian-tarball

 Thomas> is not a good idea, and we should have a better way of handling this,
 Thomas> no?

Arguably yes, but this used to work so some people might be relying on
it - So I change it back to only warn about these files.

Nevertheless, it would be good if we could come up with a better way of
handling these Debian patches.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Ralph" == Ralph Siemsen <ralphs@netwinder.org> writes:

 >> So a bunch of non-patch files. I guess our best option is to degrade the
 >> "Unsupported format" check to a warning again?

 Ralph> Hmm, none of the packages I regularly build have used this feature.

 Ralph> Going back to a warning would be a simple fix. The other option would
 Ralph> be to expand the tarballs and extract only the patches directory.
 Ralph> I'm not sure which option is better...

I've changed it back to just warn as that was the simplest to
do - Thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Yann E. MORIN]

Peter, All,

On 2013-09-16 22:49 +0200, Peter Korsgaard spake thusly:
> >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:
> 
>  Thomas> Dear Peter Korsgaard,
>  Thomas> On Mon, 16 Sep 2013 09:11:49 +0200, Peter Korsgaard wrote:
> 
>  >> So a bunch of non-patch files. I guess our best option is to degrade the
>  >> "Unsupported format" check to a warning again?
> 
>  Thomas> Another way of seeing that is that using:
> 
>  Thomas> <pkg>_PATCH = some-debian-tarball
> 
>  Thomas> is not a good idea, and we should have a better way of handling this,
>  Thomas> no?
> 
> Arguably yes, but this used to work so some people might be relying on
> it - So I change it back to only warn about these files.
> 
> Nevertheless, it would be good if we could come up with a better way of
> handling these Debian patches.

What about extracting the tarballs and only applying patches if the
filename contains (or end up with) '.patch' ?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3] apply-patches.sh: detect missing patches

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> Arguably yes, but this used to work so some people might be relying on
 >> it - So I change it back to only warn about these files.
 >> 
 >> Nevertheless, it would be good if we could come up with a better way of
 >> handling these Debian patches.

 Yann> What about extracting the tarballs and only applying patches if the
 Yann> filename contains (or end up with) '.patch' ?

I'm fairly sure that atleast some Debian packages use .diff. We could
presumably get it to work, but it feels a bit fragile / complicated to me.

-- 
Bye, Peter Korsgaard