Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 9435311A03 for ; Thu, 10 Jul 2014 22:09:09 +0000 (UTC) Received: (qmail 2698 invoked by uid 500); 10 Jul 2014 22:09:09 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 2653 invoked by uid 500); 10 Jul 2014 22:09:09 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 2636 invoked by uid 99); 10 Jul 2014 22:09:09 -0000 Received: from minotaur.apache.org (HELO minotaur.apache.org) (140.211.11.9) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 10 Jul 2014 22:09:09 +0000 Received: from localhost (HELO mail-la0-f41.google.com) (127.0.0.1) (smtp-auth username ctubbsii, mechanism plain) by minotaur.apache.org (qpsmtpd/0.29) with ESMTP; Thu, 10 Jul 2014 22:09:08 +0000 Received: by mail-la0-f41.google.com with SMTP id hz20so200253lab.28 for ; Thu, 10 Jul 2014 15:09:06 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.112.155.129 with SMTP id vw1mr41295087lbb.7.1405030146618; Thu, 10 Jul 2014 15:09:06 -0700 (PDT) Received: by 10.114.186.74 with HTTP; Thu, 10 Jul 2014 15:09:06 -0700 (PDT) In-Reply-To: References: Date: Thu, 10 Jul 2014 18:09:06 -0400 Message-ID: Subject: Re: moving rat to a profile? From: Christopher To: Accumulo Dev List Content-Type: multipart/alternative; boundary=089e01183046c9bfb104fdde1278 --089e01183046c9bfb104fdde1278 Content-Type: text/plain; charset=UTF-8 Since this came up in Review (https://reviews.apache.org/r/23391/), I'm revisiting this thread. In short, it seems there was some confusion on whether or not we had reached consensus on enabling rat by default, with rat.ignoreErrors=true set to default, and requiring any entities that should be responsible for checking legal status of contributions to turn it in (set rat.ignoreErrors=false for Jenkins, committers, etc.) I think one problem is that there was not actually consensus. This was a proposal made previously by Bill Havanki to do the above, after which we agreed to minimally making changes to make "git clean -df" sufficient. But there did not appear to be any consensus reached on disabling error checking by default, as proposed. Billie had expressed concerns about encouraging committers to not take care in checking the legal status of their commits, and not only do I share those concerns, I'd also extend my concerns to any contributor. This is simply something that everybody contributing to ASF should be aware of. Committers have the extra responsibility, but even contributors and potential committers should understand the contribution requirements, the development process, and have put in some minimal amount of thought about whether they have the ability to contribute a particular piece of code legally. Additionally, I'm concerned about who we'd be easing things for. There's clearly an increased risk of commits/contributions without strict checking for licenses (we've seen past commits with these errors... just do `git log | grep -i license` to see that). So, we're accepting that risk by making the easy route one that serves who, exactly? The ASF makes it quite clear that SCM is for developers, not end users, and this is an SCM-specific problem (specifically branch switching in git). So, I do not think there is consensus. There are certainly outstanding concerns for disabling the rat check by default. However, I think there is room for further discussion and compromise, and I'd love to consider more solutions (or discuss the merits of the proposed solutions). For instance, one area of compromise I've conceded (as stated in the review) is that I think it'd be a good idea to put the rat check in a profile which can be optionally disabled, so that it doesn't take the 3 extra seconds to run, which could impact some iterative testing (like running one IT at a time, or doing automated `git bisect`). -- Christopher L Tubbs II http://gravatar.com/ctubbsii On Thu, Jun 19, 2014 at 11:15 AM, Christopher wrote: > Agreed. That's a minimum. > > > -- > Christopher L Tubbs II > http://gravatar.com/ctubbsii > > > On Thu, Jun 19, 2014 at 10:54 AM, Bill Havanki > wrote: > >> I've filed ACCUMULO-2927 to make 'git clean -df' sufficient. No matter how >> we decide about the rat plugin, I think not requiring -x is a worthwhile >> goal. >> >> >> On Tue, Jun 17, 2014 at 5:43 PM, Christopher wrote: >> >> > I agree that instructing users to use this option to modify the build >> isn't >> > acceptable and I wouldn't recommend this as a response to users... I was >> > only stating this as a fact, to point out that a special profile on by >> > default with an option to disable isn't needed, since that's the current >> > behavior. >> > >> > I'm more interested in the targeted .gitignore with the recommended "git >> > clean -df" option without -x. This helps contributors understand build >> > tools, makes them aware of the differences between branches, and doesn't >> > hide problems introduced by switching branches in an obscure error, all >> > without blowing away their IDE build files. (though switching branches >> > often warrants blowing these IDE files away anyway, since different >> modules >> > in different branches will be problematic for most IDEs). >> > >> > >> > -- >> > Christopher L Tubbs II >> > http://gravatar.com/ctubbsii >> > >> > >> > On Tue, Jun 17, 2014 at 4:47 PM, Alex Moundalexis < >> alexm@clouderagovt.com> >> > wrote: >> > >> > > This kind of response is hardly conducive to prospective contributors. >> > > >> > > We should consider ourselves lucky whenever a contributor provides a >> > patch, >> > > let alone runs a build. Expecting a new contributor be fully aware of >> the >> > > Apache licensing details isn't realistic, much less being aware of the >> > > arguments concerning Rat; if the ignoreErrors argument is TheWay, it >> > ought >> > > to be mentioned prominently in the source documentation [1], but I >> don't >> > > think that's correct either... >> > > >> > > I don't want to encourage contributors to skip the build. I want >> > > contributors to be aware of the licensing requirements, but not at the >> > > expense of providing otherwise-viable patches. I'd recommend relaxing >> the >> > > Rat checks for contributors, and making it a required part of the >> profile >> > > for automated Jenkins builds and during the release process. >> > > >> > > The onus should be on the committers to ensure that all of the >> licensing >> > is >> > > in place before the release, but preferably long before that point by >> > > guiding the contributor to make the necessary license additions before >> > the >> > > commit. >> > > >> > > I've been told to correct whitespace at the end of a line before and >> to >> > > re-submit a patch, seems trivial to address missing licensing files in >> > the >> > > same way. >> > > >> > > [1] https://accumulo.apache.org/source.html >> > > >> > > On Tue, Jun 17, 2014 at 3:15 PM, Christopher >> > wrote: >> > > >> > > > There's already a way to skip it for those who don't understand why >> its >> > > > failing and are incapable/unwilling to troubleshoot: >> > > > -Drat.ignoreErrors=true >> > > > >> > > > >> > > > -- >> > > > Christopher L Tubbs II >> > > > http://gravatar.com/ctubbsii >> > > > >> > > > >> > > > On Tue, Jun 17, 2014 at 3:09 PM, Billie Rinaldi < >> > > billie.rinaldi@gmail.com> >> > > > wrote: >> > > > >> > > > > I'm not thrilled about turning it off by default. How about >> putting >> > it >> > > > in >> > > > > a profile that would be enabled by default, but could be disabled >> > with >> > > a >> > > > > flag for those who don't understand why it's failing? >> > > > > >> > > > > >> > > > > On Tue, Jun 17, 2014 at 11:44 AM, Sean Busbey < >> busbey@cloudera.com> >> > > > wrote: >> > > > > >> > > > > > I've had a few different new-to-Accumulo contributors recently >> run >> > > into >> > > > > the >> > > > > > issue of Rat failing the build after changing branches. >> > > > > > >> > > > > > I know we already have a warning about this[1], but AFAICT it's >> > over >> > > > the >> > > > > > threshold for consumable information. >> > > > > > >> > > > > > Even after pointing people to the warning, the existing >> workaround >> > > > > tripped >> > > > > > up atleast one of them. Despite the warning about using "git >> > clean," >> > > > the >> > > > > > destruction of their local IDE changes were surprising. >> > > > > > >> > > > > > For contributions to Accumulo that aren't coming from >> committers, >> > the >> > > > Rat >> > > > > > plugin seems much more likely to give a false positive than to >> > catch >> > > an >> > > > > > error. Additionally, whatever committer is reviewing the >> > contribution >> > > > > > should be checking for license compliance anyways. >> > > > > > >> > > > > > In the interests of reducing the surprise for new contributors, >> I'd >> > > > like >> > > > > to >> > > > > > move our use of Rat to a profile that is only default enabled >> > during >> > > a >> > > > > > release run. >> > > > > > >> > > > > > The profile would still let those who want rat to run on every >> > build >> > > to >> > > > > > enable it and we could update the guide for handling new >> > > contributions >> > > > to >> > > > > > say committers should enable the rat profile to help guard >> against >> > > > > errors. >> > > > > > >> > > > > > Any objections? >> > > > > > >> > > > > > [1]: http://accumulo.apache.org/source.html#running-a-build >> > > > > > >> > > > > > -- >> > > > > > Sean >> > > > > > >> > > > > >> > > > >> > > >> > >> >> >> >> -- >> // Bill Havanki >> // Solutions Architect, Cloudera Govt Solutions >> // 443.686.9283 >> > > --089e01183046c9bfb104fdde1278--