Code review comment for lp:~qzhang/lava-dispatcher/gen-test-tarball

On Tue, Mar 01, 2011, Spring Zhang wrote:
> > > + hwpack_name = hwpack.rsplit("/", 1)[1]
> > > + rootfs_name = rootfs.rsplit("/", 1)[1]
> > > + qemuimg = "%s_sd.img" % boardtype
> > > + lmc_cmd = "linaro-media-create --rootfs ext3 --image_file %s" \
> > > + " --dev %s --binary %s --hwpack %s" % (qemuimg, boardtype,
> > > + rootfs_name, hwpack_name)
> >
> > Hmm why do you actually need to pass just the filename to l-m-c? Just
> > pass hwpack_name and rootfs_name directly?
> >
> Initial hwpack or rootfs is an URL, it will get by the previous function
> to the local, and here l-m-c uses the hwpack/rootfs name.

 Hmm ok, then I think the vars could be named _url (hwpack_url,
 rootfs_url) and we could have rootfs_file vars when you've downloaded
 them locally.

 I am always scared to rely on the cwd or to use "cd"

 (Also, I think you were running "cd" in a subshell, so it probably
 didn't affect the current working directory)

> good, and we can include it in some config file you mentioned above
> > do you actually want to copy, or would moving the tarball be ok?

 Put the tarball directly in the right place? Unless it's some
 permanent mirror, in which case just refer to its full pathname

> > > +if __name__ == "__main__":
> > > + cmd = cmd_deploy_linaro_image("bbg01")
> > > + cmd.generate_tarballs("mx51evk", "http://pkgserver/original/hwpack_linaro-imx51_20110210-0_armel_unsupported.tar.gz", "http://pkgserver/original/linaro-natty-headless-tar-20110216-0.tar.gz")
> >
> > lots of hardcoding here
> It's for a test.

 Maybe this should be parsed from sys.argv instead so that others can
 use it; or just write a local test script, but don't commit it

   Thanks!
--
Loïc Minier