Closed
Bug 1382775
Opened 7 years ago
Closed 7 years ago
Refactor |mach try| to support multiple different "trychoosers"
Categories
(Testing :: General, enhancement)
Testing
General
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 | ||
Updated•7 years ago
|
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-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 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 5•7 years ago
|
||
mozreview-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 6•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 9•7 years ago
|
||
mozreview-review-reply |
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 10•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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 :/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
bugherder |
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
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1384257
You need to log in
before you can comment on or make changes to this bug.
Description
•