[prev in list] [next in list] [prev in thread] [next in thread]
List: cassandra-dev
Subject: Re: [DISCUSSIONS] Replace ant eclipse-warnings with CheckerFramework
From: Ekaterina Dimitrova <e.dimitrova () gmail ! com>
Date: 2023-06-18 17:45:14
Message-ID: CAN+t3-j5CPVi1H1Z3HiNh+M5Z=WTRKEOjo7eWV8eJ3kkRm7pZw () mail ! gmail ! com
[Download RAW message or body]
Thank you all! It seems there is a consensus here so I updated accordingly
CASSANDRA-18239
On Fri, 16 Jun 2023 at 8:56, Jeremiah Jordan <jeremiah.jordan@gmail.com>
wrote:
> +1 from me.
>
> On Jun 15, 2023 at 1:01:01 PM, Ekaterina Dimitrova <e.dimitrova@gmail.com=
>
> wrote:
>
>> Hi everyone,
>> Happy Thursday!
>> Some time ago, Jacek raised the point that ant eclipse-warnings is gener=
ating too many false positives and not really working as expected. (CASSAND=
RA-18239)
>>
>> Reminder: ant eclipse-warnings is a task we run with the goal to check C=
assandra code - static analysis to warn on unsafe use of Autocloseable inst=
ances; checks against two related particular compiler options
>>
>> While trying to upgrade ECJ compiler that we use for this task (CASSANDR=
A-18190) so we can switch the task from running it with JDK8 to JDK11 in pr=
eparation for dropping JDK8, I hit the following issues:
>> - the latest version of ECJ is throwing more than 300 Potential Resource=
Leak warnings. I looked at 10-15, and they were all false positives.
>> - Even if we file a bug report to the Eclipse community, JDK11 is about =
to be removed with the next version of the compiler
>>
>> So I shared this information with Jacek. He came up with a different sol=
ution:
>> It seems we already pull through Guava CheckerFramework with an MIT lice=
nse, which appears to be acceptable according to this link - https://www.a=
pache.org/legal/resolved.html#category-a
>> He already has an initial integration with Cassandra which shows the fol=
lowing:
>> - CheckerFramework does not understand the @SuppressWarnings("resource")=
(there is a different one to be used), so it is immediately visible how it=
does not report all those false positives that eclipse-warnings does. On t=
he flip side, I got the feedback that what it has witnessed so far is somet=
hing we should investigate.
>> - Also, there are additional annotations like @Owning that let you fix m=
any problems at once because the tool understands that the ownership of the=
resources was passed to another entity; It also enables you to do somethin=
g impossible with eclipse-warnings - you can tell the tool that there is an=
other method that needs to be called to release the resources, like release=
, free, disconnect, etc.
>> - the tool works with JDK8, JDK11, JDK17, and JDK20, so we can backport =
it even to older branches (while at the same time keeping eclipse-warnings =
there)
>> - though it runs 8 minutes so, we should not run it with every test, som=
e reorganization around ant tasks will be covered as even for eclipse-warni=
ngs it was weird to call it on every single test run locally by default
>>
>>
>> If there are no concerns, we will continue replacing ant eclipse-warning=
s with the CheckerFramework as part of CASSANDRA-18239 and CASSANDRA-18190 =
in trunk.
>>
>> Best regards,
>>
>> Ekaterina
>>
>>
[Attachment #3 (text/html)]
<div dir="auto">Thank you all! It seems there is a consensus here so I updated \
accordingly CASSANDRA-18239</div><div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Fri, 16 Jun 2023 at 8:56, Jeremiah Jordan <<a \
href="mailto:jeremiah.jordan@gmail.com">jeremiah.jordan@gmail.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204,204,204)"><div><div \
dir="ltr"> +1 from me.</div></div><div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Jun 15, 2023 at 1:01:01 PM, Ekaterina \
Dimitrova <<a href="mailto:e.dimitrova@gmail.com" \
target="_blank">e.dimitrova@gmail.com</a>> wrote:<br></div> <blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204,204,204)" \
type="cite"> <pre style="white-space:pre-wrap;word-spacing:1px;box-sizing:inherit;mar \
gin-top:4px;margin-bottom:4px;padding:8px;font-size:0.75rem;font-variant-ligatures:non \
e;line-height:1.50001;word-break:normal;border-radius:4px;overflow-y:hidden;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)">Hi everyone, \
Happy Thursday! Some time ago, Jacek raised the point that ant eclipse-warnings is \
generating too many false positives and not really working as expected. \
(CASSANDRA-18239)</pre><pre \
style="white-space:pre-wrap;word-spacing:1px;box-sizing:inherit;margin-top:4px;margin- \
bottom:4px;padding:8px;font-size:0.75rem;font-variant-ligatures:none;line-height:1.500 \
01;word-break:normal;border-radius:4px;overflow-y:hidden;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)" \
dir="auto">Reminder: ant eclipse-warnings is a task we run with the goal to check \
Cassandra code - static analysis to warn on unsafe use of Autocloseable instances; \
checks against two related particular compiler options</pre><pre \
style="white-space:pre-wrap;word-spacing:1px;box-sizing:inherit;margin-top:4px;margin- \
bottom:4px;padding:8px;font-size:0.75rem;font-variant-ligatures:none;line-height:1.500 \
01;word-break:normal;border-radius:4px;overflow-y:hidden;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)" dir="auto">While \
trying to upgrade ECJ compiler that we use for this task (CASSANDRA-18190) so we can \
switch the task from running it with JDK8 to JDK11 in preparation for dropping JDK8, \
I hit the following issues:
- the latest version of ECJ is throwing more than 300 Potential Resource Leak \
warnings. I looked at 10-15, and they were all false positives.
- Even if we file a bug report to the Eclipse community, JDK11 is about to be removed \
with the next version of the compiler
So I shared this information with Jacek. He came up with a different solution:
It seems we already pull through Guava CheckerFramework with an MIT license, which \
appears to be acceptable according to this link - <a \
href="https://www.apache.org/legal/resolved.html#category-a" rel="noopener \
noreferrer" style="box-sizing:inherit;text-decoration:none;font-size:0.75rem;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)" \
target="_blank">https://www.apache.org/legal/resolved.html#category-a</a> He already \
has an initial integration with Cassandra which shows the following:
- CheckerFramework does not understand the @SuppressWarnings("resource") \
(there is a different one to be used), so it is immediately visible how it does not \
report all those false positives that eclipse-warnings does. On the flip side, I got \
the feedback that what it has witnessed so far is something we should \
investigate.
- Also, there are additional annotations like @Owning that let you fix many problems \
at once because the tool understands that the ownership of the resources was passed \
to another entity; It also enables you to do something impossible with \
eclipse-warnings - you can tell the tool that there is another method that needs to \
be called to release the resources, like release, free, disconnect, \
etc.
- the tool works with JDK8, JDK11, JDK17, and JDK20, so we can backport it even to \
older branches (while at the same time keeping eclipse-warnings \
there)
- though it runs 8 minutes so, we should not run it with every test, some \
reorganization around ant tasks will be covered as even for eclipse-warnings it was \
weird to call it on every single test run locally by default<br>
If there are no concerns, we will continue replacing ant eclipse-warnings with the \
CheckerFramework as part of CASSANDRA-18239 and CASSANDRA-18190 in trunk.</pre><pre \
style="white-space:pre-wrap;word-spacing:1px;box-sizing:inherit;margin-top:4px;margin- \
bottom:4px;padding:8px;font-size:0.75rem;font-variant-ligatures:none;line-height:1.500 \
01;word-break:normal;border-radius:4px;overflow-y:hidden;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)" dir="auto">Best \
regards,</pre><pre style="white-space:pre-wrap;word-spacing:1px;box-sizing:inherit;mar \
gin-top:4px;margin-bottom:4px;padding:8px;font-size:0.75rem;font-variant-ligatures:non \
e;line-height:1.50001;word-break:normal;border-radius:4px;overflow-y:hidden;font-family:Monaco,Menlo,Consolas,"Courier \
New",monospace;border-color:rgb(29,28,29);color:rgb(29,28,29)" \
dir="auto">Ekaterina</pre>
</blockquote>
</div></div>
</blockquote></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic