diff mbox

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

Message ID 20130915141314.GA18166@harvey.netwinder.org
State Accepted
Commit d245fbb41dc148a956df59658cc0dc1262612e09
Headers show

Commit Message

Ralph Siemsen Sept. 15, 2013, 2:13 p.m. UTC 
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

Comments

Peter Korsgaard Sept. 15, 2013, 8:09 p.m. UTC | #1 
>>>>> "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?
Peter Korsgaard Sept. 16, 2013, 7:11 a.m. UTC | #2 
>>>>> "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?
Ralph Siemsen Sept. 16, 2013, 12:36 p.m. UTC | #3 
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
Ralph Siemsen Sept. 16, 2013, 12:46 p.m. UTC | #4 
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
Peter Korsgaard Sept. 16, 2013, 1:42 p.m. UTC | #5 
>>>>> "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.
Thomas Petazzoni Sept. 16, 2013, 3:24 p.m. UTC | #6 
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
Ralph Siemsen Sept. 16, 2013, 7:45 p.m. UTC | #7 
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
Arnout Vandecappelle Sept. 16, 2013, 8:07 p.m. UTC | #8 
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
Peter Korsgaard Sept. 16, 2013, 8:49 p.m. UTC | #9 
>>>>> "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.
Peter Korsgaard Sept. 16, 2013, 8:50 p.m. UTC | #10 
>>>>> "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.
Yann E. MORIN Sept. 16, 2013, 8:51 p.m. UTC | #11 
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.
Peter Korsgaard Sept. 16, 2013, 8:57 p.m. UTC | #12 
>>>>> "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.
diff mbox

Patch

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