From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9B6B0A04EF; Mon, 25 May 2020 18:09:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2177B1D959; Mon, 25 May 2020 18:09:33 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id A4EAE1D94C; Mon, 25 May 2020 18:09:30 +0200 (CEST) IronPort-SDR: fXHVOF1LbA7luxkfTK40vaPq6ADmUXZsX6PUqfbVit0bN/QbaPJn278J22rrN3A/K75iyIxCKz CUhPfgNkercA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 May 2020 09:09:29 -0700 IronPort-SDR: jVZKZ+VWROvWREji16wL/gsMUA1xCJ6NaQjt7Erw+P/NGXfXMU2ISvg7c1LAvxfkBxWQgJWhhh QjpnICFxDEzg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,433,1583222400"; d="scan'208";a="468088807" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.212.230.177]) ([10.212.230.177]) by fmsmga006.fm.intel.com with ESMTP; 25 May 2020 09:09:27 -0700 To: Maxime Coquelin , Jerin Jacob , Thomas Monjalon Cc: =?UTF-8?Q?Morten_Br=c3=b8rup?= , dpdk-dev , techboard@dpdk.org, "Jim St. Leger" References: <98CBD80474FA8B44BF855DF32C47DC35C60FEA@smartserver.smartshare.dk> <2346940.LZvDnYUUCF@thomas> <354a7cf6-788b-debf-1939-541410a1099b@intel.com> <3551245.iDPhyKTcbK@thomas> <04ce22d8-1e23-5c75-5947-44c8bca040d5@redhat.com> <9de4a537-b5d9-1c3f-90c4-174ca7a1b72a@intel.com> <6512da71-09a0-3357-27b1-58939597bcf1@redhat.com> From: "Burakov, Anatoly" Message-ID: <1d1c7a90-934b-3db4-b7d6-308a0ebb7ee4@intel.com> Date: Mon, 25 May 2020 17:09:27 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <6512da71-09a0-3357-27b1-58939597bcf1@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [dpdk-techboard] Consider improving the DPDK contribution processes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 25-May-20 5:04 PM, Maxime Coquelin wrote: > > > On 5/25/20 5:59 PM, Burakov, Anatoly wrote: >> On 25-May-20 4:52 PM, Maxime Coquelin wrote: >>> >>> >>> On 5/25/20 5:35 PM, Jerin Jacob wrote: >>>> On Mon, May 25, 2020 at 8:52 PM Thomas Monjalon >>>> wrote: >>>>> >>>>> 25/05/2020 16:28, Burakov, Anatoly: >>>>>> On 25-May-20 1:53 PM, Thomas Monjalon wrote: >>>>>>> 25/05/2020 13:58, Jerin Jacob: >>>>>>>> 25/05/2020 11:34, Morten Brørup: >>>>>>>>> sending patches over an >>>>>>>>> email as opposed to a well-integrated web interface workflow is >>>>>>>>> so alien >>>>>>>>> to most people that it definitely does discourage new >>>>>>>>> contributions. >>>>>>>>> >>>>>>>>> I understand the advantages of mailing lists (vendor independence, >>>>>>>>> universal compatibility, etc.), but after doing reviews in >>>>>>>>> Github/Gitlab >>>>>>>>> for a while (we use those internally), going through DPDK >>>>>>>>> mailing list >>>>>>>>> and reviewing code over email fills me with existential dread, >>>>>>>>> as the >>>>>>>>> process feels so manual and 19th century to me. >>>>>>>> >>>>>>>> Agree. I had a difference in opinion when I was not using those >>>>>>>> tools. >>>>>>>> My perspective changed after using Github and Gerrit etc. >>>>>>>> >>>>>>>> Github pull request and integrated public CI(Travis, Shippable , >>>>>>>> codecov) makes collaboration easy. >>>>>>>> Currently, in patchwork, we can not assign a patch other than the >>>>>>>> set >>>>>>>> of maintainers. >>>>>>>> I think, it would help the review process if the more fine-grained >>>>>>>> owner will be responsible for specific >>>>>>>> patch set. >>>>>>> >>>>>>> The more fine-grain is achieved with Cc in mail. >>>>>>> But I understand not everybody knows/wants/can configure correctly >>>>>>> an email client. Emails are not easy for everybody, I agree. >>>>>>> >>>>>>> I use GitHub as well, and I really prefer the clarity of the mail >>>>>>> threads. >>>>>>> GitHub reviews tend to be line-focused, messy and not >>>>>>> discussion-friendly. >>>>>>> I think contribution quality would be worst if using GitHub. >>>>>> >>>>>> I have more experience with Gitlab than Github, but i really don't see >>>>>> it that way. >>>>>> >>>>>> For one, reviewing in Gitlab makes it easier to see context in which >>>>>> changes appear. I mean, obviously, you can download the patch, >>>>>> apply it, >>>>>> and then do whatever you want with it in your editor/IDE, but it's >>>>>> just >>>>>> so much faster to do it right in the browser. Reviewing things with >>>>>> proper syntax highlighting and side-by-side diff with an option to see >>>>>> more context really makes a huge difference and is that much faster. >>>>> >>>>> OK >>>>> >>>>> >>>>>> I would also vehemently disagree with the "clarity" argument. There is >>>>>> enforced minimum standard of clarity of discussion in a tool such as >>>>>> Gitlab. I'm sure you noticed that some people top-post, some >>>>>> bottom-post. Some will remove extraneous lines of patches while some >>>>>> will leave on comment in a 10K line patch and leave the rest as is, in >>>>>> quotes. Some people do weird quoting where they don't actually >>>>>> quote but >>>>>> just copy text verbatim, making it hard to determine where the quote >>>>>> starts. If the thread is long enough, you'd see the same text quoted >>>>>> over and over and over. All of that is not a problem within a single >>>>>> patch email, but it adds up to lots of wasted time on all sides. >>>>> >>>>> Yes >>>>> >>>>> My concern about clarity is the history of the discussion. >>>>> When we post a new versions in GitHub, it's very hard to keep track >>>>> of the history. >>>>> As a maintainer, I need to see the history to understand what happened, >>>>> what we are waiting for, and what should be merged. >>>> >>>> IMO, The complete history is available per pull request URL. >>>> I think, Github also email notification mechanism those to prefer to see >>>> comments in the email too. >>>> >>>> In addition to that, Bugzilla, patchwork, CI stuff all integrated into >>>> one place. >>>> I am quite impressed with vscode community collaboration. >>>> https://github.com/Microsoft/vscode/pulls >>> >>> Out of curiosity, just checked the git history and I'm not that >>> impressed. For example last commit on the master branch: >>> >>> https://github.com/microsoft/vscode/commit/2a4cecf3f2f72346d06990feeb7446b3915d6148 >>> >>> >>> Commit title: " Fix #98530 " >>> Commit message empty, no explanation on what the patch is doing. >>> >>> Then, let's check the the issue it is pointed to: >>> https://github.com/microsoft/vscode/issues/98530 >>> >>> Issue is created 15 minutes before the patch is being merged. All that >>> done by the same contributor, without any review. >>> >> >> Just because they do it wrong doesn't mean we can't do it right :) This >> says more about Microsoft's lack of process around VSCode than it does >> about Github the tool. >> > > True. I was just pointing out that is not the kind of process I would > personally want to adopt. > You won't find disagreement here, but this "process" is not due to the tool. You can just as well allow Thomas to merge stuff without any review because he has commit rights, no Github needed - and you would be faced with the same problem. So, i don't think Jerin was suggesting that we degrade our merge/commit rules. Rather, the point was that (whatever you think of VSCode's review/merge process) there are a lot of pull requests and there is healthy community collaboration. I'm not saying we don't have that, obviously, but i have a suspicion that we'll get more of it if we lower the barrier for entry (not the barrier for merge!). I think there is a way to lower the secondary skill level needed to contribute to DPDK without lowering coding/merge standards with it. -- Thanks, Anatoly