1272414 - Ditch browser/b2g/qemu filtering in marionette manifests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Comment 1

[:Gijs (he/him)]

I could be convinced to make this work only for true|True|TRUE|false|False|FALSE, but this seems like it follows "simplest thing that could possibly work".

Comment 2

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests should work irrespective of case, r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/52233/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52233/

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8751828 [details]
MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests should work irrespective of case, r?chmanchester

https://reviewboard.mozilla.org/r/52233/#review49291

I tried this on top of the issue in bug 888624, and it doesn't address that issue. Marionette is using a part of the manifest parser api that doesn't evaluate values in sections as expressions, it's just comparing strings.

Either way, I think this patch is a little heavy handed. Ideally marionette would use manifest filters like skip-if, evaluate them like other harnesses do, and fail on something like "browser = True" because True is an unknown identifier.

Comment 4

[:Gijs (he/him)]

(In reply to Chris Manchester (:chmanchester) from comment #3)
> Comment on attachment 8751828 [details]
> MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests
> should work irrespective of case, r?chmanchester
> 
> https://reviewboard.mozilla.org/r/52233/#review49291
> 
> I tried this on top of the issue in bug 888624, and it doesn't address that
> issue. Marionette is using a part of the manifest parser api that doesn't
> evaluate values in sections as expressions, it's just comparing strings.
> 
> Either way, I think this patch is a little heavy handed. Ideally marionette
> would use manifest filters like skip-if, evaluate them like other harnesses
> do, and fail on something like "browser = True" because True is an unknown
> identifier.

OK. It seems the skip-ifs are already honoured ( https://dxr.mozilla.org/mozilla-central/search?q=skip-if%20path%3Amanifest.ini&=mozilla-central&redirect=true ).

Could we basically just convert the "b2g" and "browser" and "qemu" stuff to things that evaluate as expressions we could use for skip-if, and then replace the browser/qemu/b2g stuff in the manifest files with skip-ifs/run-ifs? Is that what you're suggesting or were you suggesting something else? Would this be acceptable to the marionette folks?

Comment 5

[:Gijs (he/him)]

(FWIW, I would personally still be happier if the standard skip-if notation, too, would understand "True" and "TRUE" and so on, but if that's not what other people want, we can keep it as-is...)

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Yes, I think that would be reasonable. If we like the current annotations, possibly we could translate them to skip-if expressions and pass them to a manifestparser filter in the harness. However, the supported parameters look useful mostly for enabling/disabling tests on b2g, so a more extensive update might be appropriate.

Comment 7

[David Burns :automatedtester]

I am ok with doing that. Those are hangovers from Marionette starting out as a B2G only tool and growing. Since we are switching off B2G related things and slowly removing b2g-isms from the runner we should totally do this.

Comment 8

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2de8567733e4

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1272414 - rip out qemu/browser/b2g flags from marionette manifests, r?AutomatedTester

Review commit: https://reviewboard.mozilla.org/r/53994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53994/

Comment 10

[David Burns :automatedtester]

Comment on attachment 8754489 [details]
MozReview Request: Bug 1272414 - rip out qemu/browser/b2g flags from marionette manifests, r?AutomatedTester

https://reviewboard.mozilla.org/r/53994/#review50768

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3ccec9b45d

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f3ccec9b45d

Comment 13

[Mark Banner (:standard8)]

FTR I also ported/landed the loop affecting part in the Loop repo:

https://github.com/mozilla/loop/commit/6616599dd0edde58bd8e4799a4d84918915334d1

Comment 14

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #13)
> FTR I also ported/landed the loop affecting part in the Loop repo:
> 
> https://github.com/mozilla/loop/commit/
> 6616599dd0edde58bd8e4799a4d84918915334d1

As noted on IRC at the time, if this gets uplifted into repos that don't have this cset, then your tests might not run on infra.