Patchwork oeqa/controllers/beaglebonetarget.py: add conditions for files copied to /boot

login
register
mail settings
Submitter Corneliu Stoicescu
Date May 26, 2014, 3:26 p.m.
Message ID <1401117992-30582-1-git-send-email-corneliux.stoicescu@intel.com>
Download mbox | patch
Permalink /patch/72705/
State New
Headers show

Comments

Corneliu Stoicescu - May 26, 2014, 3:26 p.m.
Sometimes the rootfs archive contains the kernel file (core-image-full-cmdline) or the dtbs files (core-image-sato). Adding verification to add them if they don't allready exist.

Without this, the first condition would fail and the image deploy task would stop.

Signed-off-by: Corneliu Stoicescu <corneliux.stoicescu@intel.com>
---
 meta-yocto-bsp/lib/oeqa/controllers/beaglebonetarget.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Anders Darander - May 27, 2014, 6:46 a.m.
* Corneliu Stoicescu <corneliux.stoicescu@intel.com> [140526 15:23]:

> Sometimes the rootfs archive contains the kernel file
> (core-image-full-cmdline) or the dtbs files (core-image-sato). Adding
> verification to add them if they don't allready exist.

> Without this, the first condition would fail and the image deploy task would stop.

What failure and errors are you seeing?

> -                '[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-kernel /mnt/testrootfs/boot/uImage',
> +                'if [ ! -e /mnt/testrootfs/boot/uImage ]; then cp ~/test-kernel /mnt/testrootfs/boot/uImage; fi',

It might be that I've had no coffee yet, but I can't see what difference
your change is making (apart from being more verbose)?

The original code checks for the existance of
/mnt/testrootfs/boot/uImage, negates the result and copies the
test-kernel if the negated result is true, i.e. if
/mnt/testrootfs/boot/uImage doesn't exist, copy the test-kernel.

> -            self.deploy_cmds.append('[ ! -e /mnt/testrootfs/boot/{0} ] && cp ~/{0} /mnt/testrootfs/boot/'.format(dtbfn))
> +            self.deploy_cmds.append('if [ ! -e /mnt/testrootfs/boot/{0} ]; then cp ~/{0} /mnt/testrootfs/boot/'.format(dtbfn) + '; fi')

See above...

Cheers,
Anders
Corneliu Stoicescu - May 27, 2014, 7:02 a.m.
> -----Original Message-----
> From: Anders Darander [mailto:anders@chargestorm.se]
> Sent: Tuesday, May 27, 2014 9:46 AM
> To: Stoicescu, CorneliuX
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH] oeqa/controllers/beaglebonetarget.py: add
> conditions for files copied to /boot
> 
> * Corneliu Stoicescu <corneliux.stoicescu@intel.com> [140526 15:23]:
> 
> > Sometimes the rootfs archive contains the kernel file
> > (core-image-full-cmdline) or the dtbs files (core-image-sato). Adding
> > verification to add them if they don't allready exist.
> 
> > Without this, the first condition would fail and the image deploy task would
> stop.
> 
> What failure and errors are you seeing?
> 

'[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-kernel/mnt/testrootfs/boot/uImage' returns exit code 1 if the first condition is not met, and thus the deploy task fails. My version would return exit code 0 weather or not the first condition is met. 

We could also modify the way the commands are run to ignore that exit code but that would involve more changes, many of them not needed or expected by other commands and/or target controllers.

> > -                '[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-kernel
> /mnt/testrootfs/boot/uImage',
> > +                'if [ ! -e /mnt/testrootfs/boot/uImage ]; then cp
> > + ~/test-kernel /mnt/testrootfs/boot/uImage; fi',
> 
> It might be that I've had no coffee yet, but I can't see what difference your
> change is making (apart from being more verbose)?
> 
> The original code checks for the existance of /mnt/testrootfs/boot/uImage,
> negates the result and copies the test-kernel if the negated result is true, i.e.
> if /mnt/testrootfs/boot/uImage doesn't exist, copy the test-kernel.
> 
> > -            self.deploy_cmds.append('[ ! -e /mnt/testrootfs/boot/{0} ] && cp
> ~/{0} /mnt/testrootfs/boot/'.format(dtbfn))
> > +            self.deploy_cmds.append('if [ ! -e
> > + /mnt/testrootfs/boot/{0} ]; then cp ~/{0}
> > + /mnt/testrootfs/boot/'.format(dtbfn) + '; fi')
> 
> See above...
> 
> Cheers,
> Anders
> 
> --
> Anders Darander
> ChargeStorm AB / eStorm AB
Corneliu Stoicescu - May 27, 2014, 7:12 a.m.
> -----Original Message-----
> From: Stoicescu, CorneliuX
> Sent: Tuesday, May 27, 2014 10:02 AM
> To: 'Anders Darander'
> Cc: openembedded-core@lists.openembedded.org
> Subject: RE: [OE-core] [PATCH] oeqa/controllers/beaglebonetarget.py: add
> conditions for files copied to /boot
> 
> 
> 
> > -----Original Message-----
> > From: Anders Darander [mailto:anders@chargestorm.se]
> > Sent: Tuesday, May 27, 2014 9:46 AM
> > To: Stoicescu, CorneliuX
> > Cc: openembedded-core@lists.openembedded.org
> > Subject: Re: [OE-core] [PATCH] oeqa/controllers/beaglebonetarget.py:
> > add conditions for files copied to /boot
> >
> > * Corneliu Stoicescu <corneliux.stoicescu@intel.com> [140526 15:23]:
> >
> > > Sometimes the rootfs archive contains the kernel file
> > > (core-image-full-cmdline) or the dtbs files (core-image-sato).
> > > Adding verification to add them if they don't allready exist.
> >

My explanation of the patch was kind of wrong, there was a check already but I interpreted it as more like an "association" of circumstances in my head, but this time not because of lack of coffee but because it was late :D. I can change that if necessary.

> > > Without this, the first condition would fail and the image deploy
> > > task would
> > stop.
> >
> > What failure and errors are you seeing?
> >
> 
> '[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-
> kernel/mnt/testrootfs/boot/uImage' returns exit code 1 if the first condition
> is not met, and thus the deploy task fails. My version would return exit code
> 0 weather or not the first condition is met.
> 
> We could also modify the way the commands are run to ignore that exit
> code but that would involve more changes, many of them not needed or
> expected by other commands and/or target controllers.
> 
> > > -                '[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-kernel
> > /mnt/testrootfs/boot/uImage',
> > > +                'if [ ! -e /mnt/testrootfs/boot/uImage ]; then cp
> > > + ~/test-kernel /mnt/testrootfs/boot/uImage; fi',
> >
> > It might be that I've had no coffee yet, but I can't see what
> > difference your change is making (apart from being more verbose)?
> >
> > The original code checks for the existance of
> > /mnt/testrootfs/boot/uImage, negates the result and copies the test-
> kernel if the negated result is true, i.e.
> > if /mnt/testrootfs/boot/uImage doesn't exist, copy the test-kernel.
> >
> > > -            self.deploy_cmds.append('[ ! -e /mnt/testrootfs/boot/{0} ] && cp
> > ~/{0} /mnt/testrootfs/boot/'.format(dtbfn))
> > > +            self.deploy_cmds.append('if [ ! -e
> > > + /mnt/testrootfs/boot/{0} ]; then cp ~/{0}
> > > + /mnt/testrootfs/boot/'.format(dtbfn) + '; fi')
> >
> > See above...
> >
> > Cheers,
> > Anders
> >
> > --
> > Anders Darander
> > ChargeStorm AB / eStorm AB
Anders Darander - May 27, 2014, 7:15 a.m.
* Stoicescu, CorneliuX <corneliux.stoicescu@intel.com> [140527 09:02]:
> > -----Original Message-----
> > From: Anders Darander [mailto:anders@chargestorm.se]
> > * Corneliu Stoicescu <corneliux.stoicescu@intel.com> [140526 15:23]:

> > > Sometimes the rootfs archive contains the kernel file
> > > (core-image-full-cmdline) or the dtbs files (core-image-sato). Adding
> > > verification to add them if they don't allready exist.

> > > Without this, the first condition would fail and the image deploy task would
> > stop.

> > What failure and errors are you seeing?


> '[ ! -e /mnt/testrootfs/boot/uImage ] && cp
> ~/test-kernel/mnt/testrootfs/boot/uImage' returns exit code 1 if the
> first condition is not met, and thus the deploy task fails. My version
> would return exit code 0 weather or not the first condition is met. 

Oh, that's right. If /mnt/testrootfs/boot/uImage do exist, we'll have an
exit code of 1... -ENOCOFFE like I said!

However, then I think that the commit message ought to be changed.
Reading the message, it sounds as you're adding a check to add uImage if
it doesn't exist. Rather, you're modifying the existing check to
gracefully handle the case where uImage already exists.

Could you update the commit message to better reflect the intention with
the change?

> We could also modify the way the commands are run to ignore that exit
> code but that would involve more changes, many of them not needed or
> expected by other commands and/or target controllers.

No, the above change is fine. We should error out if something goes
wrong, so I'd be against changing the way to run commands.

Cheers,
Anders

Patch

diff --git a/meta-yocto-bsp/lib/oeqa/controllers/beaglebonetarget.py b/meta-yocto-bsp/lib/oeqa/controllers/beaglebonetarget.py
index 53f454b..af56bda 100644
--- a/meta-yocto-bsp/lib/oeqa/controllers/beaglebonetarget.py
+++ b/meta-yocto-bsp/lib/oeqa/controllers/beaglebonetarget.py
@@ -41,12 +41,12 @@  class BeagleBoneTarget(MasterImageHardwareTarget):
                 'mount -L testrootfs /mnt/testrootfs',
                 'rm -rf /mnt/testrootfs/*',
                 'tar xzvf ~/test-rootfs.tar.gz -C /mnt/testrootfs',
-                '[ ! -e /mnt/testrootfs/boot/uImage ] && cp ~/test-kernel /mnt/testrootfs/boot/uImage',
+                'if [ ! -e /mnt/testrootfs/boot/uImage ]; then cp ~/test-kernel /mnt/testrootfs/boot/uImage; fi',
                 ]
 
         for _, dtbfn in self.dtbs:
             # Kernel and dtb files may not be in the image, so copy them if not
-            self.deploy_cmds.append('[ ! -e /mnt/testrootfs/boot/{0} ] && cp ~/{0} /mnt/testrootfs/boot/'.format(dtbfn))
+            self.deploy_cmds.append('if [ ! -e /mnt/testrootfs/boot/{0} ]; then cp ~/{0} /mnt/testrootfs/boot/'.format(dtbfn) + '; fi')
 
         if not self.serialcontrol_cmd:
             bb.fatal("This TEST_TARGET needs a TEST_SERIALCONTROL_CMD defined in local.conf.")