Code review comment for lp:~salgado/linaro-image-tools/hwpack-install

On Mon, 2010-09-13 at 14:25 +0000, James Westby wrote:
> On Mon, 13 Sep 2010 13:31:15 -0000, Guilherme Salgado <email address hidden> wrote:
> > > 64 + sudo rm -f $HWPACK_PKGS_SOURCE
> > > 65 + sudo apt-get update -qq
> > >
> > > I don't see why sudo would be needed? This will run as root won't it?
> >
> > Not necessarily. When you run it inside the chroot you might run it as
> > any user, and that's how I've been testing it.
>
> Hmm, are you using schroot or manually changing user in the chroot, or
> something else?

schroot, but the main reason why I think it'd be nice (although
certainly not necessary) to run it as non-root is when you're logged
into the system and want to upgrade a hwpack. I'm not sure that's an
important use case and it'd be trivial to run the script as root in that
case anyway, so I'm happy to change this if you see any advantages in
requiring the script to run as root.

>
> > > 101 +for file in `ls "${HWPACK_DIR}"/sources.list.d.gpg/`; do
> > >
> > > This can use a * expansion as the sources.list.d code does couldn't it?
> >
> > At first I thought I could but after trying it I realized that may not
> > be desirable as it would cause the file variable to point to
> > '/.../sources.list.d.gpg/*' when the directory is empty, and that will
> > cause apt-key to fail. Unless we can assume hwpacks will always include
> > a non-empty sources.list.d.gpg, I think we might want to keep it that
> > way. And maybe even change the other loop for sources.list.d.
>
> Ah, good point. sources.list.d.gpg will be empty with our current
> hwpacks, as I haven't implemented anything to put keys there. In general
> though the specification doesn't state that there must be files there,
> so I think your current code is good.
>
> As for sources.list.d, the specification is equally quiet on that front,
> and Alexander has a use case where it would be empty ("private" archives
> of some sense), so I think that changing that loop would be good.

I've changed that.

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