[ovirt-devel] oVirt Engine checkstyle upgrade
Vojtech Szocs
vszocs at redhat.com
Tue Mar 28 13:47:44 UTC 2017
On Mon, Mar 27, 2017 at 11:46 PM, Allon Mureinik <amureini at redhat.com>
wrote:
>
>
> On Mon, Mar 27, 2017 at 8:48 PM, Vojtech Szocs <vszocs at redhat.com> wrote:
>
>>
>>
>> On Mon, Mar 27, 2017 at 2:10 PM, Allon Mureinik <amureini at redhat.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, Mar 27, 2017 at 3:09 PM, Allon Mureinik <amureini at redhat.com>
>>> wrote:
>>>
>>>> Indeed, Greg.thanks
>>>>
>>>> As a temporary solution, you could cherry-pick https://gerrit.ovi
>>>> rt.org/#/c/74611 and work above it.
>>>> It should solve the issue (even though IMHO it isn't ready for merging
>>>> to master).
>>>>
>>>> Vojta - are you updating https://gerrit.ovirt.org/#/c/74611 as per my
>>>> comments there, or should I take it over?
>>>>
>>> I literally have no idea who I did made this typo.
>>> I meant "Vojtech", of course.
>>>
>>
>> In Czech language, Vojta actually means Vojtech, in a less formal way =)
>>
> In that case, I totally meant to type that :-)
>
>
>>
>> I've just updated https://gerrit.ovirt.org/#/c/74611/ according to your
>> comments.
>>
> I cleaned up some white-spacing noise there that seemed unrelated to the
> patch and merged.
>
Well, whenever I see:
class Foo {
int a;
void bar() {
}
}
it hurts my eyes because there's no symmetry - whitespace before class
declaration end, but no whitespace after class declaration start.
Additionally, I prefer instance.method() to be on the same line (unless you
intend to chain calls), e.g. following:
matcher
.start();
is IMHO less readable than:
matcher.start();
but following is OK:
foo
.bar()
.baz();
> From my local testing, using -Pgwtdev now works just find.
>
> Votech (or Vojta, if you prefer ^_^) - many thanks for your efforts here.
>
> Marek, and everyone else - please let me know if this still causes you any
> grief.
>
>
>
>
>>
>> The reason why GWT debugger fails to start is because one of the
>> non-localized strings (which comes from `gwt-extension` and therefore
>> shouldn't be checked in the first place..) contains a {} placeholder,
>> NlsCheck logs it, and eventually crashes because it thinks it's a log
>> message placeholder, but it's just something we want printed out.
>>
>>
>>>
>>>
>>>
>>>>
>>>> On Mon, Mar 27, 2017 at 3:03 PM, Greg Sheremeta <gshereme at redhat.com>
>>>> wrote:
>>>>
>>>>> From the comments in 74619
>>>>>
>>>>> "" "
>>>>> So, tl;dr - it /won't/ work with this patch but without 74611. This
>>>>> patch should be applied before 74611
>>>>> "" "
>>>>>
>>>>> On Mon, Mar 27, 2017 at 7:42 AM, Marek Libra <mlibra at redhat.com>
>>>>> wrote:
>>>>>
>>>>>> With
>>>>>> https://gerrit.ovirt.org/#/c/74619/
>>>>>>
>>>>>> applied, following is still failing:
>>>>>> make gwt-debug DEBUG_MODULE=webadmin DEV_EXTRA_BUILD_FLAGS_GWT_DEFA
>>>>>> ULTS="-Dgwt.cssResourceStyle=pretty -Dgwt.userAgent=gecko1_8"
>>>>>>
>>>>>> with message:
>>>>>> [WARNING] The requested profile "gwt-user" could not be activated
>>>>>> because it does not exist.
>>>>>> [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check
>>>>>> (checkstyle) on project webadmin: Failed during checkstyle configuration:
>>>>>> Exception was thrown while processing /home/mlibra/IdeaProjects/ovir
>>>>>> t-engine/frontend/webadmin/modules/gwt-extension/src/main/ja
>>>>>> va/org/ovirt/engine/ui/uioverrides/org/slf4j/Logger.java: can't
>>>>>> parse argument number: For input string: "" -> [Help 1]
>>>>>>
>>>>>>
>>>>>> Any hint, please?
>>>>>> Marek
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Mar 24, 2017 at 4:14 PM, Vojtech Szocs <vszocs at redhat.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Mar 24, 2017 at 3:51 PM, Vojtech Szocs <vszocs at redhat.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Found the problem after debugging NlsCheck.
>>>>>>>>
>>>>>>>> First of all, it checks all kinds of Java sources, including the
>>>>>>>> generated ones. That's silly and one of the reasons why Checkstyle
>>>>>>>> execution takes a rather long time. I'll fix that.
>>>>>>>>
>>>>>>>> Next, when checking a Java source that contains string "{}"
>>>>>>>> (without quotes) it will log the problem, but Checkstyle message logging
>>>>>>>> infra things that "{}" is a placeholder to resolve, but it's not, and it
>>>>>>>> fails on NumberFormatException. I'll fix that too.
>>>>>>>>
>>>>>>>
>>>>>>> https://gerrit.ovirt.org/#/c/74611/
>>>>>>>
>>>>>>>
>>>>>>>> Vojtech
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Mar 24, 2017 at 3:19 PM, Vojtech Szocs <vszocs at redhat.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Allon,
>>>>>>>>>
>>>>>>>>> I think I found some strange Checkstyle related problems on master.
>>>>>>>>>
>>>>>>>>> Engine build with (GWT compilation enabled) works OK.
>>>>>>>>>
>>>>>>>>> Next, trying to start GWT debugger:
>>>>>>>>>
>>>>>>>>> $ make gwt-debug DEBUG_MODULE=webadmin \
>>>>>>>>> DEV_EXTRA_BUILD_FLAGS_GWT_DEFAULTS="-Dgwt.userAgent=gecko1_8,safari"
>>>>>>>>> \
>>>>>>>>> DEV_EXTRA_BUILD_FLAGS="-Dgwt.logLevel=INFO -Dgwt.locale=en_US
>>>>>>>>> -Dgwt.compiler.localWorkers=1" \
>>>>>>>>> DEV_BUILD_GWT_SUPER_DEV_MODE=1
>>>>>>>>>
>>>>>>>>> maven-checkstyle-plugin:check execution fails on
>>>>>>>>>
>>>>>>>>> frontend/webadmin/modules/gwt-extension/src/main/java/org/ov
>>>>>>>>> irt/engine/ui/uioverrides/org/ovirt/engine/core/compat/Forma
>>>>>>>>> tterDotnet.java
>>>>>>>>> can't parse argument number: (\\d)\\: For input string: "(\\d)\\"
>>>>>>>>>
>>>>>>>>> the class isn't used, removed it, retry. Now it fails on:
>>>>>>>>>
>>>>>>>>> frontend/webadmin/modules/gwt-extension/src/main/java/org/ov
>>>>>>>>> irt/engine/ui/uioverrides/org/slf4j/Logger.java
>>>>>>>>> can't parse argument number: For input string: ""
>>>>>>>>>
>>>>>>>>> I guess it's a bug in our NON-NLS check? But why doesn't the
>>>>>>>>> problem occur during Engine build?
>>>>>>>>>
>>>>>>>>> I'm thinking about disabling Checkstyle for gwt-extension module,
>>>>>>>>> as it contains custom GWT RPC serializers and GWT class overrides, and
>>>>>>>>> maybe the file path src/main/java/org/ovirt/engine
>>>>>>>>> /ui/uioverrides/here/goes/actual/pkg is confusing the Checkstyle
>>>>>>>>> now.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vojtech
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Mar 22, 2017 at 10:33 PM, Allon Mureinik <
>>>>>>>>> amureini at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> As per [1], I've just merged a series of patches that upgrades
>>>>>>>>>> the oVirt engine to use the latest maven-checkstyle-plugin and checkstyle
>>>>>>>>>> packages.
>>>>>>>>>>
>>>>>>>>>> Please note that the newer checkstyle is a tad stricter than the
>>>>>>>>>> old one we used to have (read: it contains several fixes for bugs where the
>>>>>>>>>> old checkstyle was supposed to find issues but missed them).
>>>>>>>>>> I also took the opportunity and added a couple of new checks that
>>>>>>>>>> enforce rules we were de-facto adhering to anyway.
>>>>>>>>>>
>>>>>>>>>> If any problems come up, please let me know.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -Your friendly neighborhood cleanup dude
>>>>>>>>>>
>>>>>>>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1433408
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> Devel mailing list
>>>>>>>>>> Devel at ovirt.org
>>>>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Devel mailing list
>>>>>>> Devel at ovirt.org
>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Devel mailing list
>>>>>> Devel at ovirt.org
>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Greg Sheremeta, MBA
>>>>> Red Hat, Inc.
>>>>> Sr. Software Engineer
>>>>> gshereme at redhat.com
>>>>>
>>>>> _______________________________________________
>>>>> Devel mailing list
>>>>> Devel at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170328/5f192690/attachment-0001.html>
More information about the Devel
mailing list