Merge lp://qastaging/~deeptik/linaro-image-tools/fix_bug_Bug816767 into lp://qastaging/linaro-image-tools/11.11

Hi Deepti,

This looks quite good; I've only a few suggestions.

On Thu, 2011-08-04 at 16:38 +0000, Deepti B. Kalakeri wrote:
> === modified file 'linaro-hwpack-replace'
> --- linaro-hwpack-replace 2011-08-03 11:29:43 +0000
> +++ linaro-hwpack-replace 2011-08-04 16:38:27 +0000
> @@ -36,10 +36,12 @@
>
>
> parser = argparse.ArgumentParser()
> -parser.add_argument("-t", "--hwpack_name", dest="hwpack_name",
> - help="Specific hwpack_name to use (default: None)")
> -parser.add_argument("-p", "--deb_pack", dest="deb_pack",
> +parser.add_argument("-t", "--hwpack-name", dest="hwpack_name",
> + help="Specific hwpack-name to use (default: None)")
> +parser.add_argument("-p", "--deb-pack", dest="deb_pack",
> help="Specific debian package to replace (default: None).")
> +parser.add_argument("-r", "--prefix-pkg-remove", dest="prefix_pkg_remove",
> + help="Specific debian package to remove (default: None).")

The user actually specifies the name prefix of the packages that should
be removed and not the actual package, so the help text above is not
correct. It also mentions a default value, which doesn't make much sense
to me as the argument is required, no?

> parser.add_argument("-d", "--debug-output", action="store_true", dest="debug",
> help="Verbose messages are displayed when specified")
>
> -def modify_manifest_info(tempdir, new_debpack_info, deb_pack_found):
> +def modify_manifest_info(tempdir, new_debpack_info, prefix_pkg_remove):
> """ Modify the manifest file to include the new debian information """
>
> debpack_manifest_fname = os.path.join(tempdir, "manifest")
> new_debpack_line = '%s=%s\n' % (new_debpack_info.name, new_debpack_info.version)
>
> for line in fileinput.FileInput(debpack_manifest_fname, inplace=1):
> - if '=' in line:
> - package_name, version = line.split('=')
> - old_debpack = '%s=%s' % (package_name, version)
> - else:
> - package_name = line.rstrip("\n")
> - old_debpack = '%s' % package_name
> -
> - if new_debpack_info.name == package_name:
> - deb_pack_found = 1
> - line = new_debpack_line
> - sys.stdout.write(line)
> -
> - if deb_pack_found == 0:
> - logger.debug("Adding the new debian package info to manifest")
> - fout = open(debpack_manifest_fname, "a")
> - fout.write(new_debpack_line)
> - fout.close()
> - else:
> - logger.debug("Replaced the old debian package information "\
> - "with the new information")
> -
> -
> -def modify_Packages_info(debpack_dirname, new_debpack_info):
> + if not (line.startswith("%s" % prefix_pkg_remove) or \

You don't need the backslash here because of the parenthesis around the
two line.startswith() calls.

> + line.startswith("hwpack-linaro")):

I, for one, will surely not remember why we need this when I come back
from holidays, and I'm sure others who didn't participate in the
discussion where we realized it was necessary would be equally confused,
so it's important to leave a comment here...

Hello Salgado,

Thanks for the review comments.
Please see my reply inline.

Regards,
Deepti.

On Thu, Aug 4, 2011 at 5:58 PM, Guilherme Salgado <
<email address hidden>> wrote:

> Hi Deepti,
>
> This looks quite good; I've only a few suggestions.
>
> On Thu, 2011-08-04 at 16:38 +0000, Deepti B. Kalakeri wrote:
> > === modified file 'linaro-hwpack-replace'
> > --- linaro-hwpack-replace 2011-08-03 11:29:43 +0000
> > +++ linaro-hwpack-replace 2011-08-04 16:38:27 +0000
> > @@ -36,10 +36,12 @@
> >
> >
> > parser = argparse.ArgumentParser()
> > -parser.add_argument("-t", "--hwpack_name", dest="hwpack_name",
> > - help="Specific hwpack_name to use (default: None)")
> > -parser.add_argument("-p", "--deb_pack", dest="deb_pack",
> > +parser.add_argument("-t", "--hwpack-name", dest="hwpack_name",
> > + help="Specific hwpack-name to use (default: None)")
> > +parser.add_argument("-p", "--deb-pack", dest="deb_pack",
> > help="Specific debian package to replace (default:
> None).")
> > +parser.add_argument("-r", "--prefix-pkg-remove",
> dest="prefix_pkg_remove",
> > + help="Specific debian package to remove (default:
> None).")
>
> The user actually specifies the name prefix of the packages that should
> be removed and not the actual package, so the help text above is not
> correct. It also mentions a default value, which doesn't make much sense
> to me as the argument is required, no?
>
>
Ah! Thanks I will correct the description.
Yes the argument is not optional, its made compulsory and the check for the
existence of
the value other than None for this option is made below in the main
function.
Hence we expect the user to give a value something else than None and make
it compulsory in that way.

> > parser.add_argument("-d", "--debug-output", action="store_true",
> dest="debug",
> > help="Verbose messages are displayed when
> specified")
> >
> > -def modify_manifest_info(tempdir, new_debpack_info, deb_pack_found):
> > +def modify_manifest_info(tempdir, new_debpack_info, prefix_pkg_remove):
> > """ Modify the manifest file to include the new debian information
> """
> >
> > debpack_manifest_fname = os.path.join(tempdir, "manifest")
> > new_debpack_line = '%s=%s\n' % (new_debpack_info.name,
> new_debpack_info.version)
> >
> > for line in fileinput.FileInput(debpack_manifest_fname, inplace=1):
> > - if '=' in line:
> > - package_name, version = line.split('=')
> > - old_debpack = '%s=%s' % (package_name, version)
> > - else:
> > - package_name = line.rstrip("\n")
> > - old_debpack = '%s' % package_name
> > -
> > - if new_debpack_info.name == package_name:
> > - deb_pack_found = 1
> > - line = new_debpack_line
> > - sys.stdout.write(line)
> > -
> > - if deb_pack_found == 0:
> > - logger.debug("Adding the new debian package info to manifest")
> > - fout = open(debpack_manifest_fname, "a")
> > - fout.write(new_debpack_line)
> > - fout.close()
> > - else:
> > - logger.debug("Replaced the old deb...

On Thu, 04 Aug 2011 16:58:57 -0000, Guilherme Salgado <email address hidden> wrote:
> > + line.startswith("hwpack-linaro")):
>
> I, for one, will surely not remember why we need this when I come back
> from holidays, and I'm sure others who didn't participate in the
> discussion where we realized it was necessary would be equally confused,
> so it's important to leave a comment here explaining why it's needed.

Also, the prefix added by hwpack create is "hwpack-". We just use
"linaro-" as a convention in the names of our hwpacks. This means that
the code above will only work for linaro people.

Thanks,

James

> On Thu, 04 Aug 2011 16:58:57 -0000, Guilherme Salgado
> <email address hidden> wrote:
> > > + line.startswith("hwpack-linaro")):
> >
> > I, for one, will surely not remember why we need this when I come back
> > from holidays, and I'm sure others who didn't participate in the
> > discussion where we realized it was necessary would be equally confused,
> > so it's important to leave a comment here explaining why it's needed.
>
> Also, the prefix added by hwpack create is "hwpack-". We just use
> "linaro-" as a convention in the names of our hwpacks. This means that
> the code above will only work for linaro people.
>
> Thanks,
>
> James

Done Thanks James.

Regards,
Deepti.

I have pushed the changes to include James comments. Can someone please review the same.

Thanks and Regards,
Deepti

66 + if not (line.startswith("%s" % prefix_pkg_remove) or
67 + line.startswith("hwpack-linaro")):

This needs to be changed to be "hwpack-" as well.

In addition I'd like to change to have one function we can call to decide
whether to remove a package, something like:

  def should_remove(package_name, prefix):
      if package_name.startswith(prefix) or package_name.startswith("hwpack-"):
          return True
      return False

which we can then call from the three places where we remove files (packages file, manifest file,
and the actual files.)

This will reduce duplication and mean that we don't forget to remove the
hwpack metapackage somewhere for instance.

Thanks,

James

> 66 + if not (line.startswith("%s" % prefix_pkg_remove) or
> 67 + line.startswith("hwpack-linaro")):
>
> This needs to be changed to be "hwpack-" as well.
>
> In addition I'd like to change to have one function we can call to decide
> whether to remove a package, something like:
>
> def should_remove(package_name, prefix):
> if package_name.startswith(prefix) or
> package_name.startswith("hwpack-"):
> return True
> return False
>
> which we can then call from the three places where we remove files (packages
> file, manifest file,
> and the actual files.)
>
> This will reduce duplication and mean that we don't forget to remove the
> hwpack metapackage somewhere for instance.
>
> Thanks,
>
> James

I have submitted the changes please review.

Thanks and Regards,
Deepti.

27 + if (package_name.startswith("%s" % prefix_pkg_remove) or

It's a minor thing, but I'm not sure that the string substitution is needed
there. I think this could be written

  package_name.startswith(prefix_pkg_remove)

111 + # hwpack-linaro* Package is a metadata package that contain reference to the
112 + # linux-linaro-omap that was previously present in the hwpack.
113 + # We need to make sure we dont write the hwpack-linaro related
114 + # package information into Package, otherwise it would try to download the old
115 + # kernel package that was present in the hwpack than installing the new one.

I think that comment should move to be in the should_remove function now,
as that's where it's adding that check.

Also the comment still mentions "hwpack-linaro*" when it should probably
say "hwpack-*".

It's a great idea to have the comment explaining this though :-)

Thanks,

James

> 27 + if (package_name.startswith("%s" % prefix_pkg_remove) or
>
> It's a minor thing, but I'm not sure that the string substitution is needed
> there. I think this could be written
>
> package_name.startswith(prefix_pkg_remove)
>
> 111 + # hwpack-linaro* Package is a metadata package that contain
> reference to the
> 112 + # linux-linaro-omap that was previously present in the hwpack.
> 113 + # We need to make sure we dont write the hwpack-linaro related
> 114 + # package information into Package, otherwise it would try to
> download the old
> 115 + # kernel package that was present in the hwpack than installing the
> new one.
>
> I think that comment should move to be in the should_remove function now,
> as that's where it's adding that check.
>
> Also the comment still mentions "hwpack-linaro*" when it should probably
> say "hwpack-*".
>
> It's a great idea to have the comment explaining this though :-)
>
> Thanks,
>
> James
Ah! thanks James. Addressed all the comments and submitted the changes.
Please review the same and merge if it passes review.

Thanks and Regards,
Deepti.