1242083 - Support --setenv in try syntax to set environment variables

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Matthew N. [:MattN]]

For mochitest-browser-screenshots, the env. variable MOZSCREENSHOTS_SETS lets one pick which browser combinations to capture e.g. `MOZSCREENSHOTS_SETS=LightweightThemes,Tabs,WindowSize` and it would be convenient if developers could easily specify that in try syntax to get non-default screenshots.

There are some other environment variables used by our applications that could also use this feature.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76971f07ef86

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1242083 - Support --setenv in try syntax to set environment variables. r=chmanchester

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8711234 [details]
MozReview Request: Bug 1242083 - Support --setenv in try syntax to set environment variables. r=chmanchester

So the try push reminded me why this may not be ideal: the argument is passed to harnesses that give an error due to the unknown argument.

Should I implement a solution like --try-test-paths?

Comment 3

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/32035/#review29089

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:48
(Diff revision 1)
>      known_try_arguments = {
> +        '--setenv': {
> +            'action': 'append',
> +            'dest': 'environment',
> +            'default': [],
> +            "metavar": "NAME=VALUE",
> +        },

As you've found this will cause problems for harnesses that don't take this argument. It looks like it would be pretty straightforward to have an idea here of what flavors an argument is acceptable for, make harness_extra_args a dict mapping flavors to args, and then take arguments based on flavor in the try_args function.

How does that sound?

Comment 4

[Armen [:armenzg]]

MattN and I spoke about this to be possible for both try syntax as well as properties when scheduled out of band with mozci.

From looking at the code [1], TryToolsMixin is available for test jobs.
I will be reading tomorrow my comment in [2] and see what exactly we need to support properties for test jobs as well (since that comment was about talos).

This is just a heads up about our intentions.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/testbase.py#102
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1241535#c2

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Armen, comment 3 is all we need to do this from try syntax. Let me know if you'd like me to write the patch.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81a76cc88110

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Matt, can you confirm comment 6 has the expected effect?

Comment 8

[Matthew N. [:MattN]]

Yep!

> --setenv MOZSCREENSHOTS_SETS=LightweightThemes,Tabs,WindowSize

led to https://treeherder.mozilla.org/logviewer.html#?job_id=16306592&repo=try#L1501
> console.info: mozscreenshots: 3 sets: Array ["LightweightThemes","Tabs","WindowSize"]

Works great

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1242083 - Add --setenv to the arguments accepted by mozharness to pass to the harness command, add a way to specify flavors an argument applies to. r=armenzg

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

Comment 10

[Armen [:armenzg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff86f11f8f5

Comment 11

[Armen [:armenzg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62eac18c77fa

Comment 12

[Armen [:armenzg]]

Comment on attachment 8715931 [details]
MozReview Request: Bug 1242083 - Add --setenv to the arguments accepted by mozharness to pass to the harness command, add a way to specify flavors an argument applies to. r=armenzg

https://reviewboard.mozilla.org/r/33659/#review30451

wfm

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:53
(Diff revision 1)
> -        },
> +        }, ('marionette', 'xpcshell', 'mochitest', 'browser-chrome',

Could you please re-org the suites alphabetically?
It will make it easier in the long term what suites are applicable to each argument.
Perhaps listing one per line.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b059e04fa4

Comment 14

[Phil Ringnalda (:philor)]

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