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 explaining why it's needed.

> +            sys.stdout.write(line)

The indentation on the 3 lines above seems to be one level too deep.
Can you double check that there are no TAB characters in this file as
that might be why you didn't notice the wrong indentation before.

> +
> +    logger.debug("Adding the new debian package info to manifest")
> +    fout = open(debpack_manifest_fname, "a")
> +    fout.write(new_debpack_line)
> +    fout.close()
> +
> +
> +def modify_Packages_info(debpack_dirname, new_debpack_info, prefix_pkg_remove):
>      """ Modify the Packages file to include the new debian information """
>  
>      debpack_Packages_fname = os.path.join(debpack_dirname, "Packages")
> -    f = open(debpack_Packages_fname, "r+")
>      try:
>          output = []
>          def should_remove(package_name):
> -            return package_name == new_debpack_info.name
> +            return package_name.startswith("%s" % prefix_pkg_remove)
> +
> +        f = open(debpack_Packages_fname, "r+")
>          for stanza in Packages.iter_paragraphs(f):
> -            if not should_remove(stanza["Package"]):
> -                output.append(stanza)
> +            if not (should_remove(stanza["Package"]) or \

No need for the backslash here either. :)

> +                    stanza["Package"].startswith("hwpack-linaro")):
> +                    output.append(stanza)

The line above is indented one extra level, making it look like it's
just another condition of the if.

>          output.append(DummyStanza(new_debpack_info))
> +
>          f.seek(0,0)
> -
> +        f.truncate()
>          for stanza in output:
>              stanza.dump(f)
>              f.write("\n")
> @@ -154,7 +146,8 @@
>  def main():
>      # Validate that all the required information is passed on the command line
>      args = parser.parse_args()
> -    if args.hwpack_name == None or args.deb_pack == None:
> +    if args.hwpack_name == None or args.deb_pack == None or\
> +       args.prefix_pkg_remove == None:

Here you can avoid the backslash by using paranthesis, which is the
PEP-8 recommended style.

>          parser.print_help()
>          parser.error("You must specify both hwpack name "\
>                       "and the debian package information\n")


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