Post by Andrew FishPost by Laszlo ErsekOn Wed, May 13, 2015 at 11:01 AM, Peterson, Joe
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
- 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.
Post by Andrew FishWe 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.
Post by Andrew FishAlso 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
Post by Andrew FishI 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
------------------------------------------------------------------------------