New: apply-patches.sh reorders patches given on the command line

https://bugs.busybox.net/show_bug.cgi?id=4796

Summary: apply-patches.sh reorders patches given on the command
line
Product: buildroot
Version: 2012.02
Platform: PC
OS/Version: Linux
Status: NEW
Severity: normal
Priority: P5
Component: Other
AssignedTo: unassigned at buildroot.uclibc.org
ReportedBy: develop at kristov.de
CC: buildroot at uclibc.org
Estimated Hours: 0.0

Created attachment 4052
--> https://bugs.busybox.net/attachment.cgi?id=4052
prevents reordering patches

When support/scripts/apply-patches.sh is given an explicit list of patches instead of a template, they are sorted by name before being applied. This is bad as sometimes the order of patches is important, e.g. if one patch depends on another one. This happens e.g. when applying Debian patches whose order is explicitly described in the file debian/patches/series.

The culprit is a "ls -d" which has to be changed to "ls -dU" in order to prevent sorting to take place. I have attached a corresponding patch.

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

Hello,

Sorry I don't know if it's the good way to answer on the mailing list instead of bugzilla.

Since my goal is to discuss about sorting or not sorting in relation with the patch is sent, I choose the mailing list.

This patch contradicts the patches I have done to manage archive containing several patches in a proper way.

For instance, we have an archive containing patches (from a git branch) whose names should be 0001-patch1, 0002-patch2, etc. If we don't use sorting they would not be applied in the good order.

I was suspecting that we would have some issues with some kind of patches as debian ones.

What should be the good solution? Changing patch naming rules? Managing in a specific way some kind of patch? Don't sort a list of patches but sort the patches extracted into a directory?

Regards

Ludovic

I agree with you maybe the best way is to use a quilt

I use this ofen and it's great it will allow also to have different patch series depending on the version of the package

convert a git patch series in a quilt is quite easy

Best Regards,
J.

Good choice; however, you should have included the reported in CC (he may
not be subscribed to the list). I've done that now.

[snip]

We actually had a very brief discussion about this on the developer day.
From the report:

Series files: we don't accept it in our tree, but we would accept support for it in apply-patches.sh. The latter is useful because some board vendors supply the kernel patches as an unordered tarball with a series file.

Bottom line: if there is a series file (in the directory or in the
tarball), it should be used to set the ordering. If there is no series file, use ls for sorting.

Regards,
Arnout

Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
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

Hello,

This is a patch built on the top of the serie I've sent previously.

Do you think it is sufficient. I was looking for a package using debian patches to test it. I notice that CVS was using this kind of patch but there is no series file into the 'patches' directory so the behavior is still the same (patch order is ls sorting order).

Regards

Ludovic

From: Ludovic Desroches <[email protected]>

If a series file is present use it to determine the proper order to apply patches instead of using ls sorting order.

---
 support/scripts/apply-patches.sh |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

index 51e6c48..c78e947 100755
@@ -59,19 +59,27 @@ function scan_patchdir {
     shift 1
     patches=${@-*}
 
-    for i in `cd $path; ls -d $patches 2> /dev/null` ; do
-        if [ -d "${path}/$i" ] ; then
-            echo "${path}/$i skipped"
-        elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
-            unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
-            rm -rf "$unpackedarchivedir" 2> /dev/null
-            mkdir "$unpackedarchivedir"
-            tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
-            scan_patchdir "$unpackedarchivedir"
-        else
+    # If there is a series file, use it instead of using ls sort order
+    # to apply patches.
+    if [ -e "${path}/series" ] ; then
+        for i in `cat ${path}/series 2> /dev/null` ; do
             apply_patch "$path" "$i" || exit 1
-        fi
-    done
+        done
+    else
+        for i in `cd $path; ls -d $patches 2> /dev/null` ; do
+            if [ -d "${path}/$i" ] ; then
+                echo "${path}/$i skipped"
+            elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
+                unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
+                rm -rf "$unpackedarchivedir" 2> /dev/null
+                mkdir "$unpackedarchivedir"
+                tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
+                scan_patchdir "$unpackedarchivedir"
+            else
+                apply_patch "$path" "$i" || exit 1
+            fi
+        done
+    fi
 }
 
 scan_patchdir $patchdir $patchpattern

1.7.5.4

Hi Ludovic, all,

Maybe i miss something, but i don't see how you handle disabled patches in the series file, i means line starting by '#'?

Regards,

Sam

Hi Samuel,

In fact, no...

I didn't know that I could face such case so I didn't handle it.

Is simply checking '#' presency at the beginning of the the patch name enough?

Regards

Ludovic

Hi Ludovic,

Afaik, yes.

See page 3: http://www.suse.de/~agruen/quilt.pdf

Regards,

Sam

From: Ludovic Desroches <[email protected]>

If a series file is present use it to determine the proper order to apply patches instead of using ls sorting order.

---
 support/scripts/apply-patches.sh |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

index 51e6c48..80736e5 100755
@@ -59,19 +59,27 @@ function scan_patchdir {
     shift 1
     patches=${@-*}
 
-    for i in `cd $path; ls -d $patches 2> /dev/null` ; do
-        if [ -d "${path}/$i" ] ; then
-            echo "${path}/$i skipped"
-        elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
-            unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
-            rm -rf "$unpackedarchivedir" 2> /dev/null
-            mkdir "$unpackedarchivedir"
-            tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
-            scan_patchdir "$unpackedarchivedir"
-        else
+    # If there is a series file, use it instead of using ls sort order
+    # to apply patches. Skip line starting with a dash.
+    if [ -e "${path}/series" ] ; then
+        for i in `cat ${path}/series | grep -Ev "^#" 2> /dev/null` ; do
             apply_patch "$path" "$i" || exit 1
-        fi
-    done
+        done
+    else
+        for i in `cd $path; ls -d $patches 2> /dev/null` ; do
+            if [ -d "${path}/$i" ] ; then
+                echo "${path}/$i skipped"
+            elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
+                unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
+                rm -rf "$unpackedarchivedir" 2> /dev/null
+                mkdir "$unpackedarchivedir"
+                tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
+                scan_patchdir "$unpackedarchivedir"
+            else
+                apply_patch "$path" "$i" || exit 1
+            fi
+        done
+    fi
 }
 
 scan_patchdir $patchdir $patchpattern

1.7.5.4

Hi Ludovic,

Or simply:
for i in grep -Ev "^#" ${path}/series 2> /dev/null ; do

Regards,

Sam

why not simply use quilt?

it will simply also patch series upgrade

we just have to add quilt to the host tools

Best regards,
J.

Can you use quilt without using git?
Note that not every user puts buildroot in an actual repository. And even those who do, may use another version control system than git.

Best regards,
Thomas

quilt does need any scm

it's a patch series management tools

Best Regards,
J.

+1

Regards,
Julien

I have just followed Arnout comments since they have already discussed about this topic.

Why not.

I have never used quilt so I have read the man page and I don't see how it could make our life easier to simply apply patches.

From what I have seen (the man page and a small tutorial), it is requiring a series file so I don't see how it can be useful to apply a patch serie without this series file.

As I said, I have never used quilt so I'm probably wrong. Maybe I have missed some useful and powerful commands. Please let me know how it can feet our needs.

Regards

Ludovic

You can easly manage conflict when upgrading the series

and you just need to do quilt push

yes it's quilt manage the series file and you can push or pop patch and it's used for 20 years IIRC originally for the kernel to maintain patch series when GIT does not even exist

Best Regards,
J.

Le Wed, 14 Mar 2012 10:54:58 +0100,
Nicolas Ferre <[email protected]> a ?crit :

I definitely agree with Nicolas here. Ludovic's set of patches have been around for months and months, and both Atmel and Armadeus need this feature. It would be great if we could make progress on this. Additional improvements can be done later (even though I'm personally not sure we want to depend on quilt).

Best regards,

Thomas

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

Hi Thomas,

So what can I do to move forward?

I have sent my set of patches after having reworked it from Arnout advices.

I will send a new set with the patch discussed here in order to keep the patch order of the series file if there is one.

Can I send it with your ack? Do you want me to test it with some specific configurations? As I said, I tested my set of patches with an armadeus configuration and with an atmel configuration using an archive with many patches for the kernel.

Regards

Ludovic

Le Wed, 14 Mar 2012 11:42:23 +0100,
Ludovic Desroches <[email protected]> a ?crit :

I'll give it a test tonight and report with an Ack if everything goes well.

Thanks,

Thomas

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

Thank you Thomas.

Please find the previous set of patches plus the one about series file.

Regards

Ludovic

From: Ludovic Desroches <[email protected]>

with an armadeus_apf9328_defconfig build

---
 support/scripts/apply-patches.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

index 1aef47e..b145b46 100755
@@ -19,6 +19,13 @@ if [ ! -d "${patchdir}" ] ; then
     echo "Aborting.  '${patchdir}' is not a directory."
     exit 1
 fi
+
+# Check for rejects because if there are some, even if patches are well
+# applied, at the end it will complain about rejects into targetdir.
+if [ "`find ${targetdir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then
+	rm -f "${targetdir}/*.rej" 2> /dev/null
+	echo "Remaining reject files in ${targetdir} have been removed"
+fi
     
 for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do 
     apply="patch -g0 -p1 -E -d"

1.7.5.4

Le Wed, 14 Mar 2012 16:33:52 +0100,
ludovic.desroches at atmel.com a ?crit :

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

ludovic> From: Ludovic Desroches <[email protected]> ludovic> Signed-off-by: Ludovic Desroches <[email protected]> ludovic> Tested-by: Ludovic Desroches <[email protected]> ludovic> with an armadeus_apf9328_defconfig build
ludovic> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> ludovic> ---

 ludovic>  support/scripts/apply-patches.sh |    7 +++++++

ludovic> 1 files changed, 7 insertions(+), 0 deletions(-)

ludovic> diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh ludovic> index 1aef47e..b145b46 100755
ludovic> --- a/support/scripts/apply-patches.sh
ludovic> +++ b/support/scripts/apply-patches.sh
ludovic> @@ -19,6 +19,13 @@ if [ ! -d "${patchdir}" ] ; then
ludovic> echo "Aborting. '${patchdir}' is not a directory."
ludovic> exit 1
ludovic> fi
ludovic> +
ludovic> +# Check for rejects because if there are some, even if patches are well ludovic> +# applied, at the end it will complain about rejects into targetdir. ludovic> +if [ "find ${targetdir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print" ] ; then
ludovic> + rm -f "${targetdir}/*.rej" 2> /dev/null

This is wrong, as the *.rej files might be in subdirs, so I changed this to just do the find ... | xargs rm -f without notifying the user, similar to how we do it for *.orig files. I also adjusted the comment and commit message to match.

Committed with those changes, thanks.

Bye, Peter Korsgaard

Ok, thanks for this improvement.

Regards

Ludovic

From: Ludovic Desroches <[email protected]>

targetdir is not the output/target directory as it can suggest.

with an armadeus_apf9328_defconfig build

---
 support/scripts/apply-patches.sh |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

index b145b46..7cd8abe 100755
@@ -6,13 +6,13 @@
 # (c) 2002 Erik Andersen <[email protected]>
 
 # Set directories from arguments, or use defaults.
-targetdir=${1-.}
+builddir=${1-.}
 patchdir=${2-../kernel-patches}
 shift 2
 patchpattern=${@-*}
 
-if [ ! -d "${targetdir}" ] ; then
-    echo "Aborting.  '${targetdir}' is not a directory."
+if [ ! -d "${builddir}" ] ; then
+    echo "Aborting.  '${builddir}' is not a directory."
     exit 1
 fi
 if [ ! -d "${patchdir}" ] ; then

@@ -21,10 +21,10 @@ if [ ! -d "${patchdir}" ] ; then
 fi
 
 # Check for rejects because if there are some, even if patches are well
-# applied, at the end it will complain about rejects into targetdir.
-if [ "`find ${targetdir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then
-	rm -f "${targetdir}/*.rej" 2> /dev/null
-	echo "Remaining reject files in ${targetdir} have been removed"
+# applied, at the end it will complain about rejects into builddir.
+if [ "`find ${builddir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then
+	rm -f "${buildir}/*.rej" 2> /dev/null
+	echo "Remaining reject files in ${buildir} have been removed"
 fi
     
 for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do 

@@ -55,8 +55,8 @@ for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do
     esac fi
     echo ""
     echo "Applying ${i} using ${type}: " 
-	echo ${i} | cat >> ${targetdir}/.applied_patches_list
-    ${uncomp} ${patchdir}/${i} ${uncomp_parm} | ${apply} ${targetdir} 
+	echo ${i} | cat >> ${builddir}/.applied_patches_list
+    ${uncomp} ${patchdir}/${i} ${uncomp_parm} | ${apply} ${builddir}
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix $i!"
 	exit 1

@@ -64,10 +64,10 @@ for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do
 done
 
 # Check for rejects...
-if [ "`find $targetdir/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then
+if [ "`find $builddir/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then
     echo "Aborting.  Reject files found."
     exit 1
 fi
 
 # Remove backup files
-find $targetdir/ '(' -name '*.orig' -o -name '.*.orig' ')' -exec rm -f {} \;
+find $builddir/ '(' -name '*.orig' -o -name '.*.orig' ')' -exec rm -f {} \;

1.7.5.4

Le Wed, 14 Mar 2012 16:33:53 +0100,
ludovic.desroches at atmel.com a ?crit :

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

ludovic> From: Ludovic Desroches <[email protected]> ludovic> targetdir is not the output/target directory as it can suggest.

Committed after manual fixup (because of the changes I had to do to patch 1) - Thanks.

Bye, Peter Korsgaard

From: Ludovic Desroches <[email protected]>

Add quoting and remove redundant command.

with an armadeus_apf9328_defconfig build

---
 support/scripts/apply-patches.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 7cd8abe..8c38007 100755
@@ -55,8 +55,8 @@ for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do
     esac fi
     echo ""
     echo "Applying ${i} using ${type}: " 
-	echo ${i} | cat >> ${builddir}/.applied_patches_list
-    ${uncomp} ${patchdir}/${i} ${uncomp_parm} | ${apply} ${builddir}
+	echo ${i} >> ${builddir}/.applied_patches_list
+    ${uncomp} "${patchdir}/${i}" ${uncomp_parm} | ${apply} "${builddir}"
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix $i!"
 	exit 1

1.7.5.4

Le Wed, 14 Mar 2012 16:33:54 +0100,
ludovic.desroches at atmel.com a ?crit :

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

ludovic> From: Ludovic Desroches <[email protected]> ludovic> Add quoting and remove redundant command.

Committed, thanks.

Bye, Peter Korsgaard

From: Ludovic Desroches <[email protected]>

When a directory is found in patchdir, it is skipped.

with an armadeus_apf9328_defconfig build

---
 support/scripts/apply-patches.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

index 8c38007..25cf8ce 100755
@@ -31,10 +31,7 @@ for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do
     apply="patch -g0 -p1 -E -d"
     uncomp_parm=""
     if [ -d "${patchdir}/$i" ] ; then
-	type="directory overlay"
-	uncomp="tar cf - --exclude=.svn --no-anchored -C"
-	uncomp_parm="."
-	apply="tar xvf - -C"
+	echo "${patchdir}/$i skipped"
     else case "$i" in
 	*.gz)
 	type="gzip"; uncomp="gunzip -dc"; ;; 

1.7.5.4

Le Wed, 14 Mar 2012 16:33:55 +0100,
ludovic.desroches at atmel.com a ?crit :

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

ludovic> From: Ludovic Desroches <[email protected]> ludovic> When a directory is found in patchdir, it is skipped.

Committed, thanks.

Bye, Peter Korsgaard

From: Ludovic Desroches <[email protected]>

The way archives were managed was incorrect because the uncompressed archives were sent directly to the patch command. It means that alphabetical patch order was not respected.

with an armadeus_apf9328_defconfig build

---
 support/scripts/apply-patches.sh |   50 +++++++++++++++++++++++++-------------
 1 files changed, 33 insertions(+), 17 deletions(-)

index 25cf8ce..51e6c48 100755
@@ -26,13 +26,11 @@ if [ "`find ${builddir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; th
 	rm -f "${buildir}/*.rej" 2> /dev/null
 	echo "Remaining reject files in ${buildir} have been removed"
 fi
-    
-for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do 
-    apply="patch -g0 -p1 -E -d"
-    uncomp_parm=""
-    if [ -d "${patchdir}/$i" ] ; then
-	echo "${patchdir}/$i skipped"
-    else case "$i" in
+
+function apply_patch {
+    path=$1
+    patch=$2
+    case "$patch" in
 	*.gz)
 	type="gzip"; uncomp="gunzip -dc"; ;; 
 	*.bz)

@@ -43,22 +41,40 @@ for i in `cd ${patchdir}; ls -d ${patchpattern} 2> /dev/null` ; do
 	type="zip"; uncomp="unzip -d"; ;; 
 	*.Z)
 	type="compress"; uncomp="uncompress -c"; ;; 
-	*.tgz)
-	type="tar gzip"; uncomp="gunzip -dc"; apply="tar xvf - -C"; ;; 
-	*.tar)
-	type="tar"; uncomp="cat"; apply="tar xvf - -C"; ;; 
 	*)
 	type="plaintext"; uncomp="cat"; ;; 
-    esac fi
+    esac
     echo ""
-    echo "Applying ${i} using ${type}: " 
-	echo ${i} >> ${builddir}/.applied_patches_list
-    ${uncomp} "${patchdir}/${i}" ${uncomp_parm} | ${apply} "${builddir}"
+    echo "Applying $patch using ${type}: "
+	echo $patch >> ${builddir}/.applied_patches_list
+    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}"
     if [ $? != 0 ] ; then
-        echo "Patch failed!  Please fix $i!"
+        echo "Patch failed!  Please fix ${patch}!"
 	exit 1
     fi
-done
+}
+
+function scan_patchdir {
+    path=$1
+    shift 1
+    patches=${@-*}
+
+    for i in `cd $path; ls -d $patches 2> /dev/null` ; do
+        if [ -d "${path}/$i" ] ; then
+            echo "${path}/$i skipped"
+        elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
+            unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
+            rm -rf "$unpackedarchivedir" 2> /dev/null
+            mkdir "$unpackedarchivedir"
+            tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
+            scan_patchdir "$unpackedarchivedir"
+        else
+            apply_patch "$path" "$i" || exit 1
+        fi
+    done
+}
+
+scan_patchdir $patchdir $patchpattern
 
 # Check for rejects...
 if [ "`find $builddir/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then

1.7.5.4

Le Wed, 14 Mar 2012 16:33:56 +0100,
ludovic.desroches at atmel.com a ?crit :

I was initially a bit surprised by the usage of --strip-components=1, which means that the tarball must contain one directory, and this directory should contain all the patches. I kind of excepted the patches to be at the root of the tarball, but I don't feel very strongly about this, so:

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

A comment is really missing here to explain why I used it. In fact, it's quite old and this set of patches has changed a lot from the first version. I can't remember why I used it and if it is still needed.

I will keep you inform if I remember in which case I need it.

Regards

Ludovic

ludovic> From: Ludovic Desroches <[email protected]>

ludovic> The way archives were managed was incorrect because the ludovic> uncompressed archives were sent directly to the patch ludovic> command. It means that alphabetical patch order was not ludovic> respected.

Committed after some manual fixup, thanks.

Bye, Peter Korsgaard

From: Ludovic Desroches <[email protected]>

If a series file is present use it to determine the proper order to apply patches instead of using ls sorting order.

add a series file with a wrong patch order into an archive containing several patches whose correct order is the alphabetical one

---
 support/scripts/apply-patches.sh |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

index 51e6c48..1229213 100755
@@ -59,19 +59,27 @@ function scan_patchdir {
     shift 1
     patches=${@-*}
 
-    for i in `cd $path; ls -d $patches 2> /dev/null` ; do
-        if [ -d "${path}/$i" ] ; then
-            echo "${path}/$i skipped"
-        elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
-            unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
-            rm -rf "$unpackedarchivedir" 2> /dev/null
-            mkdir "$unpackedarchivedir"
-            tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
-            scan_patchdir "$unpackedarchivedir"
-        else
+    # If there is a series file, use it instead of using ls sort order
+    # 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
-        fi
-    done
+        done
+    else
+        for i in `cd $path; ls -d $patches 2> /dev/null` ; do
+            if [ -d "${path}/$i" ] ; then
+                echo "${path}/$i skipped"
+            elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
+                unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
+                rm -rf "$unpackedarchivedir" 2> /dev/null
+                mkdir "$unpackedarchivedir"
+                tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
+                scan_patchdir "$unpackedarchivedir"
+            else
+                apply_patch "$path" "$i" || exit 1
+            fi
+        done
+    fi
 }
 
 scan_patchdir $patchdir $patchpattern

1.7.5.4

Le Wed, 14 Mar 2012 16:33:57 +0100,
ludovic.desroches at atmel.com a ?crit :

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

ludovic> From: Ludovic Desroches <[email protected]>

ludovic> If a series file is present use it to determine the proper ludovic> order to apply patches instead of using ls sorting order.

Committed, thanks.

Bye, Peter Korsgaard

Thanks to take this set of patches.

Regards

Ludovic

Ludovic> Thanks to take this set of patches.

I'm afraid I'll have to revert patch 5+6 unless we can fix it up though, as the tarball change broke the build for (atleast) liblockfile:

http://buildroot.humanoidz.org/results/cf2f653df3e083b75d3f8d0f4020a68100588d2d/build-end.log

The patch tarball contains:

.
??? debian

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

I guess it worked more by accident than anything else before, but I think we should atleast ensure we only apply files matching *.{patch,diff} inside the tarballs.

I don't have a problem with having to add a specific post-patch hook for liblockfile similar to E.G. argus, but we need to get it to work.

Care to have a look?

Bye, Peter Korsgaard

I was afraid by this kind of tarball. Nightmare becomes real!

I will have a look o this issue

Regards

Ludovic

Hi Peter,

Please find a fix for the issue you have encountered.

At the beginning, recursivity was present into this set of patches but was removed since I have no real case needing it. I was thinking at this kind of tarball. So I put it again and do the corrections necessary for this feature:
- Only .patch* and .diff* files are considered as valid patches. The wildcard at the end is for case such as .patch.arm for example.
- Remove strip-components option which is no more necessary since we no more skip directories.

I have tested it with liblockfile and by building a rootfs for my target with a tarball containing patches for the kernel.

Regards

Ludovic

Hello,

Le Wed, 21 Mar 2012 14:26:26 +0100,
ludovic.desroches at atmel.com a ?crit :

Do we really want to support this use case within apply-patches.sh directly? Packages such as argus, cvs, input-tools, mii-diag, setserial, sysklogd, sysvinit and thttpd all handle this by adding a special xxx_POST_PATCH_HOOK that applies the patches in debian/patches/. In other words:

* The package infrastructure applies the Debian patch, which creates the debian/patches/ directory

* The package, through a post-patch hook, is responsible for applying the patches in debian/patches/.

I think liblockfile behaviour was a bug, as it shouldn't have relied on the fact that apply-patches.sh was applying patches recursively inside a tarball.

It would, IMO, make more sense to keep that special behaviour within package recipes (and therefore make liblockfile more similar to argus and others) rather than complicating further the apply-patches.sh script.

Best regards,

Thomas

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

Hi,

Thomas> Do we really want to support this use case within apply-patches.sh Thomas> directly? Packages such as argus, cvs, input-tools, mii-diag, Thomas> setserial, sysklogd, sysvinit and thttpd all handle this by adding a Thomas> special xxx_POST_PATCH_HOOK that applies the patches in Thomas> debian/patches/. In other words:

Thomas> * The package infrastructure applies the Debian patch, which creates
Thomas> the debian/patches/ directory

The difference is that the Debian diff with quilt format is no longer distributed as a (compressed) diff, but a tarball extra files, so the current <pkg>_PATCH handling doesn't really work for that.

But like I mentioned earlier, I don't have a problem with us dropping support for this special case as long as we find a solution for packages like liblockfile.

For now I applied the patch so liblockfile builds again.

Bye, Peter Korsgaard

From: Ludovic Desroches <[email protected]>

Recursivity is needed with some tarballs containing debian patches:
.
debian
changelog
control
patches
02-COPYRIGHT.patch
[...]

Since we can find some files which are not patches in those directories, only consider .patch* and .diff* files as valid patches.
Due to recursivity, strip-components option is no more necessary so it has been removed.

---
 support/scripts/apply-patches.sh |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

index 968e2a4..e4b98bc 100755
@@ -40,8 +40,14 @@ function apply_patch {
 	type="zip"; uncomp="unzip -d"; ;; 
 	*.Z)
 	type="compress"; uncomp="uncompress -c"; ;; 
+	*.diff*)
+	type="diff"; uncomp="cat"; ;;
+	*.patch*)
+	type="patch"; uncomp="cat"; ;;
 	*)
-	type="plaintext"; uncomp="cat"; ;; 
+	echo "Unsupported format file for ${patch}, skip it";
+	return 0;
+	;;
     esac
     echo ""
     echo "Applying $patch using ${type}: "

@@ -67,12 +73,12 @@ function scan_patchdir {
     else
         for i in `cd $path; ls -d $patches 2> /dev/null` ; do
             if [ -d "${path}/$i" ] ; then
-                echo "${path}/$i skipped"
+                scan_patchdir "${path}/$i"
             elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then
                 unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked"
                 rm -rf "$unpackedarchivedir" 2> /dev/null
                 mkdir "$unpackedarchivedir"
-                tar -C "$unpackedarchivedir" --strip-components=1 -xaf "${path}/$i"
+                tar -C "$unpackedarchivedir" -xaf "${path}/$i"
                 scan_patchdir "$unpackedarchivedir"
             else
                 apply_patch "$path" "$i" || exit 1

1.7.5.4

ludovic> From: Ludovic Desroches <[email protected]> ludovic> Recursivity is needed with some tarballs containing debian patches:
ludovic> .
ludovic> debian

 ludovic>     changelog
 ludovic>     control
 ludovic>     patches

ludovic> 02-COPYRIGHT.patch
ludovic> [...]

Committed, thanks!

Bye, Peter Korsgaard

Hello Ludovic,

Le Wed, 21 Mar 2012 09:23:35 +0100,
Ludovic Desroches <[email protected]> a ?crit :

Thank you for your persistence about these patches, and sorry for the huge time that has been needed to merge them. We would definitely appreciate seeing further contributions from you.

Best regards,

Thomas

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

Hello Ludovic,

Le Wed, 14 Mar 2012 16:33:51 +0100,
ludovic.desroches at atmel.com a ?crit :

I did test your patches and found them working as expected. As you have probably seen, I have sent a mail to Peter to ask him to pull these patches quickly. I hope this will happen soon.

Would it be possible for you to write a little help text/documentation in a comment at the beginning of the apply-patches.sh script? The semantic of this script has become quite complicated, and some documentation has become necessary I think. It would also be good to update the help text of the BR2_LINUX_KERNEL_PATCH option? Maybe we also need to change the U-Boot BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR so that it is capable of using a tarball of patches and not only a directory, and ditto for BR2_TARGET_BAREBOX_CUSTOM_PATCH_DIR and BR2_TARGET_AT91BOOTSTRAP_CUSTOM_PATCH_DIR.

Of course, all of those suggestions can be implemented later, and they are not an additional condition to the merge of the existing 6 patches.

Thanks for your work and persistence,

Thomas

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

It's a good news. Thanks for testing it.

Ok I will send a new patch for this.

No problem, thanks for your help and thanks to Arnout for the first reviews and acks.

Regards

Ludovic

https://bugs.busybox.net/show_bug.cgi?id=4796

Christoph Schulz <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

   Attachment #4052|0                           |1

is obsolete| |

--- Comment #1 from Christoph Schulz <[email protected]> 2012-03-15 10:46:09 UTC --- Created attachment 4142
--> https://bugs.busybox.net/attachment.cgi?id=4142
prevents reordering patches (improved)

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

https://bugs.busybox.net/show_bug.cgi?id=4796

--- Comment #2 from Christoph Schulz <[email protected]> 2012-03-15 10:46:35 UTC --- According to some comments on the mailing list, here is an improved version of the patch. Expanded patterns are sorted but arguments to apply-patches.sh are not generally reordered. That means that

apply-patches a z f* c

given the patches "a c f1 f2 z" in the patch directory causes the patches to be processed as follows:

a z f1 f2 c

This should make apply-patches.sh conforming to both requirements (sorting of patches specified by a pattern and no reordering of arguments).

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

https://bugs.busybox.net/show_bug.cgi?id=4796

Samuel Martin <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

             Status|NEW                         |RESOLVED
         Resolution|                            |WONTFIX

--- Comment #3 from Samuel Martin <[email protected]> 2013-05-26 11:57:34 UTC --- What are the packages affected by this bug?

Note that using the latest patch policy (see the new patch naming convention):
http://buildroot.org/downloads/manual/manual.html#patch-policy

After fixing the culprit patch filenames, this bug should not occurs anymore.

So, it's better to fix the packages themselves.

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

https://bugs.busybox.net/show_bug.cgi?id=4796

--- Comment #4 from Christoph Schulz <[email protected]> 2013-05-26 12:30:04 UTC --- (In reply to comment #3)

We have extended the uClibc buildroot by many additional packages which sometimes contain patches in debian/patches/. These patches are not ordered by name, rather there is a file debian/patches/series which contains the patches to be applied line by line. These are _not_ patches that lie in buildroot/package/<name>/.

You should not assume that users of the uClibc buildroot only use packages that are contained therein. This is a mistake. Typically, the buildroot is extended by other self-made or customized packages in order to provide additional, target-specific functionality. In our case, we e.g. provide packages for the various ISDN-related tools coming with the isdnutils package. We take the isdnutils sources from Debian which distributes them in two archives: the original sources and separate patches, structured as described above.

Best regards,

Christoph

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

https://bugs.busybox.net/show_bug.cgi?id=4796

--- Comment #5 from Thomas Petazzoni <[email protected]> 2013-05-26 12:39:15 UTC --- If the path given to the apply-patches.sh script is a directory that contains a "series" file, then this series file is used to know the order in which patches should be applied.

Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.