Open Bug 1258451 Opened 4 years ago Updated 11 months ago

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

Categories

(Firefox Build System :: Try, defect)

defect
Not set

Tracking

(firefox50 fixed)

REOPENED
Tracking Status
firefox50 --- fixed

People

(Reporter: jaws, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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'?"
Component: Build Config → General
Product: Core → Testing
See Also: → 1277127
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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.
Attachment #8759040 - Flags: review?(cmanchester)
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/
Attachment #8759040 - Attachment description: MozReview Request: Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites. r?chmanchester → Bug 1258451 - Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites.
Attachment #8759040 - Flags: review?(cmanchester)
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 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?
Attachment #8759040 - Flags: review?(cmanchester)
(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
(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 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
Attachment #8759040 - Flags: review+
Attachment #8759040 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c407d7f3ba96
Add a whitelist to 'mach try' for platforms, unittest suites, and talos suites. r=chmanchester
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c407d7f3ba96
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
So `mach try -p all` is not valid anymore?
Flags: needinfo?(cmanchester)
(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.
Flags: needinfo?(cmanchester)
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b34ff89e8bba
Fixup to make `-p all` an acceptable platform selection in mach try. r=me
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.
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.
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.
Blocks: 1283163
Assignee: jaws → nobody
Component: General → Try
Product: Testing → Firefox Build System
Target Milestone: mozilla50 → ---
You need to log in before you can comment on or make changes to this bug.