ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpav...@apache.org>
Subject Re: Code inspection
Date Fri, 15 Mar 2019 16:19:22 GMT
Hi Igniters,

I see that a new TeamCity is released: Version: 2018.2.3.

Probably it could solve recently introduced problem related to:
Unexpected error during build messages processing in TeamCity;

Peter I., could you please check?

Sincerely,
Dmitriy Pavlov

пт, 15 мар. 2019 г. в 12:04, Павлухин Иван <vololo100@gmail.com>:

> I agree to gather some votes according to Maxim's proposal.
>
> Personally, I will not put my vote here. Both options will work for me
> today.
>
> But I would like to say a bit about agility. As I said both options
> sounds fine for me today. And I believe that we can switch from one to
> another easily in future. Let's do our best to be flexible.
>
> пт, 15 мар. 2019 г. в 12:04, Павлухин Иван <vololo100@gmail.com>:
> >
> > Maxim,
> >
> > > As far as I understand your case, you want to fix all code styles
> > issues right before getting the final TC results. Right? ...
> >
> > Actually, I mostly worry about accidental failures. For simple tasks
> > my workflow looks like:
> > 1. Create a branch.
> > 2. Write some code lines and tests.
> > 3. Run the most closely related tests from IDEA.
> > 4. Push changes to the branch.
> > 5. Launch TC.
> > 6. Take a cup of coffee ;-)
> > 7. Check TC results after a couple of hours.
> >
> > And in such workflow I can accidentally leave styling error (IDEA does
> > not fail compilation). And I will receive not very valuable report
> > from TC. And will have to wait for another couple of hours.
> >
> > Yes, usually I do not execute "mvn clean install" locally before
> > triggering TC. And I think that generally we should not do it because
> > TC does it.
> >
> > If not everybody uses a bot visas it sounds bad for me. For patches
> > touching the code it should be mandatory. Also, as you might know
> > there are different kind of visas and for some trivial patches we can
> > request Checkstyle visa. Committers should check formalities.
> >
> > пт, 15 мар. 2019 г. в 10:29, Nikolay Izhikov <nizhikov@apache.org>:
> > >
> > > +1 to enable code style checks in compile time.
> > >
> > > We can add option to disable maven codestyle profile with some
> environment variable.
> > >
> > > Anyone who want violate common project rules in their local branch can
> set this variable and write some nasty code before push :)
> > >
> > > пт, 15 марта 2019 г., 9:40 Maxim Muzafarov <maxmuzaf@gmail.com>:
> > >>
> > >> Ivan,
> > >>
> > >> > 1. I can write code and execute tests successfully even if there are
> > >> some style problems. I can imagine that a style error can arise
> > >> occasionally. And instead of getting test results after several hours
> > >> I will get a build failure without any tests run. I will simply lose
> > >> my time. But if the tests are allowed to proceed I will get a red flag
> > >> from code style check, fix those issues and rerun style check.
> > >>
> > >> As far as I understand your case, you want to fix all code styles
> > >> issues right before getting the final TC results. Right? It's doable
> > >> you can disable checkstyle in your local branch and revet it back when
> > >> you've done with all your changes to get the final visa. But the
> > >> common case here is building the project locally and checking all
> > >> requirements for PR right before pushing it to the GitHub repo. I
> > >> always do so. The "Checklist before push" [1] have such
> > >> recommendations. Build the project before push will eliminate your use
> > >> case.
> > >>
> > >> ---
> > >>
> > >> Igniters,
> > >>
> > >> To summarize the options we have with code checking behaviour:
> > >>
> > >> 1)  (code style will be broken more often) Run checkstyle in the
> > >> separate TC suite and include it to the Bot visa.
> > >> - not all of us run TC for their branches especially for simple fixes
> > >> (it's the most common case when a new check style errors occur)
> > >> - not all of us use TC.Bot visa to verify their branches
> > >> - if this checkstyle suite starts to fail it will be ignored for some
> > >> time (not blocks the development process)
> > >> - a lot of suites for code checking (license, checkstyle, something
> > >> else in future)
> > >>
> > >> + a bit comfortable way of TC tests execution for local\prototyped PRs
> > >> (it's a matter of taste)
> > >> + build the project and execute test suites a bit earlier (checkstyle
> > >> on the separate suite does not affect other suites)
> > >>
> > >> 2) (code style will be broken less often) Run checkstyle on project
> build stage.
> > >> - increases a bit the build time procedure
> > >> - require additional operations to switch it off for prototyping
> branches
> > >>
> > >> + do not require TC.Bot visa if someone of the community doesn't use
> it
> > >> + code style errors will be fixed immediately if the master branch
> > >> starts to fail
> > >> + the single place for code checks on maven code validation stage
> > >> (license check suite can be removed)
> > >>
> > >>
> > >> Please, add another advantages\disadvantages that I've missed.
> > >> Let's vote and pick the most suitable option for the Apache Ignite
> needs.
> > >>
> > >> ---
> > >>
> > >> Personally, I'd like to choose the 2) option.
> > >>
> > >> The JIRA [2] and PR [3] with the checkstyle enabled plugin is ready
> > >> for the review.
> > >>
> > >> [1]
> https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-Checklistbeforepush
> > >> [2] https://issues.apache.org/jira/browse/IGNITE-11277
> > >> [3] https://github.com/apache/ignite/pull/6119
> > >>
> > >> On Thu, 7 Mar 2019 at 11:19, Павлухин Иван <vololo100@gmail.com>
> wrote:
> > >> >
> > >> > Maxim,
> > >> >
> > >> > From use cases described by you I use only 1 and 2. And actually I
> > >> > think that we can concentrate on 1 and forget about others for now.
> > >> > But please address my worries from previous letter:
> > >> > ====Quoted text====
> > >> > 1. I can write code and execute tests successfully even if there are
> > >> > some style problems. I can imagine that a style error can arise
> > >> > occasionally. And instead of getting test results after several
> hours
> > >> > I will get a build failure without any tests run. I will simply lose
> > >> > my time. But if the tests are allowed to proceed I will get a red
> flag
> > >> > from code style check, fix those issues and rerun style check.
> > >> > 2. Style check takes some time. With simple checks it is almost
> > >> > negligible. But it can grow if more checks are involved.
> > >> > ====End of quoted text====
> > >> >
> > >> > Some clarifications. 1 is about running from IDEA first. 2 is
> related
> > >> > to opinions that we should involve much more checks, e.g. using
> > >> > abbreviations.
> > >> >
> > >> > чт, 7 мар. 2019 г. в 10:36, Maxim Muzafarov <maxmuzaf@gmail.com>:
> > >> > >
> > >> > > Ivan,
> > >> > >
> > >> > > Let's take a look at all the options we have (ordered by the
> frequency of use):
> > >> > >
> > >> > > 1. Check ready for merge branches (main case)
> > >> > > 2. Run tests on TC without checkstyle (prototyping branches)
> > >> > > 3. Local project build
> > >> > > 4. Quick build without any additional actions on TC
> > >> > >
> > >> > > In the other projects (kafka, netty etc.) which I've checked the
> checkstyle plugin is used in the <build> section, so the user has no chance
> in common cases to disable it via command line (correct me if I'm wrong).
> In the PR [1] I've moved checkstyle configuration to the separate profile.
> I've set activation checkstyle profile if -DskipTests specified, so it will
> run with the basic build configuration. It can also be disabled locally if
> we really need it.
> > >> > >
> > >> > > Back to our use cases:
> > >> > >
> > >> > > 1. For checking the ready to merge branches we will fail the
> ~Build Apache Ignite~ suite, so no configured checkstyle rules will be
> violated. If we will use the TC.Bot approach someone will merge the branch
> without running TC.Bot on it, but no one will merge the branch with compile
> errors.
> > >> > >
> > >> > > 2. For the prototyping branches, you can turn off checkstyle in
> your local PR by removing activation configuration. It's ok as these type
> of branches will never be merged to the master.
> > >> > >
> > >> > > 3. From my point, local builds should be always run with the
> checkstyle enabled profile. The common build action as `mvn clean install
> -DskipTests` will activate the profile.
> > >> > >
> > >> > > 4. The checkstyle profile can be disabled explicitly on TC by
> specifying -P !checkstyle option. A don't see any use cases of it, but it's
> completely doable.
> > >> > >
> > >> > > Please, correct me if I've missed something.
> > >> > >
> > >> > > I propose to merge PR [1] as it is, with the configured set of
> rules.
> > >> > >
> > >> > > [1] https://github.com/apache/ignite/pull/6119
> > >> > >
> > >> > > On Tue, 5 Mar 2019 at 19:02 Павлухин Иван <vololo100@gmail.com>
> wrote:
> > >> > >>
> > >> > >> Maxim,
> > >> > >>
> > >> > >> I like an idea of being IDE agnostic. I am ok with currently
> enabled
> > >> > >> checks, they are a must-have in my opinion (even for prototypes).
> > >> > >>
> > >> > >> But I am still do not like an idea of preventing tests execution
> if
> > >> > >> style check finds a problem. I checked out PR, installed a
> plugin and
> > >> > >> tried it out. Here are my concerns:
> > >> > >> 1. I can write code and execute tests successfully even if there
> are
> > >> > >> some style problems. I can imagine that a style error can arise
> > >> > >> occasionally. And instead of getting test results after several
> hours
> > >> > >> I will get a build failure without any tests run. I will simply
> lose
> > >> > >> my time. But if the tests are allowed to proceed I will get a
> red flag
> > >> > >> from code style check, fix those issues and rerun style check.
> > >> > >> 2. Style check takes some time. With simple checks it is almost
> > >> > >> negligible. But it can grow if more checks are involved.
> > >> > >>
> > >> > >> On the bright side I found nice integration of Checkstyle plugin
> with
> > >> > >> IDEA commit dialog. There is a checkbox "Scan with Checkstyle"
> which I
> > >> > >> think is quite useful.
> > >> > >>
> > >> > >> пн, 4 мар. 2019 г. в 15:00, Maxim Muzafarov <maxmuzaf@gmail.com
> >:
> > >> > >> >
> > >> > >> > Ivan,
> > >> > >> >
> > >> > >> > I like that Jetbrains inspections are integrated with IDE and
> TC out
> > >> > >> > of the box, but currently, they are working not well enough on
> TC.
> > >> > >> > Actually, they are not checking our source code at all.
> > >> > >> >
> > >> > >> > Let's try a bit another approach and try to be IDE-agnostic
> with code
> > >> > >> > style checking. I've checked popular java projects: hadoop,
> kafka,
> > >> > >> > spark, hive, netty. All of them are using
> maven-checkstyle-plugin in
> > >> > >> > their <build> section by default, so why don't we? It sounds
> > >> > >> > reasonable for me at least to try so.
> > >> > >> >
> > >> > >> > Can you take a look at my changes below?
> > >> > >> >
> > >> > >> >
> > >> > >> > Igniters,
> > >> > >> >
> > >> > >> > PR [2] has been prepared. All the details I've mentioned in my
> comment
> > >> > >> > in JIRA [4].
> > >> > >> > Can anyone take a look at my changes?
> > >> > >> >
> > >> > >> > JIRA: [1]
> > >> > >> > PR: [2]
> > >> > >> > Upsource: [3]
> > >> > >> >
> > >> > >> > Questions to discuss:
> > >> > >> > 1) There is no analogue for inspections RedundantSuppression
> and
> > >> > >> > SizeReplaceableByIsEmpty (all code style checks [5]). Propose
> to merge
> > >> > >> > without them.
> > >> > >> > 2) Checkstyle plugin has it's own maven profile and enabled by
> > >> > >> > default. It can be turned off for prototype branches.
> > >> > >> > 3) I've removed the inspections configuration for the TC suite
> and
> > >> > >> > propose to disable it as not working.
> > >> > >> >
> > >> > >> >
> > >> > >> > [1] https://issues.apache.org/jira/browse/IGNITE-11277
> > >> > >> > [2] https://github.com/apache/ignite/pull/6119
> > >> > >> > [3]
> https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1018
> > >> > >> > [4]
> https://issues.apache.org/jira/browse/IGNITE-11277?focusedCommentId=16771200&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16771200
> > >> > >> > [5] http://checkstyle.sourceforge.net/checks.html
> > >> > >> >
> > >> > >> > On Thu, 14 Feb 2019 at 16:21, Павлухин Иван <
> vololo100@gmail.com> wrote:
> > >> > >> > >
> > >> > >> > > Nikolay,
> > >> > >> > >
> > >> > >> > > > All community members are forced to follow code style.
> It's harder to achieve it with dedicated suite.
> > >> > >> > > Why it is easier to follow code style with use of maven
> checkstyle
> > >> > >> > > plugin? Is it integrated into IDEA out of box? As I got it
> additional
> > >> > >> > > IDEA plugin is needed as well. Who will enforce everybody to
> install
> > >> > >> > > it?
> > >> > >> > >
> > >> > >> > > Also, as I see a common good practice today is using TC Bot
> visa. Visa
> > >> > >> > > includes result from running inspections job.
> > >> > >> > >
> > >> > >> > > чт, 14 февр. 2019 г. в 16:08, Nikolay Izhikov <
> nizhikov@apache.org>:
> > >> > >> > > >
> > >> > >> > > > Ivan,
> > >> > >> > > >
> > >> > >> > > > > Could you please outline the benefits you see of failing
> compilation and
> > >> > >> > > > skipping tests execution if inspections detect a problem?
> > >> > >> > > >
> > >> > >> > > > All community members are forced to follow code style.
> > >> > >> > > > It's harder to achieve it with dedicated suite.
> > >> > >> > > >
> > >> > >> > > >
> > >> > >> > > > чт, 14 февр. 2019 г. в 15:21, Павлухин Иван <
> vololo100@gmail.com>:
> > >> > >> > > >
> > >> > >> > > > > Nikolay,
> > >> > >> > > > >
> > >> > >> > > > > > Should the community spend TC resources for  prototype?
> > >> > >> > > > > Why not? I think it is not bad idea to run all tests
> against some
> > >> > >> > > > > changes into core classes. If I have a clever idea which
> is easy to
> > >> > >> > > > > test drive I can do couple of prototype-test iterations.
> If tests
> > >> > >> > > > > shows me that everything is bad then the idea was not so
> clever and
> > >> > >> > > > > easy. But if I was lucky then I should discuss the idea
> with other
> > >> > >> > > > > Igniters. I think it is the cheapest way to check the
> idea because the
> > >> > >> > > > > check is fully automated. Requiring a human feedback is
> much more
> > >> > >> > > > > expensive in my opinion.
> > >> > >> > > > > > But, If our code style is not convinient for every day
> coding for many
> > >> > >> > > > > contributors, should you initiate discussion to change
> it?
> > >> > >> > > > > Generally I am fine with our codestyle requirements.
> > >> > >> > > > >
> > >> > >> > > > > Also, I would like to keep a focus on the subject. Could
> you please
> > >> > >> > > > > outline the benefits you see of failing compilation and
> skipping tests
> > >> > >> > > > > execution if inspections detect a problem?
> > >> > >> > > > >
> > >> > >> > > > > чт, 14 февр. 2019 г. в 14:14, Nikolay Izhikov <
> nizhikov@apache.org>:
> > >> > >> > > > > >
> > >> > >> > > > > > Hello, Ivan.
> > >> > >> > > > > >
> > >> > >> > > > > > > Requirements for a prototype code are not the same
> as for a patch ready
> > >> > >> > > > > > to merge
> > >> > >> > > > > >
> > >> > >> > > > > > True.
> > >> > >> > > > > >
> > >> > >> > > > > > > I do not see much need in writing good javadocs for
> prototype.
> > >> > >> > > > > >
> > >> > >> > > > > > We, as a community, can't force you to do it.
> > >> > >> > > > > >
> > >> > >> > > > > > > Why should I stub it to be able run any build on TC?
> > >> > >> > > > > >
> > >> > >> > > > > > Should the community spend TC resources for  prototype?
> > >> > >> > > > > > You always can check tests for your prototype locally.
> > >> > >> > > > > >
> > >> > >> > > > > > And when it's ready, at least from code style point of
> view run it on TC.
> > >> > >> > > > > >
> > >> > >> > > > > > I, personally, always try to follow project code
> style, even for
> > >> > >> > > > > prototypes.
> > >> > >> > > > > > But, If our code style is not convinient for every day
> coding for many
> > >> > >> > > > > > contributors, should you initiate discussion to change
> it?
> > >> > >> > > > > >
> > >> > >> > > > > >
> > >> > >> > > > > > ср, 13 февр. 2019 г. в 16:45, Павлухин Иван <
> vololo100@gmail.com>:
> > >> > >> > > > > >
> > >> > >> > > > > > > Maxim,
> > >> > >> > > > > > >
> > >> > >> > > > > > > Oh, my poor tabs.. Joke.
> > >> > >> > > > > > >
> > >> > >> > > > > > > I am totally ok with currently enabled checks. But I
> am mostly
> > >> > >> > > > > > > concerned about a general approach. I would like to
> outline one thing.
> > >> > >> > > > > > > Requirements for a prototype code are not the same
> as for a patch
> > >> > >> > > > > > > ready to merge (see a little bit more in the end of
> that message).
> > >> > >> > > > > > >
> > >> > >> > > > > > > We have a document defining code style which every
> contributor should
> > >> > >> > > > > > > follow [1]. And many points can be checked
> automatically. Personally,
> > >> > >> > > > > > > I do not see much need in writing good javadocs for
> prototype. Why
> > >> > >> > > > > > > should I stub it to be able run any build on TC?
> > >> > >> > > > > > >
> > >> > >> > > > > > > Also, we a have a review process which should be
> applied to every
> > >> > >> > > > > > > patch. Partially it is described in [2]. And due to
> this process every
> > >> > >> > > > > > > patch should not introduce new failures on TC. So,
> the patch should
> > >> > >> > > > > > > not be merged if inspections failed.
> > >> > >> > > > > > >
> > >> > >> > > > > > > P.S. Something more about prototypes and production
> code. There is a
> > >> > >> > > > > > > common bad practice in software engineering. It is
> turning prototypes
> > >> > >> > > > > > > into production code. Often it is much faster to
> create a prototype by
> > >> > >> > > > > > > price of violating some rules of writing "clean
> code". And often
> > >> > >> > > > > > > prototype after successful piloting is turned into
> production code.
> > >> > >> > > > > > > And it is very easy in practice to keep some pieces
> of initially
> > >> > >> > > > > > > "dirty" prototype code. I believe human factor plays
> a great role
> > >> > >> > > > > > > here. How should it be done right then? In my
> opinion good production
> > >> > >> > > > > > > code should be designed as "good production code"
> from the beginning.
> > >> > >> > > > > > > So, only ideas are taken from the prototype and a
> code is fully
> > >> > >> > > > > > > rewritten.
> > >> > >> > > > > > >
> > >> > >> > > > > > > [1]
> > >> > >> > > > >
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> > >> > >> > > > > > > [2]
> > >> > >> > > > >
> https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist
> > >> > >> > > > > > >
> > >> > >> > > > > > > ср, 13 февр. 2019 г. в 15:05, Maxim Muzafarov <
> maxmuzaf@gmail.com>:
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > Ivan,
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > As the first implementation of this addition, I'd
> prefer to make it
> > >> > >> > > > > > > > working like _Licenses Headers_ suite check. It
> will fail when some
> > >> > >> > > > > of
> > >> > >> > > > > > > > the code style checks violated. Moreover, these
> licenses header
> > >> > >> > > > > checks
> > >> > >> > > > > > > > can be included in the checkstyle plugin
> configuration.
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > In general, I'd prefer to have a compilation fail
> error with code
> > >> > >> > > > > > > > style checks and after we will get a stable
> checkstyle suite I
> > >> > >> > > > > propose
> > >> > >> > > > > > > > to change it in a "compilation error" way. If we
> are talking about
> > >> > >> > > > > the
> > >> > >> > > > > > > > coding style convenient for most of the community
> members I see no
> > >> > >> > > > > > > > difference with coding sketches or
> production-ready branches equally.
> > >> > >> > > > > > > > Indeed, no one will be against unused imports [or
> spaces instead of
> > >> > >> > > > > > > > tabs :-) ] in their PRs or prototypes, right? (for
> instance, it can
> > >> > >> > > > > be
> > >> > >> > > > > > > > automatically removed by IDE at commit phase).
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > Please, note currently enabled checks are:
> > >> > >> > > > > > > >  - list.isEmpty() instead of list.size() == 0
> > >> > >> > > > > > > >  - unused imports
> > >> > >> > > > > > > >  - missing @Override
> > >> > >> > > > > > > >  - sotred modifiers checks (e.g. pulic static
> final ..)
> > >> > >> > > > > > > >  - redundunt suppersion checks
> > >> > >> > > > > > > >  - spaces insted of tabs.
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > Are you really what to violate these checks in
> your sketches? Hope
> > >> > >> > > > > not
> > >> > >> > > > > > > :-)
> > >> > >> > > > > > > >
> > >> > >> > > > > > > > On Wed, 13 Feb 2019 at 10:25, Nikolay Izhikov <
> nizhikov@apache.org>
> > >> > >> > > > > > > wrote:
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > Actually, I dont see anything wrong with failing
> *compilation*
> > >> > >> > > > > task.
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > I think one should use project code style for
> everyday coding, not
> > >> > >> > > > > > > only for
> > >> > >> > > > > > > > > ready-to-merge PRs.
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > If we cant use code style for everyday coding,
> we should change the
> > >> > >> > > > > > > > > codestyle.
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > What do you think?
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > ср, 13 февр. 2019 г., 10:11 Petr Ivanov
> mr.weider@gmail.com:
> > >> > >> > > > > > > > >
> > >> > >> > > > > > > > > > I guess that was about failing build
> configuration with
> > >> > >> > > > > Checkstype,
> > >> > >> > > > > > > not
> > >> > >> > > > > > > > > > compilation build itself.
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > > > > > > On 12 Feb 2019, at 18:03, Павлухин Иван <
> vololo100@gmail.com>
> > >> > >> > > > > > > wrote:
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > > Folks,
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > > Are you going to fail job compiling Ignite
> sources [1] if some
> > >> > >> > > > > > > > > > > inspection found a problem? Can we avoid it?
> It is quite common
> > >> > >> > > > > > > > > > > pattern to start some feature implementation
> with making a
> > >> > >> > > > > sketch
> > >> > >> > > > > > > and
> > >> > >> > > > > > > > > > > running tests against it. I found it
> convenient to skip some
> > >> > >> > > > > style
> > >> > >> > > > > > > > > > > requirements for such sketches (e.g. well
> formed javadocs).
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > > [1]
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > >
> https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_BuildApacheIgnite
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > > пн, 11 февр. 2019 г. в 11:38, Nikolay
> Izhikov <
> > >> > >> > > > > nizhikov@apache.org
> > >> > >> > > > > > > >:
> > >> > >> > > > > > > > > > >>
> > >> > >> > > > > > > > > > >> Petr, we should have 1 configuration for
> project, may be 1
> > >> > >> > > > > > > configuration
> > >> > >> > > > > > > > > > >> per programming language.
> > >> > >> > > > > > > > > > >>
> > >> > >> > > > > > > > > > >> пн, 11 февр. 2019 г., 11:33 Petr Ivanov
> mr.weider@gmail.com:
> > >> > >> > > > > > > > > > >>
> > >> > >> > > > > > > > > > >>> I was asking about how many build
> configuration is intended?
> > >> > >> > > > > One
> > >> > >> > > > > > > for
> > >> > >> > > > > > > > > > all
> > >> > >> > > > > > > > > > >>> and multiple per module?
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>> With IDEA inspections it was going to be
> build configuration
> > >> > >> > > > > per
> > >> > >> > > > > > > > > > module.
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>>> On 11 Feb 2019, at 11:24, Nikolay Izhikov
> <
> > >> > >> > > > > nizhikov@apache.org>
> > >> > >> > > > > > > > > > wrote:
> > >> > >> > > > > > > > > > >>>>
> > >> > >> > > > > > > > > > >>>> Hello, Petr.
> > >> > >> > > > > > > > > > >>>>
> > >> > >> > > > > > > > > > >>>> Are you saying that we have not single
> build task? And each
> > >> > >> > > > > > > module
> > >> > >> > > > > > > > > > builds
> > >> > >> > > > > > > > > > >>>> when it required? If yes, then I propose
> to create a task
> > >> > >> > > > > like
> > >> > >> > > > > > > > > > "Licence
> > >> > >> > > > > > > > > > >>>> check" which will be run for every patch.
> > >> > >> > > > > > > > > > >>>>
> > >> > >> > > > > > > > > > >>>> My point is that violation of codestyle
> should be treated as
> > >> > >> > > > > > > hard as
> > >> > >> > > > > > > > > > >>>> compile error.
> > >> > >> > > > > > > > > > >>>>
> > >> > >> > > > > > > > > > >>>> пн, 11 февр. 2019 г., 11:16 Petr Ivanov
> mr.weider@gmail.com
> > >> > >> > > > > :
> > >> > >> > > > > > > > > > >>>>
> > >> > >> > > > > > > > > > >>>>> Is build configuration Inspections
> [Core] meant to
> > >> > >> > > > > transform
> > >> > >> > > > > > > into
> > >> > >> > > > > > > > > > single
> > >> > >> > > > > > > > > > >>>>> all-modules check build configuration
> (without module
> > >> > >> > > > > > > subdivision)?
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>>>> On 11 Feb 2019, at 11:02, Nikolay
> Izhikov <
> > >> > >> > > > > > > nizhikov@apache.org>
> > >> > >> > > > > > > > > > wrote:
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> Hello, Maxim.
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> +1 from me for migrating to checkstyle.
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> Oleg, there is plugin for IDEA with
> 2mln downloads -
> > >> > >> > > > > > > > > > >>>>>>
> https://plugins.jetbrains.com/plugin/1065-checkstyle-idea
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> I propose do the following:
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> 1. Migrate current checks to checkstyle.
> > >> > >> > > > > > > > > > >>>>>> 2. Apply checks to all Ignite modules.
> Currently, only
> > >> > >> > > > > core
> > >> > >> > > > > > > module
> > >> > >> > > > > > > > > > are
> > >> > >> > > > > > > > > > >>>>>> checked.
> > >> > >> > > > > > > > > > >>>>>> I will review and commit this patch, or
> do it by my own.
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> 3. Include code style checks to "Build
> Apache Ignite"
> > >> > >> > > > > suite.
> > >> > >> > > > > > > Ignite
> > >> > >> > > > > > > > > > has
> > >> > >> > > > > > > > > > >>>>> to
> > >> > >> > > > > > > > > > >>>>>> fail to build if patch violates
> codestyle.
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>> вс, 10 февр. 2019 г. в 07:54, Павлухин
> Иван <
> > >> > >> > > > > > > vololo100@gmail.com>:
> > >> > >> > > > > > > > > > >>>>>>
> > >> > >> > > > > > > > > > >>>>>>> Hi,
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>>> I also think that some warning from
> IDEA that some code
> > >> > >> > > > > > > style rule
> > >> > >> > > > > > > > > > is
> > >> > >> > > > > > > > > > >>>>>>> violated is a must-have.
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>>> вс, 10 февр. 2019 г. в 01:58,
> oignatenko <
> > >> > >> > > > > > > oignatenko@gridgain.com
> > >> > >> > > > > > > > > > >:
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> Hi Maxim,
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> I believe that whatever style checks
> we establish at
> > >> > >> > > > > > > Teamcity, we
> > >> > >> > > > > > > > > > >>>>> better
> > >> > >> > > > > > > > > > >>>>>>>> take care of making it easy for
> developers to find and
> > >> > >> > > > > fix
> > >> > >> > > > > > > > > > violations
> > >> > >> > > > > > > > > > >>>>> in
> > >> > >> > > > > > > > > > >>>>>>>> their typical dev environment (for
> Ignite this means, in
> > >> > >> > > > > > > IDEA). I
> > >> > >> > > > > > > > > > >>> think
> > >> > >> > > > > > > > > > >>>>>>> it
> > >> > >> > > > > > > > > > >>>>>>>> is important that developers can
> maintain required style
> > >> > >> > > > > > > with
> > >> > >> > > > > > > > > > minimal
> > >> > >> > > > > > > > > > >>>>>>> effort
> > >> > >> > > > > > > > > > >>>>>>>> on their side.
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> If above is doable then I am 200% for
> migrating our
> > >> > >> > > > > Teamcity
> > >> > >> > > > > > > > > > >>>>> inspections
> > >> > >> > > > > > > > > > >>>>>>> to
> > >> > >> > > > > > > > > > >>>>>>>> checkstyle / maven.
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> This is because I am very
> disappointed observing how it
> > >> > >> > > > > > > stays
> > >> > >> > > > > > > > > > broken
> > >> > >> > > > > > > > > > >>>>> for
> > >> > >> > > > > > > > > > >>>>>>> so
> > >> > >> > > > > > > > > > >>>>>>>> long. And worst of all, even when
> (if) it is fixed, I
> > >> > >> > > > > feel
> > >> > >> > > > > > > we will
> > >> > >> > > > > > > > > > >>>>>>> always be
> > >> > >> > > > > > > > > > >>>>>>>> at risk that it breaks again and that
> we will have to
> > >> > >> > > > > again
> > >> > >> > > > > > > wait
> > >> > >> > > > > > > > > > for
> > >> > >> > > > > > > > > > >>>>>>> months
> > >> > >> > > > > > > > > > >>>>>>>> for it to be fixed.
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> This is such a stark contrast with my
> experience
> > >> > >> > > > > regarding
> > >> > >> > > > > > > > > > checkstyle
> > >> > >> > > > > > > > > > >>>>>>> based
> > >> > >> > > > > > > > > > >>>>>>>> inspections. These just work and you
> just never fear
> > >> > >> > > > > that
> > >> > >> > > > > > > it is
> > >> > >> > > > > > > > > > going
> > >> > >> > > > > > > > > > >>>>> to
> > >> > >> > > > > > > > > > >>>>>>>> break for some obscure reason, this
> is so much better
> > >> > >> > > > > than
> > >> > >> > > > > > > what I
> > >> > >> > > > > > > > > > >>>>> observe
> > >> > >> > > > > > > > > > >>>>>>>> now.
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> One suggestion in case if we pick
> checkstyle - I
> > >> > >> > > > > recommend
> > >> > >> > > > > > > keeping
> > >> > >> > > > > > > > > > >>> its
> > >> > >> > > > > > > > > > >>>>>>>> config file somewhere in the project
> under version
> > >> > >> > > > > control.
> > >> > >> > > > > > > I
> > >> > >> > > > > > > > > > used to
> > >> > >> > > > > > > > > > >>>>>>>> maintain such a shared style config
> at one of past jobs
> > >> > >> > > > > and
> > >> > >> > > > > > > after
> > >> > >> > > > > > > > > > >>> some
> > >> > >> > > > > > > > > > >>>>>>>> experimenting it turned out most
> convenient to have it
> > >> > >> > > > > this
> > >> > >> > > > > > > way -
> > >> > >> > > > > > > > > > so
> > >> > >> > > > > > > > > > >>>>> that
> > >> > >> > > > > > > > > > >>>>>>>> developers could easily assess and
> discuss style
> > >> > >> > > > > settings
> > >> > >> > > > > > > and keep
> > >> > >> > > > > > > > > > >>>>> track
> > >> > >> > > > > > > > > > >>>>>>> of
> > >> > >> > > > > > > > > > >>>>>>>> changes in these. (note how Kafka
> folks from your link
> > >> > >> > > > > [5]
> > >> > >> > > > > > > appear
> > >> > >> > > > > > > > > > to
> > >> > >> > > > > > > > > > >>> be
> > >> > >> > > > > > > > > > >>>>>>>> doing it this way)
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> regards, Oleg
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> Mmuzaf wrote
> > >> > >> > > > > > > > > > >>>>>>>>> Igniters,
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> I've found that some of the
> community members have
> > >> > >> > > > > faced
> > >> > >> > > > > > > with
> > >> > >> > > > > > > > > > >>>>>>>>> `[Inspections] Core suite [1]` is
> not working well
> > >> > >> > > > > enough
> > >> > >> > > > > > > on TC.
> > >> > >> > > > > > > > > > The
> > >> > >> > > > > > > > > > >>>>>>>>> suite has a `FAILED` status for more
> than 2 months due
> > >> > >> > > > > to
> > >> > >> > > > > > > some
> > >> > >> > > > > > > > > > >>> issues
> > >> > >> > > > > > > > > > >>>>>>>>> in TeamCity application [2]. Current
> suite behaviour
> > >> > >> > > > > > > confuses not
> > >> > >> > > > > > > > > > >>> only
> > >> > >> > > > > > > > > > >>>>>>>>> new contributors but also other
> community members.
> > >> > >> > > > > > > Moreover, this
> > >> > >> > > > > > > > > > >>>>>>>>> suite is no longer checks rules we
> previously
> > >> > >> > > > > configured.
> > >> > >> > > > > > > For
> > >> > >> > > > > > > > > > >>>>>>>>> instance, in the master branch, I've
> found 11 `Unused
> > >> > >> > > > > > > imports`
> > >> > >> > > > > > > > > > which
> > >> > >> > > > > > > > > > >>>>>>>>> should have been caught earlier
> (e.g. for
> > >> > >> > > > > > > > > > >>>>>>>>> {{IgniteCachePutAllRestartTest} [3]).
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> I think we should make the next step
> to enable an
> > >> > >> > > > > > > automatic code
> > >> > >> > > > > > > > > > >>> style
> > >> > >> > > > > > > > > > >>>>>>>>> checks. As an example, we can
> consider the Apache Kafka
> > >> > >> > > > > > > code
> > >> > >> > > > > > > > > > style
> > >> > >> > > > > > > > > > >>> [5]
> > >> > >> > > > > > > > > > >>>>>>>>> way and configure for the Ignite
> project a
> > >> > >> > > > > > > > > > maven-checkstyle-plugin
> > >> > >> > > > > > > > > > >>>>>>>>> with its own maven profile and run
> it simultaneously
> > >> > >> > > > > with
> > >> > >> > > > > > > other
> > >> > >> > > > > > > > > > TC.
> > >> > >> > > > > > > > > > >>> We
> > >> > >> > > > > > > > > > >>>>>>>>> can also enable the previously
> configured inspection
> > >> > >> > > > > > > rules, so no
> > >> > >> > > > > > > > > > >>>>>>>>> coding style violations will be
> missed.
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> I see some advantages of using a
> maven plugin:
> > >> > >> > > > > > > > > > >>>>>>>>> - an IDE agnostic way for code checks
> > >> > >> > > > > > > > > > >>>>>>>>> - can be used with different CI and
> build tools
> > >> > >> > > > > (Jenkins,
> > >> > >> > > > > > > TC)
> > >> > >> > > > > > > > > > >>>>>>>>> - executable from the command line
> > >> > >> > > > > > > > > > >>>>>>>>> - the entry single point to
> configure new rules
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> I've created the ticket [4] and will
> prepare PR for it.
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> WDYT?
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> [1]
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > >
> https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_InspectionsCore&branch_IgniteTests24Java8=%3Cdefault%3E&tab=buildTypeStatusDiv
> > >> > >> > > > > > > > > > >>>>>>>>> [2]
> https://youtrack.jetbrains.com/issue/TW-58504
> > >> > >> > > > > > > > > > >>>>>>>>> [3]
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > >
> https://github.com/apache/ignite/blob/master/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCachePutAllRestartTest.java#L29
> > >> > >> > > > > > > > > > >>>>>>>>> [4]
> https://issues.apache.org/jira/browse/IGNITE-11277
> > >> > >> > > > > > > > > > >>>>>>>>> [5]
> > >> > >> > > > > https://github.com/apache/kafka/tree/trunk/checkstyle
> > >> > >> > > > > > > > > > >>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> On Fri, 21 Dec 2018 at 16:03, Petr
> Ivanov &lt;
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> mr.weider@
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> &gt; wrote:
> > >> > >> > > > > > > > > > >>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>> It seems there is bug in latest
> 2018.2 TeamCity
> > >> > >> > > > > > > > > > >>>>>>>>>> Bug is filed [1]
> > >> > >> > > > > > > > > > >>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>> [1]
> https://youtrack.jetbrains.com/issue/TW-58504
> > >> > >> > > > > > > > > > >>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>> On 19 Dec 2018, at 11:31, Petr
> Ivanov &lt;
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> mr.weider@
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> &gt; wrote:
> > >> > >> > > > > > > > > > >>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>> Investigating problem, stand by.
> > >> > >> > > > > > > > > > >>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>>> On 18 Dec 2018, at 19:41, Dmitriy
> Pavlov &lt;
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> dpavlov@
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>> &gt; wrote:
> > >> > >> > > > > > > > > > >>>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>>> Both patches were applied. Maxim,
> thank you!
> > >> > >> > > > > > > > > > >>>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>>> What about 1. An `Unexpected
> error during build
> > >> > >> > > > > messages
> > >> > >> > > > > > > > > > >>>>>>> processing in
> > >> > >> > > > > > > > > > >>>>>>>>>>>> TeamCity`, what can we do as the
> next step to fix
> > >> > >> > > > > it?
> > >> > >> > > > > > > > > > >>>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>>> Sincerely,
> > >> > >> > > > > > > > > > >>>>>>>>>>>> Dmitriy Pavlov
> > >> > >> > > > > > > > > > >>>>>>>>>>>> [cut]
> > >> > >> > > > > > > > > > >>>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>> --
> > >> > >> > > > > > > > > > >>>>>>>> Sent from:
> > >> > >> > > > > > >
> http://apache-ignite-developers.2346864.n4.nabble.com/
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>>> --
> > >> > >> > > > > > > > > > >>>>>>> Best regards,
> > >> > >> > > > > > > > > > >>>>>>> Ivan Pavlukhin
> > >> > >> > > > > > > > > > >>>>>>>
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >>>
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > >
> > >> > >> > > > > > > > > > > --
> > >> > >> > > > > > > > > > > Best regards,
> > >> > >> > > > > > > > > > > Ivan Pavlukhin
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > > > >
> > >> > >> > > > > > > --
> > >> > >> > > > > > > Best regards,
> > >> > >> > > > > > > Ivan Pavlukhin
> > >> > >> > > > > > >
> > >> > >> > > > >
> > >> > >> > > > >
> > >> > >> > > > >
> > >> > >> > > > > --
> > >> > >> > > > > Best regards,
> > >> > >> > > > > Ivan Pavlukhin
> > >> > >> > > > >
> > >> > >> > >
> > >> > >> > >
> > >> > >> > >
> > >> > >> > > --
> > >> > >> > > Best regards,
> > >> > >> > > Ivan Pavlukhin
> > >> > >>
> > >> > >>
> > >> > >>
> > >> > >> --
> > >> > >> Best regards,
> > >> > >> Ivan Pavlukhin
> > >> > >
> > >> > > --
> > >> > > --
> > >> > > Maxim Muzafarov
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > Best regards,
> > >> > Ivan Pavlukhin
> > >>
> >
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>
>

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