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 09:52:45 GMT
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