ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Вячеслав Коптилин <slava.kopti...@gmail.com>
Subject Re: Checkstyle fails Build Apache Ignite - can we split it?
Date Tue, 07 Jul 2020 10:35:50 GMT
Hello Alex,

> In my opinion checkstyle rules definitely should be checked as early as
possible and by default should fail Ignite build.
Well, perhaps I am wrong. Ok.

> But for exceptional cases (which was mentioned by Slava), can we add some
> "Run custom build" parameter on TC to be able to exclude "checkstyle"
> profile from "~Build Apache Ignite~" configuration? WDYT?
This is exactly what I wanted to say. Thank you, Alex!

Regards,
Slava.

вт, 7 июл. 2020 г. в 13:01, Alex Plehanov <plehanov.alex@gmail.com>:

> Guys,
>
> In my opinion checkstyle rules definitely should be checked as early as
> possible and by default should fail Ignite build.
> But for exceptional cases (which was mentioned by Slava), can we add some
> "Run custom build" parameter on TC to be able to exclude "checkstyle"
> profile from "~Build Apache Ignite~" configuration? WDYT?
>
> вт, 7 июл. 2020 г. в 14:53, Вячеслав Коптилин <slava.koptilin@gmail.com>:
>
> > Hi Maxim,
> >
> > > Why the auto-formatting procedure cannot be used for any PR you'd like
> to
> > run on TC?
> > > Even more, you can remove all the rules from checkstyle XML config and
> do
> > all you want in the PR branch
> > Yes, I can enable auto-formatting, which should be checked anyway, I can
> > edit pom.xml every time, etc.
> >
> > My question is the following: Why can't this check be done in parallel
> with
> > other tasks?
> >
> > Yep, I see the argument mentioned by Nikolay - TeamCity capacity (we
> should
> > re-run all tests even though we rename a variable), but I don't see that
> > our TC servers are overwhelmed for now.
> > Perhaps, I am wrong and this argument (TC capacity) is more significant
> > than it seems to me at first glance.
> >
> > Folks, please don't get me wrong. I am not against the code-style check
> at
> > all. I just want to get some freedom for dev branches only, if it is
> > possible of course.
> >
> > Thanks,
> > Slava.
> >
> > вт, 7 июл. 2020 г. в 12:21, Maxim Muzafarov <mmuzaf@apache.org>:
> >
> > > Slava,
> > >
> > > Why the auto-formatting procedure cannot be used for any PR you'd like
> > > to run on TC? Even more, you can remove all the rules from checkstyle
> > > XML config and do all you want in the PR branch.
> > >
> > > On Tue, 7 Jul 2020 at 12:17, Вячеслав Коптилин <
> slava.koptilin@gmail.com
> > >
> > > wrote:
> > > >
> > > > Hi Nikolay,
> > > >
> > > > > Why do you need to Run All on TC resources for this task?
> > > > For instance, it may be a race that cannot be reproduced locally on
> my
> > > own
> > > > hardware.
> > > > (By the way, even though if I just want to start Cache1 suite, the
> > > > Code-Style check executes anyway).
> > > >
> > > > > If we don’t want to follow the code style or considered it as a
> > «waste
> > > of
> > > > the time» we should change it.
> > > > This is absolutely not what I'm trying to say. I do not think, that
> > code
> > > > style is useless. I just want to say that this check can be done in
> > > > parallel for dev branches, for example.
> > > >
> > > > Thanks,
> > > > S.
> > > >
> > > > вт, 7 июл. 2020 г. в 11:49, Nikolay Izhikov <nizhikov@apache.org>:
> > > >
> > > > > >  Let's assume that I want to implement a dirty fix of a bug,
> check
> > a
> > > > > reproducer from the user list, etc.
> > > > >
> > > > > Why do you need to Run All on TC resources for this task?
> > > > >
> > > > > > I do not want to waste my time fixing all code style violations.
> > > > >
> > > > > I assume that you have the Ignite project locally because you edit
> > > Ignite
> > > > > source code.
> > > > > If yes, then IntelliJ IDEA has an automatic code formatting and
> does
> > > all
> > > > > the work for you.
> > > > >
> > > > > > Does this use-case look reasonable?
> > > > >
> > > > > Yes.
> > > > > But, I still don’t see what is the issue here.
> > > > >
> > > > > If we don’t want to follow the code style or considered it as a
> > «waste
> > > of
> > > > > the time» we should change it.
> > > > > As simple as that.
> > > > >
> > > > > But, we should force the code style as early as we can.
> > > > > I think we should fail maven local build on code style violations.
> > > > >
> > > > > > 7 июля 2020 г., в 11:28, Вячеслав Коптилин
<
> > slava.koptilin@gmail.com
> > > >
> > > > > написал(а):
> > > > > >
> > > > > > Nikolay,
> > > > > >
> > > > > > There is *NO *intention to ignore code style violations and
do
> > merge
> > > PRs
> > > > > > into the master branch without fixing them.
> > > > > >
> > > > > > Let's assume that I want to implement a dirty fix of a bug,
> check a
> > > > > > reproducer from the user list, etc.
> > > > > > And I do not have the intention to merge that into the master
> > branch
> > > as
> > > > > is,
> > > > > > however, I do not want to waste my time fixing all code style
> > > violations.
> > > > > > Does this use-case look reasonable?
> > > > > >
> > > > > > Thanks,
> > > > > > Slava.
> > > > > >
> > > > > > вт, 7 июл. 2020 г. в 11:07, Nikolay Izhikov <nizhikov@apache.org
> >:
> > > > > >
> > > > > >> Slava.
> > > > > >>
> > > > > >> All contributors should follow our code style during their
> > > contribution.
> > > > > >> For now, we have an automatic PR check that is integrated
to the
> > > GitHub
> > > > > >> interface.
> > > > > >>
> > > > > >> I don’t see any issue here.
> > > > > >> All open-source project that I know, uses the same approach.
> > > > > >>
> > > > > >> Anyway, If some of the experienced community members is
> interested
> > > in
> > > > > the
> > > > > >> particular contribution he or she can help a newcomer with
the
> > code
> > > > > style -
> > > > > >> GitHub interface has the edit button even for someone else’s
> PR’s
> > > > > >>
> > > > > >>
> > > > > >>> 7 июля 2020 г., в 11:01, Вячеслав Коптилин
<
> > > slava.koptilin@gmail.com>
> > > > > >> написал(а):
> > > > > >>>
> > > > > >>> Hello Nikolay,
> > > > > >>>
> > > > > >>>> Because any code style violations should be fixed
before the
> > > merge.
> > > > > >>>> Therefore after the fix, you must rerun TC.
> > > > > >>>> This means that running heavy suites when code style
is
> violated
> > > is a
> > > > > >>> waste of the resources.
> > > > > >>> This makes sense, however, to be honest, I don't see
that our
> > team
> > > city
> > > > > >>> servers are really busy.
> > > > > >>>
> > > > > >>>> Why do you want to violate code style rules in your
PR?
> > > > > >>> Please take a look at the original e-mail from Ilya:
> > > > > >>>> This means that I have completely lost an option
to run tests
> > > against
> > > > > >>> pull
> > > > > >>>> requests by new contributors - they usually compile
but will
> not
> > > pass
> > > > > >>>> Checkstyle. That's a blocker.
> > > > > >>>
> > > > > >>> This issue has also been discussed here:
> > > > > >>>
> > > > > >>
> > > > >
> > >
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html
> > > > > >>>
> > > > > >>> Thanks,
> > > > > >>> Slava.
> > > > > >>>
> > > > > >>>
> > > > > >>> вт, 7 июл. 2020 г. в 10:47, Nikolay Izhikov <
> nizhikov@apache.org
> > >:
> > > > > >>>
> > > > > >>>> All checks just force rules we agreed on.
> > > > > >>>>
> > > > > >>>>> Why this check is so important? Why do you think
it is more
> > > important
> > > > > >>>> than all other tasks/tests?
> > > > > >>>>
> > > > > >>>> Because any code style violations should be fixed
before the
> > > merge.
> > > > > >>>> Therefore after the fix, you must rerun TC.
> > > > > >>>> This means that running heavy suites when code style
is
> violated
> > > is a
> > > > > >>>> waste of the resources.
> > > > > >>>>
> > > > > >>>> Why do you want to violate code style rules in your
PR?
> > > > > >>>>
> > > > > >>>> I think instead of hiding style errors we should
make our code
> > > style
> > > > > >>>> comfortable for everyone.
> > > > > >>>> Can you, please, write - what part of the code style
hurts
> you?
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>> 7 июля 2020 г., в 10:39, Вячеслав
Коптилин <
> > > slava.koptilin@gmail.com
> > > > > >
> > > > > >>>> написал(а):
> > > > > >>>>>
> > > > > >>>>> Hello Maxim,
> > > > > >>>>>
> > > > > >>>>>> Why do you think that splitting the checkstyle
build is
> better
> > > > > option
> > > > > >>>>> than fixing code style issues reporting by the
checkstyle
> > plugin?
> > > > > >>>>> Why do you think that Ilya talks that code style
errors
> should
> > > not be
> > > > > >>>> fixed?
> > > > > >>>>>
> > > > > >>>>> It looks weird to me that we do not even start
the tests if
> > check
> > > > > style
> > > > > >>>>> plugin reports violations.
> > > > > >>>>> Why can't this check be done in parallel with
the tests and
> > > reported
> > > > > by
> > > > > >>>>> mtcga bot?
> > > > > >>>>> Why this check is so important? Why do you think
it is more
> > > important
> > > > > >>>> than
> > > > > >>>>> all other tasks/tests?
> > > > > >>>>>
> > > > > >>>>> Thanks,
> > > > > >>>>> Slava.
> > > > > >>>>>
> > > > > >>>>> пн, 6 июл. 2020 г. в 20:34, Maxim Muzafarov
<
> mmuzaf@apache.org
> > >:
> > > > > >>>>>
> > > > > >>>>>> Hello Ilya,
> > > > > >>>>>>
> > > > > >>>>>> Why do you think that splitting the checkstyle
build is
> better
> > > > > option
> > > > > >>>>>> than fixing code style issues reporting
by the checkstyle
> > > plugin?
> > > > > >>>>>>
> > > > > >>>>>> On Mon, 6 Jul 2020 at 19:43, Ilya Kasnacheev
<
> > > > > >> ilya.kasnacheev@gmail.com
> > > > > >>>>>
> > > > > >>>>>> wrote:
> > > > > >>>>>>>
> > > > > >>>>>>> Hello!
> > > > > >>>>>>>
> > > > > >>>>>>> I have just noticed today that Checkstyle
will fail Apache
> > > Ignite
> > > > > >>>> build:
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>
> > > > > >>
> > > > >
> > >
> >
> https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_BuildApacheIgnite/5443282?buildTab=log&focusLine=3&linesState=683.4289
> > > > > >>>>>>>
> > > > > >>>>>>> This means that I have completely lost
an option to run
> tests
> > > > > against
> > > > > >>>>>> pull
> > > > > >>>>>>> requests by new contributors - they
usually compile but
> will
> > > not
> > > > > pass
> > > > > >>>>>>> Checkstyle. That's a blocker.
> > > > > >>>>>>>
> > > > > >>>>>>> Can we please split Checkstyle as a
separate build which is
> > > > > triggered
> > > > > >>>>>> with
> > > > > >>>>>>> Run All?
> > > > > >>>>>>> I think we even have
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>
> > > > > >>
> > > > >
> > >
> >
> https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_CheckCodeStyle?mode=builds#all-projects
> > > > > >>>>>>>
> > > > > >>>>>>> WDYT?
> > > > > >>>>>>>
> > > > > >>>>>>> Regards,
> > > > > >>>>>>> --
> > > > > >>>>>>> Ilya Kasnacheev
> > > > > >>>>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>
> > > > > >>
> > > > >
> > > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message