582266 - Add a Mozmill version argument option on command line

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Aaron Train [:aaronmt]]

Can we please add a -v argument to mozmill in order to print the current version?

Currently, I run the program and scan for load_entry_point('mozmill==1.4.2b1') -- which seems silly.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This shouldn't be simple to do and would help us a lot. The OptionsParser already support that. Can we get this one-liner for 1.4.2?

Comment 2

[Jeff Hammel]

Make sure to do this correctly and take the number from setuptools metadata.  It should/will only work if the package is installed from setuptools

Comment 3

[Andrew Halberstadt [:ahal]]

Yep, it's almost done.  I'm planning to use '--info' instead of '-v' and print out the other metadata as well.

I'm currently using pkg_resources.get_distribution("Mozmill").get_metadata('...')

Comment 4

[Jeff Hammel]

Looks like the right approach

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Prints metadata of specified module with --info command

Prints any metadata it can find when --info is specified.

Note that the print_info method is implemented in mozrunner, so --info will work for any modules inheriting the CLI from there (i.e mozmill and jsbridge).

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Why can't we use:
http://docs.python.org/library/optparse.html#printing-a-version-string

Comment 7

[Jeff Hammel]

(In reply to comment #6)
> Why can't we use:
> http://docs.python.org/library/optparse.html#printing-a-version-string

We can, but ultimately the version should be that from setup.py

Comment 8

[Andrew Halberstadt [:ahal]]

Even if we use --version we'd still need to supply the version to optparse from somewhere, so we'd still need to access the egg metadata. And if we're going there already, why not print out the rest of the metadata while were at it?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sure, what I meant was, why can't we use print_info to return the string we can print out with --version. I'm in favor of using the default paths as much as possible. Is there something limiting us from using it?

Comment 10

[Jeff Hammel]

Comment on attachment 460687 [details] [diff] [review]
Prints metadata of specified module with --info command

>diff --git a/jsbridge/jsbridge/__init__.py b/jsbridge/jsbridge/__init__.py
>index 2003b4b..1398316 100644
>--- a/jsbridge/jsbridge/__init__.py
>+++ b/jsbridge/jsbridge/__init__.py
>@@ -85,6 +85,8 @@ def wait_and_create_network(host, port, timeout=wait_to_create_timeout):
> 
> class CLI(mozrunner.CLI):
>     """Command line interface."""
>+    
>+    module = "jsbridge"

I would prefer not noting the name of the module here (and in the other two places), but I can't think of a clever way of doing this off the top of my head, so I guess this is fine.


>@@ -47,6 +47,7 @@ import zipfile
> import optparse
> import killableprocess
> import subprocess
>+import pkg_resources
> from xml.etree import ElementTree
> from distutils import dir_util
> from time import sleep
>@@ -472,6 +473,7 @@ class CLI(object):
> 
>     runner_class = FirefoxRunner
>     profile_class = FirefoxProfile
>+    module = "mozrunner"
> 
>     parser_options = {("-b", "--binary",): dict(dest="binary", help="Binary path.",
>                                                 metavar=None, default=None),
>@@ -480,6 +482,9 @@ class CLI(object):
>                       ('-a', "--addons",): dict(dest="addons", 
>                                                 help="Addons paths to install.",
>                                                 metavar=None, default=None),
>+                      ("--info",): dict(dest="info", default=False,
>+                                        action="store_true",
>+                                        help="Print module information.")
>                      }
> 
>     def __init__(self):
>@@ -489,11 +494,26 @@ class CLI(object):
>             self.parser.add_option(*names, **opts)
>         (self.options, self.args) = self.parser.parse_args()
> 
>+        if self.options.info:
>+            self.print_info()
>+            sys.exit(0)
>+            

Ideally, the metadata should be obtained first and stored on the instance object.  This way, if an item of metadata, such as the version, is needed in the program, it may be programmatically retrieved.  Also, if the metadata is not available (i.e. if you are running the package without installing it via setuptools) then you can not add the --info switch.  This is currently difficult due to the way we store options, but when I fix this for 2.0 it should be done.  Then the print info can just print out this dictionary.

>         # XXX should use action='append' instead of rolling our own
>         try:
>             self.addons = self.options.addons.split(',')
>         except:
>             self.addons = []
>+            
>+    def print_info(self):
>+        dist = pkg_resources.get_distribution(self.module)
>+        if dist.has_metadata("PKG-INFO"):
>+            metadata = ["Name", "Version", "Summary", "Home-page", "Author", 
>+                        "Author-email", "License", "Description", "Platform"]

See above comment.  Also, this should be a set()

>+            for line in dist.get_metadata_lines("PKG-INFO"):
>+                if line[0:line.find(":")] in metadata:
>+                    print line
>+        if dist.has_metadata("requires.txt"):
>+            print "\nDependencies: \n" + dist.get_metadata("requires.txt")            
> 
>     def create_runner(self):
>         """ Get the runner object """

Please fetch the metadata first and store on the CLI instance.

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: Attaches metadata to CLI instance and allows --version as well

Stores the metadata in a dictionary which is attached to the CLI instance.  Also allows --version and adds a convenience print_metadata method.

Comment 12

[Jeff Hammel]

Comment on attachment 460969 [details] [diff] [review]
Attaches metadata to CLI instance and allows --version as well

>@@ -472,6 +473,8 @@ class CLI(object):
> 
>     runner_class = FirefoxRunner
>     profile_class = FirefoxProfile
>+    module = "mozrunner"
>+    metadata = {}

Is there a reason to set this on the class-level?
 
>     parser_options = {("-b", "--binary",): dict(dest="binary", help="Binary path.",
>                                                 metavar=None, default=None),
>@@ -480,20 +483,44 @@ class CLI(object):
>                       ('-a', "--addons",): dict(dest="addons", 
>                                                 help="Addons paths to install.",
>                                                 metavar=None, default=None),
>+                      ("--info",): dict(dest="info", default=False,
>+                                        action="store_true",
>+                                        help="Print module information")
>                      }
> 
>     def __init__(self):
>         """ Setup command line parser and parse arguments """
>-        self.parser = optparse.OptionParser()
>+        self.metadata = self.get_metadata_from_egg()
>+        self.parser = optparse.OptionParser(version="%prog " + self.metadata["Version"])
>         for names, opts in self.parser_options.items():
>             self.parser.add_option(*names, **opts)
>         (self.options, self.args) = self.parser.parse_args()
> 
>+        if self.options.info:
>+            self.print_metadata()
>+            sys.exit(0)
>+            
>         # XXX should use action='append' instead of rolling our own
>         try:
>             self.addons = self.options.addons.split(',')
>         except:
>             self.addons = []
>+            
>+    def get_metadata_from_egg(self):
>+        ret = {}
>+        dist = pkg_resources.get_distribution(self.module)
>+        if dist.has_metadata("PKG-INFO"):
>+            for line in dist.get_metadata_lines("PKG-INFO"):
>+                ret[line[0:line.find(":")]] = line[line.find(":") + 1:]

This is a bit ugly.  How about `key, value = line.split(':', 1); ret[key] = value`

>+        if dist.has_metadata("requires.txt"):
>+            ret["Dependencies"] = "\n" + dist.get_metadata("requires.txt")    
>+        return ret
>+        
>+    def print_metadata(self, data=["Name", "Version", "Summary", "Home-page", 
>+                                   "Author", "Author-email", "License", "Platform", "Dependencies"]):

You should use a tuple for data as a default argument, not a list

>+        for key in data:
>+            if key in self.metadata:
>+                print key + ": " + self.metadata[key]
> 
>     def create_runner(self):
>         """ Get the runner object """

Looks good, basically, but would recommend those changes.

Comment 13

[Andrew Halberstadt [:ahal]]

Comments addressed and changes checked in.

master: http://github.com/mozautomation/mozmill/commit/59808df2f140ade7628bcbf34ff3ecf7b230c4d9

1.4.2: http://github.com/mozautomation/mozmill/commit/0842f69b7564200f7f818b4052742d22d6399f67

Comment 14

[Andrew Halberstadt [:ahal]]

Sorry, realized I had somehow reverted uncommenting a line.

master: http://github.com/mozautomation/mozmill/commit/ccd2b3618ee0d0bed69b8efadca41975c309dd6d

1.4.2: http://github.com/mozautomation/mozmill/commit/c3ce85e87992a1505c7436f6db424c1072b17781

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That looks fantastic. Now it's much easier to keep track of Mozmill versions. Thanks!