>> Also I still strictly against adding checkstyle to project build as minor >> issues in checkstyle should not be blocker for test run.
Me too. Looks like everything works fine. Thanks! The only problem is long time of build ~10 minutes, because of dependencies resolving. Maxim, please double check it. On Tue, Apr 23, 2019 at 12:26 PM Petr Ivanov <mr.wei...@gmail.com> wrote: > > Vyacheslav, can you check this build please [1] if everything was ran as > expected? > > Also I still strictly against adding checkstyle to project build as minor > issues in checkstyle should not be blocker for test run. > > > [1] > https://ci.ignite.apache.org/viewLog.html?buildId=3678000&buildTypeId=IgniteTests24Java8_CheckCodeStyle&tab=artifacts&branch_IgniteTests24Java8=%3Cdefault%3E# > > > > On 23 Apr 2019, at 11:59, Petr Ivanov <mr.wei...@gmail.com> wrote: > > > > I'll check it. > > > > > > Also, please pass TC build for review next time and do not add to Run All > > without it. > > > > Thanks! > > > > > >> On 23 Apr 2019, at 11:53, Vyacheslav Daradur <daradu...@gmail.com> wrote: > >> > >> This is quite strange error, in case of "install" phase it'd be better > >> just add "checkstyle" profile to "Build" [1] configuration. > >> > >> [1] > >> https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_BuildApacheIgnite > >> > >> On Tue, Apr 23, 2019 at 11:43 AM Petr Ivanov <mr.wei...@gmail.com> wrote: > >>> > >>> The suite is malformed. > >>> If no ~/.m2/repository/org/apache/ignite artifact are installed in > >>> system, the build will fail [1] > >>> > >>> It seems that we should use install instead of validate. > >>> > >>> > >>> [1] > >>> https://ci.ignite.apache.org/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=3677858&_focus=288 > >>> > >>> > >>> On 23 Apr 2019, at 00:25, Vyacheslav Daradur <daradu...@gmail.com> wrote: > >>> > >>> Maxim, I merged your changes to master. > >>> > >>> Also, I've created a new build plan "Check Code Style" on TC [1] and > >>> included in RunAll build. > >>> The report of check-style attaches in artifacts once build is finished. > >>> > >>> Please check that it works as expected once again and announce new > >>> requirements in a separate thread to avoid confusion of community. > >>> > >>> cc Petr, Pavel (JFYI) > >>> > >>> [1] > >>> https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_CheckCodeStyle&tab=buildTypeBranches > >>> > >>> On Sun, Apr 21, 2019 at 10:18 PM Vyacheslav Daradur <daradu...@gmail.com> > >>> wrote: > >>> > >>> > >>> Maxim, > >>> > >>> I left some comments in Jira, let's resolve them and I'll assist you > >>> with merge and TC configuring. > >>> > >>> On Fri, Apr 19, 2019 at 5:50 PM Maxim Muzafarov <maxmu...@gmail.com> > >>> wrote: > >>> > >>> > >>> Vyacheslav, > >>> > >>> Thank you for your interest in making Ignite coding style better. > >>> > >>> The short answer is - there are no different checkstyle > >>> configurations. One for the JetBrains Inspections, and one the > >>> Checkstyle plugin. This is a completely different approach for > >>> checking the Ignites source code. > >>> > >>> Currently, we have two different configurations for the JetBrains IDEA > >>> Inspection check: > >>> - ignite\.idea\inspectionProfiles\Project_Default.xml - this is > >>> default on the IDE level and used silently by every developer whos > >>> checkout Ignite project (it remains) > >>> - ignite\idea\ignite_inspections_teamcity.xml - this is the > >>> configuration of the inspection for the TC suite (it will be deleted) > >>> It's unobvious to maintain both of them. Previously we've planned to > >>> fix all the inspection rules one by one and add them one by one to the > >>> TC inspection configuration file (something like storing the > >>> intermediate result), but it didn't happen cause the inspection TC > >>> suite got broken after migration to 2018 version. > >>> > >>> Now it seems to me, that it is better to use the best open source > >>> practices like checkstyle plugin (380K usages on github repositories) > >>> rather than proprietary software. So, we will: > >>> - keep IDE level inspection configuration (the Project_Default.xml > >>> config); > >>> - add a new checkstyle plugin configuration file (checkstyle.xml > >>> config) which will be used simultaneously for checking code style on > >>> build procedure and for the IDE-checkstyle plugin; > >>> > >>> On Wed, 17 Apr 2019 at 21:23, Vyacheslav Daradur <daradu...@gmail.com> > >>> wrote: > >>> > >>> > >>> Maxim, > >>> > >>> I looked through the PR and it looks good to me in general. > >>> > >>> The only question how it's planned to maintain check styles in 2 > >>> different configurations, for IDEA and check style plugin? > >>> > >>> On Mon, Mar 25, 2019 at 12:30 PM Maxim Muzafarov <maxmu...@gmail.com> > >>> wrote: > >>> > >>> > >>> Igniters, > >>> > >>> The issue [1] with enabled maven-checkstyle-plugin is ready for the > >>> review. > >>> All changes are prepared according to e-mail [2] the second option > >>> point (include the plugin in the maven build procedure by default). > >>> > >>> JIRA: IGNITE-11277 [1] > >>> PR: [3] > >>> Upsource: [4] > >>> > >>> How can take a look? > >>> > >>> [1] https://issues.apache.org/jira/browse/IGNITE-11277 > >>> [2] > >>> http://apache-ignite-developers.2346864.n4.nabble.com/Code-inspection-tp27709p41297.html > >>> [3] https://github.com/apache/ignite/pull/6119 > >>> [4] https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1018 > >>> > >>> On Fri, 15 Mar 2019 at 19:19, Dmitriy Pavlov <dpav...@apache.org> wrote: > >>> > >>> > >>> 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, Павлухин Иван <vololo...@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, Павлухин Иван <vololo...@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 <nizhi...@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 <maxmu...@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, Павлухин Иван <vololo...@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 <maxmu...@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 Павлухин Иван <vololo...@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 <maxmu...@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, Павлухин Иван < > >>> > >>> vololo...@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 < > >>> > >>> nizhi...@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, Павлухин Иван < > >>> > >>> vololo...@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 < > >>> > >>> nizhi...@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, Павлухин Иван < > >>> > >>> vololo...@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 < > >>> > >>> maxmu...@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 < > >>> > >>> nizhi...@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.wei...@gmail.com: > >>> > >>> > >>> I guess that was about failing build > >>> > >>> configuration with > >>> > >>> Checkstype, > >>> > >>> not > >>> > >>> compilation build itself. > >>> > >>> On 12 Feb 2019, at 18:03, Павлухин Иван < > >>> > >>> vololo...@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 < > >>> > >>> nizhi...@apache.org > >>> > >>> : > >>> > >>> > >>> Petr, we should have 1 configuration for > >>> > >>> project, may be 1 > >>> > >>> configuration > >>> > >>> per programming language. > >>> > >>> пн, 11 февр. 2019 г., 11:33 Petr Ivanov > >>> > >>> mr.wei...@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 > >>> > >>> < > >>> > >>> nizhi...@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.wei...@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 < > >>> > >>> nizhi...@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, Павлухин > >>> > >>> Иван < > >>> > >>> vololo...@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 < > >>> > >>> oignate...@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 < > >>> > >>> > >>> mr.weider@ > >>> > >>> > >>> > 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 < > >>> > >>> > >>> mr.weider@ > >>> > >>> > >>> > wrote: > >>> > >>> > >>> Investigating problem, stand by. > >>> > >>> > >>> On 18 Dec 2018, at 19:41, Dmitriy > >>> > >>> Pavlov < > >>> > >>> > >>> dpavlov@ > >>> > >>> > >>> > 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 > >>> > >>> > >>> > >>> > >>> > >>> -- > >>> Best Regards, Vyacheslav D. > >>> > >>> > >>> > >>> > >>> -- > >>> Best Regards, Vyacheslav D. > >>> > >>> > >>> > >>> > >>> -- > >>> Best Regards, Vyacheslav D. > >>> > >>> > >> > >> > >> -- > >> Best Regards, Vyacheslav D. > > > -- Best Regards, Vyacheslav D.
