Yeah. Agree. Time extend is not huge and it's worth to add it in verify phase.
Regards JB On Feb 10, 2017, 10:13, at 10:13, Aviem Zur <aviem...@gmail.com> wrote: >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 >> >> > > >> >> >> > > >> >> > >> >> >>
