Closed
Bug 1084288
Opened 8 years ago
Closed 8 years ago
Gip suite is having --type set twice and causing incorrect type to be used.
Categories
(Release Engineering :: General, defect)
Tracking
(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)
People
(Reporter: zcampbell, Assigned: zcampbell)
References
Details
Attachments
(1 file, 9 obsolete files)
8.00 KB,
patch
|
jgriffin
:
review+
jgriffin
:
checked-in+
|
Details | Diff | Splinter Review |
In the Gip job, the --type is being custom set: http://hg.mozilla.org/build/mozharness/file/475f036e77dd/scripts/marionette.py#l425 and then subsequently overridden here: http://hg.mozilla.org/build/mozharness/file/475f036e77dd/scripts/marionette.py#l445
Assignee | ||
Comment 1•8 years ago
|
||
f? from Dave before I ask for r? from releng.
Attachment #8506778 -
Flags: feedback?(dave.hunt)
Comment on attachment 8506778 [details] [diff] [review] bug_1084288.patch Review of attachment 8506778 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/marionette.py @@ +459,5 @@ > > if self.config.get('emulator') or self.config.get('gaiatest'): > config_fmt_args['symbols_path'] = self.symbols_path > > + if not config_fmt_args.has_key('type'): This will always be false because we're adding --type earlier by extending cmd. You could create a test_type variable and if it doesn't get set fall back to the configuration.
Attachment #8506778 -
Flags: feedback?(dave.hunt) → feedback-
Assignee | ||
Comment 3•8 years ago
|
||
Fix it up proper to avoid this kind of thing in the future.
Attachment #8506778 -
Attachment is obsolete: true
Attachment #8506880 -
Flags: review?(sbruno)
Assignee | ||
Comment 4•8 years ago
|
||
^ in the above patch I removed the reference to tbpl-manifest.ini as we intend to remove that file. I removed as many cmd.append calls as I could to avoid clashes between setting via cmd.append and the config_fmt_args object.
Comment 5•8 years ago
|
||
Comment on attachment 8506880 [details] [diff] [review] bug_1084288.patch Review of attachment 8506880 [details] [diff] [review]: ----------------------------------------------------------------- Apart from my really minor comment (about a comment), this looks ok at first glance. Given the extension of the patch, though, I warmly recommend testing on a staging environment before landing. I can do that at the beginning of next week. ::: scripts/marionette.py @@ +433,5 @@ > + config_fmt_args['total-chunks'] = self.config.get('total_chunks') > + > + # Bug 1046694 > + # using a different manifest if a specific gip-suite is specified > + # --type parameter depends on gip-suite I would remove this comment, since --type command line parameter will be no longer used
Attachment #8506880 -
Flags: feedback+
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8506880 [details] [diff] [review] bug_1084288.patch Review of attachment 8506880 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/marionette.py @@ +433,5 @@ > + config_fmt_args['total-chunks'] = self.config.get('total_chunks') > + > + # Bug 1046694 > + # using a different manifest if a specific gip-suite is specified > + # --type parameter depends on gip-suite Simone, we do still use this, but we only set it for "functional"
Comment 7•8 years ago
|
||
Hi zac, I applied your patch to my test environment, see e.g.: http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6 The mozharness version being used is https://hg.mozilla.org/users/sbruno_mozilla.com/mozharness, which includes your patch; it seems to me that --type parameter is still passed twice (in this case, first with with an entry like "'--type', 'b2g-external'", then with "'--type=b2g'").
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Simone Bruno [:simone] from comment #7) > Hi zac, > > I applied your patch to my test environment, see e.g.: > http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/ > b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6 > Simone, I can't reach this domain from the London office. Can you pop the content of that into a pastebin or bugzilla attachment?
Comment 10•8 years ago
|
||
Hi zac, The master was temporary down, now you should be able to access http://dev-master1.srv.releng.scl3.mozilla.com:8401/builders/b2g_ubuntu64_vm%20cedar%20opt%20test%20gaia-ui-test-functional-1/builds/6 If you cannot access it now, I will send you the output in other ways.
Flags: needinfo?(sbruno)
Assignee | ||
Comment 11•8 years ago
|
||
Simone, I still can't reach that I'm sorry.
Flags: needinfo?(sbruno)
Comment 12•8 years ago
|
||
Hi zac, here is the output of the functional suite, in which --type parameter is set twice on command line. The mozharness version being used here is https://hg.mozilla.org/users/sbruno_mozilla.com/mozharness (which includes your patch).
Flags: needinfo?(sbruno)
Assignee | ||
Comment 13•8 years ago
|
||
Jgriffin, I've spent quite some time on this and I cannot find where the second --type=b2g is coming from. Can you find the time to help me, or guide me a bit? Happy to do the patch, if only I can find what's causing the problem!
Flags: needinfo?(jgriffin)
Comment 14•8 years ago
|
||
One copy is coming from here: http://hg.mozilla.org/build/mozharness/file/601b5ea63b77/scripts/marionette.py#l432 The other copy is coming from here: http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/marionette.py#47 My preference would be to get rid of the first one.
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 15•8 years ago
|
||
Jgriffin, my patch from comment #3 gets rid of that (in code) but in practice it's still coming back. Looking at my patch again, shouldn't the config from mozilla-central be formatted with config_fmt_args on line 467?
Comment 16•8 years ago
|
||
(In reply to Zac C (:zac) from comment #15) > Jgriffin, my patch from comment #3 gets rid of that (in code) but in > practice it's still coming back. > > Looking at my patch again, shouldn't the config from mozilla-central be > formatted with config_fmt_args on line 467? It should, and it is, so maybe I don't understand the question. Could you provide a log for how this fails with your patch?
Assignee | ||
Comment 17•8 years ago
|
||
The log is in comment #12. Simone ran it on his staging server.
Comment 18•8 years ago
|
||
It looks to me like the patch was not applied there; simone, can you confirm it was?
Flags: needinfo?(sbruno)
Comment 19•8 years ago
|
||
Sorry guys, my fault :-| ! I pushed the patch to the production branch of my copy of the mozharness repo, but the test is configured to use default... I am going to re-run the test after fixing the environment setup. Apologies for the time you wasted because of this.
Flags: needinfo?(sbruno)
Assignee | ||
Comment 20•8 years ago
|
||
I suspect now that the chunking will break because it's not set here: http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/marionette.py#47 I don't have time to revise that today, but Simone can you run it anyway and I will work with that and any other errors there may be?
Comment 21•8 years ago
|
||
zac, the gaia-ui-test-functional-1 test now executes successfully, and the --type parameter duplication is not present anymore. Also the chunking parameters are set correctly.
Attachment #8513550 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
Comment on attachment 8506880 [details] [diff] [review] bug_1084288.patch Review of attachment 8506880 [details] [diff] [review]: ----------------------------------------------------------------- Test results in Comment 21 look ok to me, so - unless you have further concerns about how chunking parameters are managed now - this can be landed.
Attachment #8506880 -
Flags: review?(sbruno) → review+
Assignee | ||
Comment 23•8 years ago
|
||
The chunking is not set correctly. I need to file another patch to update the file in m-c file and then probably re-visit this patch again too!
Assignee | ||
Comment 24•8 years ago
|
||
I made a patch in bug 1092132 to set the m-c defaults but it can't be landed without this patch and vice versa. I need to think a bit more about a way to land portions of each patch separately without such great dependencies on one another.
Assignee | ||
Comment 25•8 years ago
|
||
Simone, I have had to add back in the cmd.extend code that sets the chunks until bug 1092123 has landed. Then after these have landed I can start on bug 1092132 which will remove the redundancy and tidy up the scripts in general. Can you run this again on your staging environment? Thanks.
Attachment #8506880 -
Attachment is obsolete: true
Attachment #8514443 -
Attachment is obsolete: true
Attachment #8515588 -
Flags: review?(sbruno)
Attachment #8515588 -
Flags: feedback?(jgriffin)
Comment 26•8 years ago
|
||
Comment on attachment 8515588 [details] [diff] [review] bug_1084288.patch Review of attachment 8515588 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/marionette.py @@ +465,5 @@ > > options_group = self._get_options_group(self.config.get('emulator'), > self.config.get('gaiatest')) > + > + if config_fmt_args.has_key('binary'): I know this has just been moved, but it was weird in the original too. config_fmt_args always has a binary key defined (it's defined in the original definition), so we can just eliminate the 'if' clause here, I think.
Attachment #8515588 -
Flags: feedback?(jgriffin) → feedback+
Assignee | ||
Comment 27•8 years ago
|
||
Fixing the bit rot. Mgerva, can you try this on mozharness-ash for me? Thanks!
Attachment #8515588 -
Attachment is obsolete: true
Attachment #8515588 -
Flags: review?(sbruno)
Flags: needinfo?(mgervasini)
Comment 28•8 years ago
|
||
(In reply to Zac C (:zac) from comment #27) > Created attachment 8527634 [details] [diff] [review] > bug_1084288_2.patch > > Fixing the bit rot. > > Mgerva, can you try this on mozharness-ash for me? Thanks! Landed in ash-mozharness
Flags: needinfo?(mgervasini)
Assignee | ||
Comment 29•8 years ago
|
||
Mgerva, the last patch was not passing the correct binary. Can you push this one to mozharness-ash for me? Thanks
Attachment #8527634 -
Attachment is obsolete: true
Flags: needinfo?(mgervasini)
Comment 30•8 years ago
|
||
hey Zac, your patch did not apply directly on ash-mozharness so I've applied it to mozharness instead and copied the patched marionette.py back into ash-mozharenss. Let me know if you have any problems
Flags: needinfo?(mgervasini)
Assignee | ||
Comment 31•8 years ago
|
||
Ah, it probably depended upon my last one being reverted first (I didn't consider that!)
Assignee | ||
Updated•8 years ago
|
Attachment #8527813 -
Flags: review?(jgriffin)
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8528292 [details] [diff] [review] [ash-mozharness] Bug 1084288 - updated patch for ash-mozharness.patch Review of attachment 8528292 [details] [diff] [review]: ----------------------------------------------------------------- Jgriffin, can you review these two patches? Initially they regress chunking and gecko-log output, but I will follow this up with a patch in here that hopefully we can land asap afterwards. https://bugzilla.mozilla.org/show_bug.cgi?id=1092132
Attachment #8528292 -
Flags: review?(jgriffin)
Assignee | ||
Updated•8 years ago
|
Attachment #8527813 -
Attachment is obsolete: true
Attachment #8527813 -
Flags: review?(jgriffin)
Assignee | ||
Updated•8 years ago
|
Attachment #8528292 -
Attachment is obsolete: true
Attachment #8528292 -
Flags: review?(jgriffin)
Assignee | ||
Comment 33•8 years ago
|
||
Now I am sure this one will work! Jgriffin, can you review it? As it will temporarily regress chunking and gecko-log output, I'll prepare the patch as per bug 1092132 so we can land it ASAP afterwards. Mgerva, can you push this to mozharness-ash please? Thankyou.
Flags: needinfo?(mgervasini)
Attachment #8528348 -
Flags: review?(jgriffin)
Assignee | ||
Updated•8 years ago
|
Comment 35•8 years ago
|
||
Landed in ash-mozharness as requested. http://hg.mozilla.org/build/ash-mozharness/rev/d613af0323e2
Flags: needinfo?(mgervasini)
Comment 36•8 years ago
|
||
Comment on attachment 8528348 [details] [diff] [review] bug_1084288_4.patch Review of attachment 8528348 [details] [diff] [review]: ----------------------------------------------------------------- r- to clarify the issue around tbpl-manifest.ini, but overall looks good. ::: scripts/marionette.py @@ +384,5 @@ > + 'binary': self.binary_path, > + 'address': self.config.get('marionette_address'), > + 'raw_log_file': os.path.join(dirs["abs_blob_upload_dir"], > + "mn_structured_full.log"), > + 'gecko_log': self.query_abs_dirs()['abs_blob_upload_dir'] might as well use the dirs variable here, rather than calling self.query_abs_dirs() again @@ -412,5 @@ > - if not os.access(binary, os.F_OK): > - binary = os.path.join(binary_path, 'b2g') > - > - manifest = os.path.join(dirs['abs_gaiatest_dir'], 'gaiatest', 'tests', > - 'tbpl-manifest.ini') I _think_ we need to keep this line for tests running on older branches, like 1.4; those won't hit the self.config.get('gip_suite') block below.
Attachment #8528348 -
Flags: review?(jgriffin) → review-
Assignee | ||
Comment 37•8 years ago
|
||
Updated as per previous review comments.
Attachment #8528348 -
Attachment is obsolete: true
Attachment #8529011 -
Flags: review?(jgriffin)
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #36) > I _think_ we need to keep this line for tests running on older branches, > like 1.4; those won't hit the self.config.get('gip_suite') block below. Yes I think you're right - at least let's play it safe and put it back!
Assignee | ||
Comment 39•8 years ago
|
||
ahal, are you comfortable reviewing this while jgriffin is off for thanksgiving? We'd really like to get it merged as it's causing some issues with stability on Gip/Treeherder. If you're not or too busy it can wait.
Flags: needinfo?(ahalberstadt)
Comment 40•8 years ago
|
||
Comment on attachment 8529011 [details] [diff] [review] bug_1084288_5.patch Review of attachment 8529011 [details] [diff] [review]: ----------------------------------------------------------------- lgtm; we might want to land and test on ash to make sure there are no surprises.
Attachment #8529011 -
Flags: review?(jgriffin) → review+
Updated•8 years ago
|
Flags: needinfo?(ahalberstadt)
Comment 41•8 years ago
|
||
Zac Is there anything else to do here?? This is blocking multiple bugs can we get this merged?
Flags: needinfo?(zcampbell)
Assignee | ||
Comment 42•8 years ago
|
||
Nothing Bebe! Let's ask Mgerva to push it to Ash as per Jgriffin's request. Mgerva, can you push it to Ash for a test run (but first remove any other test patches of mine left from previous testing)?
Flags: needinfo?(zcampbell) → needinfo?(mgervasini)
Comment 43•8 years ago
|
||
(In reply to Zac C (:zac) from comment #42) > Nothing Bebe! Let's ask Mgerva to push it to Ash as per Jgriffin's request. > > Mgerva, can you push it to Ash for a test run (but first remove any other > test patches of mine left from previous testing)? Landed on ash-mozharness
Flags: needinfo?(mgervasini)
Assignee | ||
Comment 44•8 years ago
|
||
I've re-triggered some Ash jobs and I'll review the results soon.
Assignee | ||
Comment 45•8 years ago
|
||
Results are good as far as I can see. It 'breaks' the chunking but that is expected and fixed by landing Bug 1092132 immediately afterwards.
Keywords: checkin-needed
Comment 46•8 years ago
|
||
Comment on attachment 8529011 [details] [diff] [review] bug_1084288_5.patch https://hg.mozilla.org/build/mozharness/rev/8a973eeef512
Attachment #8529011 -
Flags: checked-in+
Comment 47•8 years ago
|
||
In production: https://hg.mozilla.org/build/mozharness/rev/8a973eeef512
Assignee | ||
Comment 48•8 years ago
|
||
Just checked a few test runs on TBPL and everything is running nicely.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → affected
status-b2g-v2.1S:
--- → affected
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
Updated•5 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•