Closed Bug 1382775 Opened 7 years ago Closed 7 years ago

Refactor |mach try| to support multiple different "trychoosers"

Categories

(Testing :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files)

I'm working on a new method of scheduling tasks on try that doesn't use try syntax. I'm also working on a few new "trychoosers" that will use this new method. However, I want to all of these trychoosers (including the existing autotry), to have the same |mach try| cli.

To accomplish this, we'll need to refactor the mach_commands.py behind |mach try| pretty substantially. Before I get into details, here are a few goals:

    * Be backwards compatible. This refactor should not change any autotry behaviour. In fact, developers should not even notice that it landed.

    * Make it easy to add new trychoosers. I want there to be N different interfaces that could conceivably exist at the same time. They should be distinct and not step on one another's toes.

    * Share common code between trychoosers. Some things, like the in-memory commit+push portion of |mach try|, will need to be shared across all trychoosers.

To accomplish this I have a plan of action. Not all steps are necessary, but while I'm doing a big scary refactor anyway, I figured might as well do some cleaup.

1. Create a new module under tools/tryselect
2. Move |mach try| command to tools/tryselect/mach_commands.py
3. Move testing/tools/autotry to tools/tryselect/selectors/autotry
4. Move all autotry specific logic in the mach_commands.py to autotry
5. Implement autotry as an @SubCommand, e.g: |mach try syntax|
6. Make |mach try| without subcommands dispatch to |mach try syntax|

Under this setup, new trychoosers can be added as subcommands to |mach try|. For example, I have a fuzzy finder trychooser under |mach try fuzzy|. Step 6 ensures we preserve backwards compatibility for now. In the future we may want to make a different trychooser be the default, but for now no point in rocking the boat.

I won't tackle adding new trychoosers in this bug. This is purely about refactoring the |mach try| mach_commands.py + module.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment on attachment 8888491 [details]
Bug 1382775 - Move testing/tools/autotry to tools/tryselect/selectors/syntax.py,

https://reviewboard.mozilla.org/r/159456/#review165228

How about just `tools/try/`? `tryselect` isn't that evocative, and neither is `autotry`, but I don't think we need to have both names around, so if we go with this can we change `autotry` references to `tryselect`?

::: build/virtualenv_packages.txt
(Diff revision 1)
>  mozilla.pth:testing/marionette/harness
>  mozilla.pth:testing/marionette/harness/marionette_harness/runner/mixins/browsermob-proxy-py
>  mozilla.pth:testing/marionette/puppeteer/firefox
>  packages.txt:testing/mozbase/packages.txt
> -mozilla.pth:testing/taskcluster
> +mozilla.pth:tools
> -mozilla.pth:testing/tools/autotry

Is this removing `testing/taskcluster` from the list? Is that on purpose?
Attachment #8888491 - Flags: review?(cmanchester) → review+
Comment on attachment 8888492 [details]
Bug 1382775 - Add tools/tryselect to flake8 linter,

https://reviewboard.mozilla.org/r/159458/#review165238

Please also mention that you fixed a couple lint issues when adding this to flake8.
Attachment #8888492 - Flags: review?(cmanchester) → review+
Comment on attachment 8888493 [details]
Bug 1382775 - Move autotry logic from |mach try| into autotry proper,

https://reviewboard.mozilla.org/r/159460/#review165244

::: tools/tryselect/mach_commands.py:45
(Diff revision 1)
> +        The |mach try| command is a frontend to several different try
> +        selectors that are run by a subcommand (listed below). If no

Or... it will be? I don't see any other selectors implemented here.

::: tools/tryselect/mach_commands.py
(Diff revision 1)
>          """
> -
>          from mozbuild.testing import TestResolver
>          from tryselect.selectors.autotry import AutoTry
>  
> -        print("mach try is under development, please file bugs blocking 1149670.")

Ok, ok, we can get rid of this message.
Attachment #8888493 - Flags: review?(cmanchester) → review+
Comment on attachment 8888491 [details]
Bug 1382775 - Move testing/tools/autotry to tools/tryselect/selectors/syntax.py,

https://reviewboard.mozilla.org/r/159456/#review165228

Good point, I think I prefer `tools/try` better too. What if `autotry` references were changed to `syntax` to match the subcommand? So the new module would be `tools/try/selectors/syntax.py`.

In case you were wondering, the reason I wanted to put stuff in a `selectors` directory was to keep the parent reserved for shared code (stuff like pushing to vcs and getting the list of tasks from taskcluster).

> Is this removing `testing/taskcluster` from the list? Is that on purpose?

Yeah it was a drive-by cleanup, testing/taskcluster doesn't exist anymore.
Comment on attachment 8888493 [details]
Bug 1382775 - Move autotry logic from |mach try| into autotry proper,

https://reviewboard.mozilla.org/r/159460/#review165244

> Or... it will be? I don't see any other selectors implemented here.

True. I'm hoping to get a new selector added sometime by the end of August, but I'll change this to make a bit more sense in the meantime.

> Ok, ok, we can get rid of this message.

Heh, I couldn't slip it by you! Seriously though, if you want it to stay, I'll add it back in.

Though now that 100% of tasks will soon be going through taskcluster's try parser, I was thinking of asking to create a new subcomponent of Taskcluster for try related issues.
Comment on attachment 8888491 [details]
Bug 1382775 - Move testing/tools/autotry to tools/tryselect/selectors/syntax.py,

https://reviewboard.mozilla.org/r/159456/#review165228

Yeah, that sounds fine!
Comment on attachment 8888493 [details]
Bug 1382775 - Move autotry logic from |mach try| into autotry proper,

https://reviewboard.mozilla.org/r/159460/#review165244

> Heh, I couldn't slip it by you! Seriously though, if you want it to stay, I'll add it back in.
> 
> Though now that 100% of tasks will soon be going through taskcluster's try parser, I was thinking of asking to create a new subcomponent of Taskcluster for try related issues.

I'm ok with removing it.
Comment on attachment 8888491 [details]
Bug 1382775 - Move testing/tools/autotry to tools/tryselect/selectors/syntax.py,

https://reviewboard.mozilla.org/r/159456/#review165228

Ah wait, we can't call it `tools/try` because then `import try` conflicts with the try/except keyword. I agree `tryselect` isn't great, but I wanted to avoid calling it `trychooser` again and couldn't come up with anything better :/
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27b5181a3c77
Move testing/tools/autotry to tools/tryselect/selectors/syntax.py, r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/7f34be19c84b
Add tools/tryselect to flake8 linter, r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/c94bf66df122
Move autotry logic from |mach try| into autotry proper, r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/27b5181a3c77
https://hg.mozilla.org/mozilla-central/rev/7f34be19c84b
https://hg.mozilla.org/mozilla-central/rev/c94bf66df122
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.