> On July 10, 2014, 4:42 p.m., Christopher Tubbs wrote: > > -1 > > There's absolutely no reason to create a profile and options to skip the > > execution of the plugin since there is a built-in mechanism for controlling > > the rat check's impact on the build (eg. by setting rat.ignoreErrors). This > > change unnecessarily complicates the pom files, with insufficient utility > > to justify it, because the rat check is relatively fast, and there's > > already a mechanism to ignore its results. Even if we were able to come to > > consensus on defaulting rat.ignoreErrors to true, I'd still be opposed to > > the added profiles and property to control that profile, due to this added > > complexity with almost no utility. > > Sean Busbey wrote: > Even with ignore errors on, the plugin itself running introduces overhead > for those attempting to iterate tightly on e.g. a particular IT or unit test. > > "Relatively fast" is subjective. I know that both myself and Keith have > complained about the overhead in the past. > > John Vines wrote: > Review board is not a place for discussions about whether or not work > should be done. This is a place for analyzing patches, plain and simple. > Please take this conversation to JIRA. > > Christopher Tubbs wrote: > FWIW, I just did 'mvn validate' with and without rat defined in the pom, > and it was a total of 3 seconds difference (~7 seconds, vs. ~4 seconds). > > I'd concede to moving the existing rat plugin moved to a single profile, > so that it could be disabled with "-P '!rat'" (I think that syntax is > correct) or with the presence of a property. That would allow disabling, for > tight iterations of tests, but with minimal added complexity. The other > option would be to patch rat upstream to have a proper skip (rather than only > a process-and-ignore) option. > > Sean Busbey wrote: > latest patch moves to a single profile.
The second patch looks good to me, regarding the profile, but I'm still concerned about the rat.ignoreErrors=true by default. I think that warrants further discussion on the mailing list, and I commented on the prior thread. So, I'm +1 to this profile arrangement, but -1 to rat.ignoreErrors=true. Do you want to split into two patches (the first without the rat.ignoreErrors property) and apply the first until consensus is reached on that issue? Because, I wouldn't object to that. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23391/#review47614 ----------------------------------------------------------- On July 10, 2014, 6:10 p.m., Sean Busbey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23391/ > ----------------------------------------------------------- > > (Updated July 10, 2014, 6:10 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2986 > https://issues.apache.org/jira/browse/ACCUMULO-2986 > > > Repository: accumulo > > > Description > ------- > > * puts rat plugin into profile > * activates profile by default > * ignores rat errrors by default > > > Diffs > ----- > > pom.xml 2bc87cf082dfeb7bfe1a3fac1fe4fba1eaa87edd > > Diff: https://reviews.apache.org/r/23391/diff/ > > > Testing > ------- > > verified rat warnings and failures given move from master -> 1.6.1-SNAPSHOT > branch with no definitions, -Drat.skip=true, -Drat.skip=false, > -Drat.ignoreErrors=true, and profile to check errors in ~/.m2/settings.xml > > > Thanks, > > Sean Busbey > >
