Merge lp:~martin-ohlson/linaro-image-tools/apt-get-notification-bug-651906 into lp:linaro-image-tools/11.11

[Martin Ohlsson]

Notify user that "apt-get" will be run in target roofs.

Bug #651906.

This adds a notification to the user that "apt-get" will be run in target
rootfs when installing hwpack.

I haven't been able to run the script for test/verification. I'm waiting on the IT department to install Ubuntu on my company computer. This branch is proposed for merging because it contains a very trivial fix.

Regards,
Martin

[Guilherme Salgado (salgado)]

Hi Martin,

Thanks for this fix! I have just a few comments/suggestions below.

 review approve

On Thu, 2010-11-18 at 08:48 +0000, Martin Ohlsson wrote:
[...]
> This adds a notification to the user that "apt-get" will be run in target
> rootfs when installing hwpack.
>
> I haven't been able to run the script for test/verification. I'm waiting on the IT department to install Ubuntu on my company computer. This branch is proposed for merging because it contains a very trivial fix.

It is trivial indeed, but to be double sure it doesn't break anything I
did create an image using your branch and it worked just fine.

> === modified file 'linaro-media-create'
> --- linaro-media-create 2010-11-09 20:48:16 +0000
> +++ linaro-media-create 2010-11-18 08:48:13 +0000
> @@ -360,6 +360,8 @@
> sudo umount -v "$chroot/proc"
> }
>
> + echo ""
> + echo "Installing (apt-get) hwpack $HWPACK_FILE in target roofs."

The package filename should make it clear it's a hwpack so you can
probably drop the 'hwpack' from the sentence above. Oh, and there's a
typo there: roofs

> sudo LC_ALL=C chroot "$chroot" linaro-hwpack-install /"$(basename "$HWPACK_FILE")"
>

I think it'd be nice to also print something when the installation in
the rootfs has finished, maybe using a dashed line (for it to stand out)
or something like that. What do you think?

If you like the idea you may also want to use a dashed line (instead of
a blank one) in the previous message.

> # Revert some changes we did to the rootfs as we don't want them in the
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

review: Approve

[Martin Ohlsson (martin-ohlson)]

Thanks for the comments Guilherme,

I will fix the following things:

* Remove the "hwpack" word.
* Take care of the typo (good that you spotted that).
* Add a dashed line before and after the rootfs installation.

Regards,
Martin

On 18 November 2010 13:12, Guilherme Salgado <email address hidden> wrote:

> Review: Approve
> Hi Martin,
>
> Thanks for this fix! I have just a few comments/suggestions below.
>
> review approve
>
> On Thu, 2010-11-18 at 08:48 +0000, Martin Ohlsson wrote:
> [...]
> > This adds a notification to the user that "apt-get" will be run in target
> > rootfs when installing hwpack.
> >
> > I haven't been able to run the script for test/verification. I'm waiting
> on the IT department to install Ubuntu on my company computer. This branch
> is proposed for merging because it contains a very trivial fix.
>
> It is trivial indeed, but to be double sure it doesn't break anything I
> did create an image using your branch and it worked just fine.
>
>
> > === modified file 'linaro-media-create'
> > --- linaro-media-create 2010-11-09 20:48:16 +0000
> > +++ linaro-media-create 2010-11-18 08:48:13 +0000
> > @@ -360,6 +360,8 @@
> > sudo umount -v "$chroot/proc"
> > }
> >
> > + echo ""
> > + echo "Installing (apt-get) hwpack $HWPACK_FILE in target roofs."
>
> The package filename should make it clear it's a hwpack so you can
> probably drop the 'hwpack' from the sentence above. Oh, and there's a
> typo there: roofs
>
> > sudo LC_ALL=C chroot "$chroot" linaro-hwpack-install /"$(basename
> "$HWPACK_FILE")"
> >
>
> I think it'd be nice to also print something when the installation in
> the rootfs has finished, maybe using a dashed line (for it to stand out)
> or something like that. What do you think?
>
> If you like the idea you may also want to use a dashed line (instead of
> a blank one) in the previous message.
>
> > # Revert some changes we did to the rootfs as we don't want them in
> the
> >
>
> --
> Guilherme Salgado <https://launchpad.net/~salgado<https://launchpad.net/%7Esalgado>
> >
>
>
> https://code.launchpad.net/~martin-ohlson/linaro-image-tools/apt-get-notification-bug-651906/+merge/41138<https://code.launchpad.net/%7Emartin-ohlson/linaro-image-tools/apt-get-notification-bug-651906/+merge/41138>
> You are the owner of
> lp:~martin-ohlson/linaro-image-tools/apt-get-notification-bug-651906.
>

[Martin Ohlsson (martin-ohlson)]

Thanks for the comments Guilherme,

I will fix the following things:

* Remove the "hwpack" word.
* Take care of the typo (good that you spotted that).
* Add a dashed line before and after the rootfs installation.

Regards,
Martin

[lp:~martin-ohlson/linaro-image-tools/apt-get-notification-bug-651906]

168. By Martin Ohlsson on 2010-11-18

Emphasize the hwpack installation in target rootfs.

Bug #651906.

This commit adds an emphasized section for the hwpack installation
in target rootfs.

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-11-12 17:21:46 +0000
+++ linaro-media-create	2010-11-19 06:20:08 +0000
@@ -360,7 +360,13 @@
     sudo umount -v "$chroot/proc"
   }
 
+  echo ""
+  echo "---------------------------------------------------"
+  echo "Installing (apt-get) $HWPACK_FILE in target rootfs."
+  echo ""
   sudo LC_ALL=C chroot "$chroot" linaro-hwpack-install /"$(basename "$HWPACK_FILE")"
+  echo "---------------------------------------------------"
+  echo ""
 
   # Revert some changes we did to the rootfs as we don't want them in the
   # image.