651974 - tracking bug for mozharness 0.4

Status: RESOLVED FIXED

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

Description

[Aki Sasaki (not active)]

Mozharness is kind of in the grey area between useful and not.
Let's make it useful.

Some things in my todo list that may be good for this:

* Moar unit tests
** Windows unit tests green
* turn on/off actions in config file
* BaseScript.initial_config ?
* logrotate, copy_to_upload_dir
* deal with upload_dir clobber timing issue
* revisit summaries
* error context lines <- may be in 0.5+

We also need some useful scripts that leverage the above.

Bug 650882 (port hgtool to mozharness) and bug 650887 (desktop talos runner) seem like good candidates.  Hgtool will be useful for any mercurial actions; the talos runner will create the framework for future mozharness unittest/talos work.

Comment 1

[Aki Sasaki (not active)]

(In reply to comment #0)
> * Moar unit tests

Now at 82% coverage.  The bulk of the missing tests require scratchbox.

> ** Windows unit tests green

As of http://hg.mozilla.org/users/asasaki_mozilla.com/talosrunner/rev/2f96864b96fc , unit.sh passes on msys on XP as well as Snow Leopard.  I'll doublecheck RHEL before I'm done.

> * turn on/off actions in config file
> * BaseScript.initial_config ?
> * logrotate, copy_to_upload_dir
> * deal with upload_dir clobber timing issue
> * revisit summaries
> * error context lines <- may be in 0.5+

Still need to do these.  0.4 is a good target for some; 0.5 for others.

> Bug 650882 (port hgtool to mozharness)

Done, and tested on OSX and Windows.  I've asked Rail and Catlee for feedback, which may be a while since a) they're busy and b) it's a huge patch and things are still changing.

> and bug 650887 (desktop talos runner)
> seem like good candidates.

Still targeting this.

I thought of bug 498425 (mozmill release update tests); that may wait til 0.5.
I also thought of continuing the talosrunner work to include SUT remote talos (bug 650890) since we'd like to get some Real World benefits sooner rather than later.

Comment 2

[Aki Sasaki (not active)]

https://github.com/escapewindow/mozharness/tree/0.4

When I diff the 0.4 branch against master, or when I

 hg clone http://hg.mozilla.org/build/mozharness
 cd mozharness
 rm -rf *
 cp ../git-mozharness/* .
 # also deal w/ .hgtags and .hgignore
 hg addremove
 hg diff | wc -l

I've got a ~6500 line diff, and I'm not done yet.

Thinking about getting some of the low-impact high-line-count diffs in beforehand to get that number down.

Comment 3

[Aki Sasaki (not active)]

sut talosrunner takes precedence over desktop talosrunner due to its current instability.

Comment 4

[Aki Sasaki (not active)]

Attachment: trimmed down 5475 line patch

More info available at https://github.com/escapewindow/mozharness/blob/0.4/CHANGES .

* added .gitignore -- how much github stuff should land in hg.m.o ?

* added CHANGES

* bumped version

* added MPL to files

* OptionGroup -- mainly to make --help more readable/parsable.

* Got rid of ExtendedOptionParser.variables hack

* Started raising exceptions instead of deciding whether to error/fatal in situations where the python script is at fault, rather than the user.

* --only-ACTION is a little verbose/ugly; allowed for and eventually will move exclusively to --ACTION

* Split out various mixins to avoid massive objects with duplication of code
** LogMixin
** OSMixin
** ShellMixin

* Added a VirtualEnvMixin to use/create virtualenvs

* Added list capability to run_command()

* Refactored get_output_from_command()

* Created sourcetool with ported hgtool code from build/tools... slightly out of date.
** vcsbase/sourcetool are written to [hopefully] be able to plug in whatever VCS mixin behind the scenes and Just Work.
** updated all scripts to use vcsbase + new MercurialScript.

* Partially written MozmillUpdate script -- we can punt on this for 0.4, since the bug is blocked by internal-mirrors-only-for-staging.

* Ported hgtool tests into mozharness tests for sourcetool

* Updated unit.sh to be quieter, allow for testing on linux+win32 as well as osx
** Split tests out to non-networked and networked. Will remove/pare down the networked unit tests later.

* Changed location of upload_dir, log_dir; added copy_to_upload_dir() with log rotation (needs tests)
** default work_dir is now ./build/
** default upload_dir is now ./build/upload_dir/
** default log_dir is now ./logs/; copied to ./build/upload_dir/logs/

* BaseScript._pre_config_lock()

Comment 5

[Aki Sasaki (not active)]

Comment on attachment 559917 [details] [diff] [review]
trimmed down 5475 line patch

Tag!
I know this is a huge patch; let me know if you have questions or want to go over this with me. Also tagging Lukas.

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 559917 [details] [diff] [review]
trimmed down 5475 line patch

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

Part 1. I reviewed the changes made in mozharness directory only.

::: mozharness/base/config.py
@@ +223,5 @@
> +         type="string", help="Specify the config file (required)"
> +        )
> +
> +        # Logging
> +        log_option_group = OptionGroup(self.config_parser, "Logging")

Using OptionGroup makes the help message nicer!

::: mozharness/base/log.py
@@ +81,5 @@
> +                                            exit_code=exit_code)
> +        if level == 'info':
> +            if self._log_level_at_least(level):
> +                self._print(message)
> +        elif level == 'debug':

I think it would be safer to use something like
DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL = ('debug', 'info', 'warning', 'error', 'critical', 'fatal')
and compare against variables:
if level == DEBUG
At least typos will be eliminated.

::: mozharness/base/python.py
@@ +79,5 @@
> +    def create_virtualenv(self):
> +        c = self.config
> +        if not c.get('virtualenv_path'):
> +            self.add_summary("No virtualenv specified; not creating virtualenv!", level="warning")
> +            return -1

Why not raise an exception here? It also valid for some other places.

::: mozharness/base/script.py
@@ +168,5 @@
> +                shutil.move(src, dest)
> +            # http://docs.python.org/tutorial/errors.html
> +            except IOError as (errno, strerror):
> +                self.log("IO error({0}): {1}".format(errno, strerror),
> +                         level=error_level, exit_code=exit_code)

This shouldn't work in python-2.5, JFYI.

@@ +474,5 @@
> +        self.config.lock()
> +
> +    def _possibly_run_method(self, method_name, error_if_missing=False):
> +        if hasattr(self, method_name) and callable(getattr(self, method_name)):
> +            return getattr(self, method_name)()

Do you think that it would be better to add args=() and kwargs={} as parameters and expand them while calling method_name? something like getattr(self, method_name)(*args, **kwargs).

@@ +504,5 @@
> +                method_name = action.replace("-", "_")
> +                self.action_message("Running %s step." % action)
> +                self._possibly_run_method("preflight_%s" % method_name)
> +                self._possibly_run_method(method_name, error_if_missing=True)
> +                self._possibly_run_method("postflight_%s" % method_name)

I liked pre and post methods. It makes it more flexible.

@@ +633,5 @@
> +                        self.move(os.path.join(dest_dir, dest_file, '.%d' % backup_num),
> +                                  os.path.join(dest_dir, dest_file, '.%d' % backup_num +1))
> +                if self.move(dest, "%s.1" % dest):
> +                    self.log("Unable to move %s!" % dest, level=error_level)
> +                    return -1

Why not raise an exception here and above where return -1 used?

::: mozharness/base/vcs/mercurial.py
@@ +166,5 @@
> +        self.info("%s." % msg)
> +        if revision is not None:
> +            cmd = ['hg', 'update', '-C', '-r', revision]
> +            self.run_command(cmd, cwd=dest, error_list=HgErrorList)
> +        else:

if you set both branch and revision, branch will be ignored. Please add this to the docstring or prevent setting both values.

@@ +179,5 @@
> +
> +            self.run_command(cmd, cwd=dest, error_list=HgErrorList)
> +        return self.get_revision_from_path(dest)
> +
> +    def clone(self, repo, dest, branch=None, revision=None, update_dest=True):

The same here and some other places.

Random question, maybe for brainstorming: why not use Mercurial's Python module for some/most operations? it may be faster and should eliminate output parsing.

Comment 7

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] from comment #6)
> Part 1. I reviewed the changes made in mozharness directory only.

Thanks!

> ::: mozharness/base/config.py
> @@ +223,5 @@
> > +         type="string", help="Specify the config file (required)"
> > +        )
> > +
> > +        # Logging
> > +        log_option_group = OptionGroup(self.config_parser, "Logging")
> 
> Using OptionGroup makes the help message nicer!

Yeah, I was finding the --help output a little unreadable before.

> I think it would be safer to use something like
> DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL = ('debug', 'info', 'warning',
> 'error', 'critical', 'fatal')

Done: https://github.com/escapewindow/mozharness/commit/79a6b082b85f6a7744709980590a004b96c4e508

> ::: mozharness/base/python.py
> @@ +79,5 @@
> > +    def create_virtualenv(self):
> > +        c = self.config
> > +        if not c.get('virtualenv_path'):
> > +            self.add_summary("No virtualenv specified; not creating virtualenv!", level="warning")
> > +            return -1
> 
> Why not raise an exception here? It also valid for some other places.

I like exceptions for developers but not very much for users.

In many places, I can set the error_level of a call, and if I really want it to throw I can set the error_level to 'fatal'.  Otherwise I can look at the return value.

Determining where to throw exceptions and where to show error messages is a bit tricky, but not hard to change. This error might be best at a fatal level, f/e.

> ::: mozharness/base/script.py
> @@ +168,5 @@
> > +                shutil.move(src, dest)
> > +            # http://docs.python.org/tutorial/errors.html
> > +            except IOError as (errno, strerror):
> > +                self.log("IO error({0}): {1}".format(errno, strerror),
> > +                         level=error_level, exit_code=exit_code)
> 
> This shouldn't work in python-2.5, JFYI.

Hm, how should I do it?

    except IOError, e:
        self.log("IO error: %s" % str(e), level=error_level, exit_code=exit_code)

?

> @@ +474,5 @@
> > +        self.config.lock()
> > +
> > +    def _possibly_run_method(self, method_name, error_if_missing=False):
> > +        if hasattr(self, method_name) and callable(getattr(self, method_name)):
> > +            return getattr(self, method_name)()
> 
> Do you think that it would be better to add args=() and kwargs={} as
> parameters and expand them while calling method_name? something like
> getattr(self, method_name)(*args, **kwargs).

Possibly?
So far _possibly_run_method is only used in self.run(), which doesn't send any arguments... everything's in self.config.  So currently nothing needs args/kwargs.

> @@ +633,5 @@
> > +                        self.move(os.path.join(dest_dir, dest_file, '.%d' % backup_num),
> > +                                  os.path.join(dest_dir, dest_file, '.%d' % backup_num +1))
> > +                if self.move(dest, "%s.1" % dest):
> > +                    self.log("Unable to move %s!" % dest, level=error_level)
> > +                    return -1
> 
> Why not raise an exception here and above where return -1 used?

Same as above. If we want to ignore/continue on errors we can ignore the return code. If we want to catch it, we can look at the return code. If we want to halt, we can set error_level to 'fatal'.

It's just a different way of doing it that doesn't involve wrapping everything in try/except.

> ::: mozharness/base/vcs/mercurial.py
> @@ +166,5 @@
> > +        self.info("%s." % msg)
> > +        if revision is not None:
> > +            cmd = ['hg', 'update', '-C', '-r', revision]
> > +            self.run_command(cmd, cwd=dest, error_list=HgErrorList)
> > +        else:
> 
> if you set both branch and revision, branch will be ignored. Please add this
> to the docstring or prevent setting both values.

Ok.  https://github.com/escapewindow/mozharness/commit/014a5c7a827c911c93ff1d4b44dcd7b591e682ca
I actually copied this from http://hg.mozilla.org/build/tools/file/edca76df663e/lib/python/util/hg.py#l79 :)

> @@ +179,5 @@
> > +
> > +            self.run_command(cmd, cwd=dest, error_list=HgErrorList)
> > +        return self.get_revision_from_path(dest)
> > +
> > +    def clone(self, repo, dest, branch=None, revision=None, update_dest=True):
> 
> The same here and some other places.

Done; original here http://hg.mozilla.org/build/tools/file/edca76df663e/lib/python/util/hg.py#l100

> Random question, maybe for brainstorming: why not use Mercurial's Python
> module for some/most operations? it may be faster and should eliminate
> output parsing.

Possibly.

For:

* I pretty much just ported hgtool over without a lot of thought.
* Official module would remove a lot of custom code here.

Against:

* I like keeping the number of prereqs to the minimum.
* We had a "port hgtool to mozharness" bug; this is the result of that work. If we prefer the Mercurial python module over hgtool, then I'm fine going that direction.
* This route makes it very easy to drop in a GitVCS without changing very much in the scripts.

Comment 8

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #7)
> (In reply to Rail Aliiev [:rail] from comment #6)
> > ::: mozharness/base/python.py
> > @@ +79,5 @@
> > > +    def create_virtualenv(self):
> > > +        c = self.config
> > > +        if not c.get('virtualenv_path'):
> > > +            self.add_summary("No virtualenv specified; not creating virtualenv!", level="warning")
> > > +            return -1
> > 
> > Why not raise an exception here? It also valid for some other places.
> 
> I like exceptions for developers but not very much for users.
> 
> In many places, I can set the error_level of a call, and if I really want it
> to throw I can set the error_level to 'fatal'.  Otherwise I can look at the
> return value.
> 
> Determining where to throw exceptions and where to show error messages is a
> bit tricky, but not hard to change. This error might be best at a fatal
> level, f/e.

Changed this warning to a FATAL:

https://github.com/escapewindow/mozharness/commit/871f40660f849a58ec6f1935d910aa074117f66a

So it'll exit the script, but since this is most likely a user-caused error, it won't give an ugly stack dump.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 559917 [details] [diff] [review]
trimmed down 5475 line patch

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

In overall the patch looks good.

Please check the files against trailing spaces. I saw some of them in the hg diff output.

::: scripts/mozmill_updates.py
@@ +88,5 @@
> +     {"action": "store",
> +      "dest": "mozmill_url",
> +      "default": "http://pypi.python.org/packages/source/m/mozmill/mozmill-1.5.4b6.tar.gz#md5=ac0b0710f90012991e8cc54cf01d1010",
> +      "help": "Specify the mozmill pip url"
> +     }

At some point we'll need these URLs be internal. What do you think about using a list of URLs here? The first will be pointing to the internal server and the others to external servers (with fallback). In this case this code will also work for devs and community. Or should those URLs go to the configs?

::: scripts/sourcetool.py
@@ +152,5 @@
> +     {"action": "store",
> +      "dest": "vcs_shared_dir",
> +      "default": os.environ.get('HG_SHARE_BASE_DIR'),
> +      "help": "clone to a shared directory"
> +     }

IIRC, git uses --reference and --shared for this purpose.

Comment 10

[Armen [:armenzg]]

Comment on attachment 559917 [details] [diff] [review]
trimmed down 5475 line patch

I won't hold this off with rail's review.

Comment 11

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] from comment #9)
> In overall the patch looks good.

Yay!

> Please check the files against trailing spaces. I saw some of them in the hg
> diff output.

=\
I hate trailing whitespace; not sure why vim isn't highlighting it.
I'll fix that and try to get my .vimrc updated.

> ::: scripts/mozmill_updates.py
> @@ +88,5 @@
> > +     {"action": "store",
> > +      "dest": "mozmill_url",
> > +      "default": "http://pypi.python.org/packages/source/m/mozmill/mozmill-1.5.4b6.tar.gz#md5=ac0b0710f90012991e8cc54cf01d1010",
> > +      "help": "Specify the mozmill pip url"
> > +     }
> 
> At some point we'll need these URLs be internal. What do you think about
> using a list of URLs here? The first will be pointing to the internal server
> and the others to external servers (with fallback). In this case this code
> will also work for devs and community. Or should those URLs go to the
> configs?

I think the default in the script should be developer oriented.
The config files we point the scripts to can specify

  'mozmill_url': "http://internal.url/to/package"

per branch or however we want to lay out the config files.

> ::: scripts/sourcetool.py
> @@ +152,5 @@
> > +     {"action": "store",
> > +      "dest": "vcs_shared_dir",
> > +      "default": os.environ.get('HG_SHARE_BASE_DIR'),
> > +      "help": "clone to a shared directory"
> > +     }
> 
> IIRC, git uses --reference and --shared for this purpose.

Added https://bugzilla.mozilla.org/show_bug.cgi?id=671345#c1 :)

I'll merge + kill trailing whitespace + test before landing on mozharness default.
Thanks again Rail!

Comment 12

[Aki Sasaki (not active)]

http://www.vim.org/scripts/script.php?script_id=3201 in my ~/.vim/

:)

Comment 13

[Aki Sasaki (not active)]

Comment on attachment 559917 [details] [diff] [review]
trimmed down 5475 line patch

http://hg.mozilla.org/build/mozharness/rev/2091ae4c21ec

Comment 14

[Aki Sasaki (not active)]

Backed out; broken hg share from hgtool port is breaking Android multilocale.