ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Petr Ivanov <mr.wei...@gmail.com>
Subject Re: Code inspection
Date Wed, 13 Feb 2019 07:11:42 GMT
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


Mime
View raw message