1139461 - Uplift mozbase changes to mozilla-b2g32_v2_0

Status: RESOLVED DUPLICATE of bug 1101497

(Testing :: Mozbase, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

In order to uplift bug 1101497 we need some patches uplifted for mozbase packages. Rather that painstakingly going through dozens of patches, Will suggested that we could just do a single patch that brings the branch up to date.

It's certainly worth a try...

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift mozbase changes to mozilla-b2g32_v2_0

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e9ffa0164a

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift mozbase changes to mozilla-b2g32_v2_0

My previous patch only included updated files, this one also contains new files.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e355b98ce3bf

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift mozdevice, mozlog, and mozversion changes to mozilla-b2g32_v2_0

I don't think it's going to work uplifting all mozbase changes, so this patch just takes mozdevice, mozlog, and mozversion changes. This should cover what's needed to unblock the dependent bug.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b86aeb2bfcd

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm getting confusing results in my try run. For example, Linux opt is failing because there's no package named manifestparser. This is true on mozilla-b2g32_v2_0 as the package was renamed from ManifestDestiny to manifestparser, however it appears that this is passing on the branch builds... Am I doing something wrong with my try run?

Comment 6

[Andrew Halberstadt [:ahal]]

I'm not sure which part is confusing, in your last try run there's no manifestparser module, so that error makes sense to me:
https://hg.mozilla.org/try/file/2bbc4c5c8863/testing/mozbase

You'll at least need to rename manifestdestiny->manifestparser. Or remove manifestdestiny and copy the full manifestparser module over.

Comment 7

[Andrew Halberstadt [:ahal]]

Note at least these files also need to be updated:
https://hg.mozilla.org/try/file/2bbc4c5c8863/testing/mozbase/packages.txt
https://hg.mozilla.org/try/file/2bbc4c5c8863/testing/mozbase/Makefile.in#l12
https://hg.mozilla.org/try/file/2bbc4c5c8863/testing/mozbase/test-manifest.ini#l11
https://hg.mozilla.org/try/file/2bbc4c5c8863/build/mach_bootstrap.py#l61

I could be missing some.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The part that's confusing is that these builds are passing on the mozilla-b2g32_v2_0 branch on buildbot: https://treeherder.mozilla.org/#/jobs?repo=mozilla-b2g32_v2_0&revision=6054cd825c71

Somehow that's succeeding in installing manifestparser even though it shouldn't exist on that branch. My patch doesn't introduce a need for manifestparser...

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8573199 [details] [diff] [review]
Uplift mozdevice, mozlog, and mozversion changes to mozilla-b2g32_v2_0

According to Ryan the Try failures could be explained by:

RyanVM|sheriffduty: my only thought is a mozharness-buildbot rev mismatch
RyanVM|sheriffduty: since we use in-tree pointers now
RyanVM|sheriffduty: there could be a change that was made that explicitly ignores b2g32 but gets hit on Try

Ed suggested pushing and watching the results, backing out if any bustage is caused.

I'm not expecting this patch to be reviewed in depth. I basically updated my local mozilla-central repository and copied the mozdevice, mozlog, and mozversion directories into my local mozilla-b2g32_v2_0 repository, and then added all the new/updated files to this patch.

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8573199 [details] [diff] [review]
Uplift mozdevice, mozlog, and mozversion changes to mozilla-b2g32_v2_0

r+ on the approach, I didn't look at the patch itself

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G test bustage.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3fd54fefcb7a

https://treeherder.mozilla.org/logviewer.html#?job_id=107481&repo=mozilla-b2g32_v2_0

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The uplift failed because the patch for bug 997244 modified both mozdevice and the XPCShell tests. The question is, do I modify the XPCShell tests in my uplift? The alternative of uplifting the individual commits isn't appealing. There would appear to be >60 commits for mozdevice, >80 for mozlog, and >20 for mozversion. Hoping wlach has a suggestion. :)

Comment 13

[William Lachance (:wlach)]

(In reply to Dave Hunt (:davehunt) from comment #12)
> The uplift failed because the patch for bug 997244 modified both mozdevice
> and the XPCShell tests. The question is, do I modify the XPCShell tests in
> my uplift? The alternative of uplifting the individual commits isn't
> appealing. There would appear to be >60 commits for mozdevice, >80 for
> mozlog, and >20 for mozversion. Hoping wlach has a suggestion. :)

It's pretty difficult to see exactly what's going on here as so many changes were wrapped together, but maybe try just replacing device.getTestRoot() with device.deviceRoot (note not a function call anymore). Really the changes in mozdevice shouldn't affect xpcshell materially, I think you might just need to adapt to some changes we made in the API. Sorry for the confusion / difficulty.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

My mistake, it was bug 1036530 that changed this area. I'll update the patch to address the remote XPCShell tests and request review.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift mozbase changes to mozilla-b2g32_v2_0

Revisiting uplifting all mozbase changes as doing just sub-directories means the docs would be out of sync. This has been taken from mozilla-central at revision 58c9d079f318.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update XPCShell tests to be compatible with uplifted mozbase

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Giving Try another chance:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e8f1a76916e

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

So now it looks like bug 1044657 made a change to moz.build and some associated files, and uplifting these has caused bustage in my Try run. I'll try just excluding these mozbase changes. The Mulet build is failing for a different reason, but I wonder if this is meant to build for this branch - I don't see it on the Treeherder results for the branch.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Revert mozbuild changes from uplift

Reverting mozbuild changes. Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=487becbbcf3f

Comment 20

[William Lachance (:wlach)]

Comment on attachment 8576611 [details] [diff] [review]
Update XPCShell tests to be compatible with uplifted mozbase

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

The xpcshell changes look fine, but what's with all the other modifications? r+ with either those removed or a good explanation. :)

Comment 21

[William Lachance (:wlach)]

Comment on attachment 8576624 [details] [diff] [review]
Revert mozbuild changes from uplift

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

Please merge all this into one previous patch before landing.

Comment 22

[William Lachance (:wlach)]

Comment on attachment 8576610 [details] [diff] [review]
Uplift mozbase changes to mozilla-b2g32_v2_0

Fine by me as long as it doesn't break anything

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to William Lachance (:wlach) from comment #20)
> Comment on attachment 8576611 [details] [diff] [review]
> The xpcshell changes look fine, but what's with all the other modifications?
> r+ with either those removed or a good explanation. :)

These were the other changes made to that file in the patch from bug 1036530. I wanted to keep the file as much in sync as possible, however I can certainly experiment with removing these changes.

(In reply to William Lachance (:wlach) from comment #21)
> Comment on attachment 8576624 [details] [diff] [review]
> Please merge all this into one previous patch before landing.

I can do that, but I thought it might be better to keep them separate. Happy to do whatever is preferred, but will keep the patches separate at least for review.

(In reply to William Lachance (:wlach) from comment #22)
> Comment on attachment 8576610 [details] [diff] [review]
> Fine by me as long as it doesn't break anything

Latest try run is failing because of the manifestdestiny -> manifestparser rename. I'll continue working on patches and try runs but will hold off requesting further reviews until try results look good.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I think I'm at the point of giving up on this. The latest try results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=813cf5a7e672 and while a lot of the builds are now completing, tests are failing due to a manifestparser import (which isn't making a lot of sense to me) or due to a change in mozrunner that affects Marionette.

Krupa: Before I invest too much more time on this, please could you indicate how important it is to continue running the Marketplace tests on Firefox OS v2.0. At present this branch is not capable of using our remote device lab, which means we'll lose coverage once we shut down out Mountain View lab.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Okay, I have a new approach. I've uplifted the changes from bug 1101655, bug 1102140 (including other uplifts as needed), and bug 1101497. It's working well locally (on a remote device), and I've kicked off a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec5e332f6323

I think I'll probably roll all of these changes up into a single patch for review, and if needed will repeat the exercise for v2.1 in a separate bug.

Clearing needinfo and I've spoken to Krupa, and understand the need to get these running on a released version of Firefox OS.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

There were still some issues related to some of the uplifts preceding bug 1102140 so I went back to the original patch and directly uplifted the changes. Here's the new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d21147ce324

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

RyanVM: Mind having a look over https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d21147ce324 to see if there's anything to be concerned about?

Will: Would you prefer uplift patches for the three associated bugs, a combined patch on this bug, or a combined patch on bug 1101497?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

LGTM

Comment 29

[William Lachance (:wlach)]

I don't think it matters hugely, but a combined patch sounds good to me.

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Closing this in favour of a combined uplift patch on bug 1101497. Thanks all!