From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.459.1589227254253232617 for ; Mon, 11 May 2020 13:00:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=bAkV+KVh; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589227253; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=d3IyQhAiaCgGQx2JAIpje/p/QiU4zOJYejOoOOX1qwA=; b=bAkV+KVhIcDC3/5UJ9CVUX+99VLp6LsS9drOJm6Bi1WyM7NOv9ZT+2onbZjIZ8Dxk3litM /il2f73nAFeTXvU+tjW34VR6blYTdJTI9rCD/e3jYRS+x4RHotFsxu2I7pQChKPl/ZJ4/L xa+4G6wFEyrkJiB4WKIyGYmo4XM3JwY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-22-niuWJIJ5OQWocNAGIzX6uQ-1; Mon, 11 May 2020 16:00:36 -0400 X-MC-Unique: niuWJIJ5OQWocNAGIzX6uQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B761680B70B; Mon, 11 May 2020 20:00:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-11.ams2.redhat.com [10.36.113.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id AE40A5D9DC; Mon, 11 May 2020 20:00:34 +0000 (UTC) Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: devel@edk2.groups.io, rebecca@bsdio.com, michael.d.kinney@intel.com, "rfc@edk2.groups.io" References: <8ff350e5-64ed-d338-af93-6d12f80004f5@bsdio.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 11 May 2020 22:00:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 05/10/20 23:43, Rebecca Cran wrote: > Mike, > > On 5/10/20 3:29 PM, Michael D Kinney wrote: > >> There is no difference between CI checks run during code review >> and the final CI checks before merge.  I think it is an interesting >> conversation to decide how many times those CI checks should be >> run and if they should run automatically on every change during >> review or on demand. > > I'd suggest following what other Github projects do, which I think is to > run the CI checks automatically on every change that's made in a pull > request - I don't know if it might also be necessary to run them during > the merge, if master has changed in the meantime. That gives the > _submitter_ feedback about any changes they need to make, instead of > having to wait until the maintainer tells them their change has broken > something: it speeds up the development process. Build-testing at every stage through a patch series is important for ensuring bisectability. But there's a critical ingredient to that: based on the assumption that our build system / build rules are good, the builds mentioned above should be *incremental*. That is, if we have a patch set with 10 patches, then then the first patch in the series should trigger a complete build, and the 9 later patches should trigger only incremental builds. (During a bisection, the same commits wouldn't be visited in that same order of course, but that's where the sanity of the build system / build rules comes in! Basically, if your builds succeed with a linear progression through the series, then the build system / build rules ought to *guarantee* that the same "tree states" will build incrementally just fine when visited in any particular order. "git checkout" updates the relevant files, and the build system should be able to derive the minimum set of necessary actions. Anyway, digression ends.) The incremental nature of builds is important for saving energy, and also for saving developer time. The above 10-part example series should not take 10 times as long to build as 10 independent patches, submitted in isolation. Patches#2 through #10 should only rebuild a few modules each (unless lib class headers, protocol headers and such are modified). > >> Mergify is more flexible.  We want to make sure the git history >> is linear with not git merges and supports both single patches >> and patch series without squashing.  GitHub merge button by >> default squashes all commits into a single commit. > > Wouldn't disabling all but "Allow rebase merging" do the same thing > without the additional potential failure point? Though it sounds like > we've resolved the problems with Mergify, so it's not important. > > https://help.github.com/en/github/administering-a-repository/configuring-commit-squashing-for-pull-requests mergify has been pretty stable for me! Thanks, Laszlo