Opened JIRA ticket: https://issues.apache.org/jira/browse/BEAM-1457
On Fri, Feb 10, 2017 at 4:54 PM Jean-Baptiste Onofré <[email protected]> wrote: > 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 <[email protected]> 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é <[email protected]> > >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 <[email protected]> > >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 > ><[email protected]> > >> >wrote: > >> > > >> >> +1 to what Dan said > >> >> > >> >> On Wed, 25 Jan 2017 at 21:40 Kenneth Knowles > ><[email protected]> > >> >> wrote: > >> >> > >> >> > +1 > >> >> > > >> >> > On Jan 25, 2017 11:15, "Jean-Baptiste Onofré" <[email protected]> > >> >wrote: > >> >> > > >> >> > > +1 > >> >> > > > >> >> > > It sounds good to me. > >> >> > > > >> >> > > Thanks Dan ! > >> >> > > > >> >> > > Regards > >> >> > > JB > >> >> > > > >> >> > > On Jan 25, 2017, 19:39, at 19:39, Dan Halperin > >> >> > <[email protected]> > >> >> > > 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é > >> >> > > ><[email protected]> > >> >> > > >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 > >> ><[email protected]> > >> >> > > >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 > >> ><[email protected]> > >> >> > > >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 > >> >> > > >>>>> > >> >> > > >>>> <[email protected] > >> >> > > >>>> > >> >> > > >>>>> > >> >> > > >>>>>> wrote: > >> >> > > >>>>> > >> >> > > >>>>> On Mon, Jan 9, 2017 at 3:49 AM, Aljoscha Krettek > >> >> > > ><[email protected]> > >> >> > > >>>>>> 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 > >> >> > > ><[email protected]> > >> >> > > >>>>>>> > >> >> > > >>>>>> 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 > >> >> > > >>>>>>>> > >> >> > > >>>>>>> <[email protected]> > >> >> > > >>>>>> > >> >> > > >>>>>>> wrote: > >> >> > > >>>>>>>> > >> >> > > >>>>>>>> On Thu, Jan 5, 2017 at 9:28 AM, Jesse Anderson < > >> >> > > >>>>>>>>> > >> >> > > >>>>>>>> [email protected] > >> >> > > >>>>> > >> >> > > >>>>>> > >> >> > > >>>>>>> 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 > >> >> > > >>>>>>>>>> > >> >> > > >>>>>>>>> <[email protected] > >> >> > > >>>>>>>> > >> >> > > >>>>>>>>> > >> >> > > >>>>>>>>>> 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 < > >> >> > > >>>>>>>>>>> > >> >> > > >>>>>>>>>> [email protected]> > >> >> > > >>>>>>>> > >> >> > > >>>>>>>>> wrote: > >> >> > > >>>>>>>>>>> > >> >> > > >>>>>>>>>>> @Eugene, yes that failed on the checkstyle. > >> >> > > >>>>>>>>>>>> > >> >> > > >>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:27 PM Eugene Kirpichov > >> >> > > >>>>>>>>>>>> <[email protected]> wrote: > >> >> > > >>>>>>>>>>>> > >> >> > > >>>>>>>>>>>> Try just -Prelease. > >> >> > > >>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:21 PM Jesse Anderson < > >> >> > > >>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>> [email protected] > >> >> > > >>>>>>>>> > >> >> > > >>>>>>>>>> > >> >> > > >>>>>>>>>>> wrote: > >> >> > > >>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>> Fails because I don't have a secret key. > >> >> > > >>>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:03 PM Jean-Baptiste > >> >Onofré < > >> >> > > >>>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>> [email protected] > >> >> > > >>>>>>>>>> > >> >> > > >>>>>>>>>>> > >> >> > > >>>>>>>>>>>> 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é > >> >> > > >>>>>>>>>>>>>>> [email protected] > >> >> > > >>>>>>>>>>>>>>> http://blog.nanthrax.net > >> >> > > >>>>>>>>>>>>>>> Talend - http://www.talend.com > >> >> > > >>>>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>>> > >> >> > > >>>>>>>>>>>> > >> >> > > >>>>>>>>>>> > >> >> > > >>>>>>>>>> > >> >> > > >>>>>>>>> > >> >> > > >>>>>>>> > >> >> > > >>>>>> > >> >> > > >>>>> > >> >> > > >>>> > >> >> > > >>> > >> >> > > >> -- > >> >> > > >> Jean-Baptiste Onofré > >> >> > > >> [email protected] > >> >> > > >> http://blog.nanthrax.net > >> >> > > >> Talend - http://www.talend.com > >> >> > > >> > >> >> > > > >> >> > > >> >> > >> >
