On 05/26/15 22:33, Andrew Fish wrote: > >> On May 26, 2015, at 6:56 AM, Laszlo Ersek <ler...@redhat.com >> <mailto:ler...@redhat.com>> wrote: >> >> On 05/26/15 14:35, B Cran wrote: >>> On Wed, May 13, 2015 at 11:01 AM, Peterson, Joe >>> <joe.peter...@intel.com <mailto:joe.peter...@intel.com> >>> <mailto:joe.peter...@intel.com>> wrote: >>> >>> Sorry for the delay in responding to this. With regard to involving >>> the community in a discussion of solutions, yes, there is no >>> intention of making this an "Intel only" thing. The posting of the >>> list shouldn't be seen as "this is what will be done," rather, this >>> is the list we have developed based upon feedback from the community >>> to date. If you have feedback or know of anything we missed, please >>> provide your feedback here via the mailing list. Also, we do not >>> intend to make this a home grown solution if we can avoid it. >>> >>> Please be encouraged to post questions/comment/concerns to the >>> mailing list. >>> >>> >>> I don't know how many others feel similarly, but I've seen very negative >>> feedback on Gerrit in the past at $work, mostly around its unfriendly >>> and ugly UI. If a code review system is going to be put into place, it >>> might make sense to use something that's more popular, in use by more >>> open source projects? >> >> Nothing comes close to reviews done in email (*). As I stated earlier >> elsewhere: >> >> - No web based application will ever be as flexible as free flowing text >> for expressing thoughts about patches. I skimmed some upstream Gerrit >> page / examples before, and what I saw could certainly not accommodate >> the amount & format of text (eg. ASCII diagrams) that I >> sometimes produce. >> >> - Mailing lists are unbeaten at preserving threading, and at being >> archived / mirrored without coordination. >> >> (*) assuming (a) contributors use git-send-email to post patches, and >> (b) reviewers use sensible MUAs that don't mangle plaintext, >> monospace font emails. >> >> Gerrit may be okay for tight-knit internal teams, but for >> distributed development it's not appropriate in my opinion. Unless I'm >> wrong, Gerrit has been designed for in-house development, from the >> grounds up. >> >> I don't intend to use Gerrit, and I very much hope that all contributors >> will continue posting patches to, and accepting feedback from, the list. >> > > Is it possible to do both? So the Gerrit part is just a link that is > auto added to the commit message? > > I’ve only played with Gerrit one time, so I may be way off…. But this > would be cool. > > 1) Submit patch to Gerrit, much like we do today to the mailing list. > 2) Gerrit runs that patch against the server farm of known compilers and > makes sure everything compiles. Kicks back the patch to the author on a > build fail. > 3) If everything compiles patch is sent to edk2 mailing list with link > to Gerrit auto-added.
We do something similar in the kernel and virt teams at RH. All patch series (which are backports, most frequently) must be submitted to a build farm first. The build must succeed for all supported platforms, and (ideally) the developer should perform his / her final personal testing with those binary packages that result from said build. When everything's fine, the developer is allowed to post the series to the appropriate internal list. All postings must carry (a) a Bugzilla number, (b) a build identifier (a URL, practically). The build URL sticks around forever -- packages are not preserved after a while, but the metadata stays around forever. Assuming the series gathers a sufficient number of ACKs (three on the virt lists, variable / approx. three on the kernel lists), the respective maintainers pick up the patches and apply them to the internal repos. ... So, I definitely agree with the build farm idea; that would help immensely in avoiding warnings-treated-as-errors that hit only on some compilers. However, I'd prefer if the build URL / build ID were stuck manually in the patch submission. I'd like to keep the integration / automation between these two phases (central build & patch submission) minimal. I'd like to keep retain control over the patch submission. If there was very good tooling support (ie. git command line integration) with gerrit, and the gerrit server was extremely stable, then I might change my mind I guess. I've been using the process described above for years; it matches / extends how big upstream projects are developed (Linux kernel and QEMU for example) -- I trust that workflow. On the other hand the gerrit sites / examples I've seen thus far (from a distance, admittedly) looked awful, so I'm wary. > We can still required all the comments happen in the mailing list. > To me the win for Gerrit is workflow automation, getting to test compile > against other tools. I agree about the test compile 100%. I just wouldn't like to relinquish control over the patch formatting and the submission. The mailing list should remain the primary interface for contributions -- we must accommodate drive-by contributors as well. Uploading patches to the build farm and firing off test builds causes very real costs (electricity, cooling, bandwidth) for the operator, so it would likely be tied to authentication (just like RH's internal build farm, which is similar to <http://koji.fedoraproject.org/>, is tied to authentication). We shouldn't introduce barriers for newbies / one-off contributors for *posting* their patches. For applying their patches, we can enforce whatever we want; a seasoned developer could test-build patches for an occasional contributor before applying & pushing his/her patches. > Also I’m a visual diff kind of person, so my brain > likes seeing the visual diff of the change, vs a command line diff. It > would be nice to have access to this on the web, vs. having to apply the > patch to branch manually. Access on the web looks like a step forward (eg. it provides syntax highlighting), but it's actually a small step at a steep price. The price is that a web browser (and a central server) are required. A browser should not be a hard requirement at any point of the submission and review process. (Test builds are a different matter, although command line tools for those would be preferred as well.) I wrote to Bruce earlier, in private, with regard to reviewing patches against a context: > I agree. Which is why the reviewer should apply the patches from the > mailing list (or pull them from the submitter's public repo) and > review them with context. > > Whenever I review a longer series, I create (or pull) a branch with > the patches, and as I progress through the review, I advance with the > local checkout as well. This way I have the full tree at my disposal, > in my own programmer's editor, and I can check even very distant > contexts against the patch, with the full power of my usual work > environment / desktop. I can grep, jump to tags, build in the middle > of the series, and so on. > > There's no substitute for that. > > And when I have things to say, I can insert them at the exact location > in the patch email; I can format or draw my comments the way I want... So, patches, especially each patch in a larger series, should be reviewed against a tree that has all *prior* patches in the series applied -- ultimately that's how the contributor developed the patches in the first place. In my opinion, a web based tool has no chance at being as flexible at this (ie. at moving around in the entire tree mid-series) as everyone's own desktop environment. Assuming we agree on the above, let's discuss how people grab patches. Normally, the easiest way to bring the patches from the mailing list to a reviewer's desktop environment / local branch is (1) save the emails from your MUA (a few clicks or keypresses), (2) apply them with "git am" (a few more keypresses in your terminal). I agree that this presents difficulties to many edk2 developers, because: - They (have to?) use *gravely* sub-standard MUAs. Let's not name names, but I'll say that a MUA that cannot get the absolute basics right (comment at the bottom by default; keep threading pristine; only reflow lines that end with a space etc) cannot be considered anything but utterly broken. In theory such MUAs are plainly unsuitable for distributed open source development; in practice we must cope with them. - edk2 uses CRLF line terminators internally, and git-am has admittedly problems with that, *if* the patches cross MTA boundaries (which is the norm of course). Therefore, we've grown to push edk2 series to personal github repos / branches, and we reference them in the blurbs. Those branches can be fetched very easily. (I do concede that with the same effort we might as well push our branches to a gerrit server, if such pushing makes sense.) ... Okay, this email is again too long; sorry about that. Summary of my opinion: (1) The build farm is a great idea, and we should make its use a requirement for *applying* someone's patches. (For example, Olivier already does this with the ARM-internal continuous integration environment.) (2) The barrier to *submit* patches should be low: the primary interface should remain the mailing list. Pulling should be kept easy for reviewers' (and testers') sake. (3) A web browser should not be a required tool in performing the actions inherent in submission and review. A browser and a web app might be slightly more convenient for *consuming* short series than plaintext email, but (a) for longer series, where individual patches need to be reviewed against a continually advancing context (ie. tree), a web-based tool is much weaker than a local development environment, providing little benefit, (b) a web app will never be as flexible at *producing* careful comments (think diagrams) as plain-text tools / MUAs. (4) A web app is likely worse at preserving the threading of a discussion than a mailing list. Also, data kept in a web app is less suitable for independent archival / mirroring than a mailing list. ... In fact this makes me recall: https://lwn.net/Articles/638090/ (article) https://lwn.net/Articles/639051/ (comment by me) https://lwn.net/Articles/639071/ (comment by me) Thanks Laszlo ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
