This goes back to the original discussion in this thread - reduce the amount of things pull requesters should know and keep the maven command in the PR checklist as: 'mvn clean verify'.
So if rat and findbugs do not take that long to run I think they should be run by 'mvn clean verify' I ran a quick test on my laptop to see how much time they add to the build (of the entire project): 'mvn clean install -DskipTests' => Total time: 03:51 min 'mvn clean install apache-rat:check findbugs:check -DskipTests' => Total time: 05:29 min (Added 01:38 min) 'mvn clean install' => Total time: 09:37 min 'mvn clean install apache-rat:check findbugs:check' => Total time: 11:13 min (Added 01:36 min) Are these times reasonable enough to add rat and findbugs to the default build? On Fri, Feb 10, 2017 at 1:55 PM Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > Hi > > We discussed about that at the beginning of the project. We agreed to > execute rat and findbugs in a specific profile to reduce the build time for > dev. > > That's why I do mvn clean install -Prelease before submitting a PR and > just clean install when I'm developing. > > No problem to change that. > > Regards > JB > > On Feb 10, 2017, 07:51, at 07:51, Aviem Zur <aviem...@gmail.com> wrote: > >Can we consider adding rat-plugin and findbugs to the default verify > >phase? > >Currently they only run when the `release` profile is enabled. > > > >On Thu, Jan 26, 2017 at 11:42 AM Aljoscha Krettek <aljos...@apache.org> > >wrote: > > > >> +1 to what Dan said > >> > >> On Wed, 25 Jan 2017 at 21:40 Kenneth Knowles <k...@google.com.invalid> > >> wrote: > >> > >> > +1 > >> > > >> > On Jan 25, 2017 11:15, "Jean-Baptiste Onofré" <j...@nanthrax.net> > >wrote: > >> > > >> > > +1 > >> > > > >> > > It sounds good to me. > >> > > > >> > > Thanks Dan ! > >> > > > >> > > Regards > >> > > JB > >> > > > >> > > On Jan 25, 2017, 19:39, at 19:39, Dan Halperin > >> > <dhalp...@google.com.INVALID> > >> > > wrote: > >> > > >Here is my summary of the threads: > >> > > > > >> > > >Overwhelming agreement: > >> > > > > >> > > >- rename `release` to something more appropriate. > >> > > >- add `checkstyle` to the default build (it's basically a > >compile > >> > > >error) > >> > > >- add more information to contributor guide > >> > > > > >> > > >Reasonable agreement > >> > > > > >> > > >- don't update the github instructions to make passing `mvn > >verify > >> > > >-P<all > >> > > >checks>` mandatory. Maybe add a hint that this is a good proxy > >for > >> what > >> > > >Jenkins will run. > >> > > > > >> > > >Unresolved: > >> > > > > >> > > >- whether all checks should be in `mvn verify` > >> > > >- whether `mvn test` is useful for most workflows > >> > > > > >> > > >I'll propose to proceed with the overwhelmingly agreed-upon > >changes, > >> > > >and as > >> > > >we see increasingly many new contributors re-evaluate the > >remaining > >> > > >issues. > >> > > > > >> > > >Thanks, > >> > > >Dan > >> > > > > >> > > >On Tue, Jan 24, 2017 at 12:51 PM, Jean-Baptiste Onofré > >> > > ><j...@nanthrax.net> > >> > > >wrote: > >> > > > > >> > > >> +1 to at least update the contribution guide and improve the > >profile > >> > > >name. > >> > > >> > >> > > >> Regards > >> > > >> JB > >> > > >> > >> > > >> > >> > > >> On 01/24/2017 09:49 PM, Kenneth Knowles wrote: > >> > > >> > >> > > >>> My impression is that we don't have consensus on whether all > >checks > >> > > >or > >> > > >>> minimal checks should be the default, or whether we can have > >both > >> > > >via `mvn > >> > > >>> test` and `mvn verify`. > >> > > >>> > >> > > >>> But that doesn't prevent us from giving -P release a better > >name > >> and > >> > > >>> mentioning it in the dev guide and in some manner in our PR > >> > > >template. > >> > > >>> > >> > > >>> Right now we are living with the combination of the bad > >aspects - > >> > > >default > >> > > >>> is not thorough but not actually fast and a thorough check is > >> > > >>> undocumented. > >> > > >>> > >> > > >>> On Tue, Jan 24, 2017 at 2:22 AM, Ismaël Mejía > ><ieme...@gmail.com> > >> > > >wrote: > >> > > >>> > >> > > >>> I just wanted to know if we have achieved some consensus > >about > >> this, > >> > > >I > >> > > >>>> just > >> > > >>>> saw this PR that reminded me about this discussion. > >> > > >>>> > >> > > >>>> https://github.com/apache/beam/pull/1829 > >> > > >>>> > >> > > >>>> It is important that we mention the existing profiles (and > >the > >> > > >intended > >> > > >>>> checks) in the contribution guide (e.g. -Prelease (or > >-Pall-checks > >> > > >>>> triggers > >> > > >>>> these validations). > >> > > >>>> > >> > > >>>> I can add this to the guide if you like once we define the > >checks > >> > > >per > >> > > >>>> stage/profile. > >> > > >>>> > >> > > >>>> Ismaël > >> > > >>>> > >> > > >>>> > >> > > >>>> On Wed, Jan 11, 2017 at 8:12 AM, Aviem Zur > ><aviem...@gmail.com> > >> > > >wrote: > >> > > >>>> > >> > > >>>> I agree with Dan and Lukasz. > >> > > >>>>> Developers should not be expected to know beforehand which > >> > > >specific > >> > > >>>>> profiles to run. > >> > > >>>>> The phase specified in the PR instructions (`verify`) > >should run > >> > > >all the > >> > > >>>>> relevant verifications and be the "slower" build, while a > >> > > >preceding > >> > > >>>>> lifecycle, such as `test`, should run the "faster" > >verifications. > >> > > >>>>> > >> > > >>>>> Aviem. > >> > > >>>>> > >> > > >>>>> On Mon, Jan 9, 2017 at 7:57 PM Robert Bradshaw > >> > > >>>>> > >> > > >>>> <rober...@google.com.invalid > >> > > >>>> > >> > > >>>>> > >> > > >>>>>> wrote: > >> > > >>>>> > >> > > >>>>> On Mon, Jan 9, 2017 at 3:49 AM, Aljoscha Krettek > >> > > ><aljos...@apache.org> > >> > > >>>>>> wrote: > >> > > >>>>>> > >> > > >>>>>>> I also usually prefer "mvn verify" to to the expected > >thing but > >> > > >I see > >> > > >>>>>>> > >> > > >>>>>> that > >> > > >>>>>> > >> > > >>>>>>> quick iteration times are key. > >> > > >>>>>>> > >> > > >>>>>> > >> > > >>>>>> I see > >> > > >>>>>> https://maven.apache.org/guides/introduction/ > >> > > >>>>>> > >> > > >>>>> introduction-to-the-lifecycle.html > >> > > >>>>> > >> > > >>>>>> > >> > > >>>>>> verify - run any checks on results of integration > >tests to > >> > > >ensure > >> > > >>>>>> quality criteria are met > >> > > >>>>>> > >> > > >>>>>> Of course our integration tests are long enough that we > >> shouldn't > >> > > >be > >> > > >>>>>> putting all of them here, but I too would expect > >checkstyle. > >> > > >>>>>> > >> > > >>>>>> Perhaps we could introduce a verify-fast or somesuch for > >fast > >> > > >(but > >> > > >>>>>> lower coverage) turnaround time. I would expect "mvn > >verify > >> test" > >> > > >to > >> > > >>>>>> pass before submitting a PR, and would want to run that > >before > >> > > >asking > >> > > >>>>>> others to look at it. I think this should be our criteria > >(i.e. > >> > > >what > >> > > >>>>>> will a new but maven-savvy user run before pushing their > >code). > >> > > >>>>>> > >> > > >>>>>> As long as the pre-commit hooks still check everything I'm > >ok > >> > > >with > >> > > >>>>>>> > >> > > >>>>>> making > >> > > >>>>> > >> > > >>>>>> the default a little more lightweight. > >> > > >>>>>>> > >> > > >>>>>> > >> > > >>>>>> The fact that our pre-commit hooks take a long time to run > >does > >> > > >change > >> > > >>>>>> things. Nothing more annoying than seeing that your PR > >failed 3 > >> > > >hours > >> > > >>>>>> later because you had some trailing whitespace... > >> > > >>>>>> > >> > > >>>>>> On Thu, 5 Jan 2017 at 21:49 Lukasz Cwik > >> > > ><lc...@google.com.invalid> > >> > > >>>>>>> > >> > > >>>>>> wrote: > >> > > >>>>>> > >> > > >>>>>>> > >> > > >>>>>>> I was hoping that the default mvn verify would be the > >slow > >> build > >> > > >>>>>>>> > >> > > >>>>>>> and a > >> > > >>>> > >> > > >>>>> profile could be enabled that would skip checks to make > >things > >> > > >>>>>>>> > >> > > >>>>>>> faster > >> > > >>>> > >> > > >>>>> for > >> > > >>>>>> > >> > > >>>>>>> regular contributors. This way a person doesn't need to > >have > >> > > >>>>>>>> > >> > > >>>>>>> detailed > >> > > >>>> > >> > > >>>>> knowledge of all our profiles and what they do (typically > >mvn > >> > > >>>>>>>> > >> > > >>>>>>> verify) > >> > > >>>> > >> > > >>>>> will > >> > > >>>>>> > >> > > >>>>>>> do the right thing most of the time. > >> > > >>>>>>>> > >> > > >>>>>>>> On Thu, Jan 5, 2017 at 9:30 AM, Dan Halperin > >> > > >>>>>>>> > >> > > >>>>>>> <dhalp...@google.com.invalid> > >> > > >>>>>> > >> > > >>>>>>> wrote: > >> > > >>>>>>>> > >> > > >>>>>>>> On Thu, Jan 5, 2017 at 9:28 AM, Jesse Anderson < > >> > > >>>>>>>>> > >> > > >>>>>>>> je...@smokinghand.com > >> > > >>>>> > >> > > >>>>>> > >> > > >>>>>>> wrote: > >> > > >>>>>>>>> > >> > > >>>>>>>>> @dan are you saying that mvn verify isn't doing > >checkstyle > >> > > >>>>>>>>>> > >> > > >>>>>>>>> anymore? > >> > > >>>>> > >> > > >>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>> `mvn verify` alone should not be running checkstyle, if > >> > > >modules > >> > > >>>>>>>>> > >> > > >>>>>>>> are > >> > > >>>> > >> > > >>>>> configured correctly. > >> > > >>>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>> Some of > >> > > >>>>>>>>>> the checkstyles are still running for a few modules. > >Also, > >> > > >the > >> > > >>>>>>>>>> > >> > > >>>>>>>>> contribution > >> > > >>>>>>>>> > >> > > >>>>>>>>>> docs will need to change. > >> > > >>>>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>> Yes. The PR includes discussion of these other needed > >> changes, > >> > > >>>>>>>>> unfortunately one PR can't change two repositories. > >> > > >>>>>>>>> > >> > > >>>>>>>>> Please continue the discussion on the PR, then I will > >> > > >summarize it > >> > > >>>>>>>>> > >> > > >>>>>>>> back > >> > > >>>>>> > >> > > >>>>>>> into the dev thread. > >> > > >>>>>>>>> > >> > > >>>>>>>>> Thanks, > >> > > >>>>>>>>> Dan > >> > > >>>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>> They say to run mvn verify before commits. > >> > > >>>>>>>>>> > >> > > >>>>>>>>>> On Thu, Jan 5, 2017 at 9:25 AM Dan Halperin > >> > > >>>>>>>>>> > >> > > >>>>>>>>> <dhalp...@google.com.invalid > >> > > >>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>>>> wrote: > >> > > >>>>>>>>>> > >> > > >>>>>>>>>> Several folks seem to have been confused after > >BEAM-246, > >> > > >where > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>> we > >> > > >>>>> > >> > > >>>>>> moved > >> > > >>>>>>>> > >> > > >>>>>>>>> the > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> "slow things" into the release profile. I've started > >a > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>> discussion > >> > > >>>>> > >> > > >>>>>> with > >> > > >>>>>>>> > >> > > >>>>>>>>> https://github.com/apache/beam/pull/1740 to see if > >there are > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>> things > >> > > >>>>>> > >> > > >>>>>>> we > >> > > >>>>>>>> > >> > > >>>>>>>>> can > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> do to fill these gaps. > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>> Would love folks to chime in with opinions. > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>> Dan > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>> On Wed, Jan 4, 2017 at 1:34 PM, Jesse Anderson < > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>> je...@smokinghand.com> > >> > > >>>>>>>> > >> > > >>>>>>>>> wrote: > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>> @Eugene, yes that failed on the checkstyle. > >> > > >>>>>>>>>>>> > >> > > >>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:27 PM Eugene Kirpichov > >> > > >>>>>>>>>>>> <kirpic...@google.com.invalid> wrote: > >> > > >>>>>>>>>>>> > >> > > >>>>>>>>>>>> Try just -Prelease. > >> > > >>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:21 PM Jesse Anderson < > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>> je...@smokinghand.com > >> > > >>>>>>>>> > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> wrote: > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>>> Fails because I don't have a secret key. > >> > > >>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:03 PM Jean-Baptiste > >Onofré < > >> > > >>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>> j...@nanthrax.net > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>>> wrote: > >> > > >>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> Hi Jesse, > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> Could you try the same with: > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> mvn verify -Prelease,apache-release > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> ? > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> Regards > >> > > >>>>>>>>>>>>>>> JB > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> On 01/04/2017 09:53 PM, Jesse Anderson wrote: > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> For some reason, running "mvn verify" isn't > >running > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> checkstyle > >> > > >>>>>>>>> > >> > > >>>>>>>>>> on > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> everything. I had checkstyle errors in > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> beam-sdks-java-core > >> > > >>>>>> > >> > > >>>>>>> that > >> > > >>>>>>>>> > >> > > >>>>>>>>>> weren't > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> being found. > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> I thought this was due to the extra parameters. > >I > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> reran > >> > > >>>>> > >> > > >>>>>> with > >> > > >>>>>>>> > >> > > >>>>>>>>> the > >> > > >>>>>>>>>> > >> > > >>>>>>>>>>> plain > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> "mvn > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> verify" and it still didn't find them. From the > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> output, > >> > > >>>>> > >> > > >>>>>> it > >> > > >>>>>> > >> > > >>>>>>> doesn't > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>>>> look > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> like they're being run at all. > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> Thanks, > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> Jesse > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> -- > >> > > >>>>>>>>>>>>>>> Jean-Baptiste Onofré > >> > > >>>>>>>>>>>>>>> jbono...@apache.org > >> > > >>>>>>>>>>>>>>> http://blog.nanthrax.net > >> > > >>>>>>>>>>>>>>> Talend - http://www.talend.com > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>>> > >> > > >>>>>>>>>>>>> > >> > > >>>>>>>>>>>> > >> > > >>>>>>>>>>> > >> > > >>>>>>>>>> > >> > > >>>>>>>>> > >> > > >>>>>>>> > >> > > >>>>>> > >> > > >>>>> > >> > > >>>> > >> > > >>> > >> > > >> -- > >> > > >> Jean-Baptiste Onofré > >> > > >> jbono...@apache.org > >> > > >> http://blog.nanthrax.net > >> > > >> Talend - http://www.talend.com > >> > > >> > >> > > > >> > > >> >
