Closed Bug 1332557 Opened 4 years ago Closed 4 years ago
Consider enabling sync success logs by default on Nightly
4.03 KB, patch
|Details | Diff | Splinter Review|
1.03 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/x-review-board-request
We regularly get bug reports from Nightly users. It would be helpful, and within the scope of what Nightly is for, to have services.sync.log.appender.file.logOnSuccess be true on Nightly by default - that way we could ask such users for their logs and be more likely to have something useful. (We could also argue this for Aurora, but one yak at a time ;)
Can I still take this?
Of course! Do you already have your Firefox build environment set up? If not, check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction. You'll want to add the pref to http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/services/sync/services-sync.js, and change the `moz.build` file in that directory so that it preprocesses the prefs file. Have a look at the DevTools prefs file (http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/devtools/client/preferences/devtools.js) and its build manifest for an example of how to set a Nightly-only pref. Please feel free to ask questions here, or on IRC in the #sync channel, if you get stuck. Thanks for taking this on!
I have taken on this bug as it was unassigned and have submitted on https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc however, it is not appearing as a MozReview Request here, so I'm attaching it separately.
Comment on attachment 8843560 [details] [diff] [review] Enabled sync success logs by default only on Nightly That looks exactly correct, thanks.
Assignee: nobody → bharatraghunthan9767
This needs to be in diff format to be landed.
Should I make obsolete the previous attachment #8843560 [details] [diff] [review] or leave it as it is?
Comment on attachment 8843834 [details] [diff] [review] Enabled sync success logs by default only on Nightly - submitting as diff/patch Review of attachment 8843834 [details] [diff] [review]: ----------------------------------------------------------------- Thanks - I previously looked at the reviewboard commit - you should probably work out what went wrong there as using reviewboard is much simpler for reviewers.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c651974bb5ce Enable sync success logs by default only on Nightly r=markh
Comment to attachment #8843834 [details] [diff] [review] https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc Is there anything more to be done or is the bug fixed?
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=81812063&repo=mozilla-inbound
Backout by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/af18a377c790 Backed out changeset c651974bb5ce for test failures in test_service_migratePrefs.js
Should I modify test_service_migratePrefs.js to test for Nightly or not, or change the code in services-sync.js (like changing order of the prefs etc.)?
Flags: needinfo?(bharatraghunthan9767) → needinfo?(markh)
Thanks for your contribution, and my apologies for not noticing the change would cause test failures. The test should be changed (even though it's now out of date) - if you add near the top of the file: Cu.import("resource://gre/modules/AppConstants.jsm"); const LOG_ON_SUCCESS_DEFAULT = AppConstants.NIGHTLY_BUILD; then change: do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), false); to: do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), LOG_ON_SUCCESS_DEFAULT); The test will pass. Seeing as it has been backed out once, we really need to run a try build for this, so you will need to work out how to push to mozreview, and we can trigger a test build from that. Please catch us in IRC in the #sync channel if you still have trouble pushing to mozreview. I also opened bug 1345009 to remove the code that caused the failure, so I guess if you are keen you could take that other bug first, after which no further changes would be needed here :)
Pushed to the reviewboard in order for the patch to be landed
Try is failing with an "E" error that indicates your local tree is a week or so old and needs updating - could you please rebase your patches against a current mozilla-central, modify the commit message so it has ". r?markh" at the end, then re-push to reviewboard? You could then push you own try build too (or I'm happy to do that) - I expect try will then pass and I can push it for you. https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F has more info. Thanks for your patches and your patience!
Please review the MozReview Request of Comment 20 not Comment 19 as it was a mistake.
Something strange has gone on with that patch - https://reviewboard.mozilla.org/r/118104/diff/3/ shows only one .patch file. It's not possible to review an earlier iteration of the patch, so I think you need to push a new patch with only the changes and no .patch files.
I'm sorry, it was because my local copy of the mozilla-central repo was in a mess with multiple branches suddenly created that may have caused the problem. I hope it's fine now?
Comment on attachment 8844774 [details] Bug 1332557 - Enable sync success logs by default on Nightly. https://reviewboard.mozilla.org/r/118104/#review120802 The try run looks good! :) Can you please change the commit message to "Bug 1332557 - Enable sync success logs by default on Nightly. r?markh" and re-push it, then I'll land it. Thanks!
Changed the commit message accordingly and re-pushed it. Now please try landing it.
Comment on attachment 8844774 [details] Bug 1332557 - Enable sync success logs by default on Nightly. https://reviewboard.mozilla.org/r/118104/#review120964 Thanks!
Attachment #8844774 - Flags: review?(markh) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/b0abf85ea5e1 Enable sync success logs by default on Nightly. r=markh
You need to log in before you can comment on or make changes to this bug.