Re: Just Autoland It
Note that we have problems on the tree due to https://bugzilla.mozilla.org/show_bug.cgi?id=1243276 - to avoid more problems from broken autolander landings we will set the trees to approval-only to avoid incomplete landings from autolander. - Tomcat ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
>> On 25/01/16 05:44 PM, Eric Rescorla wrote: >> >On Mon, Jan 25, 2016 at 1:58 PM, Mike Hommey wrote: >> > >> >>It's also painful to use MozReview's comment system. The comments in the >> >>reviews pane don't show much diff context, and while I just realized >> >>it's possible to make it show more by hovering the diff part for a >> >>little while, it's not really great, as it doesn't actually show a diff >> >>view like the diff pane does, and switching to the diff pane a) is slow >> >>for large diffs and b) has an entirely different comment UX that doesn't >> >>seem really great either. >> >> >> > >> >Indeed. It would be great if it would just include 5-8 lines of context by >> >default. > On Mon, Jan 25, 2016 at 06:15:10PM -0500, Andrew Halberstadt wrote: >> It's not terribly obvious, but instead of clicking on a line number you can >> click and drag on the numbers to set the exact amount of context you want. Mike Hommey writes: > Which is something for the person writing the comment to do. Yes, please. > Also, even when they do that, most of the time, that won't > contain the surrounding code. AFAICT it is a diff view, and, as you say, hovering provides access to the rest of the function/file, but the animation is quite slow. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
Mike Hommey writes: > On Mon, Jan 25, 2016 at 11:23:59AM -0800, Steve Fink wrote: >> Heh. Your list of UI complaints is very similar to mine. Some comments: >> >> >> On 01/25/2016 04:26 AM, Honza Bambas wrote: > Also, iirc, when you reply diff comments in MozReview, the resulting > comments sent to bugzilla lack context (but I could be misremembering). I think you are remembering splinter here. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
On Fri, 22 Jan 2016 14:24:38 -0500, Ehsan Akhgari wrote: > What about the case where the information doesn't exist in the > repository because the author, for example, cherry-picked a > specific commit on a throw-away branch because the rest of the > dependencies are still being worked on? Or, as another example, > what if the patch needs to be checked in only after another patch > (perhaps done by a different author) that is not connected to the > review at hand? I assume that, if the patches have a dependency on other work, then that would be explained to the reviewer, so that they can know how the patch will work. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
Boris Zbarsky writes: > On 1/23/16 9:48 PM, Mike Hommey wrote: >> Note that if /other/ changes from other bugs have happened to the same >> files between the last reviewed iteration and the rebase before landing, >> the interdiff will show them without any kind of visual cues. > > Ah, that's unfortunate. > > I agree that this is a hard problem, though (interdiff across > rebases). I guess that does mean that you can't really use the > final attached thing for the "I want to see the interdiff" use > case; need to push something to MozReview before rebasing to > address that. Yes, from the reviewing efficiency perspective, it is best to push MozReview updates for re-review on the same base revision. i.e. separate from the rebase/landing process. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: e10s tests
On Tuesday, January 5, 2016 at 2:38:30 PM UTC-5, Felipe G wrote: > (cross-posted to fx-dev and dev.platform) > > As we drive towards shipping e10s, we are working on making sure that our > tests work with e10s. As you already know, there's a number of tests that > are disabled from running on e10s, and we need to enable them. We've > created a spreadsheet that lists every test that is disabled from running > on e10s in order to track the remaining work to do. .. We've managed to enable a good number of tests in the last couple of weeks. Thanks to those who have looked at the disabled tests list and added comments or fixed tests: https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit?pref=2&pli=1#gid=0 Many tests still lack owners, or at least a mentor who could help someone else fix and/or enable the test. If a test shouldn't be enabled in e10s at all, please add a comment to the manifest file (such as mochitest.ini) indicating the reason. browser-plain: many of these are under dom/ browser-chrome: good progress here. Several browser/ components need owners and work. devtools tests: hundreds of tests here. The devtools team have assigned owners to these (thanks!) but I assume they will need lots of help. I expanded the documentation at https://wiki.mozilla.org/Electrolysis/e10s_test_tips for browser-chrome tests. These tips describe utility functions that can help for converting existing tests and for writing new tests. This is based on a similar talk I gave during the last work week. I can help anyone with converting and enabling tests; if not, the rest of the e10s team will be happy to help with any test issues. Neil ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Moving FirefoxOS into Tier 3 support
Hi, I'm on the engineering productivity team, and work a lot on continuous integration and test harnesses. I stood up initial Gecko unittests on B2G emulator, worked on B2G automation for several years up until the divide, and still help nominally maintain the emulator and mulet unittests. So that's the hat I'll be wearing for this post. I'd like to just state a few of the implications that this, and the following quote from a gecko-all mail have: This means that all engineering on Firefox OS by Platform Engineering and Platform Operations will stop. The purpose of this post is simply to make sure everyone involved is aware of the following ramifications: 1. Gecko based test harnesses (mochitest, xpcshell, reftest, etc) will eventually break and their corresponding jobs will be disabled. At some point, the test harness will need to undergo a large enough change that it will require a non-trivial amount of effort to not break B2G emulators and mulet. I'd estimate for most harnesses, this will happen sooner rather than later (within a quarter or two). When this happens, the jobs will be turned off completely so as not to waste money on our AWS bill. To be crystal clear, this means no more mochitest, reftest or xpcshell on B2G emulators. Mulet will likely last a little longer as it is similar enough to Firefox desktop. 2. If at any point CD wishes to rejoin mainline development and run the set of Gecko unittests once again, re-integration will be a long and difficult process. 3. Gaia tests will still need a substantial effort to keep green. This one is more obvious, but still worth stating. It's really hard to keep a job green after the fact. In my experience, keeping jobs in Tier 3 for any extended period of time is not sustainable. CD will likely need to fork if they want to keep these jobs green. I think a question worth asking, is should we bother with Tier 3 at all? Or should we jump straight to disabling CD specific jobs. I guess it doesn't hurt to leave them running while they last, but in some cases this will be a very short time frame. Cheers, Andrew ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Moving FirefoxOS into Tier 3 support
If we jump on the Marshmallow bandwagon we can drop stlport, Google already did that too. https://bugzilla.mozilla.org/show_bug.cgi?id=1213259 https://bugzilla.mozilla.org/show_bug.cgi?id=1218802 - specifically patch [07] On Mon, Jan 25, 2016 at 6:34 PM, Nathan Froyd wrote: > On Mon, Jan 25, 2016 at 12:30 PM, Ehsan Akhgari > wrote: > >> For example, for a long time b2g partners held back our minimum supported >> gcc. Now that there are no such partner requirements, perhaps we can >> consider bumping up the minimum to gcc 4.8? (bug 1175546) >> >> I'm sure others have similar examples to fill in. >> > > One current example is b2g's reliance on stlport and changing the build to > support a modern C++ library like libc++. > > -Nathan > > ___ > dev-fxos mailing list > dev-f...@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-fxos > > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
On 1/26/2016 16:46, Benjamin Smedberg wrote: On 1/26/2016 10:26 AM, Boris Zbarsky wrote: On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a category of documentation that's not written yet. Not just. For example, there is no way to "r-" in mozreview. You can only "r+" or remove the review request, in Bugzilla terms. There is a pattern that some teams have been using where they never mark "r-" on a change. I think the rationale for this is that it feels negative and discouraging to receive the "review not granted" email, especially for new contributors. Instead, they will either clear the review or mark an f+ and ask for a new patch. I don't like this practice: I encourage people to use the r- flag. It's important to make clear in bugzilla that something has already been reviewed and that the result is that something is not ready. Without r- or something very similar, it's difficult to distinguish between various important cases: * I'm too busy/not the right person to review this (clear the review or redirect it) * I started the review and have a question (leave the r? flag, add a NEEDINFO) * This isn't good enough (r-) * I've looked this over and it's ok in general, but it still needs a detailed code review: mark f+, redirect the r? flag as appropriate In addition, I've seen several contributors become confused receiving an f+ and not realizing that what it really meant is "not good enough, please make changes". Our reviewboard tooling should support explicit r- and not just clearing review flags. +1 and I also like the feature of bugzilla UI to allow clicking the r- flag by the patch to jump to the actual feedback comment (that e.g. I gave two weeks ago and don't remember). It's good to set at least some flag on a patch when review/feedback is done with whatever result. -hb- --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
FWIW, adding r- abilities is bug 1197879[1]. There's a prototype patch that adds the UI, but I believe the MozReview team was still trying to sort out the best terminology to use. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1197879 On 26/01/2016 10:46 AM, Benjamin Smedberg wrote: > > > On 1/26/2016 10:26 AM, Boris Zbarsky wrote: >> On 1/26/16 7:38 AM, Axel Hecht wrote: >>> Which is basically what I do whenever I want to do something. I have a >>> clear idea and intention on what I want to show up on bugzilla, but not >>> on what to do on reviewboard to get there. Which might just be a >>> category of documentation that's not written yet. >> >> Not just. For example, there is no way to "r-" in mozreview. You can >> only "r+" or remove the review request, in Bugzilla terms. > > There is a pattern that some teams have been using where they never mark > "r-" on a change. I think the rationale for this is that it feels > negative and discouraging to receive the "review not granted" email, > especially for new contributors. Instead, they will either clear the > review or mark an f+ and ask for a new patch. > > I don't like this practice: I encourage people to use the r- flag. It's > important to make clear in bugzilla that something has already been > reviewed and that the result is that something is not ready. Without r- > or something very similar, it's difficult to distinguish between various > important cases: > > * I'm too busy/not the right person to review this (clear the review >or redirect it) > * I started the review and have a question (leave the r? flag, add a >NEEDINFO) > * This isn't good enough (r-) > * I've looked this over and it's ok in general, but it still needs a >detailed code review: mark f+, redirect the r? flag as appropriate > > In addition, I've seen several contributors become confused receiving an > f+ and not realizing that what it really meant is "not good enough, > please make changes". > > Our reviewboard tooling should support explicit r- and not just clearing > review flags. > > --BDS > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
On 1/26/2016 10:26 AM, Boris Zbarsky wrote: On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a category of documentation that's not written yet. Not just. For example, there is no way to "r-" in mozreview. You can only "r+" or remove the review request, in Bugzilla terms. There is a pattern that some teams have been using where they never mark "r-" on a change. I think the rationale for this is that it feels negative and discouraging to receive the "review not granted" email, especially for new contributors. Instead, they will either clear the review or mark an f+ and ask for a new patch. I don't like this practice: I encourage people to use the r- flag. It's important to make clear in bugzilla that something has already been reviewed and that the result is that something is not ready. Without r- or something very similar, it's difficult to distinguish between various important cases: * I'm too busy/not the right person to review this (clear the review or redirect it) * I started the review and have a question (leave the r? flag, add a NEEDINFO) * This isn't good enough (r-) * I've looked this over and it's ok in general, but it still needs a detailed code review: mark f+, redirect the r? flag as appropriate In addition, I've seen several contributors become confused receiving an f+ and not realizing that what it really meant is "not good enough, please make changes". Our reviewboard tooling should support explicit r- and not just clearing review flags. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
On 1/26/16 7:38 AM, Axel Hecht wrote: Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a category of documentation that's not written yet. Not just. For example, there is no way to "r-" in mozreview. You can only "r+" or remove the review request, in Bugzilla terms. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Just Autoland It
Piling on: I'm using mozreview mostly as an occasional patch author: Plus, I can schedule a try build. Minus, I need to bother the reviewer with a published request in order to do so. Resorted to add yet another hg extension to my local .hg/hgrc. My most frequent concern is that bugzilla and mozreview use jargon and UX flows that have nothing in common. I don't think that either are good or better in their own right, too. And the mapping of one to the other isn't documented. "I want to cancel a review, or r-" doc is non-existent to hard-to-find. I just randomly click buttons. Which is basically what I do whenever I want to do something. I have a clear idea and intention on what I want to show up on bugzilla, but not on what to do on reviewboard to get there. Which might just be a category of documentation that's not written yet. Why I consider that to be a problem is gonna be in a separate reply to a different post on this thread. Axel On 25/01/16 13:26, Honza Bambas wrote: Writing both as a patch author and a reviewer as well. - as a patch author I want a full control on when the patch actually lands (dependencies, any other timing reasons, that only the author knows the best), "Don't land yet" comment somewhere will easily be overlooked - as a reviewer I don't want to bare the decision to land or not to land, at all - I want to preserve the mechanism "r+ with comments", which means to let the patch be landed by the author after updated (means reviewer doesn't need to look over it again) - as an author I want to write comments about the patch (about the structure, what it does, why) to make the review simpler for the reviewer ; commit message description may not be the best place, right? - will it be possible to still be using hg patch queues? - I (and others too) am not fun of MozReview UI. As a reviewer I found it very hard to orient in it: - what is the difference between [Reviews] and [Diff] tab? what is exactly it's content - where exactly to click to start a reivew of a patch I want to review now? Is in the "Commits" table? And is it under "Diff" or "Reviews"? - how can I mark hunks/files are already reviewed (something I like on Splinter)? - how can I see only a single file diff and easily navigate between files? (as in Splinter) - few weeks ago I didn't even know how to give an r+!! it's hidden under the [Finish review...] *tab*? - simply said: the UI is everything but self-explanatory and highly unfriendly, until that is fixed I'm not much willing to use MozReview mainly as a reviewer -hb- On 1/22/2016 3:35, Gregory Szorc wrote: If you have level 3 source code access (can push to central, inbound, fx-team) and have pushed to MozReview via SSH, as of a few weeks ago you can now land commits from the "Automation" drop down menu on MozReview. (Before only the review request author could trigger autoland.) This means that anyone [with permissions] can land commits with a few mouse clicks! It will even rewrite commit messages with "r=" annotations with the review state in MozReview. So if someone does a drive-by review, you don't have to update the commit message to reflect that reviewer. Neato! I've gotten into the habit of just landing things if I r+ them and I think they are ready to land. This has startled a few people because it is a major role reversal of how we've done things for years. (Typically we require the patch submitter to do the landing.) But I think reviewer-initiated landing is a better approach: code review is a gate keeping function so code reviewers should control what goes through the gate (as opposed to patch authors [with push access] letting themselves through or sheriffs providing a support role for people without push access). If nothing else, having the reviewer land things saves time: the ready-to-land commit isn't exposed to bit rot and automation results are available sooner. One downside to autoland is that the rebase will happen remotely and your local commits may linger forever. But both Mercurial and Git are smart enough to delete the commits when they turn into no-ops on rebase. We also have bug 1237778 open for autoland to produce obsolscence markers so Mercurial will hide the original changesets when you pull down the rebased versions. There is also potential for some Mercurial or Git command magic to reconcile the state of MozReview with your local repo and delete local commits that have been landed. This is a bit annoying. But after having it happen to me a few times, I think this is a minor annoyance compared to the overhead of pulling, rebasing, rewriting commit messages, and pushing locally, possibly hours or days after review was granted. I encourage my fellow reviewers to join me and "just autoland it" when granting review on MozReview. gps ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform __
Re: Moving FirefoxOS into Tier 3 support
On 26/01/2016 10:51, jma...@mozilla.com wrote: > I would assume all gecko patches would get landed on either mozilla-inbound > or fx-team. If you use mozreview and take advantage of the autoland feature, > then these specific patches will land on mozilla-inbound. Thanks, I'll definitely have a look at mozreview/autoland. Gabriele signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Moving FirefoxOS into Tier 3 support
> Same here. I'm now unable to land the gecko dependencies for bug 1227980 > [1] and that's annoying to say the least considering it took a month to > write and test that thing (not even mentioning the time spent by the > many reviewers and QA people involved). What are we supposed to do with > those kind of patches which are b2g-only? Shall we land them ourselves > on mozilla-central? I would assume all gecko patches would get landed on either mozilla-inbound or fx-team. If you use mozreview and take advantage of the autoland feature, then these specific patches will land on mozilla-inbound. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Moving FirefoxOS into Tier 3 support
On 26/01/2016 10:19, Shawn Huang wrote: > Hi Fabrice, > Following this comment, I'm confused. Do we need to check-in into > b2g-inbound by hand? "checked-in" keyboard no longer supports FirefoxOS > project? Does this mean only people who have Level 3 access permission can > land code? Same here. I'm now unable to land the gecko dependencies for bug 1227980 [1] and that's annoying to say the least considering it took a month to write and test that thing (not even mentioning the time spent by the many reviewers and QA people involved). What are we supposed to do with those kind of patches which are b2g-only? Shall we land them ourselves on mozilla-central? Gabriele [1] [Dialer] Merge the callscreen app back into the dialer and remove the associated system app hacks https://bugzilla.mozilla.org/show_bug.cgi?id=1227980 signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Moving FirefoxOS into Tier 3 support
Hi Fabrice, Following this comment, I'm confused. Do we need to check-in into b2g-inbound by hand? "checked-in" keyboard no longer supports FirefoxOS project? Does this mean only people who have Level 3 access permission can land code? https://bugzilla.mozilla.org/show_bug.cgi?id=1201778#c102 "dropping the checkin needed keyword here, alphan with the new tier 3 change for b2g and the drop of sheriff support for tier 3 we do not do checkin needed anymore for b2g-inbound - if this needs to land on mozilla-central let me know" On 26 January 2016 at 00:00, Fabrice Desré wrote: > Hi all, > > With the smartphone activity shifting to a more exploratory status, we > have been discussing with the platform & releng people which setup would > allow us to keep improving the product and supporting our community of > users while minimizing impact on other parts of the organization. > > The decision we ended up with is to move Firefox OS into Tier 3 > support category (see > https://developer.mozilla.org/en-U/docs/Supported_build_configurations). > That means that other teams won't be backed out and yelled at by > sheriffs for breaking b2g tests, and that the FirefoxOS team will be > responsible for fixing such breakage. Landing code for FirefoxOS stays > the same as today - we don't fork. > We're also working on a solution to move the b2g builds & tests to > their own infrastructure from which we'll ship our builds & updates. > > I want to thank all the people from various corners of the platform > that helped us so far. I guess I'll have to bribe you now! > > Feel free to reach out to me if you need any more details on the subject, > > -- > Fabrice Desré > Connected Devices > Mozilla Corporation > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > -- Regards, Shawn Huang ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
