827446 - update mochitest, reftest, xpcshell to use mozcrash

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

mozcrash is now at 0.3 and should be used for all harnesses.  We need to update a few files to make this happen.

Comment 1

[Andrew Halberstadt [:ahal]]

The tricky(ish) part will be how to import mozcrash. When everything is running in mozharness this won't be a problem, but in the meantime we either need to use makefiles to copy mozcrash.py to the tests.zip or figure out a way to get buildbot to activate a virtualenv from the copy of mozbase that's already in tests.zip.

I think doing the latter would go a long way toward our effort of refactoring everything on top of mozbase but it's probably outside the scope of this bug.

Comment 2

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> The tricky(ish) part will be how to import mozcrash. When everything is
> running in mozharness this won't be a problem, but in the meantime we either
> need to use makefiles to copy mozcrash.py to the tests.zip or figure out a
> way to get buildbot to activate a virtualenv from the copy of mozbase that's
> already in tests.zip.
> 
> I think doing the latter would go a long way toward our effort of
> refactoring everything on top of mozbase but it's probably outside the scope
> of this bug.

OTOH, we'll have to do this some time.  I find I spend more time working around work-arounds

Comment 3

[(not currently active) Ted Mielczarek]

Well, mozharness is the longer-term solution, and it's running on cedar. aki told me it could be running on mozilla-central this quarter, but I don't know whether that's actually happening.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: use mozcrash instead of automationutils.checkForCrashes (1.0)

This allows us to pass the build time checks (leaktest), reftest, mochitest and xpcshell.  Once we have a solid virtualenv to work with, we can remove the sys.path.insert(...) logic at the top of the files.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 713351 [details] [diff] [review]
use mozcrash instead of automationutils.checkForCrashes (1.0)

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

::: build/Makefile.in
@@ +115,5 @@
>  _LEAKTEST_DIR = $(DEPTH)/_leaktest
>  GARBAGE_DIRS += $(_LEAKTEST_DIR)
>  
> +_MOZBASE_DIR = $(topsrcdir)/testing/mozbase
> +GARBAGE_DIRS += $(_MOZBASE_DIR)

This doesn't make any sense. You're asking the build system to remove the directory from the srcdir.

@@ +158,4 @@
>  
>  libs:: $(_LEAKTEST_FILES)
>  	$(INSTALL) $^ $(_LEAKTEST_DIR)
> +	$(INSTALL) $(_MOZBASE_DIR) $(DEPTH)

This is unnecessary. The mozbase modules are all available in the virtualenv in the objdir, you shouldn't need to copy any files around to make things work in-tree.

::: build/automation.py.in
@@ +24,5 @@
>  sys.path.insert(0, SCRIPT_DIR)
>  import automationutils
>  
> +here = os.path.dirname(__file__)
> +mozbase = os.path.realpath(os.path.join(here, '..', 'mozbase'))

This is just to handle the packaged tests case? If so, you should file a bug about removing this code when we get mozharness for desktop tests, and put a TODO comment here with the bug number.

Also you might want to do something like:
try:
  import mozcrash
except ImportError:
  <all this path-fidling nonsense>
  import mozcrash

So that this code becomes a no-op if we have mozbase available in the virtualenv.

@@ +31,5 @@
> +        'mozlog']
> +for dep in deps:
> +    module = os.path.join(mozbase, dep)
> +    if module not in sys.path:
> +        sys.path.insert(0, module)

This is kind of weird, it only checks if this exact string path is already in sys.path.

Comment 6

[Jeff Hammel]

> @@ +31,5 @@
> > +        'mozlog']
> > +for dep in deps:
> > +    module = os.path.join(mozbase, dep)
> > +    if module not in sys.path:
> > +        sys.path.insert(0, module)
> 
> This is kind of weird, it only checks if this exact string path is already in sys.path.

I'm not exactly sure why this is weird.  If we're using realpath, then it should be canonical.  I would change insert() to append() but that's not a big deal.

> Also you might want to do something like:
> try:
>   import mozcrash
> except ImportError:
>   <all this path-fidling nonsense>
>   import mozcrash

I'm a big +1 on this, maybe with a comment even.  I don't know how far we're going to get down this path before we can dedicate ourselves to the path we really want to file, but far enough, I'm sure, that there will be no small amount of cleanup. Anything to make it easier is greatly appreciated.

Comment 7

[Jeff Hammel]

Comment on attachment 713351 [details] [diff] [review]
use mozcrash instead of automationutils.checkForCrashes (1.0)

Pretty much "what ted said" as he gave a more thorough review than I was likely to anyway ;) I'd also be inclined to add comments.

+here = os.path.dirname(__file__)
+mozbase = os.path.realpath(os.path.join(here, '..', 'mozbase'))

I vaguely recall but could be completely wrong that there were circumstances that os.path.dirname(__file__) would fail and it needed to be os.path.dirname(os.path.realpath(__file__)).  But if it works as is, I probably don't care.

I would also tend to do os.path.join(os.path.dirname(here), 'mozbase')) instead of the '..' although it is more typing.  But not important.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mozcrash integration while using make leaktest (2.0)

I have updated this patch with all the feedback and review comments from before.  This passes on all platforms on try server.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 723920 [details] [diff] [review]
mozcrash integration while using make leaktest (2.0)

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

::: build/automation.py.in
@@ +1148,4 @@
>      return foundZombie
>  
>    def checkForCrashes(self, profileDir, symbolsPath):
> +    return mozcrash.check_for_crashes(os.path.join(profileDir, "minidumps"), symbolsPath, test_name=self.lastTestSeen)

When we fix the virtualenv thing, we can just remove this method entirely and have callers call mozcrash.check_for_crashes directly, right?

::: build/automationutils.py
@@ -132,4 @@
>                      help = "prevents the test harness from redirecting "
>                          "stdout and stderr for interactive debuggers")
>  
> -def checkForCrashes(dumpDir, symbolsPath, testName=None):

Did you check to see that we ported all of edmorley's changes to this method to mozcrash?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for the quick feedback!

I verified mozcrash on github has all the latest changes from automationutils.py.

For the shim function call, we would need to modify all the callers of it to import mozcrash and then call mozcrash directly.  The one thing we would need to keep track of better in the harness would be test_name=self.lastTestSeen (stored in the Automation object).

Comment 11

[Jeff Hammel]

So this does not require a new mirroring of mozcrash from github?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

in my try run it detect and reported a crash successfully, we are mirroring all of mozbase to m-c this week anyway, so this works as is and any future changes would be rolled in this week if they are not already there.

Comment 13

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #12)
> in my try run it detect and reported a crash successfully, we are mirroring
> all of mozbase to m-c this week anyway, so this works as is and any future
> changes would be rolled in this week if they are not already there.

If we are mirroring all of it, this is the first I've heard of it.  I've been working on bug 838374 , which is a subset of packages.  Want me to add mozcrash there?

Comment 14

[Jeff Hammel]

Comment on attachment 723920 [details] [diff] [review]
mozcrash integration while using make leaktest (2.0)

Not a thorough audit of all that depends on it, but LGTM

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a02276859d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=20601166&tree=Mozilla-Inbound

make[2]: Leaving directory `/builds/slave/m-in-l64-pgo-00000000000000000/build/obj-firefox'
rm -f obj-firefox/jarlog/en-US.log
MOZ_PGO_INSTRUMENTED=1 OBJDIR=obj-firefox JARLOG_FILE=obj-firefox/jarlog/en-US.log python obj-firefox/_profile/pgo/profileserver.py 10
Traceback (most recent call last):
  File "obj-firefox/_profile/pgo/profileserver.py", line 18, in <module>
    from automation import Automation
  File "/builds/slave/m-in-l64-pgo-00000000000000000/build/obj-firefox/_profile/pgo/automation.py", line 35, in <module>
    import mozcrash
ImportError: No module named mozcrash
make[1]: Leaving directory `/builds/slave/m-in-l64-pgo-00000000000000000/build'
make[1]: *** [profiledbuild] Error 1
make: *** [build] Error 2

Comment 16

[Ed Morley [:emorley]]

Also, judging by:
{
06:29:45  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/workers/test/test_fileReadSlice.xul | Exited with code -1073741819 during test run
06:29:57     INFO -  mozcrash ERROR | PROCESS-CRASH | chrome://mochitests/content/chrome/dom/workers/test/test_fileReadSlice.xul | application crashed [@ xul.dll + 0x22f6a97]
06:29:57     INFO -  PROCESS-CRASH | chrome://mochitests/content/chrome/dom/workers/test/test_fileReadSlice.xul | application crashed [@ xul.dll + 0x22f6a97]
06:30:03    ERROR - Return code: 1
}

...the in-tree mozcrash is further behind upstream mozcrash than I realised - which will impact starring ability considerably (notably it's missing the fix for bug 828324). 

Please can the relanding of this wait until we're mirrored mozcrash into m-c again? :-)

Comment 17

[Andrew Halberstadt [:ahal]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> Please can the relanding of this wait until we're mirrored mozcrash into m-c
> again? :-)

Jeff is working on that in bug 838374 and hopes to have it landed by end of week.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7cc921857de

Nice shot kid, but don't get cocky.

Comment 19

[Ed Morley [:emorley]]

(In reply to Andrew Halberstadt [:ahal] from comment #17)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> > Please can the relanding of this wait until we're mirrored mozcrash into m-c
> > again? :-)
> 
> Jeff is working on that in bug 838374 and hopes to have it landed by end of
> week.

That sadly didn't land before this after all, which means TBPL starring is now partly broken, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20983129&tree=Mozilla-Inbound

Comment 20

[y-ii]

still get ther same error in comment15.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Can you explain how you got to the error?  This is working in automation and in various other local scenarios.

Comment 22

[y-ii]

(In reply to Joel Maher (:jmaher) from comment #21)
> Can you explain how you got to the error?  This is working in automation and
> in various other local scenarios.

when build pgo version, error came from the second "import mozcrash". it seems to me that the path is wrong, since automation.py is in ${PGO_OBJDIR}/_profile/pgo/,while mozcrash is in  $(TOPSRCDIR)/testing/mozbase

try:
    import mozcrash
except:
    deps = ['mozcrash',
            'mozlog']
    for dep in deps:
        module = os.path.join(mozbase, dep)
        if module not in sys.path:
            sys.path.append(module)
    import mozcrash

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

which operating system are you using and which product are you building?

Comment 24

[(not currently active) Ted Mielczarek]

It's supposed to be pulling mozcrash in from the virtualenv in the objdir. If you're running "python .../profileserver.py" directly then that's not going to work anymore. You'll need to use the make target like the official mozconfigs:
http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/win32/nightly#4

Comment 25

[y-ii]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #24)
> It's supposed to be pulling mozcrash in from the virtualenv in the objdir.
> If you're running "python .../profileserver.py" directly then that's not
> going to work anymore. You'll need to use the make target like the official
> mozconfigs:
> http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/
> win32/nightly#4

i build firefox on win8 with the mozconfig incluing the profileserver.py as PROFILE_GEN_SCRIPT. maybe the problem is caused by --disable-tests

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

can you paste the exact error you get?   I want to make sure you are using the python from the virtualenv created in the objdir instead of python from the system.

Comment 27

[y-ii]

(In reply to Joel Maher (:jmaher) from comment #26)
> can you paste the exact error you get?   I want to make sure you are using
> the python from the virtualenv created in the objdir instead of python from
> the system.

how to tell which one is used. And what's the difference.  

I used the old script "mk_add_options PROFILE_GEN_SCRIPT='$(PYTHON) $(MOZ_OBJDIR)/_profile/pgo/profileserver.py' " .

Comment 28

[(not currently active) Ted Mielczarek]

That's no longer correct, you need to use the line from the mozconfig I linked.

Also comment 19 got resolved, so this is fixed again.