[OE-core] [PATCH] package_ipk: handle exception for subprocess command

[richard.purdie at linuxfoundation.org]

On Tue, 2019-04-16 at 09:10 +0200, Andrey Zhizhikin wrote:
> On Mon, Apr 15, 2019 at 6:45 PM Richard Purdie
> <richard.purdie at linuxfoundation.org> wrote:
> > On Sun, 2019-04-14 at 16:21 +0200, Andrey Zhizhikin wrote:
> > > Ping.
> > > 
> > > On Thu, Mar 28, 2019 at 10:47 AM Andrey Zhizhikin <
> > > andrey.z at gmail.com
> > > > wrote:
> > > > When opkg-build command fails to execute, subprocess is
> > > > returned
> > > > with
> > > > exception instead of printing to stderr. This causes the error
> > > > logging
> > > > not to be printed out, as the "finally" statement does not
> > > > contain
> > > > any
> > > > bitbake error output.
> > > > 
> > > > One example of this behavior is when the package name contains
> > > > uppercase
> > > > character, which are rejected by opkg-build,
> > > > subprocess.check_output
> > > > would except and no error log would be produced.
> > > > 
> > > > This commit catches the exception subprocess.CalledProcessError
> > > > and
> > > > produces bb.error output visible to the user.
> > > > 
> > > > Signed-off-by: Andrey Zhizhikin <andrey.z at gmail.com>
> > > > ---
> > > >  meta/classes/package_ipk.bbclass | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/meta/classes/package_ipk.bbclass
> > > > b/meta/classes/package_ipk.bbclass
> > > > index d1b317b42b..f181f5b4fd 100644
> > > > --- a/meta/classes/package_ipk.bbclass
> > > > +++ b/meta/classes/package_ipk.bbclass
> > > > @@ -234,6 +234,8 @@ def ipk_write_pkg(pkg, d):
> > > >              ipk_to_sign = "%s/%s_%s_%s.ipk" % (pkgoutdir,
> > > > pkgname,
> > > > ipkver, d.getVar('PACKAGE_ARCH'))
> > > >              sign_ipk(d, ipk_to_sign)
> > > > 
> > > > +    except subprocess.CalledProcessError as exc:
> > > > +        bb.error("OPKG Build failed: %s" % exc.output)
> > > >      finally:
> > > >          cleanupcontrol(root)
> > > >          bb.utils.unlockfile(lf)
> > 
> > My main concern is why isn't the raised exception being caught and
> > causing its own error...
> 
> The raised exception is actually caught by a finally: statement
> below, and the build gracefully terminates. The problem is that
> finally: block does not contain any valuable output to inform user
> what actually happened.

This isn't how python works. The exception should be "re-raised after
the finally clause has been executed" to quote the python manual:
https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions

> subprocess.check_output() would throw this exception every time the
> command in sub-process is terminated with the error code, and since
> we
> do tell it to dump stderr -> stdout - the error message would be
> contained in the exception output.
> This additional handling of the subprocess.check_output() exception
> would extract the stdout from the failed process here and just shows
> to the user the actual output from command processing, so that he is
> aware what was wrong.
> 
> The case where I personally needed it the most is when the package
> name contained upper and lower case characters, which were rejected
> by
> the opkg-build command and until I introduced the handler - I just
> had
> an erroneous build failure without any additional information on what
> went wrong.
> 
> > This feels like a workaround rather than fixing the underlying
> > problem
> > which I suspect might be in the parallel execution code exception
> > handling.
> The exception from  subprocess.check_output() is actually expected
> and
> perfectly handled, so there is no problem with that. This patch would
> just deliver a bit more information in the output for user to react
> proper.


See my comment above, the finally should not be blocking this exception
from being raised...

Cheers,

Richard