| Message ID | 20130813160017.GA2590@harvey.netwinder.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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 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
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.
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
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}!"