Closed
Bug 978846
Opened 10 years ago
Closed 9 years ago
Allow mozharness to take any command line parameters from the try chooser
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(firefox38 wontfix, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox-esr31 fixed, firefox-esr38 fixed)
People
(Reporter: vikstrous, Assigned: chmanchester)
References
Details
(Keywords: trychooser)
Attachments
(2 files, 2 obsolete files)
In bug 967630 we were looking for a way to allow anyone pushing to try to choose to turn on profiling for their talos tests. This would have been trivial if we could pass custom mozharness command line parameters in the commit message for try pushes. We are planning to implement a solution that is very specific to our use case, but I think this should be done in a more general way so that any command line parameter can be passed to mozharness. Based on my understanding of mozharness, it seems like this would be a difficult thing to do because of the complex way in which the config is parsed. It might require significant refactoring.
Comment 1•10 years ago
|
||
As a possible alternative that would already work, is it possible to turn on profiling just through manipulating https://mxr.mozilla.org/mozilla-central/source/testing/talos/talos.json? You can host your own talos zip files on people.m.o, and modify options with talos_options.
Flags: needinfo?(vstanchev)
Reporter | ||
Comment 2•10 years ago
|
||
I thought that when you push to try mozharness is run by buildbot somehow and it's not using talos.json from your repository. I thought that's used only by mach. Is that not the case?
Flags: needinfo?(vstanchev) → needinfo?(bhearsum)
Comment 3•10 years ago
|
||
(In reply to Viktor Stanchev [:vikstrous] from comment #2) > I thought that when you push to try mozharness is run by buildbot somehow > and it's not using talos.json from your repository. I thought that's used > only by mach. Is that not the case? I don't think so...I'm pretty sure talos.json was originally added for Buildbot. I just verified this by looking at a log: https://tbpl.mozilla.org/php/getParsedLog.php?id=35539497&tree=Try&full=1 09:58:30 INFO - Downloading https://hg.mozilla.org/try/raw-file/bccea8228a/testing/talos/talos.json to /builds/slave/talos-slave/test/build/talos.json 09:58:30 INFO - retry: Calling <bound method Talos._download_file of <mozharness.mozilla.testing.talos.Talos object at 0xb72f224c>> with args: ('https://hg.mozilla.org/try/raw-file/bccea8228a/testing/talos/talos.json', '/builds/slave/talos-slave/test/build/talos.json'), kwargs: {}, attempt #1 09:58:31 INFO - Downloaded 4701 bytes. 09:58:31 INFO - {'global': {'talos_repo': 'https://hg.mozilla.org/build/talos', 09:58:31 INFO - 'talos_revision': '7674ffd4b494'}, 09:58:31 INFO - 'mobile-suites': {'remote-tcanvasmark': {'talos_options': ['--noChrome'], etc.
Flags: needinfo?(bhearsum)
Comment 4•10 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #1) > As a possible alternative that would already work, is it possible to turn on > profiling just through manipulating > https://mxr.mozilla.org/mozilla-central/source/testing/talos/talos.json? You > can host your own talos zip files on people.m.o, and modify options with > talos_options. The goal is to specify profiling from trychooser. Modifying talos.json would require us to push a local patch /w changes so I don't consider to solve the original problem.
Comment 5•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #4) > (In reply to Ben Hearsum [:bhearsum] from comment #1) > > As a possible alternative that would already work, is it possible to turn on > > profiling just through manipulating > > https://mxr.mozilla.org/mozilla-central/source/testing/talos/talos.json? You > > can host your own talos zip files on people.m.o, and modify options with > > talos_options. > > The goal is to specify profiling from trychooser. Modifying talos.json would > require us to push a local patch /w changes so I don't consider to solve the > original problem. But you're pushing a new commit anyways when you push to try, aren't you? (Even if it's content free, you've got a new commit for try syntax.) Our strategy for modified builds or tests on try is generally in-tree file changes (eg mozconfigs, or the talos config in this case) unless that's not possible. It doesn't mean that other ways aren't possible, but I think it will be harder for us to prioritize them when there's already another way.
Updated•10 years ago
|
Keywords: trychooser
Updated•10 years ago
|
Component: General Automation → Mozharness
Comment 6•10 years ago
|
||
bug 1066636, bug 791924 and the future in-tree scheduling can potentially fix this.
Assignee | ||
Comment 7•9 years ago
|
||
/r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r 97a5161cb24b1e87a65fc461a1ddbc229b5a446d https://reviewboard-hg.mozilla.org/build-mozharness
Assignee | ||
Comment 8•9 years ago
|
||
/r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 72b4516abca67200ede809e8ae4f908319b34b5a https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Updated•9 years ago
|
Attachment #8594095 -
Flags: feedback?(ahalberstadt)
Comment 9•9 years ago
|
||
Comment on attachment 8594095 [details]
MozReview Request: bz://978846/chmanchester
Looks good, we'll need some kind of config like this I think. But could you elaborate how this will work? What will use it?
Attachment #8594095 -
Flags: feedback?(ahalberstadt) → feedback+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #9) > Comment on attachment 8594095 [details] > MozReview Request: bz://978846/chmanchester > > Looks good, we'll need some kind of config like this I think. But could you > elaborate how this will work? What will use it? There are two mozreview requests here, one with the config file and another with a patch in mozharness to use it. Maybe I requested feedback on the wrong one... here's the mozharness patch: https://reviewboard.mozilla.org/r/7213/
Comment 11•9 years ago
|
||
https://reviewboard.mozilla.org/r/7213/#review5983 This is really cool, a much simpler way of implementing this than I thought! Are you still planning to make a |mach try| in addition to this? Or is this instead of that. Either way, modifying a commit message is easier for both humans and machines, so this is a big win. ::: mozharness/mozilla/testing/testbase.py (Diff revision 1) > + if 'action' in opts and opts['action'] not in ('append', 'store', 'store_true'): > + self.fatal('Passing argument with action "%s" from try syntax' > + ' to a test harness is not currently supported.' % Why are we limiting the actions? I think at the very least 'store_false' should be added here. ::: mozharness/mozilla/testing/testbase.py (Diff revision 1) > + if isinstance(value, bool) and value: nit: value is True
Assignee | ||
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/7213/#review5985 Still planning to work out a mach front end (a "preprocessor" for try syntax), but I this can yield benefit for existing workflows until that part's worked out enough to be obviously better. > Why are we limiting the actions? I think at the very least 'store_false' should be added here. I'm worried about appearing to support something that's not going to work... someone could stick a custom action in the config file I might not know how to echo it down to the harness in a way that will make sense. But store_false and probably some others will be fine, I'll work it out. In a few patches I think we'll be at a point where we can just stick try-configurable stuff next to the rest of the things a harness supports in-tree and geting out of sync wont be a problem.
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r c5d1b79a42f156434bff656bce9fd358e025b865 https://reviewboard-hg.mozilla.org/build-mozharness
Attachment #8594095 -
Flags: feedback+
Comment 14•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r c5d1b79a42f156434bff656bce9fd358e025b865 https://reviewboard-hg.mozilla.org/build-mozharness
Attachment #8594095 -
Flags: review?(jlund)
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r c5d1b79a42f156434bff656bce9fd358e025b865 https://reviewboard-hg.mozilla.org/build-mozharness
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r c5d1b79a42f156434bff656bce9fd358e025b865 https://reviewboard-hg.mozilla.org/build-mozharness
Attachment #8594095 -
Flags: review?(jlund)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r c5d1b79a42f156434bff656bce9fd358e025b865 https://reviewboard-hg.mozilla.org/build-mozharness
Attachment #8594095 -
Flags: review?(jlund)
Comment 18•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester https://reviewboard.mozilla.org/r/7213/#review6401 This is awesome. Talos does something similar but it is very specific to a particular string. It be cool to generify this and have talos avail of it too so we can get rid of: https://bug967630.bugzilla.mozilla.org/attachment.cgi?id=8390003 Though certainly unrelated to your efforts and not worthy of blocking IMO. I just have one inline issue/question (see below) ::: mozharness/mozilla/testing/testbase.py:335 (Diff revision 2) > + len([a for a in cmd if a.startswith(prefix)]) == 1): I'm slow. What are we checking for here? The first part seems like we are checking if any extra args from try msg (e) start the same as a full arg in our known current cmd (prefix). And if that's true, we then go through our known current cmd and ensure that our arg (prefix) at least starts uniquely in the current cmd list of args. I'm a bit confused about the connection between the two conditions and what is our end goal. I think we are trying to make sure that we don't duplicate args with different values? wrt the .startswith() usages, wouldn't we run into issues when different unique args end up matching like '--foo-bar'.startswith('--foo')?
Attachment #8594095 -
Flags: review?(jlund)
Comment 19•9 years ago
|
||
also, sorry for the delay in review. I was at a work meet up this week in Portland and my bugmail suffered.
Assignee | ||
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/7213/#review6419 I looked at the Talos thing -- I think this is general enough to serve that case, but there's a different syntax being used, and a bunch of stuff to rip out, so I'll file a follow up. > I'm slow. What are we checking for here? > > > The first part seems like we are checking if any extra args from try msg (e) start the same as a full arg in our known current cmd (prefix). > > And if that's true, we then go through our known current cmd and ensure that our arg (prefix) at least starts uniquely in the current cmd list of args. > > I'm a bit confused about the connection between the two conditions and what is our end goal. I think we are trying to make sure that we don't duplicate args with different values? > > wrt the .startswith() usages, wouldn't we run into issues when different unique args end up matching like '--foo-bar'.startswith('--foo')? What I'm trying to avoid is erroneously replacing options that can be specified multiple times. But it's not always true an argument that can be specified multiple times will be specified multiple times, so this isn't really sufficient, and you're right about matching too much by just checking prefixes. No harm in refusing to replace in these cases (if we agree we can risk this in practice), but either way we need to have an exact match, so I'll fix that.
Assignee | ||
Comment 21•9 years ago
|
||
https://reviewboard.mozilla.org/r/7213/#review6465 > What I'm trying to avoid is erroneously replacing options that can be specified multiple times. But it's not always true an argument that can be specified multiple times will be specified multiple times, so this isn't really sufficient, and you're right about matching too much by just checking prefixes. No harm in refusing to replace in these cases (if we agree we can risk this in practice), but either way we need to have an exact match, so I'll fix that. ahal set me straight on this today elsewhere, coincidentally. This is all based on my assumption that specifying an argument whose action isn't "append" multiple times is an error. It's not, the later values override the earlier ones, so none of this is neccesary.
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester /r/7215 - Bug 978846 - Re-parse a try syntax to detect additional harness parameters. Pull down this commit: hg pull -r 486b6b6a026762bd00f1caa59138b5ba1dad2816 https://reviewboard-hg.mozilla.org/build-mozharness
Attachment #8594095 -
Flags: review?(jlund)
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester /r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 114955a3837f9a6da002da388e6e07a575fc9531 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester /r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 114955a3837f9a6da002da388e6e07a575fc9531 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester /r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 114955a3837f9a6da002da388e6e07a575fc9531 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8594096 [details]
MozReview Request: bz://978846/chmanchester
Sorry for the bugspam. Mozreview is still having trouble setting this flag.
Attachment #8594096 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester /r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 114955a3837f9a6da002da388e6e07a575fc9531 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594096 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester /r/7219 - Bug 978846 - Add a file to the tree to tell try what argments are acceptable to pass on to the harness process. Pull down this commit: hg pull -r 114955a3837f9a6da002da388e6e07a575fc9531 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594096 -
Flags: review?(ahalberstadt)
Comment 29•9 years ago
|
||
Comment on attachment 8594095 [details] MozReview Request: bz://978846/chmanchester https://reviewboard.mozilla.org/r/7213/#review6529 Ship It!
Attachment #8594095 -
Flags: review?(jlund) → review+
Comment 30•9 years ago
|
||
looks good, since I started the review I took a look at the last diff but I'm actually on PTO now so for any follow ups, I personally won't be available until May 13th.
Comment 31•9 years ago
|
||
Comment on attachment 8594096 [details] MozReview Request: bz://978846/chmanchester https://reviewboard.mozilla.org/r/7217/#review6543 Ship It!
Attachment #8594096 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 34•9 years ago
|
||
https://hg.mozilla.org/build/mozharness/rev/dd32bdb70066
Assignee | ||
Comment 35•9 years ago
|
||
This is working on try as of today's mozharness revision bump.
Assignee: nobody → cmanchester
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 36•9 years ago
|
||
Re-landed on correct mozharness branch: https://hg.mozilla.org/build/mozharness/rev/4aaa64f9d5cf
Comment 37•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/13183431a6fe
status-firefox39:
--- → fixed
Updated•9 years ago
|
Comment 38•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/cda517b321ee
status-firefox38.0.5:
--- → fixed
Comment 40•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr31/rev/eb934c7a88b1
status-firefox-esr31:
--- → fixed
Assignee | ||
Comment 41•9 years ago
|
||
Attachment #8594096 -
Attachment is obsolete: true
Attachment #8594095 -
Attachment is obsolete: true
Attachment #8618090 -
Flags: review+
Attachment #8618091 -
Flags: review+
Assignee | ||
Comment 42•9 years ago
|
||
Assignee | ||
Comment 43•9 years ago
|
||
Assignee | ||
Comment 44•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•