729205 - migrate jetperf.py to the mozharness repo under scripts

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P3)

Description

[Jeff Hammel]

Currently the mozharness jetpack script lives in http://k0s.org/mozilla/hg/jetperf/file/0361eea4b40c/jetperf/jetperf.py 

This should be migrated to http://hg.mozilla.org/build/mozharness/file/tip/scripts

jetperf.py should also be updated to create a virtualenv for talos and fetch it.

The remainder of http://k0s.org/mozilla/hg/jetperf should be split according to bug 728442 and bug 728461 . http://k0s.org/mozilla/hg/jetperf can be kept around consuming jetperf.py as a convenience package for easy use.  Or not.

Comment 1

[Aki Sasaki (not active)]

You probably mean Release Engineering :)

Comment 2

[Jeff Hammel]

Ah indeed, thanks

Comment 3

[Jeff Hammel]

So looking at this, if we are going to modify the script to install talos in a virtualenv, this is looking a lot more like it should inherit from http://hg.mozilla.org/build/mozharness/file/ade8c55fa322/scripts/talos_script.py
(see https://bugzilla.mozilla.org/show_bug.cgi?id=713055 and https://bugzilla.mozilla.org/show_bug.cgi?id=650887).  However, if we do this, we will run into https://bugzilla.mozilla.org/show_bug.cgi?id=650887#c19 . I will work on unblocking that

Comment 4

[Jeff Hammel]

Actually this is probably already the case; I have just run it in the default way that finds firefox using `which` and so haven't seen it

Comment 5

[Jeff Hammel]

Attachment: WIP

So this work in progress is a decent port that builds on top of mozharness.mozilla.talos and extends it to make addons using cfx and runs talos with and without them (ts typically though this is specifiable).

There are large sections of commented out code that refer to making graphs and printing results from formerly http://k0s.org/mozilla/hg/jetperf/file/4645ae34d2c4/jetperf/compare.py and currently http://k0s.org/mozilla/hg/AnalyzeThis/ (also waiting on feedback, bug 728461 ).  I'm not sure what we want to do about these.  I see three options:

1. Make these actions depend on being able to `import analyzethis`
2. Remove these entirely and have this be up to the test runner.
3. Make a wrapper package that depends on AnalyzeThis and mozharness and this script that adds this functionality

I don't really like 2. since it would be less convenient for the jetpack team...and for me.  Personally I prefer 1. but 3. is fine too.  Any suggestions?

This also ignores anything else we might want to change with the patch.

Comment 6

[Aki Sasaki (not active)]

Comment on attachment 613396 [details] [diff] [review]
WIP

Phew!

(In reply to Jeff Hammel [:jhammel] from comment #5)
> 1. Make these actions depend on being able to `import analyzethis`

I think this is the way to go.

If we don't want to create the graph locally, we can skip that action. (This may be the case in production.)

If we want to create the graph, we'll have to have installed AnalyzeThis in the virtualenv.  A preflight_graph() method can verify that we can actually create the graph before attempting to do so.

>+    config_options = copy.deepcopy(Talos.config_options) + [

Hm.  I suppose I should be doing a deepcopy everywhere just in case someone tries editing their config_options?

>+        [["--checkInstall"],
>+         {'action': 'store',
>+          'dest': 'checkFile',
>+          'default': None,
>+          }],

Nit: dashes and underscores.

>+        # fix the tests to be 'ts' by default
>+        active_tests = [i for i in self.config_options
>+                        if i[-1]['dest'] == 'tests']
>+        if len(active_tests) != 1:
>+            self.fatal("system error, --tests option not found")
>+        active_tests = active_tests[0]
>+        active_tests[-1]['default'] = ['ts']

This is fairly convoluted.
I couldn't find anything else in the patch that referred to active_tests; we seem to be throwing this info away.  Do we need this?

>+        # make checkfile point to the right path
>+        self.checkFile = self.config.get('checkFile')
>+        if self.checkFile:
>+            assert not self.config.get('addons'), "--checkInstall can only be used with the pre-generated addon"

self.fatal() ?
Should we only do this check if 'test' is in self.actions ?

>+    def clone(self):
>+        """clone the jetpack repository"""
>+
>+        dest = os.path.join(self.workdir, 'addon-sdk')
>+        MercurialVCS.clone(self, self.config['repo'], dest)
>+        self.cfx = os.path.join(dest, 'bin', 'cfx')
>+        if not os.path.exists(self.cfx):
>+            self.fatal("%s not found" % self.cfx)

I usually call this pull() but am open to this. If the disparate naming becomes an issue we can rename fairly easily.

As written, we're unable to specify a tag or revision to pull here.  Is that ok?

>+    def helloworld(self):
>+        """default test addon"""
>+        # XXX: what about just the `cfx init` addon?
>+        template = """var panel = require("panel");"""
>+        if self.checkFile:
>+            template += """
>+// debugging information
>+var file = require("file");
>+exports.main = function(options, callbacks) {
>+  console.log(options.loadReason);
>+  var foo = file.open('%(FILE)s', 'w');
>+  foo.write("i is a cat!");
>+  foo.close();
>+};
>+console.log("The add-on is running.");
>+"""
>+            template = template % {'FILE': self.checkFile}
>+        return template

This is fine. If we end up needing different versioned files depending on branch we can check in small helper files.

>+    def get_addons(self):
>+        """fetch addons"""
>+
>+        if not self.config['addons']:
>+            # use a default test addon
>+            return [self.helloworld()]
>+
>+        # read the addons from filesystem paths
>+        missing = [i for i in self.config['addons']
>+                   if not os.path.exists(i)]
>+        assert not missing, "Missing addons: %s" % self.config['addons']

self.fatal().  Maybe specify that we were looking for these on disk?
I'd guess you meant '% missing' as well.
self.fatal("Can't find these addons on disk: %s" % str(missing))
?

Also, are these absolute or relative paths?  If relative, should we look in abs_work_dir ?

>+        for i in self.config['addons']:
>+            retval.append(file(i).read())

self.read_from_file() ?

Will these always stay small?  If they're large we may want to copy the file at test-time rather than sucking these into memory.

>+    def build(self):
>+        """Build each test add-on with the add-ons SDK"""
>+
>+        if not self.cfx:
>+            # clone the addon-sdk if needed
>+            self.clone()

I prefer to error out.
The user has full control over what actions to run or not run.  If they choose not to run an action, I'd rather say that's wrong, how to fix it, and error out than assume they meant to do it and do it for them silently.

One side effect you're getting from self.clone() is setting self.cfx.
We can set this ahead of time (it's hardcoded in self.clone() anyway) and verify |os.path.exists()| instead of |not self.cfx|

>+        # build the addons
>+        addons = self.get_addons()
>+        for index, addon in enumerate(addons):
>+            for ctr in range(self.config['number']):
>+                package = 'a%s-%s' % (index, ctr)
>+                path = os.path.join(self.workdir, package)
>+                if os.path.exists(path):
>+                    self.rmtree(path)
>+                os.mkdir(path)

self.mkdir_p() ?

>+                self.run_command([self.cfx, 'init'], cwd=path)

I think we need some error checking here.

>+                # - write the jetpack to main.js
>+                main_js = os.path.join(path, 'lib', 'main.js')
>+                assert os.path.exists(main_js)

self.fatal() with descriptive error message.

>+                f = file(main_js, 'w')
>+                print >> f, addon
>+                f.close()

self.write_to_file() ?

>+                # - package to .xpi:
>+                self.run_command([self.cfx, 'xpi'], cwd=path)
>+                # - run again to avoid:
>+                # """
>+                # No 'id' in package.json: creating a new ID for you.
>+                # package.json modified: please re-run 'cfx xpi'
>+                # """

Sounds like we have some error_list lines to watch for.
WARNING the first run, ERROR the second run?

>+                self.run_command([self.cfx, 'xpi'], cwd=path)
>+                xpi = os.path.join(path, '%s.xpi' % package)
>+                assert os.path.exists(xpi), "'%s' not found" % xpi

self.fatal()... "unable to create %s after two cfx runs" ?

>+                self.xpis.append(xpi)

Does this have a predictable name?
Alternately we could copy the xpi to a predictable location.

I'm asking because we may end up wanting to run the tests on a different script run than generating the xpi's.

>+    def run_talos(self, name, *args, **kw):
>+        """
>+        runs PerfConfigurator and talos
>+        """
>+
>+        # .yml file name
>+        yml = '%s.yml' % name
>+
>+        # get PerfConfigurator options
>+        args = list(args)
>+        args += self.config.get('addOptions', [])

I think this is talos_options now ?

>+        options = self.PerfConfigurator_options(args=args, output=yml, **kw)
>+
>+        # run PerfConfigurator
>+        self.generate_config(conf=yml, options=options)
>+
>+        # run talos
>+        self.run_tests(conf=yml)
>+
>+    def test(self):
>+        """run talos tests"""
>+
>+        # dependencies
>+        # ideally, these should be done with a decorator
>+        if not self.xpis:
>+            self.build()
>+        assert self.xpis, "No addons found"

self.fatal()

>+        # remove check file if it exists
>+        if self.checkFile and os.path.exists(self.checkFile):
>+            os.remove(self.checkFile)

self.rmtree()

>+
>+        # run talos
>+        args = []
>+        for xpi in self.xpis:
>+            args.extend(['--extension', xpi])
>+        self.run_talos('jetperf', *args)
>+
>+        # ensure the check file exists (=> the addon was loaded) if specified
>+        if self.checkFile:
>+            assert os.path.exists(self.checkFile), "'%s' not found; maybe the addon wasn't loaded" % self.checkFile

self.fatal()

>+
>+        # print the results file location if it is a file
>+        if self.results_url.startswith('file://'):
>+            filename = self.results_url[len('file://'):]
>+            assert os.path.exists(filename)

self.fatal()

>+            print filename

self.info()?

>+    def baseline_results_filename(self):
>+        return os.path.join(self.workdir, 'baseline.txt')
>+
>+    def baseline(self):
>+        """run baseline ts tests"""
>+        args = []
>+        filename = self.baseline_results_filename()
>+        if os.path.exists(filename):
>+            os.remove(filename)

We've got self.rmtree().  If we need a file-specific rm we can add it.

>+        self.run_talos('baseline', results_url='file://%s' % filename)
>+        assert os.path.exists(filename)

self.fatal("Tried to run a baseline talos run but there's no file %s" % filename) or something?

>+#    def graph(self):
>+#         """generate a comparison graph between baseline results and the results with jetpack addon(s)"""
>+
>+#         # check for runnability
>+#         graph_output = self.config.get('graph')
>+#         if not graph_output:
>+#             self.info("graph location not specified, returning")
>+#             return

Does it make sense to have a default location?  You can turn on/off graphing with actions.  e.g. --no-graph, or remove graph from default_actions.

>+#         if not (self.results_url and self.results_url.startswith('file://')):
>+#             self.info("graph needs a file results_url, returning")
>+#             return
>+#         try:
>+#             import compare
>+#         except ImportError:
>+#             self.info("cannot import compare.py, returning")
>+#             return

info() and return should probably be warning() or higher.
Per mozharness.mozilla.talos, it looks like self.results_url is always set, but checking doesn't hurt.

>+#         # run the tests if they have not been run
>+#         jetperf_results = self.results_url[len('file://'):]
>+#         ts_results = self.ts_results_filename()
>+#         if not os.path.exists(jetperf_results):
>+#             self.test()
>+#         if not os.path.exists(ts_results):
>+#             self.ts()

Yeah, I think we need to error out if we haven't run these, rather than assume people want a new run.  preflight_graph() might be a good place for all of these prereqs.  Also, I think self.ts() isn't defined... remnant?

>+#         # parse the results
>+#         results = compare.parse_file(jetperf_results, ts_results)
>+
>+#         # print the results
>+#         if self.filters:
>+#             compare.print_summary(results, self.filters, output=self.config.get('summary_file'))
>+
>+#         # draw the graph
>+#         compare.makegraph(graph_output, results, self.filters, sort='testname')

Does compare.* give you any output?
I'd like to log it, or state that the files are created.

We'll need tbpl status bits, but that's probably work to do in mozharness.mozilla.talos.

Let me know if you have questions or concerns about this feedback?

Comment 7

[Jeff Hammel]

One confusing thing for mozharness is when you have command line options and configuration that affects some subset of actions but not all of them.  For instance, I ran (by mistake)

./jetperf.py --binary `which firefox` --graph-output ~/graph.html --graph -n 10

appending the '-n 10' from the previous command line.  Since I only run the graph step, the -n 10 is inapplicable, but there is no indication that this is the case.  I'm not sure if there is an action item here, it is just counter-intuitive to me.

Comment 8

[Jeff Hammel]

It would be nice to have tests for this, but I'm not sure how this would be mostly possible without a path to the firefox binary.  This in general seems to be a problem with several of our test harnesses (e.g. talos, mozbase). I'm not sure how to get around this but...not having tests isn't a wonderful alternative.

Comment 9

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> One confusing thing for mozharness is when you have command line options and
> configuration that affects some subset of actions but not all of them.  For
> instance, I ran (by mistake)
> 
> ./jetperf.py --binary `which firefox` --graph-output ~/graph.html --graph -n
> 10
> 
> appending the '-n 10' from the previous command line.  Since I only run the
> graph step, the -n 10 is inapplicable, but there is no indication that this
> is the case.  I'm not sure if there is an action item here, it is just
> counter-intuitive to me.

There is nothing illegal about providing more config than is required for the set of actions running.  In fact, it's expected that if you run a subset of actions, your config will probably be greater than what's required, since running with a full config file is valid.

I think checks would have to live in _pre_config_lock() and be script-specific.  We would have to differentiate between config passed in through the command line, config passed in through the initial config dict/file, config passed in through the config file, or config passed in through OptParse defaults.

I would guess that the complexity involved here would be more problematic than helpful, but I'm certainly open to an elegant solution here.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 613396 [details] [diff] [review]
WIP

Review of attachment 613396 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/jetperf.py
@@ +156,5 @@
> +                path = os.path.join(self.workdir, package)
> +                if os.path.exists(path):
> +                    self.rmtree(path)
> +                os.mkdir(path)
> +                self.run_command([self.cfx, 'init'], cwd=path)

I don't think it is a good idea to use `cfx init`.
It would be better to have one or multiple addon(s) hosted somewhere.
In the meantine, if it is easier, you can inline a simple addon like what you are doing now,
you will just have to inline a valid package.json file too.

Currently, you just need two files in order to run an addon:
/package.json:
  {id:"unique-id", name:"addon name"}
/lib/main.js:
  ...your js code...

(It may change a little bit in future with packageless work of Irakli)

The benefit of this is that you won't have to execute `cfx xpi` twice!
(as package.json id attribute will already be set)

Comment 11

[Jeff Hammel]

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> Comment on attachment 613396 [details] [diff] [review]
> WIP
> 
> Review of attachment 613396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: scripts/jetperf.py
> @@ +156,5 @@
> > +                path = os.path.join(self.workdir, package)
> > +                if os.path.exists(path):
> > +                    self.rmtree(path)
> > +                os.mkdir(path)
> > +                self.run_command([self.cfx, 'init'], cwd=path)
> 
> I don't think it is a good idea to use `cfx init`.
> It would be better to have one or multiple addon(s) hosted somewhere.
> In the meantine, if it is easier, you can inline a simple addon like what
> you are doing now,
> you will just have to inline a valid package.json file too.
> 
> Currently, you just need two files in order to run an addon:
> /package.json:
>   {id:"unique-id", name:"addon name"}
> /lib/main.js:
>   ...your js code...
> 
> (It may change a little bit in future with packageless work of Irakli)
> 
> The benefit of this is that you won't have to execute `cfx xpi` twice!
> (as package.json id attribute will already be set)

Is there any disadvantage of using the current approach? (Running `cfx init` and replacing the main.js file?) I think this is more declarative than populating a file structure, let alone avoiding generating a unique id.

Comment 12

[Alexandre Poirot [:ochameau]]

It will start failing if we have to change cfx init template.
You should really be in control of the addon you are testing.
Testing the raw addon output of cfx init is a good idea,
hacking the result of cfx init is a bad one.

Comment 13

[Jeff Hammel]

(In reply to Alexandre Poirot (:ochameau) from comment #12)
> It will start failing if we have to change cfx init template.
> You should really be in control of the addon you are testing.
> Testing the raw addon output of cfx init is a good idea,
> hacking the result of cfx init is a bad one.

We should probably block on 717980 and be explicit about it then

Comment 14

[Jeff Hammel]

Attachment: WIP 2

Comment 15

[Jeff Hammel]

>
> (In reply to Jeff Hammel [:jhammel] from comment #5)
> > 1. Make these actions depend on being able to `import analyzethis`
>
> I think this is the way to go.
>
> If we don't want to create the graph locally, we can skip that action. (This may be the case in production.)

I've gone with this strategy.

> If we want to create the graph, we'll have to have installed
> AnalyzeThis in the virtualenv.  A preflight_graph() method can
> verify that we can actually create the graph before attempting to do
> so.

My current methodology is to check for AnalyzeThis in the same scope
as the jetperf.py script itself.  Unless there is a reason to install
this into the virtualenv, conditionally depending on it at the top
level seems to be the friendliest for Jetpack developers.  The only
tie that AnalyzeThis currently has to talos is the output format and
some helper functions.  So the version of AnalyzeThis should not be
sensitive to the version of talos installed in the virtualenv.

Alternatively, we could add AnalyzeThis to the virtualenv by default
and consume that.  Currently, AFAIK, there is no way of importing
modules from within the virtualenv (but maybe worth having?) so I'd
have to change AnalyzeThis to be able to do what we require here from
a console script.

> >+    config_options = copy.deepcopy(Talos.config_options) + [
>
> Hm.  I suppose I should be doing a deepcopy everywhere just in case someone tries editing their config_options?

Probably.  Unnecessary iff

1. The config options are not touched by any of the descendent classes
2. You don't instantiate more than one of the overlapping classes in a
given interpreter instance.

IMHO 1. is probably fair, but depending on 2. is a bad idea (even
though it is the normal case).  I *do* violate 1. in this patch, so
doing something here is necessary.  I general consider using the copy
module to be in bad form (and indicative of other issues), but perhaps
it is okay here.

> >+        [["--checkInstall"],
> >+         {'action': 'store',
> >+          'dest': 'checkFile',
> >+          'default': None,
> >+          }],
>
> Nit: dashes and underscores.

Fixed.

> >+        # fix the tests to be 'ts' by default
> >+        active_tests = [i for i in self.config_options
> >+                        if i[-1]['dest'] == 'tests']
> >+        if len(active_tests) != 1:
> >+            self.fatal("system error, --tests option not found")
> >+        active_tests = active_tests[0]
> >+        active_tests[-1]['default'] = ['ts']
>
> This is fairly convoluted.
> I couldn't find anything else in the patch that referred to active_tests; we seem to be throwing this info away.  Do we need this?

While there has been the request for the ability to run jetperf tests
with any available talos test, the case of interest for jetperf at the
moment is 'ts' so this should be the default value.  If there is a
better way to set the default I'd love a pointer. (It is not thrown
away, the configuration option is modified as we want 'ts' to be the
default for jetperf.)

> >+        # make checkfile point to the right path
> >+        self.checkFile = self.config.get('checkFile')
> >+        if self.checkFile:
> >+            assert not self.config.get('addons'), "--checkInstall can only be used with the pre-generated addon"
>
> self.fatal() ?
> Should we only do this check if 'test' is in self.actions ?

I am less inclined to do so.  There shouldn't be a reason where you
would specify --check-install and specify --addons conceptually.

> >+    def clone(self):
> >+        """clone the jetpack repository"""
> >+
> >+        dest = os.path.join(self.workdir, 'addon-sdk')
> >+        MercurialVCS.clone(self, self.config['repo'], dest)
> >+        self.cfx = os.path.join(dest, 'bin', 'cfx')
> >+        if not os.path.exists(self.cfx):
> >+            self.fatal("%s not found" % self.cfx)
>
> I usually call this pull() but am open to this. If the disparate naming becomes an issue we can rename fairly easily.

I've renamed to pull.

> As written, we're unable to specify a tag or revision to pull here.  Is that ok?

I think this is fine, for now.  Is there a good way to do this without
having to front-end too many things?

> >+    def helloworld(self):
> >+        """default test addon"""
> >+        # XXX: what about just the `cfx init` addon?
> >+        template = """var panel = require("panel");"""
> >+        if self.checkFile:
> >+            template += """
> >+// debugging information
> >+var file = require("file");
> >+exports.main = function(options, callbacks) {
> >+  console.log(options.loadReason);
> >+  var foo = file.open('%(FILE)s', 'w');
> >+  foo.write("i is a cat!");
> >+  foo.close();
> >+};
> >+console.log("The add-on is running.");
> >+"""
> >+            template = template % {'FILE': self.checkFile}
> >+        return template
>
> This is fine. If we end up needing different versioned files depending on branch we can check in small helper files.

Not sure I follow. What is meant here?

> >+    def get_addons(self):
> >+        """fetch addons"""
> >+
> >+        if not self.config['addons']:
> >+            # use a default test addon
> >+            return [self.helloworld()]
> >+
> >+        # read the addons from filesystem paths
> >+        missing = [i for i in self.config['addons']
> >+                   if not os.path.exists(i)]
> >+        assert not missing, "Missing addons: %s" % self.config['addons']
>
> self.fatal().  Maybe specify that we were looking for these on disk?
> I'd guess you meant '% missing' as well.
> self.fatal("Can't find these addons on disk: %s" % str(missing))
> ?

Done

> Also, are these absolute or relative paths?  If relative, should we look in abs_work_dir ?

These can be relative or absolute.  I'm more inclined to keep relative
files looking relative to CWD so that a user can specify
--addons=foo.js,bar.js and have these be in the CWD.  If you're using
a config file versus command line switches I see no problem listing
the absolute path or setting the CWD such that they appropriately
resolve.

> >+        for i in self.config['addons']:
> >+            retval.append(file(i).read())
>
> self.read_from_file() ?
>
> Will these always stay small?  If they're large we may want to copy the file at test-time rather than sucking these into memory.

I originally thought that we could do away with these and just copy
the files.  Looking again, the reason that I read the files -> strings
is that the default addon just lives in-file:

http://k0s.org/mozilla/hg/mozharness-patches/file/ea749f0318f9/jetperf#l188

Even if we put this in a local file we can't copy it over assuming
we're keeping the check_file functionality (I'd be open to removing
it, but if for whatever reason you're not sure the addons are being
correctly installed, --check-file makes debugging this much easier.)

The files should in general be small: probably a few kb or less. So
I'm content leaving this as is, though I'd be open to other solutions
too as long as the default cases with --addons = [] works the same way.

> >+    def build(self):
> >+        """Build each test add-on with the add-ons SDK"""
> >+
> >+        if not self.cfx:
> >+            # clone the addon-sdk if needed
> >+            self.clone()

> I prefer to error out.
> The user has full control over what actions to run or not run.  If
> they choose not to run an action, I'd rather say that's wrong, how
> to fix it, and error out than assume they meant to do it and do it
> for them silently.

I tend to think of actions as a dependency tree versus independent
actions.  The basic question is:  if action B depends on action A and
you run action B without first running action A, do you err our or run
action A? I am of the inclination that action A should be run, as in

http://hg.mozilla.org/build/mozharness/file/dd81d754a7fd/mozharness/mozilla/testing/talos.py#l191


> One side effect you're getting from self.clone() is setting self.cfx.
> We can set this ahead of time (it's hardcoded in self.clone() anyway) and verify |os.path.exists()| instead of |not self.cfx|

> >+        # build the addons
> >+        addons = self.get_addons()
> >+        for index, addon in enumerate(addons):
> >+            for ctr in range(self.config['number']):
> >+                package = 'a%s-%s' % (index, ctr)
> >+                path = os.path.join(self.workdir, package)
> >+                if os.path.exists(path):
> >+                    self.rmtree(path)
> >+                os.mkdir(path)

> self.mkdir_p() ?

Done.

> >+                self.run_command([self.cfx, 'init'], cwd=path)
>
> I think we need some error checking here.

I haven't seen any errors in this step thus far.  Do you mean checking
the return code and erring out if non-zero?

> >+                # - write the jetpack to main.js
> >+                main_js = os.path.join(path, 'lib', 'main.js')
> >+                assert os.path.exists(main_js)
>
> self.fatal() with descriptive error message.

Done.

> >+                f = file(main_js, 'w')
> >+                print >> f, addon
> >+                f.close()
>
> self.write_to_file() ?

Done

> >+                # - package to .xpi:
> >+                self.run_command([self.cfx, 'xpi'], cwd=path)
> >+                # - run again to avoid:
> >+                # """
> >+                # No 'id' in package.json: creating a new ID for you.
> >+                # package.json modified: please re-run 'cfx xpi'
> >+                # """
>
> Sounds like we have some error_list lines to watch for.
> WARNING the first run, ERROR the second run?

The reason for running twice is
https://bugzilla.mozilla.org/show_bug.cgi?id=613587 .  I'm not sure if
there is anything in particular to check for, outside of error code.
Currently, you need to run the `cfx init` twice and if either fails
then something bad has happened.

> >+                self.run_command([self.cfx, 'xpi'], cwd=path)
> >+                xpi = os.path.join(path, '%s.xpi' % package)
> >+                assert os.path.exists(xpi), "'%s' not found" % xpi
>
> self.fatal()... "unable to create %s after two cfx runs" ?

Not really sure this helps.  Ultimately, bug 613587 should be fixed.
There is no actual reason to run twice save to work around this
upstream bug.

> >+                self.xpis.append(xpi)
>
> Does this have a predictable name?
> Alternately we could copy the xpi to a predictable location.

Yes it does. It is the name of the package directory + .xpi

> I'm asking because we may end up wanting to run the tests on a different script run than generating the xpi's.

> >+    def run_talos(self, name, *args, **kw):
> >+        """
> >+        runs PerfConfigurator and talos
> >+        """
> >+
> >+        # .yml file name
> >+        yml = '%s.yml' % name
> >+
> >+        # get PerfConfigurator options
> >+        args = list(args)
> >+        args += self.config.get('addOptions', [])
>
> I think this is talos_options now ?

Thanks, fixed.

> >+        options = self.PerfConfigurator_options(args=args, output=yml, **kw)
> >+
> >+        # run PerfConfigurator
> >+        self.generate_config(conf=yml, options=options)
> >+
> >+        # run talos
> >+        self.run_tests(conf=yml)
> >+
> >+    def test(self):
> >+        """run talos tests"""
> >+
> >+        # dependencies
> >+        # ideally, these should be done with a decorator
> >+        if not self.xpis:
> >+            self.build()
> >+        assert self.xpis, "No addons found"
>
> self.fatal()

Fixed.

> >+        # remove check file if it exists
> >+        if self.checkFile and os.path.exists(self.checkFile):
> >+            os.remove(self.checkFile)
>
> self.rmtree()

Fixed.

> >+
> >+        # run talos
> >+        args = []
> >+        for xpi in self.xpis:
> >+            args.extend(['--extension', xpi])
> >+        self.run_talos('jetperf', *args)
> >+
> >+        # ensure the check file exists (=> the addon was loaded) if specified
> >+        if self.checkFile:
> >+            assert os.path.exists(self.checkFile), "'%s' not found; maybe the addon wasn't loaded" % self.checkFile
>
> self.fatal()

Fixed.

> >+
> >+        # print the results file location if it is a file
> >+        if self.results_url.startswith('file://'):
> >+            filename = self.results_url[len('file://'):]
> >+            assert os.path.exists(filename)
>
> self.fatal()

Fixed.

> >+            print filename
>
> self.info()?

Actually this was just debugging code I forgot to remove.  Removing.

> >+    def baseline_results_filename(self):
> >+        return os.path.join(self.workdir, 'baseline.txt')
> >+
> >+    def baseline(self):
> >+        """run baseline ts tests"""
> >+        args = []
> >+        filename = self.baseline_results_filename()
> >+        if os.path.exists(filename):
> >+            os.remove(filename)
>
> We've got self.rmtree().  If we need a file-specific rm we can add it.

changed -> self.rmtree

> >+        self.run_talos('baseline', results_url='file://%s' % filename)
> >+        assert os.path.exists(filename)
>
> self.fatal("Tried to run a baseline talos run but there's no file %s" % filename) or something?

Went with http://k0s.org/mozilla/hg/mozharness-patches/file/3349b3a79228/jetperf#l300

> >+#    def graph(self):
> >+#         """generate a comparison graph between baseline results and the results with jetpack addon(s)"""
> >+
> >+#         # check for runnability
> >+#         graph_output = self.config.get('graph')
> >+#         if not graph_output:
> >+#             self.info("graph location not specified, returning")
> >+#             return

> Does it make sense to have a default location?  You can turn on/off graphing with actions.  e.g. --no-graph, or remove graph from default_actions.

I would be most inclined to not have a default location and run the
step if --graph-output is given.

> >+#         if not (self.results_url and self.results_url.startswith('file://')):
> >+#             self.info("graph needs a file results_url, returning")
> >+#             return
> >+#         try:
> >+#             import compare
> >+#         except ImportError:
> >+#             self.info("cannot import compare.py, returning")
> >+#             return
>
> info() and return should probably be warning() or higher.

I've changed this to use warning() for the results_url check.  I'm not
really sure what to do for the other checks.  I do not add 'graph' to
the actions list if 'import analyzethis' fails (i.e. what compare.py
and associated software has turned into).  I've made this a warning
too.  Since not specifying --graph-output effectively means "don't do
this action", I've kept this as info() since it really means nothing
is wrong, but I can change this if desired.

> Per mozharness.mozilla.talos, it looks like self.results_url is always set, but checking doesn't hurt.

It should always be set though it may not be a file.  For the upload
case, we would have results_url = http://graphs.mozilla.org/some/path
so it is incompatible with the graph action.  We would like to have
multiple results outputs for Talos, see bug 745907, but this isn't a
high priority right now nor am I sure off the top of my head how I
would implement it for the mozharness script (intelligently).

> >+#         # run the tests if they have not been run
> >+#         jetperf_results = self.results_url[len('file://'):]
> >+#         ts_results = self.ts_results_filename()
> >+#         if not os.path.exists(jetperf_results):
> >+#             self.test()
> >+#         if not os.path.exists(ts_results):
> >+#             self.ts()

> Yeah, I think we need to error out if we haven't run these, rather
> than assume people want a new run.  preflight_graph() might be a good
> place for all of these prereqs.

See above; dependencies or no?

> Also, I think self.ts() isn't defined... remnant?

Thanks.  Should be self.baseline(). Fixed

> >+#         # parse the results
> >+#         results = compare.parse_file(jetperf_results, ts_results)
> >+
> >+#         # print the results
> >+#         if self.filters:
> >+#             compare.print_summary(results, self.filters, output=self.config.get('summary_file'))
> >+
> >+#         # draw the graph
> >+#         compare.makegraph(graph_output, results, self.filters, sort='testname')

> Does compare.* give you any output?
> I'd like to log it, or state that the files are created.

If you provide print_summary with a filename
(self.config['summary_file']), then it will open and print to the
file.  Otherwise it will write to stdout, which should(?) be logged.
Or we can give it a StringIO instance and log that.
http://k0s.org/mozilla/hg/AnalyzeThis/file/fa470e931ce0/analyzethis/analyze.py#l108

> We'll need tbpl status bits, but that's probably work to do in mozharness.mozilla.talos.

Cool.  Worth filing a follow-up bug for this?

Summary, the big things:

Outside of nits, the main issues seem to be:

- whether to install analyzethis in the virtualenv of just depend on
  (or otherwise what to do about it)

- whether actions should function like dependencies with state or
  whether they should just be standalone and if expectations aren't
  meant to just fail out

Comment 16

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #13)
> (In reply to Alexandre Poirot (:ochameau) from comment #12)
> > It will start failing if we have to change cfx init template.
> > You should really be in control of the addon you are testing.
> > Testing the raw addon output of cfx init is a good idea,
> > hacking the result of cfx init is a bad one.
> 
> We should probably block on 717980 and be explicit about it then

Will this also prevent running an addon multiple times?  With the current system, I can make e.g. 'require("panel");' and run 'cfx init && cfx xpi && cfx xpi' and get distinct addons.  As I understand it, using directories with package.json already filled out will kill this.

Comment 17

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #15)
> > (In reply to Jeff Hammel [:jhammel] from comment #5)
> > > 1. Make these actions depend on being able to `import analyzethis`
> >
> > I think this is the way to go.
> >
> > If we don't want to create the graph locally, we can skip that action. (This may be the case in production.)
>
> I've gone with this strategy.

Hm, I think I meant that graph() or preflight_graph() would check for the analyzethis module and either warn/skip or fail out.  This would allow for installing analyzethis in the create-virtualenv action.

I have mixed feelings about how this is currently, because there's no real way for a user to know about the existence of graphing support unless they read the script or installed the module beforehand.

I'm not going to block on this, though.  If it becomes an issue we can revisit.

> Alternatively, we could add AnalyzeThis to the virtualenv by default
> and consume that.  Currently, AFAIK, there is no way of importing
> modules from within the virtualenv (but maybe worth having?) so I'd
> have to change AnalyzeThis to be able to do what we require here from
> a console script.

I have this, which is a) ugly and b) used to work but will need rewriting if I want it to work with the virtualenv:

https://github.com/escapewindow/mozharness/blob/talosrunner/mozharness/mozilla/testing/device.py#L444

So yes, I think it's worth having.  Maybe figuring out where the site-packages are via a PythonMixin method and then os.path.joining with expected module path info.

> > This is fairly convoluted.
> > I couldn't find anything else in the patch that referred to active_tests; we seem to be throwing this info away.  Do we need this?
>
> While there has been the request for the ability to run jetperf tests
> with any available talos test, the case of interest for jetperf at the
> moment is 'ts' so this should be the default value.  If there is a
> better way to set the default I'd love a pointer. (It is not thrown
> away, the configuration option is modified as we want 'ts' to be the
> default for jetperf.)

You can set a config={'tests': ["ts"]} to BaseScript.__init__() via the Talos.__init__() call.  That should override the OptParse default.  Any config file that specifies 'tests', or a command line call of --tests, should override that default.

> > >+        # make checkfile point to the right path
> > >+        self.checkFile = self.config.get('checkFile')
> > >+        if self.checkFile:
> > >+            assert not self.config.get('addons'), "--checkInstall can only be used with the pre-generated addon"
> >
> > self.fatal() ?
> > Should we only do this check if 'test' is in self.actions ?
>
> I am less inclined to do so.  There shouldn't be a reason where you
> would specify --check-install and specify --addons conceptually.

If you pass a config file with addons specified, then you can never call --check-install without either editing the config file or passing in whatever non-addons information from the config file via the command line.

I'm not sure what scenarios check-install is for, specifically.  This may not be an issue at all, or it can be a bug, depending.

I'm certain we'll find other issues when we start creating config files to put jetperf into production; we can deal with them as we go.

> > As written, we're unable to specify a tag or revision to pull here.  Is that ok?
>
> I think this is fine, for now.  Is there a good way to do this without
> having to front-end too many things?

I have been handling it via VCSMixin.vcs_checkout_repos:
http://hg.mozilla.org/build/mozharness/file/eed9746c412f/mozharness/base/vcs/vcsbase.py#l57

The repo list can be specified in the config file, a la
http://hg.mozilla.org/build/mozharness/file/eed9746c412f/mozharness/base/vcs/vcsbase.py#l57

You can also build your own list before calling vcs_checkout_repos(), with c['repo'], revision c.get('repo_revision', 'default').  That would allow for specifying repo_revision via config file without cluttering the command line options, and a command line option could be added at any time that it becomes desirable.

> > This is fine. If we end up needing different versioned files depending on branch we can check in small helper files.
>
> Not sure I follow. What is meant here?

If that check_file is good for all instances, that's fine.
If we start needing a special check file for certain branches or something, we can have helper files or a dictionary of addons, and specify which one to use.

> > Will these always stay small?  If they're large we may want to copy the file at test-time rather than sucking these into memory.
> 
> I originally thought that we could do away with these and just copy
> the files.  Looking again, the reason that I read the files -> strings
> is that the default addon just lives in-file:
> 
> http://k0s.org/mozilla/hg/mozharness-patches/file/ea749f0318f9/jetperf#l188
> 
> Even if we put this in a local file we can't copy it over assuming
> we're keeping the check_file functionality (I'd be open to removing
> it, but if for whatever reason you're not sure the addons are being
> correctly installed, --check-file makes debugging this much easier.)
>
> The files should in general be small: probably a few kb or less. So
> I'm content leaving this as is, though I'd be open to other solutions
> too as long as the default cases with --addons = [] works the same way.

I'm not entirely sure why the check-file functionality means we can't have your addon on disk, but if the addons are going to stay small that's not a huge worry.

> > >+    def build(self):
> > >+        """Build each test add-on with the add-ons SDK"""
> > >+
> > >+        if not self.cfx:
> > >+            # clone the addon-sdk if needed
> > >+            self.clone()
> 
> > I prefer to error out.
> > The user has full control over what actions to run or not run.  If
> > they choose not to run an action, I'd rather say that's wrong, how
> > to fix it, and error out than assume they meant to do it and do it
> > for them silently.
> 
> I tend to think of actions as a dependency tree versus independent
> actions.  The basic question is:  if action B depends on action A and
> you run action B without first running action A, do you err our or run
> action A? I am of the inclination that action A should be run, as in
> 
> http://hg.mozilla.org/build/mozharness/file/dd81d754a7fd/mozharness/mozilla/
> testing/talos.py#l191

Ideally you error out, if you can make them independent.
So yeah, we should probably change that in talos.py.

The reasoning here is you may already have a test run completed, with info on disk.  How do you get the results graphed?  The way it's currently written, if you rerun with graph output, it'll blow away your previous run's results with a new run.

> > >+                self.run_command([self.cfx, 'init'], cwd=path)
> >
> > I think we need some error checking here.
> 
> I haven't seen any errors in this step thus far.  Do you mean checking
> the return code and erring out if non-zero?

Sure.  That or BaseErrorList would be decent starts.

> > >+                # - package to .xpi:
> > >+                self.run_command([self.cfx, 'xpi'], cwd=path)
> > >+                # - run again to avoid:
> > >+                # """
> > >+                # No 'id' in package.json: creating a new ID for you.
> > >+                # package.json modified: please re-run 'cfx xpi'
> > >+                # """
> >
> > Sounds like we have some error_list lines to watch for.
> > WARNING the first run, ERROR the second run?
> 
> The reason for running twice is
> https://bugzilla.mozilla.org/show_bug.cgi?id=613587 .  I'm not sure if
> there is anything in particular to check for, outside of error code.
> Currently, you need to run the `cfx init` twice and if either fails
> then something bad has happened.

Ok.
I'd note the bug in the comment so we know when it's safe to remove.

> > >+                self.xpis.append(xpi)
> >
> > Does this have a predictable name?
> > Alternately we could copy the xpi to a predictable location.
> 
> Yes it does. It is the name of the package directory + .xpi

Ok.  We can remove the self.build() call in test() and look for the package directory + .xpi ?

> > Per mozharness.mozilla.talos, it looks like self.results_url is always set, but checking doesn't hurt.
> 
> It should always be set though it may not be a file.  For the upload
> case, we would have results_url = http://graphs.mozilla.org/some/path
> so it is incompatible with the graph action.  We would like to have
> multiple results outputs for Talos, see bug 745907, but this isn't a
> high priority right now nor am I sure off the top of my head how I
> would implement it for the mozharness script (intelligently).

I'd lean towards creating a file by default, and allowing for posting the results from that file in a method that can be called multiple times, but not urgent.

> > >+#         # run the tests if they have not been run
> > >+#         jetperf_results = self.results_url[len('file://'):]
> > >+#         ts_results = self.ts_results_filename()
> > >+#         if not os.path.exists(jetperf_results):
> > >+#             self.test()
> > >+#         if not os.path.exists(ts_results):
> > >+#             self.ts()
> 
> > Yeah, I think we need to error out if we haven't run these, rather
> > than assume people want a new run.  preflight_graph() might be a good
> > place for all of these prereqs.
> 
> See above; dependencies or no?

I think they're dependencies, but not specifically in this script run.  If you have previously run them, I think we can have enough info on disk to not re-run.

> > >+#         # parse the results
> > >+#         results = compare.parse_file(jetperf_results, ts_results)
> > >+
> > >+#         # print the results
> > >+#         if self.filters:
> > >+#             compare.print_summary(results, self.filters, output=self.config.get('summary_file'))
> > >+
> > >+#         # draw the graph
> > >+#         compare.makegraph(graph_output, results, self.filters, sort='testname')
> 
> > Does compare.* give you any output?
> > I'd like to log it, or state that the files are created.
> 
> If you provide print_summary with a filename
> (self.config['summary_file']), then it will open and print to the
> file.  Otherwise it will write to stdout, which should(?) be logged.
> Or we can give it a StringIO instance and log that.
> http://k0s.org/mozilla/hg/AnalyzeThis/file/fa470e931ce0/analyzethis/analyze.
> py#l108

Ok.  I don't redirect stdout, so any output from compare.* will not be logged.  I'm open to solutions on how to do that in general.

> > We'll need tbpl status bits, but that's probably work to do in mozharness.mozilla.talos.
> 
> Cool.  Worth filing a follow-up bug for this?

Sure.  peptest.py has a current working example.

> Summary, the big things:
> 
> Outside of nits, the main issues seem to be:
> 
> - whether to install analyzethis in the virtualenv of just depend on
>   (or otherwise what to do about it)
> 
> - whether actions should function like dependencies with state or
>   whether they should just be standalone and if expectations aren't
>   meant to just fail out

Yeah.
I've made my opinions known above; hopefully they're clear.
If they're not clear, or you want to discuss, I'm happy to do so in a less bug commenty forum.
I've spent a decent amount of time responding to this comment and not a whole lot of time looking at the new patch.  Let me know if you want me to look at the new patch as well; otherwise I'll wait for a discussion or new patch.

Comment 18

[Jeff Hammel]

So progress is effectively blocked on decisions on bug 717980 . How cfx is run, the fate of --check-install, and whether we use strings vs files depends on decisions there. Once we have that resolved I'll update the patch and deal with the remaining outstanding issues.

Comment 19

[Aki Sasaki (not active)]

Comment on attachment 615802 [details] [diff] [review]
WIP 2

(comment 17)

Comment 20

[Jeff Hammel]

I think I will scrap the analyzethis portion for the time being.  If there is developer interest or need I can add it back in, but it feels like I am adding functionality in anticipation of what is wanted.  If there are objections let me know.

Comment 21

[Jeff Hammel]

Attachment: migrate jetperf.py to mozharness repo

Comment 22

[Jeff Hammel]

Uploaded a modern patch for review.  I've stripped out analyzethis -- developers can run this manually on the results, if desired.  Still waiting on bug 749681; when this is done, a fix can be uploaded whereby you can specify and hg repo (or that one by default) vs --adddon, which currently takes directories.

I'm not sure if it suffices to take the addon xpi name from the directory name, so there is a TODO comment left to that affect.  I'll answer other outstanding concerns shortly.

Comment 23

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #17)
> (In reply to Jeff Hammel [:jhammel] from comment #15)
> > > (In reply to Jeff Hammel [:jhammel] from comment #5)
> > > > 1. Make these actions depend on being able to `import analyzethis`
> > >
> > > I think this is the way to go.
> > >
> > > If we don't want to create the graph locally, we can skip that action. (This may be the case in production.)
> >
> > I've gone with this strategy.
> 
> Hm, I think I meant that graph() or preflight_graph() would check for the
> analyzethis module and either warn/skip or fail out.  This would allow for
> installing analyzethis in the create-virtualenv action.
> 
> I have mixed feelings about how this is currently, because there's no real
> way for a user to know about the existence of graphing support unless they
> read the script or installed the module beforehand.
> 
> I'm not going to block on this, though.  If it becomes an issue we can
> revisit.

I've taken out analyzethis entirely.  It was intended as a convenience for developers, but they can download the software and run it locally if they need to.  It won't be used for production where results will be uploaded to the graphserver.

> > Alternatively, we could add AnalyzeThis to the virtualenv by default
> > and consume that.  Currently, AFAIK, there is no way of importing
> > modules from within the virtualenv (but maybe worth having?) so I'd
> > have to change AnalyzeThis to be able to do what we require here from
> > a console script.
> 
> I have this, which is a) ugly and b) used to work but will need rewriting if
> I want it to work with the virtualenv:
> 
> https://github.com/escapewindow/mozharness/blob/talosrunner/mozharness/
> mozilla/testing/device.py#L444
> 
> So yes, I think it's worth having.  Maybe figuring out where the
> site-packages are via a PythonMixin method and then os.path.joining with
> expected module path info.
> 
> > > This is fairly convoluted.
> > > I couldn't find anything else in the patch that referred to active_tests; we seem to be throwing this info away.  Do we need this?
> >
> > While there has been the request for the ability to run jetperf tests
> > with any available talos test, the case of interest for jetperf at the
> > moment is 'ts' so this should be the default value.  If there is a
> > better way to set the default I'd love a pointer. (It is not thrown
> > away, the configuration option is modified as we want 'ts' to be the
> > default for jetperf.)
> 
> You can set a config={'tests': ["ts"]} to BaseScript.__init__() via the
> Talos.__init__() call.  That should override the OptParse default.  Any
> config file that specifies 'tests', or a command line call of --tests,
> should override that default.

Done.

> > > >+        # make checkfile point to the right path
> > > >+        self.checkFile = self.config.get('checkFile')
> > > >+        if self.checkFile:
> > > >+            assert not self.config.get('addons'), "--checkInstall can only be used with the pre-generated addon"
> > >
> > > self.fatal() ?
> > > Should we only do this check if 'test' is in self.actions ?
> >
> > I am less inclined to do so.  There shouldn't be a reason where you
> > would specify --check-install and specify --addons conceptually.
> 
> If you pass a config file with addons specified, then you can never call
> --check-install without either editing the config file or passing in
> whatever non-addons information from the config file via the command line.
> 
> I'm not sure what scenarios check-install is for, specifically.  This may
> not be an issue at all, or it can be a bug, depending.

This no longer works with the way addons are specified now so it is gone.

> I'm certain we'll find other issues when we start creating config files to
> put jetperf into production; we can deal with them as we go.
> 
> > > As written, we're unable to specify a tag or revision to pull here.  Is that ok?
> >
> > I think this is fine, for now.  Is there a good way to do this without
> > having to front-end too many things?
> 
> I have been handling it via VCSMixin.vcs_checkout_repos:
> http://hg.mozilla.org/build/mozharness/file/eed9746c412f/mozharness/base/vcs/
> vcsbase.py#l57
> 
> The repo list can be specified in the config file, a la
> http://hg.mozilla.org/build/mozharness/file/eed9746c412f/mozharness/base/vcs/
> vcsbase.py#l57
> 
> You can also build your own list before calling vcs_checkout_repos(), with
> c['repo'], revision c.get('repo_revision', 'default').  That would allow for
> specifying repo_revision via config file without cluttering the command line
> options, and a command line option could be added at any time that it
> becomes desirable.

I've punted on this.  If we need this functionality we can add it in, but it seems overkill for now.

> > > This is fine. If we end up needing different versioned files depending on branch we can check in small helper files.
> >
> > Not sure I follow. What is meant here?
> 
> If that check_file is good for all instances, that's fine.
> If we start needing a special check file for certain branches or something,
> we can have helper files or a dictionary of addons, and specify which one to
> use.

No longer applicable.

> > > Will these always stay small?  If they're large we may want to copy the file at test-time rather than sucking these into memory.
> > 
> > I originally thought that we could do away with these and just copy
> > the files.  Looking again, the reason that I read the files -> strings
> > is that the default addon just lives in-file:
> > 
> > http://k0s.org/mozilla/hg/mozharness-patches/file/ea749f0318f9/jetperf#l188
> > 
> > Even if we put this in a local file we can't copy it over assuming
> > we're keeping the check_file functionality (I'd be open to removing
> > it, but if for whatever reason you're not sure the addons are being
> > correctly installed, --check-file makes debugging this much easier.)
> >
> > The files should in general be small: probably a few kb or less. So
> > I'm content leaving this as is, though I'd be open to other solutions
> > too as long as the default cases with --addons = [] works the same way.
> 
> I'm not entirely sure why the check-file functionality means we can't have
> your addon on disk, but if the addons are going to stay small that's not a
> huge worry.

Again, no longer applicable.  Addons are directories, currently.  When bug 749681 is resolved, we can add in the ability to fetch a repo of addons as an alternate invocation to --addon

> > > >+    def build(self):
> > > >+        """Build each test add-on with the add-ons SDK"""
> > > >+
> > > >+        if not self.cfx:
> > > >+            # clone the addon-sdk if needed
> > > >+            self.clone()
> > 
> > > I prefer to error out.
> > > The user has full control over what actions to run or not run.  If
> > > they choose not to run an action, I'd rather say that's wrong, how
> > > to fix it, and error out than assume they meant to do it and do it
> > > for them silently.
> > 
> > I tend to think of actions as a dependency tree versus independent
> > actions.  The basic question is:  if action B depends on action A and
> > you run action B without first running action A, do you err our or run
> > action A? I am of the inclination that action A should be run, as in
> > 
> > http://hg.mozilla.org/build/mozharness/file/dd81d754a7fd/mozharness/mozilla/
> > testing/talos.py#l191
> 
> Ideally you error out, if you can make them independent.
> So yeah, we should probably change that in talos.py.
> 
> The reasoning here is you may already have a test run completed, with info
> on disk.  How do you get the results graphed?  The way it's currently
> written, if you rerun with graph output, it'll blow away your previous run's
> results with a new run.

Done.

> > > >+                self.run_command([self.cfx, 'init'], cwd=path)
> > >
> > > I think we need some error checking here.
> > 
> > I haven't seen any errors in this step thus far.  Do you mean checking
> > the return code and erring out if non-zero?
> 
> Sure.  That or BaseErrorList would be decent starts.

I now check the return code.

> > > >+                # - package to .xpi:
> > > >+                self.run_command([self.cfx, 'xpi'], cwd=path)
> > > >+                # - run again to avoid:
> > > >+                # """
> > > >+                # No 'id' in package.json: creating a new ID for you.
> > > >+                # package.json modified: please re-run 'cfx xpi'
> > > >+                # """
> > >
> > > Sounds like we have some error_list lines to watch for.
> > > WARNING the first run, ERROR the second run?
> > 
> > The reason for running twice is
> > https://bugzilla.mozilla.org/show_bug.cgi?id=613587 .  I'm not sure if
> > there is anything in particular to check for, outside of error code.
> > Currently, you need to run the `cfx init` twice and if either fails
> > then something bad has happened.
> 
> Ok.
> I'd note the bug in the comment so we know when it's safe to remove.

It is noted.

> > > >+                self.xpis.append(xpi)
> > >
> > > Does this have a predictable name?
> > > Alternately we could copy the xpi to a predictable location.
> > 
> > Yes it does. It is the name of the package directory + .xpi
> 
> Ok.  We can remove the self.build() call in test() and look for the package
> directory + .xpi ?

Will do so.

> > > Per mozharness.mozilla.talos, it looks like self.results_url is always set, but checking doesn't hurt.
> > 
> > It should always be set though it may not be a file.  For the upload
> > case, we would have results_url = http://graphs.mozilla.org/some/path
> > so it is incompatible with the graph action.  We would like to have
> > multiple results outputs for Talos, see bug 745907, but this isn't a
> > high priority right now nor am I sure off the top of my head how I
> > would implement it for the mozharness script (intelligently).
> 
> I'd lean towards creating a file by default, and allowing for posting the
> results from that file in a method that can be called multiple times, but
> not urgent.
> 
> > > >+#         # run the tests if they have not been run
> > > >+#         jetperf_results = self.results_url[len('file://'):]
> > > >+#         ts_results = self.ts_results_filename()
> > > >+#         if not os.path.exists(jetperf_results):
> > > >+#             self.test()
> > > >+#         if not os.path.exists(ts_results):
> > > >+#             self.ts()
> > 
> > > Yeah, I think we need to error out if we haven't run these, rather
> > > than assume people want a new run.  preflight_graph() might be a good
> > > place for all of these prereqs.
> > 
> > See above; dependencies or no?
> 
> I think they're dependencies, but not specifically in this script run.  If
> you have previously run them, I think we can have enough info on disk to not
> re-run.

No longer applicable.

> > > >+#         # parse the results
> > > >+#         results = compare.parse_file(jetperf_results, ts_results)
> > > >+
> > > >+#         # print the results
> > > >+#         if self.filters:
> > > >+#             compare.print_summary(results, self.filters, output=self.config.get('summary_file'))
> > > >+
> > > >+#         # draw the graph
> > > >+#         compare.makegraph(graph_output, results, self.filters, sort='testname')
> > 
> > > Does compare.* give you any output?
> > > I'd like to log it, or state that the files are created.
> > 
> > If you provide print_summary with a filename
> > (self.config['summary_file']), then it will open and print to the
> > file.  Otherwise it will write to stdout, which should(?) be logged.
> > Or we can give it a StringIO instance and log that.
> > http://k0s.org/mozilla/hg/AnalyzeThis/file/fa470e931ce0/analyzethis/analyze.
> > py#l108
> 
> Ok.  I don't redirect stdout, so any output from compare.* will not be
> logged.  I'm open to solutions on how to do that in general.

No longer applicable.

> > > We'll need tbpl status bits, but that's probably work to do in mozharness.mozilla.talos.
> > 
> > Cool.  Worth filing a follow-up bug for this?
> 
> Sure.  peptest.py has a current working example.

I couldn't figure out what exactly to check for here though I'm open to ideas.

> > Summary, the big things:
> > 
> > Outside of nits, the main issues seem to be:
> > 
> > - whether to install analyzethis in the virtualenv of just depend on
> >   (or otherwise what to do about it)
> > 
> > - whether actions should function like dependencies with state or
> >   whether they should just be standalone and if expectations aren't
> >   meant to just fail out
> 
> Yeah.
> I've made my opinions known above; hopefully they're clear.
> If they're not clear, or you want to discuss, I'm happy to do so in a less
> bug commenty forum.
> I've spent a decent amount of time responding to this comment and not a
> whole lot of time looking at the new patch.  Let me know if you want me to
> look at the new patch as well; otherwise I'll wait for a discussion or new
> patch.

Comment 24

[Jeff Hammel]

Attachment: introspect addonsdir for xpis instead of storing them on the instance

Comment 25

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #22)
<snip/>
> I'm not sure if it suffices to take the addon xpi name from the directory
> name, so there is a TODO comment left to that affect.  I'll answer other
> outstanding concerns shortly.

:ochameau, could you comment on this?

Comment 26

[Aki Sasaki (not active)]

Attachment: small patch to get jetperf.py working for me

Comment 27

[Aki Sasaki (not active)]

Comment on attachment 621741 [details] [diff] [review]
introspect addonsdir for xpis instead of storing them on the instance

>+               'download-and-extract',

Nit: I wonder if creating a 'download' method in TestingMixin would make more se
nse for Talos/Jetperf.  download-and-extract is already a bit unwieldy, and here it isn't accurate either.  What do you think?

I can help with that if wanted.

>+            shutil.copytree(addon, path)

I think jlund was going to create a self.copytree() that logs and shutil.copytre
e's.  Since the both of you need it, looks like we should implement it.
I'm ok not blocking on that, though.

> > > > We'll need tbpl status bits, but that's probably work to do in mozharnes
s.mozilla.talos.
> > >
> > > Cool.  Worth filing a follow-up bug for this?
> >
> > Sure.  peptest.py has a current working example.>> I couldn't figure out what exactly to check for here though I'm open to ideas.

http://hg.mozilla.org/build/mozharness/file/19811931c7b0/scripts/peptest.py#l193 through line 220 set this.
That blocks putting this into production with buildbot, but not from landing this in general.


I'm running:
mozharness/scripts/jetperf.py  --installer-url http://ftp.mozilla.org/pub/mozill
a.org/firefox/nightly/latest-mozilla-central/firefox-15.0a1.en-US.mac.dmg --addon `pwd`/jetperf-test-addons/empty-addon

and Talos is throwing an exception when trying to kill the browser.  Is that normal?  Also getting a FATAL Results file not found: /src/jhammel/build/jetperf.txt


This is close.
I'm worried about the exception, there's attachment 622243 [details] [diff] [review] that I needed to add to run this, and there's some followup work above that doesn't need to strictly block here.

Comment 28

[Jeff Hammel]

> I'm running:
> mozharness/scripts/jetperf.py  --installer-url http://ftp.mozilla.org/pub/mozill
> a.org/firefox/nightly/latest-mozilla-central/firefox-15.0a1.en-US.mac.dmg --addon `pwd`/jetperf-test-addons/empty-addon
> 
> and Talos is throwing an exception when trying to kill the browser.  Is that normal?  

Do you mean "Found processes still running: ..." or some other message?  If the former, this is a talos design decision (for better or worse).  If the latter, it is probably a talos bug.  Could you paste the error?

Comment 29

[Jeff Hammel]

> Also getting a FATAL Results file not found: /src/jhammel/build/jetperf.txt

This probably means that somethingbadhappen during the jetperf run and that the run probably didn't succeed.  Could you upload the full output of the run?

Comment 30

[Aki Sasaki (not active)]

Attachment: jetperf info log

(In reply to Jeff Hammel [:jhammel] from comment #28)
> Do you mean "Found processes still running: ..." or some other message?  If
> the former, this is a talos design decision (for better or worse).  If the
> latter, it is probably a talos bug.  Could you paste the error?

I hit the former, killed my Firefox session, then hit the latter.
The log attached is after I killed my Firefox session.

Comment 31

[Jeff Hammel]

Fascinating; this looks like a talos bug.  My guess is that we try to gracefully shutdown the process but the PID lingers for a few microseconds, then we try to kill it but the PID is already gone.  This should be easy to fix by just checking for OSError code 3 (though I haven't personally seen this).

Comment 32

[Aki Sasaki (not active)]

Comment on attachment 621741 [details] [diff] [review]
introspect addonsdir for xpis instead of storing them on the instance

r+ if we include the small patch; we can followup with the tbpl status and crash elsewhere.

Comment 33

[Jeff Hammel]

pushed with aki's follow-up: http://hg.mozilla.org/build/mozharness/rev/508ec5a03b42

I'll file follow-ups for the talos bug and the tbpl status and then close this bug

Comment 34

[Jeff Hammel]

https://bugzilla.mozilla.org/show_bug.cgi?id=753530
and 
https://bugzilla.mozilla.org/show_bug.cgi?id=753526
respectively.

I'm fine with any of the follow-ups posted in comment 27, but will close and call this part done unless there are any objections.