Issue #14396: checker-framework.yml should fail execution if report generation fails #14409

relentless-pursuit · 2024-02-02T18:32:20Z

This PR enhances the GitHub Actions workflow for the Checker Framework by ensuring execution fails upon detecting errors during report generation.

romani

Items

romani · 2024-02-04T01:09:04Z

.ci/checker-framework.groovy

    BufferedReader reader = null
    try {
        reader = new BufferedReader(new InputStreamReader(process.inputStream))
        String lineFromReader = reader.readLine()
        while (lineFromReader != null) {
            println(lineFromReader)
            checkerFrameworkLines.add(lineFromReader)
+            if (lineFromReader.contains("OutOfMemoryError")) {


We need any error, can we make code general ?
Can we simply rely on mvn exit code?

We need any error, can we make code general ? Can we simply rely on mvn exit code?

I am trying to throw an exception the moment it encounters a non-zero exit code, assuming a OOM detected. And, this probably might make the CI/CD to fail. Please let me know your thoughts on it.

romani

I like this a way more

relentless-pursuit · 2024-02-04T06:28:43Z

I like this a way more

how do we test such scenarios in our local?

romani · 2024-02-04T06:30:12Z

You can update groovy to start mvn with low memory like -xmx 512mb please google exact syntax.

relentless-pursuit · 2024-02-04T06:31:28Z

You can update groovy to start mvn with low memory like -xmx 512mb please google exact syntax.

Sure, thanks.

relentless-pursuit · 2024-02-06T07:25:25Z

You can update groovy to start mvn with low memory like -xmx 512mb please google exact syntax.

@romani, @rnveach I tried with reducing the jvm heap size to check if the code i added was running fine. But, even at 128mb, i am not being able to simulate an OOM. The other way i tried was runing a infinite loop, the CI did hint towards a OOM, but, as obvious the lines of code I added to terminal the script wasn't executed. Any alternative or correction needed?

relentless-pursuit · 2024-02-06T09:35:46Z

You can update groovy to start mvn with low memory like -xmx 512mb please google exact syntax.

@romani, @rnveach I tried with reducing the jvm heap size to check if the code i added was running fine. But, even at 128mb, i am not being able to simulate an OOM. The other way i tried was runing a infinite loop, the CI did hint towards a OOM, but, as obvious the lines of code I added to terminal the script wasn't executed. Any alternative or correction needed?

Even if I am failing to replicate OOM, but, atleast I am being able to register an exit status which I had introduced throwin a RunTimeException from the below logs. So, possibly this might also be the case when maven executions runs out of memory.

Caught: java.lang.RuntimeException: Maven process exited with error code: 1
java.lang.RuntimeException: Maven process exited with error code: 1

romani · 2024-02-07T02:19:25Z

We bumped memory usage for maven to bypass this problem in CI, if we revert such commit we should be able reproduce problem in CI, but instead of weird failure of diff on suppression we should have failed step of report generation.

Commit revert might be too much, but revert of lines as 1a041e3#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8L2938 should bring us back to CI problem to run Checker

relentless-pursuit · 2024-02-18T08:52:15Z

@nrmancuso, I have added the reports of before and after changes.

Without any changes to groovy file, we continue to get OOM but with some strange suppressions as reported previously

https://github.com/checkstyle/checkstyle/actions/runs/7947755703/job/21696975080?pr=14409

After groovy changes, we can handle the OOM properly

https://github.com/checkstyle/checkstyle/actions/runs/7947830206/job/21697133364?pr=14409

relentless-pursuit · 2024-02-18T08:52:58Z

@relentless-pursuit before we drop commits, please do the following so that we can easily revisit and quickly understand what you've done here.

Separate your changes here into three commits:

one with CI disablement

one with hack to cause failure

one with update to groovy script

Place a link in the issue to the commit that has the hack to cause CI to fail, with a short description. It is likely that we might need such a hack in the future for something else.

Please explain how your changes in the groovy script helped us, I do not see the Maven process exited with error code message in the output you've shared.

Thanks a lot for all the work on this.

Done.

relentless-pursuit · 2024-02-18T08:54:30Z

3. Please explain how your changes in the groovy script helped us, I do not see the Maven process exited with error code message in the output you've shared.

Caught: java.lang.RuntimeException: Maven process exited with error code: 1
java.lang.RuntimeException: Maven process exited with error code: 1
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at checker-framework.getCheckerFrameworkErrors(checker-framework.groovy:132)
	at checker-framework.checkCheckerFrameworkReport(checker-framework.groovy:79)
	at checker-framework.parseArgumentAndExecute(checker-framework.groovy:34)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at checker-framework.run(checker-framework.groovy:17)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

nrmancuso · 2024-02-18T11:47:13Z

pom.xml

-              <failOnError>false</failOnError>
-              <meminitial>1024m</meminitial>
-              <maxmem>8192m</maxmem>
+              <failOnError>true</failOnError>


How will the maven execution fail without this?

If I understood it right, the final commit must have the changes of groovy including failOnError be true for all checkers.

If I understood it right, the final commit must have the changes of groovy including failOnError be true for all checkers.

Yes, that’s right. Right now, we only have one updated, and it is in the “hack” to cause OOM commit.

Thank you for the review. I have made the changes. Please let me know if they are right.

Ok, let's drop other commits and prepare for merge.

@relentless-pursuit codenarc is failing in semaphore:
Violation: Rule=ThrowRuntimeException P=2 Line=132 Msg=[The type RuntimeException should not be thrown] Src=[throw new RuntimeException("Maven process exited with error code: " + exitCode)]

Is there a specific reason to throw an exception here? Can we just System.exit(1)?

System.exit(1)

After thinking on this, this is a hack that we should probably avoid. I also have doubts about whether we would execute the finally block below this to close the reader. @relentless-pursuit let's keep it the way you had it and find a more specific exception class as recommended by codenarc: https://codenarc.org/codenarc-rules-exceptions.html#:~:text=Checks%20for%20throwing%20an%20instance%20of%20java.lang.RuntimeException.%20Throw%20an%20instance%20of%20a%20more%20specific%20exception%20subclass%20instead.

https://codenarc.org/codenarc-rules-exceptions.html#:~:text=Checks%20for%20throwing%20an%20instance%20of%20java.lang.RuntimeException.%20Throw%20an%20instance%20of%20a%20more%20specific%20exception%20subclass%20instead

Sure, I was cross-checking and it didn't give me OOM error when i used System.exit(1).

Let's do IllegalStateException

Let's do IllegalStateException

Done. I also checked with the hack, and it seems to be working fine. https://github.com/checkstyle/checkstyle/actions/runs/7974597699/job/21770876534?pr=14409

….groovy to fail execution if report generation fails

nrmancuso · 2024-02-20T14:10:30Z

@relentless-pursuit thanks for working on this, I know these sorts of issues can get pretty involved!

relentless-pursuit · 2024-02-20T14:11:48Z

@relentless-pursuit thanks for working on this, I know these sorts of issues can get pretty involved!

Always happy to help Checkstyle. And, thanks to you and other maintainers for always being patient with the contributors like me.

romani

Ok to merge, elegant fix.

MANISH-K-07 mentioned this pull request Feb 3, 2024

checker-framework.yml should be able to fail execution if report generation is failed #14396

Closed

relentless-pursuit force-pushed the checker-failure branch 3 times, most recently from 7e75f68 to f46ab42 Compare February 3, 2024 05:49

romani requested changes Feb 4, 2024

View reviewed changes

relentless-pursuit force-pushed the checker-failure branch from f46ab42 to 0a9bf49 Compare February 4, 2024 04:55

romani approved these changes Feb 4, 2024

View reviewed changes

romani requested a review from rnveach February 4, 2024 06:14

romani assigned rnveach Feb 4, 2024

rnveach added the awaiting response label Feb 4, 2024

relentless-pursuit force-pushed the checker-failure branch 5 times, most recently from 4ebf7dc to af362fb Compare February 6, 2024 06:39

relentless-pursuit force-pushed the checker-failure branch 3 times, most recently from 233b364 to e7fc6c0 Compare February 6, 2024 08:42

relentless-pursuit force-pushed the checker-failure branch 6 times, most recently from df4617b to fb75b89 Compare February 7, 2024 07:06

relentless-pursuit force-pushed the checker-failure branch 4 times, most recently from adba322 to c77b498 Compare February 18, 2024 08:32

nrmancuso reviewed Feb 18, 2024

View reviewed changes

relentless-pursuit force-pushed the checker-failure branch 10 times, most recently from 3417172 to 34b1b6f Compare February 20, 2024 13:54

Issue checkstyle#14396: Made changes to pom.xml and checker-framework…

17a184b

….groovy to fail execution if report generation fails

relentless-pursuit force-pushed the checker-failure branch from 34b1b6f to 17a184b Compare February 20, 2024 14:02

nrmancuso approved these changes Feb 20, 2024

View reviewed changes

nrmancuso assigned romani Feb 20, 2024

romani approved these changes Feb 20, 2024

View reviewed changes

romani assigned rnveach and unassigned romani Feb 20, 2024

rnveach approved these changes Feb 21, 2024

View reviewed changes

rnveach merged commit 3b29018 into checkstyle:master Feb 21, 2024
110 of 113 checks passed

relentless-pursuit deleted the checker-failure branch May 27, 2024 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #14396: checker-framework.yml should fail execution if report generation fails #14409

Issue #14396: checker-framework.yml should fail execution if report generation fails #14409

relentless-pursuit commented Feb 2, 2024

romani left a comment

romani Feb 4, 2024

relentless-pursuit Feb 4, 2024

relentless-pursuit Feb 4, 2024

romani left a comment

relentless-pursuit commented Feb 4, 2024

romani commented Feb 4, 2024 •

edited

Loading

relentless-pursuit commented Feb 4, 2024

relentless-pursuit commented Feb 6, 2024 •

edited

Loading

relentless-pursuit commented Feb 6, 2024 •

edited

Loading

romani commented Feb 7, 2024 •

edited

Loading

relentless-pursuit commented Feb 18, 2024 •

edited

Loading

relentless-pursuit commented Feb 18, 2024

relentless-pursuit commented Feb 18, 2024 •

edited

Loading

nrmancuso Feb 18, 2024

relentless-pursuit Feb 18, 2024

nrmancuso Feb 18, 2024

relentless-pursuit Feb 18, 2024

nrmancuso Feb 19, 2024

nrmancuso Feb 20, 2024 •

edited

Loading

nrmancuso Feb 20, 2024

relentless-pursuit Feb 20, 2024 •

edited

Loading

nrmancuso Feb 20, 2024

relentless-pursuit Feb 20, 2024

nrmancuso commented Feb 20, 2024

relentless-pursuit commented Feb 20, 2024

romani left a comment

Issue #14396: checker-framework.yml should fail execution if report generation fails #14409

Issue #14396: checker-framework.yml should fail execution if report generation fails #14409

Conversation

relentless-pursuit commented Feb 2, 2024

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

relentless-pursuit commented Feb 4, 2024

romani commented Feb 4, 2024 • edited Loading

relentless-pursuit commented Feb 4, 2024

relentless-pursuit commented Feb 6, 2024 • edited Loading

relentless-pursuit commented Feb 6, 2024 • edited Loading

romani commented Feb 7, 2024 • edited Loading

relentless-pursuit commented Feb 18, 2024 • edited Loading

relentless-pursuit commented Feb 18, 2024

relentless-pursuit commented Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relentless-pursuit Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso commented Feb 20, 2024

relentless-pursuit commented Feb 20, 2024

romani left a comment

Choose a reason for hiding this comment

romani commented Feb 4, 2024 •

edited

Loading

relentless-pursuit commented Feb 6, 2024 •

edited

Loading

relentless-pursuit commented Feb 6, 2024 •

edited

Loading

romani commented Feb 7, 2024 •

edited

Loading

relentless-pursuit commented Feb 18, 2024 •

edited

Loading

relentless-pursuit commented Feb 18, 2024 •

edited

Loading

nrmancuso Feb 20, 2024 •

edited

Loading

relentless-pursuit Feb 20, 2024 •

edited

Loading