897370 - Use mozrunner from mozbase upstream

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[nodeless]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Jetpack uses an outdated mozrunner that has some bugs (the one I hit was flash freezing, bug 768651. The new mozrunner fixes it). It should be replaced by up-to-date packages from mozbase.

Comment 1

[nodeless]

https://github.com/mozilla/addon-sdk/pull/1133

This replaces mozrunner with the up-to-date mozrunner (and its dependencies) from mozbase.

The first commit contains the verbatim copy of mozbase code (manifestparser, mozfile, mozinfo, mozprocess, mozprofile, mozrunner), and the changes to addon-sdk to integrate with it.

The next commit adds a feature to mozprofile that is needed, and the commit after that has various bug fixes for mozprocess. Both of these commits should also be pushed upstream to mozbase (I have the full commits with testcases for mozbase; I can open bugs/patches against mozbase).

It has been tested on Windows 7 x64 and Debian with "cfx testall -b "C:\Program Files (x86)\Nightly\firefox.exe" -o -v". Nightly 25.0a1 2013-07-19 was used. Output from the tests are attached as "after_linux.txt" and "after_win32.txt". Output from the addon-sdk master branch is attached as "before_linux.txt" and "before_win32.txt". I compared this output and didn't notice any appreciable differences.

My strategy for getting the upstream code into this repo was just to copy the six needed packages. The other option would be to use git submodule to include the mozbase repo, but I decided against it because there's a lot of stuff in that repo that isn't needed by jetpack.

Comment 2

[nodeless]

Attachment: after_win32.txt

Comment 3

[nodeless]

Attachment: before_win32.txt

Comment 4

[nodeless]

Attachment: after_linux.txt

Comment 5

[nodeless]

Attachment: before_linux.txt

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: pullrequest.html

Not sure who would be the best person to review this pull request, Dave. Giving it to you so you can pass it on to the right one. :)

Comment 7

[Dave Townsend [:mossop]]

This is AWESOME. I've been hoping we could find the time to do this for ages.

(In reply to nodeless from comment #1)
> https://github.com/mozilla/addon-sdk/pull/1133
> 
> This replaces mozrunner with the up-to-date mozrunner (and its dependencies)
> from mozbase.
> 
> The first commit contains the verbatim copy of mozbase code (manifestparser,
> mozfile, mozinfo, mozprocess, mozprofile, mozrunner), and the changes to
> addon-sdk to integrate with it.
> 
> The next commit adds a feature to mozprofile that is needed, and the commit
> after that has various bug fixes for mozprocess. Both of these commits
> should also be pushed upstream to mozbase (I have the full commits with
> testcases for mozbase; I can open bugs/patches against mozbase).

I'm probably not a good reviewer for those changes, can you open those bugs against mozbase and see if they will take the changes? What are they necessary for?

> It has been tested on Windows 7 x64 and Debian with "cfx testall -b
> "C:\Program Files (x86)\Nightly\firefox.exe" -o -v". Nightly 25.0a1
> 2013-07-19 was used. Output from the tests are attached as "after_linux.txt"
> and "after_win32.txt". Output from the addon-sdk master branch is attached
> as "before_linux.txt" and "before_win32.txt". I compared this output and
> didn't notice any appreciable differences.

For most users you don't need the -b argument, mozrunner will just autodetect the Firefox binary. I've been told that that feature got removed from mozrunner after we forked it though, can you verify whether that is the case?

> My strategy for getting the upstream code into this repo was just to copy
> the six needed packages. The other option would be to use git submodule to
> include the mozbase repo, but I decided against it because there's a lot of
> stuff in that repo that isn't needed by jetpack.

Sounds fine to me.

Comment 8

[nodeless]

(In reply to Dave Townsend (:Mossop) from comment #7)
> This is AWESOME. I've been hoping we could find the time to do this for ages.
> 
> (In reply to nodeless from comment #1)
> > https://github.com/mozilla/addon-sdk/pull/1133
> > 
> > This replaces mozrunner with the up-to-date mozrunner (and its dependencies)
> > from mozbase.
> > 
> > The first commit contains the verbatim copy of mozbase code (manifestparser,
> > mozfile, mozinfo, mozprocess, mozprofile, mozrunner), and the changes to
> > addon-sdk to integrate with it.
> > 
> > The next commit adds a feature to mozprofile that is needed, and the commit
> > after that has various bug fixes for mozprocess. Both of these commits
> > should also be pushed upstream to mozbase (I have the full commits with
> > testcases for mozbase; I can open bugs/patches against mozbase).
> 
> I'm probably not a good reviewer for those changes, can you open those bugs
> against mozbase and see if they will take the changes? What are they
> necessary for?

The bugs in mozbase were all in mozprocess. It doesn't correctly handle stdout redirection, and there were some issues with its timeout code and win32 process handling. The commit message for that commit is:

"
    Modify processOutput() to behave correctly when stdout is redirected

    - Add test that redirects stdout and checks for expected output.

    - Adjust prochandler.c to have consistent output across windows and linux.

    - Add test that redirects stdout and runs process, but timeouts before
    completion.

    - Raise ValueError if outputTimeout is set when stdout is redirected.

    - processOutput() of processHandler.py now behaves correctly when stdout is
    redirected. Before it would throw an exception on Windows, and immediately
    close the process on linux.

    - With the above change, when a process runs longer than
    MAX_IOCOMPLETION_PORT_NOTIFICATION_DELAY seconds, the _processOutput() thread
    would crash on wait() with exception OSError.  Changed the _wait()
    implementation for win32 to wait indefinitely.

    - Add test for above item (test process running longer than MAX_...).
"

The other commit involves mozprofile. They made a change that installs addons to the "extensions/staged" dir, but addon-sdk needs them to go into "extensions/" because of bug 854937.

Should I push these two commits to upstream to mozbase now, or should I wait for this to get r+?

> > It has been tested on Windows 7 x64 and Debian with "cfx testall -b
> > "C:\Program Files (x86)\Nightly\firefox.exe" -o -v". Nightly 25.0a1
> > 2013-07-19 was used. Output from the tests are attached as "after_linux.txt"
> > and "after_win32.txt". Output from the addon-sdk master branch is attached
> > as "before_linux.txt" and "before_win32.txt". I compared this output and
> > didn't notice any appreciable differences.
> 
> For most users you don't need the -b argument, mozrunner will just
> autodetect the Firefox binary. I've been told that that feature got removed
> from mozrunner after we forked it though, can you verify whether that is the
> case?

Looks like that's true. Should that functionality be kept? We could pull it out of https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/__init__.py#L406 and wrap mozrunner with it. I wonder what the motivations were for removing it though?

> > My strategy for getting the upstream code into this repo was just to copy
> > the six needed packages. The other option would be to use git submodule to
> > include the mozbase repo, but I decided against it because there's a lot of
> > stuff in that repo that isn't needed by jetpack.
> 
> Sounds fine to me.

Comment 9

[Dave Townsend [:mossop]]

(In reply to nodeless from comment #8)
> (In reply to Dave Townsend (:Mossop) from comment #7)
> Should I push these two commits to upstream to mozbase now, or should I wait
> for this to get r+?

I'd prefer to see these changes land in mozbase first, then we can just take clean copies into the SDK repo.

> > > It has been tested on Windows 7 x64 and Debian with "cfx testall -b
> > > "C:\Program Files (x86)\Nightly\firefox.exe" -o -v". Nightly 25.0a1
> > > 2013-07-19 was used. Output from the tests are attached as "after_linux.txt"
> > > and "after_win32.txt". Output from the addon-sdk master branch is attached
> > > as "before_linux.txt" and "before_win32.txt". I compared this output and
> > > didn't notice any appreciable differences.
> > 
> > For most users you don't need the -b argument, mozrunner will just
> > autodetect the Firefox binary. I've been told that that feature got removed
> > from mozrunner after we forked it though, can you verify whether that is the
> > case?
> 
> Looks like that's true. Should that functionality be kept? We could pull it
> out of
> https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/
> __init__.py#L406 and wrap mozrunner with it. I wonder what the motivations
> were for removing it though?

Yeah I think we want to keep it. I don't know why it was ever removed but if upstream doesn't want it then extending it ourselves seems like the best option.

Comment 10

[nodeless]

(In reply to Dave Townsend (:Mossop) from comment #9)
> (In reply to nodeless from comment #8)
> > (In reply to Dave Townsend (:Mossop) from comment #7)
> > Should I push these two commits to upstream to mozbase now, or should I wait
> > for this to get r+?
> 
> I'd prefer to see these changes land in mozbase first, then we can just take
> clean copies into the SDK repo.

I pushed them both to mozbase. See bug 897890 and bug 897908.

> > > > It has been tested on Windows 7 x64 and Debian with "cfx testall -b
> > > > "C:\Program Files (x86)\Nightly\firefox.exe" -o -v". Nightly 25.0a1
> > > > 2013-07-19 was used. Output from the tests are attached as "after_linux.txt"
> > > > and "after_win32.txt". Output from the addon-sdk master branch is attached
> > > > as "before_linux.txt" and "before_win32.txt". I compared this output and
> > > > didn't notice any appreciable differences.
> > > 
> > > For most users you don't need the -b argument, mozrunner will just
> > > autodetect the Firefox binary. I've been told that that feature got removed
> > > from mozrunner after we forked it though, can you verify whether that is the
> > > case?
> > 
> > Looks like that's true. Should that functionality be kept? We could pull it
> > out of
> > https://github.com/mozilla/addon-sdk/blob/master/python-lib/mozrunner/
> > __init__.py#L406 and wrap mozrunner with it. I wonder what the motivations
> > were for removing it though?
> 
> Yeah I think we want to keep it. I don't know why it was ever removed but if
> upstream doesn't want it then extending it ourselves seems like the best
> option.

I found bug 703646 which has the discussion. Based on that, it doesn't sound like they would want it, so I pulled out the find_binary functionality and put it into the cuddlefish runner. I updated the pull request with this change.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 780242 [details]
pullrequest.html

r+ for the runner.py changes and to land those along with the upstream mozbase bits once those have the required changes in.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Marking as blocker as it blocks a whole set of Perf-related stuff.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

I want us to take another look at this.

Comment 15

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Mossop: how important is this wrt mochitestification?

Comment 16

[Dave Townsend [:mossop]]

(In reply to Jeff Griffiths (:canuckistani) from comment #15)
> Mossop: how important is this wrt mochitestification?

Not important at all

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry we won't be releasing any new versions of cfx, jpm is the replacement https://www.npmjs.com/package/jpm

I also see that npm is used on tbpl now, so I assume we can use jpm on tbpl eventually.