1258451 - `mach try -p win` should fail as there is no 'win' platform, only 'win32' and 'win64'

Status: REOPENED

(Developer Infrastructure :: Try, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Running `mach try -p win` will happily push a patch to tryserver requesting builds for a platform that doesn't exist (the correct value would be either 'win32' or 'win64').

We should bail out early, and possibly suggest close alternatives such as "Did you want 'win32' or 'win64'?"

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.

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

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8759040 [details]
Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.

https://reviewboard.mozilla.org/r/57126/#review54260

Thank you for the patch. Just a couple of questions/suggestsions.

::: testing/tools/autotry/autotry.py:25
(Diff revision 1)
> +            print 'Invalid choice {v!r}'.format(v=value)
> +            sys.exit(1)
> +
> +class ValidatePlatforms(argparse.Action):
> +    def __call__(self, parser, args, values, option_string=None):
> +        choices = ['linux', 'linux', 'linux64-asan', 'linux64-st-an', 'linux64-valgrind', 'linux64-haz', 'macosx64', 'macosx64-st-an', 'win32', 'win64', 'android-api-9', 'android-api-15', 'android-api-15-gradle-dependencies', 'android-api-15-frontend', 'android-x86', 'sm-arm-sim', 'sm-compacting', 'sm-generational', 'sm-plain', 'sm-rootanalysis', 'sm-warnaserr']

Some way to get the line length under control and format these by groups would be nice, as this seems like something that will need to be updated manually in the future.

::: testing/tools/autotry/autotry.py:31
(Diff revision 1)
> +        validate_choices(values, choices)
> +        setattr(args, self.dest, values)
> +
> +class ValidateUnittestSuites(argparse.Action):
> +    def __call__(self, parser, args, values, option_string=None):
> +        choices = ['none', 'reftest', 'reftest-1', 'reftest-2', 'reftest-3', 'reftest-4', 'reftest-e10s', 'reftest-no-accel', 'crashtest', 'crashtest-e10s', 'xpcshell', 'jsreftest', 'marionette', 'marionette-e10s', 'mozmill', 'cppunit', 'gtest', 'firefox-ui-functional', 'firefox-ui-functional-e10s', 'jittests', 'jittest-1', 'jittest-2', 'luciddream', 'mochitests', 'web-platform-tests', 'robocop-1', 'robocop-2', 'robocop-3', 'robocop-4', 'plain-reftest-1', 'plain-reftest-2', 'plain-reftest-3', 'plain-reftest-4', 'plain-reftest-5', 'plain-reftest-6', 'plain-reftest-7', 'plain-reftest-8', 'plain-reftest-9', 'plain-reftest-10', 'plain-reftest-11', 'plain-reftest-12', 'plain-reftest-13', 'plain-reftest-14', 'plain-reftest-15', 'plain-reftest-16', 'jsreftest-1', 'jsreftest-2', 'jsreftest-3', 'jsreftest-4', 'jsreftest-5', 'jsreftest-6', 'mochitest-1', 'mochitest-2', 'mochitest-3', 'mochitest-4', 'mochitest-5', 'mochitest-6', 'mochitest-7', 'mochitest-8', 'mochitest-9', 'mochitest-10', 'mochitest-11', 'mochitest-12', 'mochitest-13', 'mochitest-14', 'mochitest-15', 'mochitest-16', 'mochitest-17', 'mochitest-18', 'mochitest-19', 'mochitest-20', 'mochitest-chrome', 'mochitest-gl-1', 'mochitest-gl-2', 'mochitest-gl-3', 'mochitest-gl-4', 'mochitest-gl-5', 'mochitest-gl-6', 'mochitest-gl-7', 'mochitest-gl-8', 'mochitest-gl-9', 'mochitest-gl-10', 'mochitest-media-1', 'mochitest-media-2', 'crashtest-1', 'crashtest-2', 'crashtest-3', 'crashtest-4', 'xpcshell-1', 'xpcshell-2', 'xpcshell-3', 'autophone-smoketest', 'autophone-s1s2', 'autophone-webapp', 'autophone-mochitest-dom-browser-element', 'autophone-mochitest-dom-media', 'autophone-mochitest-skia', 'autophone-mochitest-toolkit-widgets']

This looks like it's missing a few things, like individual web-platform-tests chunks and e10s mochitest chunks. http://trychooser.pub.build.mozilla.org/ may be a reasonable place to start, although even that list isn't comprehensive, as I'm looking at it I think there are some taskluster only jobs that aren't included.

::: testing/tools/autotry/autotry.py:49
(Diff revision 1)
> -    parser.add_argument('-p', '--platform', dest='platforms', action='append',
> +    parser.add_argument('-p', '--platform', dest='platforms', action=ValidatePlatforms, type=csv,
>                          help='Platforms to run (required if not found in the environment as AUTOTRY_PLATFORM_HINT).')
> -    parser.add_argument('-u', '--unittests', dest='tests', action='append',
> +    parser.add_argument('-u', '--unittests', dest='tests', action=ValidateUnittestSuites, type=csv,
>                          help='Test suites to run in their entirety.')
> -    parser.add_argument('-t', '--talos', dest='talos', action='append',
> +    parser.add_argument('-t', '--talos', dest='talos', action=ValidateTalosSuites, type=csv,

We already do our own parsing of these inputs in `normalize_list` in testing/mach_commands.py. Hooking this into those checks might work out a bit better.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8759040 [details]
Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57126/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

I added in the missing individual chunks of web-platform-test and mochitest. The lists in the patch are all from trychooser.

I didn't merge this with /testing/mach_commands.py as the ArgumentParser appeared to be a better approach to doing this though if you disagree I can refactor this.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8759040 [details]
Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.

https://reviewboard.mozilla.org/r/57126/#review56778

::: testing/tools/autotry/autotry.py:25
(Diff revision 2)
> +            print 'Invalid choice {v!r}'.format(v=value)
> +            sys.exit(1)
> +
> +class ValidatePlatforms(argparse.Action):
> +    def __call__(self, parser, args, values, option_string=None):
> +        choices = ['linux', 'linux', 'linux64-asan', 'linux64-st-an',

'linux' repeated, it looks like one of these needs to be 'linux64'.

::: testing/tools/autotry/autotry.py:102
(Diff revision 2)
> -    parser.add_argument('-p', '--platform', dest='platforms', action='append',
> +    parser.add_argument('-p', '--platform', dest='platforms', action=ValidatePlatforms, type=csv,
>                          help='Platforms to run (required if not found in the environment as AUTOTRY_PLATFORM_HINT).')
> -    parser.add_argument('-u', '--unittests', dest='tests', action='append',
> +    parser.add_argument('-u', '--unittests', dest='tests', action=ValidateUnittestSuites, type=csv,
>                          help='Test suites to run in their entirety.')
> -    parser.add_argument('-t', '--talos', dest='talos', action='append',
> +    parser.add_argument('-t', '--talos', dest='talos', action=ValidateTalosSuites, type=csv,

`add_argument` takes a `choices` argument. Is there a reason you didn't use that?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Chris Manchester (:chmanchester) from comment #5)
> `add_argument` takes a `choices` argument. Is there a reason you didn't use
> that?

The `choices` argument doesn't allow combining of choices through a comma-separated list, such as -p linux64,win32

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to (Away 6/25-7/4) Jared Wein [:jaws] (please needinfo? me) from comment #6)
> (In reply to Chris Manchester (:chmanchester) from comment #5)
> > `add_argument` takes a `choices` argument. Is there a reason you didn't use
> > that?
> 
> The `choices` argument doesn't allow combining of choices through a
> comma-separated list, such as -p linux64,win32

Oh, right.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8759040 [details]
Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.

https://reviewboard.mozilla.org/r/57126/#review56790

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c407d7f3ba96
Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites. r=chmanchester

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

So `mach try -p all` is not valid anymore?

Comment 13

[Chris Manchester (limited bugmail, email directly)]

(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #12)
> So `mach try -p all` is not valid anymore?

Oh, I think that's just an oversight. I'll add that in a fixup.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34ff89e8bba
Fixup to make `-p all` an acceptable platform selection in mach try. r=me

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[James Graham [:jgraham]]

So, this will cause |mach try| to be broken every time we add a new platform or, more likely, testsuite. Not breaking in that way was an explicit goal, so I think we should reconsider this patch.

Comment 17

[Steve Fink [:sfink] [:s:]]

For the record, I'm not at all fond of having a hardcoded list for a set of things that is changing constantly, and contains stuff partially duplicated elsewhere in the tree anyway (the taskcluster job names are under taskcluster/). We already have constant mismatches between actual available jobs, the trychooser web UI, and the trychooser hg extension. This just adds another thing to get out of sync.

At the very very least, ./mach try should have a -f (--force) flag to push with the given syntax anyway. Especially since it's not infrequent for me to want to push with -p asdfasdf when testing releng infrastructural things.

I wouldn't mind a blacklist approach, if we scanned past pushes for things that have been tried and failed.

I found this bug because I told someone to push with linux64-shell-haz, and it's missing.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

I think a good middle ground here will be feeding the list from taskcluster definitions, and living with the out-of-syncness of buildbot things, as adding things to buildbot becomes less and less common. And if this is missing things from there today the patch probably needs to be backed out if this fix isn't trivial.

You should be able to use `-p none` instead of `-p asdfasdf`, for what it's worth.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Backed out the two patches here in

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbdee00e87c17c2bfcca22a0a5df6776bab4f2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/38ac7e2815b3ba688bd9b49fda912c6cc7f316f2