Patchwork [v2,5/5] shutdown-desktop: give entire path in Exec field

login
register
mail settings
Submitter Laurentiu Palcu
Date July 4, 2013, 10:58 a.m.
Message ID <11aefd86a570528092c6f4f5de1371204b1f30ec.1372933905.git.laurentiu.palcu@intel.com>
Download mbox | patch
Permalink /patch/52973/
State New
Headers show

Comments

Laurentiu Palcu - July 4, 2013, 10:58 a.m.
A normal user does not have /sbin in its PATH, by default, so having the
entire path here allows the correct execution when run as regular user.

[YOCTO #4345]

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
---
 .../shutdown-desktop/shutdown-desktop.bb           |    2 ++
 1 file changed, 2 insertions(+)
Ross Burton - July 4, 2013, 2:58 p.m.
On 4 July 2013 11:58, Laurentiu Palcu <laurentiu.palcu@intel.com> wrote:
> +    sed -i $D${datadir}/applications/shutdown.desktop -e 's#^Exec=\(.*\)#Exec=${base_sbindir}/\1#'

Doing this in postinst is pretty nasty, instead change the desktop
file to contain something like @SBIN@ and run it through sed in
do_install.

Ross
Laurentiu Palcu - July 4, 2013, 3:39 p.m.
On 07/04/2013 05:58 PM, Burton, Ross wrote:
> On 4 July 2013 11:58, Laurentiu Palcu <laurentiu.palcu@intel.com> wrote:
>> +    sed -i $D${datadir}/applications/shutdown.desktop -e 's#^Exec=\(.*\)#Exec=${base_sbindir}/\1#'
> 
> Doing this in postinst is pretty nasty, 
Nasty, in what way? How is this line different from the previous line in
the same postinstall? That one is also using sed to change 'halt' to
'reboot' for qemuarm. Is it the sed you're worried about? This
postinstall is executed on host anyway, at do_rootfs().

> instead change the desktop
> file to contain something like @SBIN@ and run it through sed in
> do_install.
Even though the change you're proposing is OK, involves changing the
desktop file to add the @SBIN@ pattern and move the sed line to
do_install(). Compared to having one single line added in the
postinstall... I would choose the latter, unless you elaborate on what
do you actually mean by "nasty".

Laurentiu
> 
> Ross
>
Ross Burton - July 4, 2013, 3:54 p.m.
On 4 July 2013 16:39, Laurentiu Palcu <laurentiu.palcu@intel.com> wrote:
>> Doing this in postinst is pretty nasty,
> Nasty, in what way? How is this line different from the previous line in
> the same postinstall? That one is also using sed to change 'halt' to
> 'reboot' for qemuarm. Is it the sed you're worried about? This
> postinstall is executed on host anyway, at do_rootfs().

The existing sed to manipulate the exec line is arguably a better
solution to making this package machine-specific and special-casing
just qemuarm.  (I'm not sure why it special-cases qemuarm, and
751212d5effdceab91d95705e647cf07e6820940 where it was introduced
doesn't clarify matters, but we'll ignore that for now).  Arguably,
but I'm not sure I agree.  Anyway, you've a path that is known at
build time so we can fix it at build time.  Instead of using install,
you can use sed.  Just because we *can* run postinst scripts when
generating the rootfs doesn't mean we *should*, do_install is for
installing the files we're going to package, and postinst is for
runtime changes that can't happen at any other time.

>> instead change the desktop
>> file to contain something like @SBIN@ and run it through sed in
>> do_install.
> Even though the change you're proposing is OK, involves changing the
> desktop file to add the @SBIN@ pattern and move the sed line to
> do_install(). Compared to having one single line added in the
> postinstall... I would choose the latter, unless you elaborate on what
> do you actually mean by "nasty".

We're not being invoiced based on the number of patch hunks, and
changing /sbin to @SBIN@ and install to sed isn't exactly a massive
time sink.

Ross
Tomas Frydrych - July 4, 2013, 3:56 p.m.
On 04/07/13 16:39, Laurentiu Palcu wrote:
> On 07/04/2013 05:58 PM, Burton, Ross wrote:
>> On 4 July 2013 11:58, Laurentiu Palcu <laurentiu.palcu@intel.com> wrote:
> Even though the change you're proposing is OK, involves changing the
> desktop file to add the @SBIN@ pattern and move the sed line to
> do_install(). Compared to having one single line added in the
> postinstall... I would choose the latter, unless you elaborate on what
> do you actually mean by "nasty".

Post install scripts should be avoided whenever possible and used only
for things that cannot be done at build or install time. This clearly
can be done at install time, so it should.

Tomas
Laurentiu Palcu - July 4, 2013, 6:09 p.m.
On Thu, Jul 04, 2013 at 04:56:45PM +0100, Tomas Frydrych wrote:
> On 04/07/13 16:39, Laurentiu Palcu wrote:
> > On 07/04/2013 05:58 PM, Burton, Ross wrote:
> >> On 4 July 2013 11:58, Laurentiu Palcu <laurentiu.palcu@intel.com> wrote:
> > Even though the change you're proposing is OK, involves changing the
> > desktop file to add the @SBIN@ pattern and move the sed line to
> > do_install(). Compared to having one single line added in the
> > postinstall... I would choose the latter, unless you elaborate on what
> > do you actually mean by "nasty".
> 
> Post install scripts should be avoided whenever possible and used only
> for things that cannot be done at build or install time. This clearly
> can be done at install time, so it should.
I agree. However, in this particular case I'm just being consistent with
what was already there. I would have personally moved the entire
postinstall to do_install, but the previous sed cannot be moved. So,
since we're stuck with that sed there, this one is just a harmless
addition. This is what I mean!

Laurentiu

> 
> Tomas
> 
> -- 
> http://sleepfive.com
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Patch

diff --git a/meta/recipes-sato/shutdown-desktop/shutdown-desktop.bb b/meta/recipes-sato/shutdown-desktop/shutdown-desktop.bb
index c5096c1..9e283e4 100644
--- a/meta/recipes-sato/shutdown-desktop/shutdown-desktop.bb
+++ b/meta/recipes-sato/shutdown-desktop/shutdown-desktop.bb
@@ -18,6 +18,8 @@  pkg_postinst_${PN} () {
     grep -q qemuarm $D${sysconfdir}/hostname && \
         sed -i $D${datadir}/applications/shutdown.desktop -e 's/^Exec=halt/Exec=reboot/' \
         || true
+
+    sed -i $D${datadir}/applications/shutdown.desktop -e 's#^Exec=\(.*\)#Exec=${base_sbindir}/\1#'
 }
 
 inherit allarch