Closed Bug 606963 Opened 14 years ago Closed 13 years ago

Port hgtools.py to mozHarness

Categories

(Release Engineering :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 650882

People

(Reporter: armenzg, Assigned: adrianp)

References

Details

(Whiteboard: [mozharness])

Attachments

(3 files, 1 obsolete file)

The original code is in:
http://hg.mozilla.org/build/tools/file/tip/buildfarm/utils/hgtool.py
and we want to port it to:
http://hg.mozilla.org/build/mozharness

adrianp already has ported most of it and has blogged about it:
http://blog.esmnetworks.com/mozharness/porting-hgtool-to-mozharness/
http://code.darkminds.org/hg/mozharness/file/tip/lib/vcs/mercurial.py
http://code.darkminds.org/hg/mozharness/file/tip/scripts/vcstool.py

adrianp could you also write some tests? up to you what makes it for your 0.1 release.

adrianp's plan is described in here:
http://zenit.senecac.on.ca/wiki/index.php/Mozharness#Project_Plan
It seems that there has been some work going on by our current intern in the last three weeks in bug 589885 where he extends the capabilities of hgtools.py (https://bugzilla.mozilla.org/attachment.cgi?id=485429).

I have also found some hg tests written in the tool's repository:
http://hg.mozilla.org/build/tools/file/tip/lib/python/test
which make sense to port to the mozharness repo (there are three examples to have a look at):
http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/file/tip/test

adrianp as you know I will be gone for a week so I won't have time to draw what the right path will be on which tests you should work on. I don't want to be duplicating any work going on to don't waste your time and will need some time to think about this.

Get a stab for your 0.1 wrt to tests just to get the feel of it but don't make it a big part of it as things are still in flux. Make sure at some point you know what the tests are doing and be able to run them. This could be part of your 0.1 or your 0.2 milestone depending on your own discernment of what it can get done and what cannot get done.
See Also: → 589885
Glad to see that a) you're working on this, and b) you've already gotten pretty far.

My main concern here is that MozTools() duplicates functionality in base.script.BaseScript but bypasses the rest of the harness.

The MercurialVCS class should probably be a mixin, similar to the existing MercurialMixin class:

http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/file/5839fd97ba92/lib/base/script.py#l386

also, you appear to have 2 MercurialVCS classes?

Let me know if you have difficulty inheriting BaseScript -- not sure if it's documented well enough or intuitive to others.
Comment on attachment 486139 [details] [diff] [review]
patch from 0.1.0 to 0.1.1

Already pointed out that lib/source/mercurial.py is still there :)

scripts/vcstool.py

>         [["--props-file"],
>          {"action": "extend",
>           "dest": "propsfile",
>           "type": "string",
>           "help": "Specify what properties file to use"
>         }],

I'm not 100% sure what this is for, but it looks to me like it's a parallel to the config file, which you can set by --config-file FILENAME or BaseConfig.initial_config_file=FILENAME.

Examples of these are in mozharness/configs/. Currently mozharness only supports json config files, but I'll add .py at some point.  mozharness/configs/ is by default in the search path for config files, so if you created a mozharness/configs/vcs/blah.json, you could |python script.py --config-file vcs/blah.json|

Also, the "action" here is "extend", which means it will be stored in a list after comma-joining-comma-splitting.  The intent of the extend action is to be able to --multi-option one,two --multi-option three --multi-option four,five and have self.config['multi_option'] set to ['one','two','three','four','five'].

I'm going to guess you want your action to be "store".

>         [["--revision"],
>          {"action": "extend",
>           "dest": "revision",
>           "type": "string",
>           "help": "Specify which revision to update to"
>         }],

Probably also a "store", unless you're looking for a list.

>         [["--vcs"],
>          {"action": "store",
>           "dest": "vcs",
>           "type": "string",
>           "help": "Specify which VCS to use"
>         }],

You can make this a store/string, but you could potentially also use a store/choice:

http://docs.python.org/library/optparse.html#optparse-standard-option-actions
http://hg.mozilla.org/users/asasaki_mozilla.com/mozharness/file/7a0f5a743e8d/lib/base/config.py#l197

>     if vcs is None:
>         sys.exit(-1)

How about self.fatal("Error message goes here") ?

>     switch_options = {
>         'revision': os.environ.get('HG_REV'),
>         'branch': os.environ.get('HG_BRANCH', 'default'),
>         'propsfile': os.environ.get('PROPERTIES_FILE'),
>         'tbox': bool(os.environ.get('PROPERTIES_FILE')),
>     }

These should go in the config file and/or commandline options.
In fact, you can specify a default to each of the config_options; why don't you

        [["--revision"],
         {"action": "extend",
          "dest": "revision",
          "type": "string",
          "default": os.environ.get('HG_REV'),
          "help": "Specify which revision to update to"
        }],

I'd argue with the environment variable name since you seem to want to make this a vcs-generic script, but that would be cleaner.

Alternately, if you specify your default (which is now in os.environ.get('HG_REV')) in your config file instead, you can use that as your default.

>     # since we can't modify vcs.config directly (lock)

Any time you're trying to get around this lock, you're probably doing things wrong.  It's strict, it's possibly a pain, it forces you to think about configuration first and config-driven behavior later.  I try to think of ways around it as well, and bend the rules sometimes.  But all in all, in my entire career, I've never been unhappy to know exactly what to expect from a script if I look at the config file + command line options.  I can't say the same about mid-script configuration changes.

>         try:
>             import json
>         except ImportError:
>             import simplejson as json
>         
>         pfile = "".join(switch_options.get('propsfile'))
>         js = json.load(open(pfile))

Yup, looks like this is --config-file propsfile.

>             print "TinderboxPrint: <a href=\"%(url)s\">revision: %(got_revision)s</a>" % locals()
>         else:
>             print "TinderboxPrint: revision: %s" % got_revision

The TinderboxPrints should probably stay as print commands.
The other print's should probably be self.info(message).

Actually, wrapper.info() since you don't have a self -- should all of the stuff in __main__ here go into the wrapper.run()?

(You should also probably respect self.config['base_work_dir'] and self.config['work_dir'], but I'll leave that for later.)

lib/vcs/mercurial.py

> """Mercurial VCS class"""
> import sys
> import os
> 
> import re
> 
> # ..lib
> #sys.path.insert(1, os.path.join(os.path.dirname(sys.path[0])))
> #from base.script import BaseScript
> 
> # MercurialVCS {{{1
> class MercurialVCS(object):
>     
>     # the vcs binary
>     vcs_command = 'hg'
>     wrapper = None
>     
>     def __init__(self, wrapper_obj=None):
>         """This class is supposed to be independant from vcstool so if no wrapper(+BaseScript)
>         is passed, create it and store it in self.wrapper
>         
>         Otherwise grab the object passed by VCSWrapper and vcstool"""
>         if wrapper_obj is None:
>             sys.path.insert(1, os.path.join(os.path.dirname(sys.path[0])))
>             from base.script import BaseScript
>             self.wrapper = BaseScript()
>         else:
>             self.wrapper = wrapper_obj
>     
>     def test(self):
>         """This should probably not make production"""
>         print self.wrapper.getOutputFromCommand('ping 127.0.0.1')

:)

So, I'm fine with this stuff being in here for testing purposes.
At the end of the day this should be a Mixin with tests, and a script or
MercurialScript object that inherits this and BaseScript (much like the
existing MercurialMixin and MercurialScript)

>         if os.path.exists(dest):
>             self.wrapper.rmtree(dest)

Maybe a method option to not clobber an existing dir.
Fine leaving that as a TODO item, though.

>         str_command = " ".join(cmd)
>         self.wrapper.runCommand(str_command)

All non-trivial runCommands should have an error_list at some point.
There's an HgErrorList in errors.py that you can use for now... I'm not sure how complete it is, but it's a start.

You may also want to check the return status.

I'm not actually checking your Mercurial logic, just trying to give an overview of the mozharness usage here for this pass.

lib/base/vcswrapper.py

> sys.path.insert(1, os.path.join(os.path.dirname(sys.path[0])))

Btw, sorry for these... already thinking about how to not need this.

>     @self.supported.vcs - tuple - a list of supported VCS"""
>     supported_vcs = ('hg',)

Aha, so the config_options here could specify --vcs as a choice, since we already have a hardcoded tuple here.

I probably missed some things, but I hope that's helpful.
Whiteboard: [mozharness]
See http://code.darkminds.org/hg/mozharness/rev/adc55ff11772 for changeset details

Aki, thank you for your help so far
Looks like you're getting it.
2 quick points, w/out getting too into detail:

1) (small one) I'm not sure I'm a big fan of $PROPERTIES_FILE driving things, but I'm fine with that being in there for now.  Mozharness does have a default of localconfig.json; you can leverage that for now (this may change, though).

I'm ok with that sticking around for your testing, though.

2) (big one) I'm hoping this is early enough to say this.  It looks like you're grokking mozharness now, so I'm going to get a bit more into overall structure.  It does involve a significant rewrite, though, definitely enough to call it 0.2.0 (or w/e).

Everything in this is tied towards the wrapper object.
I'd much rather see these as very standalone objects or mixins that have the exact same API.  The wrapper is handy if companies switched VCSes every other day, but in my experience if these switch more than every few years, that's a very visible sign that the project is highly unstable.

However, these do change, and we don't want the change to be traumatic, as it usually is. Hence the same API (NEWVCS.checkout() should hopefully still behave similarly, f.e.).

So I suppose I'm asking for you to redesign to say, I'm writing a script that will be building against hg, so I'll leverage the mercurialVCS object, and if that project switches to git at some point, we can just find and replace mercurialVCS in the script with gitVCS and expect mostly good things.  No wrapper complexity needed.

Let me know if you want or need to discuss this or ask questions.  It seems like you're getting things, just trying to nudge you back in the direction I think this should go :)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: