Closed Bug 1200293 Opened 4 years ago Closed 4 years ago

mach target for mozregression


(Testing :: mozregression, defect)

Not set


(firefox43 fixed)

Tracking Status
firefox43 --- fixed


(Reporter: wlach, Assigned: parkouss)




(2 files, 2 obsolete files)

A few days ago we discussed adding a "mach" frontend to mozregression.

16:18	ahal	parkouss: wlach: would it make sense to have a mach frontend to mozregression?
16:19	ahal	(for discoverability mainly)
16:19	wlach	ahal: I'm unsure. the nice thing about mozregression is that it's useable by non-developers, so we'd want to keep that at least
16:19	wlach	ahal: I suppose it would be a time saver to have it already set up in the mach environment
16:19	ahal	mach commands can pip install things
16:20	ahal	so you could still keep mozregression as is
16:20	ahal	but also have a very barebones mach wrapper around it
16:20	wlach	ahal: yep
16:20	wlach	ahal: I think this would be more a question for platform developers than me
16:20	Ms2ger	Sounds like a good iea
16:20	Ms2ger	idea
16:20	ahal	yeah, I was wondering how many people know about mozregression
16:20	wlach	they do seem to have trouble installing mozregression and keeping it up to date

While I think we should continue maintaining the standalone version, but I think for Mozilla developers an-always-up-to-date copy of mozregression in-tree would be a godsend for usability and accesibility.

I don't have time to work on this myself right now, but CC'ing two people who might be interested in implementation (parkouss and mikeling) and one person who knows more about what would be involved in writing such a thing (ahal).
Mach commands have access to a virtualenv manager that can install packages from pypi, see here for an example:
Well, this is a bit complicated for the options parsing, because we should add a parser attribute to the @Command decorator - but we can not get the parser from mozregression before installing it in the virtualenv. :(

Is there a way to add lazily argument parsing using mach ? I mean something like "./mach mozregression" would first install it in virtualenv, and after that we could have the parser.

I'm against parsing code duplication, in case someone ask. :)

Another possibility I can think of is trying to import mozregression. If it does not exists, print a command to install it (or just install it). If the import works, then we have the parser and we can just use it to generate the command line. Not sure how this will work with the virtualenv provided by mach, because looking at the code in comment 1 this is done inside a mach call (so we have the same problem as described above).
Yeah, that's tricky. The parser argument to the @Command decorator accepts a function that returns an ArgumentParser object, so yes it is possible to lazy load parsers. But you won't have access to self.virtualenv_manager in that context :/. I guess you could instantiate a new virtualenv_manager in that function and do the pip install there.
I looked more into that. It seems that the parser function that can be given in @Command is called before the MachCommandBase constructor, but this constructor give us some interesting paths (src and obj dir) that are useful to activate the env (or instantiate a new virtualenv_manager).

We had a discussion on irc with :ahal, and we should investigate if resolving bug 985141 can help us here.
See Also: → 985141
Bug 985141 would solve the problem, but it is a lot of work.

It should be possible to pass additional context to the parser function. You can make this backwards compatible by using inspect.getargspec to see how many arguments a function accepts. If it accepts arguments, you know it supports the new-style support for passing contexts. Now, argument parsing does happen before command dispatch. So things might be a bit wonky. e.g. the virtualenv magic is attached to MozbuildObject, which is inherited by MachCommandBase, which is the base class for all mach commands interfacing with the build system. Fortunately, it is trivial to instantiate a VirtualenvManager [1]. Unfortunately, that's a layering violation to put in a mach.* module because VirtualenvManager is part of the mozbuild package. So, what you need is for the parser function context to receive sufficient arguments to import and instantiate a VirtualenvManager. That means topsrcdir and topobjdir. And topobjdir likely means importing mozbuild.base and calling MozbuildObject.from_environment(). At which point you can just call obj.virtualenv_manager to get a handle on a VirtualenvManager.

I hope this makes sense.

This totally make sense, thanks :gps!
This is the mozregression side of the change - this basically add a mach_interface module to provide the required mach integration API.
Assignee: nobody → j.parkouss
Attachment #8655716 - Flags: review?(wlachance)
Attached patch 1200293.patch (obsolete) — Splinter Review
This is the mach side patch. This should only be landed once mozregression has been released with the above change, and we would require that to fully test anyway.

Still if you do "pip install -e /patch/to/mozregression" using the branch that contains the above patch in the objdir virtualenv you can make "mach mozregression" works.
Attachment #8655727 - Flags: review?(ahalberstadt)
Comment on attachment 8655716 [details] [review]
prepare mozregession for mach integration

Looks good! Thanks.
Attachment #8655716 - Flags: review?(wlachance) → review+
Comment on attachment 8655727 [details] [diff] [review]

Review of attachment 8655727 [details] [diff] [review]:

This is pretty much an r+, but there are a few changes and I wouldn't mind seeing the final patch once more.

::: tools/
@@ +389,5 @@
> +def mozregression_create_parser():
> +    # Create the mozregression command line parser.
> +    # if mozregression is not installed, or not up to date, it will
> +    # first be installed.
> +    cmd = MozbuildObject.from_environment()

A bit of a shame that it requires a build_obj, but good enough for now.

@@ +412,5 @@
> +            # mozregression is up to date, return the parser.
> +            return mozregression.parser()
> +    # exit if we updated or installed mozregression because
> +    # we may have already imported mozregression and running it
> +    # as this may cause issues.

You can reload the module without leaving the interpreter, see

@@ +418,5 @@
> +
> +
> +@CommandProvider
> +class MozregressionCommand(MachCommandBase):
> +    @Command('mozregression',

I think the main new consumers of this command will be the ones who don't know what mozregression is. I'd call it something like 'bisect-regression' or 'find-regression'.. that way people will know right away what it's for. You could still mention mozregression in the description if you wanted.

@@ +419,5 @@
> +
> +@CommandProvider
> +class MozregressionCommand(MachCommandBase):
> +    @Command('mozregression',
> +             category='ci',

I saw gps' comment on irc yesterday.. but this doesn't really have anything to do with ci or testing. I agree with your initial idea, I think it should go in Potpourri. It's kind of a miscellaneous tool.
Attachment #8655727 - Flags: review?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Comment on attachment 8655727 [details] [diff] [review]
> 1200293.patch

> @@ +418,5 @@
> > +
> > +
> > +@CommandProvider
> > +class MozregressionCommand(MachCommandBase):
> > +    @Command('mozregression',
> I think the main new consumers of this command will be the ones who don't
> know what mozregression is. I'd call it something like 'bisect-regression'
> or 'find-regression'.. that way people will know right away what it's for.
> You could still mention mozregression in the description if you wanted.

I don't think that's necessarily true: there's a lot of developers who know what mozregression is and have used it, but would benefit from having it in mach because it means they no longer need to worry about pip installing it (which requires all sorts of python knowledge, especially when things go wrong).

I'd personally prefer to keep the naming as it is, mozregression has a pretty strong brand that I'd like to take advantage of. `find-regression` or `bisect-regression` could mean almost anything.
A few points:

I know about reload, but this is dangerous, in the docs you can see some caveats:

When a module is reloaded, its dictionary (containing the module’s global variables) is retained. Redefinitions of names will override the old definitions, so this is generally not a problem. If the new version of a module does not define a name that was defined by the old version, the old definition remains. This feature can be used to the module’s advantage if it maintains a global table or cache of objects

Now if I reload one module 1 that previously have loaded a module 2, this module 2 won't be reloaded - so we are calling old code, and that can break. I don't want to maintain a list of what should be reloaded, I think it is a good option to just ask to retype command.

Else we could use something like or implement our reload mechanism, don't know if that worst the cost.

Hm, for the name I agree with :wlach, I like mozregression since it is mozregression that is called. It would be easier for users to look over internet for help about mozregression for example. And I'm a bit concerned that bisect-regression or find-regression, because it looks like git bisect or hg bisect and this is not the same thing since we are operating on prebuilt binaries, not source files.

For the category, Pot pourri is fine for me!
Attached patch 1200293.patch (obsolete) — Splinter Review
So I just released mozregression 1.0.0 which have the required code to make that patch works. :)

I only fixed the category to potpourri here, not sure if something else was required. :ahal, please tell me if something is not good!
Attachment #8655727 - Attachment is obsolete: true
Attachment #8657453 - Flags: review?(ahalberstadt)
Comment on attachment 8657453 [details] [diff] [review]

Review of attachment 8657453 [details] [diff] [review]:

Alright, wfm!
Attachment #8657453 - Flags: review?(ahalberstadt) → review+
Attached patch 1200293.patchSplinter Review
fixed one typo that prevented mozregression updates (tried just now). So carrying r+ from previous review.
Attachment #8657453 - Attachment is obsolete: true
Attachment #8658304 - Flags: review+
Checkin needed for Attachment #8658304 [details] [diff] - this does not require a try as this is mach interface only.

Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.