Issue #273: converts diff.groovy to use Checkstyle's CLI #747

rnveach · 2022-12-31T19:21:03Z

Issue #273

Supplemental Checkstyle PR: checkstyle/checkstyle#12576

rnveach · 2022-12-31T19:23:17Z

.ci/travis.sh

@@ -90,7 +90,7 @@ checkstyle-tester-diff-groovy-regression-single)
  export MAVEN_OPTS="-Xmx2048m"
  groovy ./diff.groovy --listOfProjects projects-to-test-on.properties \
    -pc ../../../checkstyle-tester/diff-groovy-regression-config.xml \
-    -r ../../checkstyle -xm "-Dcheckstyle.failsOnError=false" \
+    -r ../../checkstyle \


CLI doesn't have anything similar. It seems we need to build something into groovy.

If this is true, and Checkstyle reported any violations or errors, the build fails immediately after running Checkstyle

The only way we have to determine if any violations/errors are reported from the CLI is the exit code. ( https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L415 ) We just have to be careful on other exit codes ( https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L85-L89 )

Anything else requires adding custom modules to listen for violations.

I added the reverse option, continueOnError. It is inverted because we need the default to be false so when we use the option, it is enabled. It follows the POM's default value which was failsOnError = true so continueOnError = false by default.

rnveach · 2022-12-31T19:23:54Z

checkstyle-tester/diff.groovy

@@ -74,8 +74,8 @@ def getCliOptions(args) {
            + ' as a shorter version to prevent long paths. (optional, default is false)')
        m(longOpt: 'mode', args: 1, required: false, argName: 'mode', 'The mode of the tool:' \
            + ' \'diff\' or \'single\'. (optional, default is \'diff\')')
-        xm(longOpt: 'extraMvnRegressionOptions', args: 1, required: false, 'Extra arguments to pass to Maven' \
-            + 'for Checkstyle Regression run (optional, ex: -Dmaven.prop=true)')
+        xo(longOpt: 'extraRegressionOptions', args: 1, required: false, 'Extra arguments to pass ' \


I took xm as extraMvn so I changed this to xo to mean extraOptions since this is not passed to any maven now.

rnveach · 2022-12-31T19:25:22Z

checkstyle-tester/diff.groovy

        executeCmd("git checkout $cfg.branch", cfg.localGitRepo)
        executeCmd("git log -1 --pretty=MSG:%s%nSHA-1:%H", cfg.localGitRepo)
-        executeCmd("mvn -e --no-transfer-progress --batch-mode -Pno-validations clean install",
+        executeCmd("mvn -e --no-transfer-progress --batch-mode -Pno-validations clean package -Passembly -Dassembly.skipAssembly=true -Dmaven.test.skip=true",


Partial done of #591 .

rnveach · 2022-12-31T19:26:45Z

checkstyle-tester/pom.xml

-		<checkstyle-plugin.version>3.1.1</checkstyle-plugin.version>
-		<checkstyle.version>0.0-SNAPSHOT</checkstyle.version>
-		<sevntu-checkstyle.version>1.44.1</sevntu-checkstyle.version>
-		<checkstyle.config.location>https://raw.githubusercontent.com/checkstyle/checkstyle/master/src/main/resources/google_checks.xml</checkstyle.config.location>


This config is never really used as shown in #745

rnveach · 2022-12-31T19:28:05Z

checkstyle-tester/pom.xml

-		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-		<checkstyle-plugin.version>3.1.1</checkstyle-plugin.version>
-		<checkstyle.version>0.0-SNAPSHOT</checkstyle.version>
-		<sevntu-checkstyle.version>1.44.1</sevntu-checkstyle.version>


Sevntu support is completely removed until #340 / #604 . We only supported single mode runs anyways, never supported differences since the sevntu version was never changed.

There is https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/sevntu_launch_diff.sh which I have maintained.

rnveach · 2022-12-31T19:50:20Z

checkstyle-tester/diff.groovy

+
+    println "Running Checkstyle CLI on $srcDir ... with excludes {$excludes}"
+    def cliCommand = "java -jar $allJar -c $checkstyleConfig $srcDir -f xml -o " +
+        getOsSpecificPath("$saveDir", "checkstyle-result.xml") + " -e .git -e .hg"


One of the benefits of using the CLI is we now pull in all files and not just .java. Because of this we have to exclude .git and .hg from erroneously processing them. TreeWalker will still trim to work only on Java files.

These have to be made more direct paths since this are not absolutely relative to the current working directory. We may want to consider picocli's file arguments for command line especially if we have a lot of excludes if it can bypass any limitation issues.

https://picocli.info/

A command may be invoked with the @file argument, like this:
java MyCommand @/home/foo/args

checkstyle-tester/diff.groovy

romani · 2023-01-02T06:13:46Z

@rnveach , please create PR to main repo to use your branch and validate and action is working to generate diff report.

rnveach · 2023-01-02T17:22:46Z

create PR to main repo to use your branch

Done.

romani

items:

checkstyle-tester/diff.groovy

checkstyle-tester/projects-for-circle.properties

romani

Items

.ci/validation.sh

checkstyle-tester/diff.groovy

romani

if CI is green in this repo and it it works well in main repo CI - ok to merge.

rnveach · 2023-02-04T18:03:01Z

checkstyle-tester/diff.groovy

            allowExcludes:allowExcludes,
+            continueOnError:continueOnError,


Fixed a bug in continueOnError, where if new option is not listed in these areas, then we cannot reference them in the main execution as we pull them in from here.

Without listed them here, doing cfg.[option] in main code always returned null and it would not fail anywhere since we were treating it as a boolean. It would just never get enabled.

nrmancuso

Items:

checkstyle-tester/README.md

checkstyle-tester/diff.groovy

nrmancuso · 2023-02-10T04:14:28Z

checkstyle-tester/diff.groovy

-        if (!extraMvnRegressionOptions.startsWith("-")) {
-            mvnSite = mvnSite + "-"
+    println "Running Checkstyle CLI on ${mainClassOptions.srcDir} ... with excludes {${mainClassOptions.excludes}}"
+    def cliCommand = "java -jar $allJar -c ${mainClassOptions.checkstyleConfig} " +


I can't help but feel that StringBuilder would be faster/easier to read/use less memory, especially with a bunch of excludes.

You want me to switch?

Keep in mind performance is not an issue here as it is one time execution. Better to keep code easy to read. I don't mind any approach

It's not a big deal, but we certainly run this more than once (twice for each project). I was thinking more for readability since + at the end of the line bugs me. I will leave it up to @rnveach

we need to disable trailing "+" rule.
I always thought about groovy as Java script abilities, to write in pure Java but as script. All scripts grows and grow up in applications, so being plain java is good for them

It is not a rule, this is required by the interpreter

@romani Please see the message I posted for you at checkstyle/checkstyle#12725 (comment) . This is groovy itself, and not a style rule.

sad, but ok, lets follow interpreter.

nrmancuso

Did we test failure at checkstyle/checkstyle#12576, i.e. throw an exception in a check? We need to make sure that we can still fail CI in all cases, too.

rnveach · 2023-02-11T18:59:50Z

Did we test failure at checkstyle/checkstyle#12576, i.e. throw an exception in a check?

It depends on what scenario you are looking for.

Using <property name="haltOnException" value="false"/> will ignore the exception and keep going. Similar with --continueOnError since this only checks the exit code from Checkstyle. We cannot identify an exception from the CLI, only the exit code and violations and exception both produce exit code > 0.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L135
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L141

I did confirm locally that removing haltOnException and continueOnError, diff.groovy will stop when an exception occurs. I tested an exception from the config:

<module name = "Checker">
    <property name="charset" value="UTF-8"/>

    <!-- do not change severity to 'error', as that will hide errors caused by exceptions -->
    <property name="severity" value="warning"/>

    <module name="RegexpSingleline">
        <property name="format" value="test(.*"/>
    </module>
</module>

With output:

> groovy diff.groovy -c M:\checkstyleWorkspaceEclipse\contribution\checkstyle-tester\my_check.xml -r Z:\checkstyle -b master -p master -l M:\checkstyleWorkspaceEclipse\contribution\checkstyle-tester\projects-to-test-on.properties

....
Running command: java -jar Z:\checkstyle\target\checkstyle-10.7.1-SNAPSHOT-all.jar -c M:\checkstyleWorkspaceEclipse\contribution\checkstyle-tester\my_check.xml repositories\guava -f xml -o reports\guava\checkstyle-result.xml -e repositories\guava\.git -e repositories\guava\.hg
Exception in thread "main" java.util.regex.PatternSyntaxException: Unclosed group near index 7
test(.*
        at java.base/java.util.regex.Pattern.error(Pattern.java:2027)
        at java.base/java.util.regex.Pattern.accept(Pattern.java:1877)
        at java.base/java.util.regex.Pattern.group0(Pattern.java:3060)
        at java.base/java.util.regex.Pattern.sequence(Pattern.java:2123)
        at java.base/java.util.regex.Pattern.expr(Pattern.java:2068)
        at java.base/java.util.regex.Pattern.compile(Pattern.java:1782)
        at java.base/java.util.regex.Pattern.<init>(Pattern.java:1428)
        at java.base/java.util.regex.Pattern.compile(Pattern.java:1094)
        at com.puppycrawl.tools.checkstyle.checks.regexp.DetectorOptions$Builder.createPattern(DetectorOptions.java:264)
        at java.base/java.util.Optional.map(Optional.java:265)
        at com.puppycrawl.tools.checkstyle.checks.regexp.DetectorOptions$Builder.build(DetectorOptions.java:249)
        at com.puppycrawl.tools.checkstyle.checks.regexp.RegexpSinglelineCheck.beginProcessing(RegexpSinglelineCheck.java:227)
        at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:218)
        at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:415)
        at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:338)
        at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:195)
        at com.puppycrawl.tools.checkstyle.Main.main(Main.java:130)
Caught: groovy.lang.GroovyRuntimeException: Error: !
groovy.lang.GroovyRuntimeException: Error: !
        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 diff.executeCliCmd(diff.groovy:698)
        at diff$executeCliCmd$17.callCurrent(Unknown Source)
        at diff.runCliExecution(diff.groovy:660)
        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 diff$_generateCheckstyleReport_closure4.doCall(diff.groovy:309)
        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 diff.generateCheckstyleReport(diff.groovy:271)
        at diff$generateCheckstyleReport$10.callCurrent(Unknown Source)
        at diff.launchCheckstyleReport(diff.groovy:233)
        at diff$launchCheckstyleReport$6.callCurrent(Unknown Source)
        at diff.run(diff.groovy:31)
        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)
>

rnveach · 2023-02-11T19:11:13Z

All our no-exception tests seem to use continueOnError.
https://github.com/checkstyle/checkstyle/blob/04fea7164f272af3b73621e4753df5f207033f92/.ci/no-exception-test.sh#L127

This will have to be removed, but there is the one config I pointed out to that uses error.
https://github.com/checkstyle/checkstyle/blob/d3a5f9a5ba4a1f2df28900dd70f2204c92943521/.ci/no-exception-test.sh#L44-L45

It looks like continueOnError is not a solution, and something in main repo will be needed.

Example run: https://github.com/checkstyle/checkstyle/actions/runs/4152812321/jobs/7184025441#step:5:320
No exception run kept going after exception and only failed at diff-report generation.

romani · 2023-07-02T14:32:09Z

@nrmancuso , @rnveach , is there easy way to finish this PR ?

rnveach · 2023-07-03T19:35:23Z

The only easy way is to complete everything needed for the transfer ( #747 (comment) ). This is the only PR in this repo I was working on for this issue.

rnveach requested review from romani and nrmancuso December 31, 2022 19:21

rnveach commented Dec 31, 2022

View reviewed changes

rnveach force-pushed the issue_273 branch from 8259be0 to cf934e9 Compare December 31, 2022 19:43

rnveach commented Dec 31, 2022

View reviewed changes

rnveach force-pushed the issue_273 branch from cf934e9 to 64f6f1d Compare December 31, 2022 20:10

rnveach commented Dec 31, 2022

View reviewed changes

checkstyle-tester/diff.groovy Show resolved Hide resolved

rnveach force-pushed the issue_273 branch from 64f6f1d to 3abc78c Compare December 31, 2022 21:28

rnveach assigned nrmancuso Dec 31, 2022

rnveach marked this pull request as ready for review December 31, 2022 21:33

nrmancuso assigned rnveach and unassigned nrmancuso Jan 2, 2023

This was referenced Jan 2, 2023

minor: removes dead code in diff.groovy #758

Closed

supplemental: contribution#273 converts diff.groovy to use Checkstyle's CLI checkstyle/checkstyle#12576

Closed

rnveach assigned romani and unassigned rnveach and romani Jan 2, 2023

rnveach force-pushed the issue_273 branch from 3abc78c to 91004f9 Compare January 2, 2023 17:53

romani requested changes Jan 3, 2023

View reviewed changes

checkstyle-tester/diff.groovy Show resolved Hide resolved

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

checkstyle-tester/projects-for-circle.properties Outdated Show resolved Hide resolved

rnveach force-pushed the issue_273 branch from 91004f9 to b5a814e Compare January 3, 2023 03:59

rnveach assigned romani Jan 3, 2023

rnveach force-pushed the issue_273 branch from b5a814e to ec78ea9 Compare January 9, 2023 01:41

romani requested changes Jan 9, 2023

View reviewed changes

.ci/validation.sh Outdated Show resolved Hide resolved

checkstyle-tester/diff.groovy Outdated Show resolved Hide resolved

rnveach force-pushed the issue_273 branch 5 times, most recently from ac4361b to 78a613a Compare January 10, 2023 21:59

rnveach force-pushed the issue_273 branch from 78a613a to b401951 Compare January 27, 2023 15:16

romani approved these changes Feb 4, 2023

View reviewed changes

romani assigned rnveach and unassigned romani Feb 4, 2023

rnveach force-pushed the issue_273 branch from b401951 to 7d347b0 Compare February 4, 2023 16:00

rnveach assigned nrmancuso and unassigned rnveach Feb 4, 2023

rnveach force-pushed the issue_273 branch 2 times, most recently from 9365bd0 to d5364f5 Compare February 4, 2023 18:01

rnveach commented Feb 4, 2023

View reviewed changes

rnveach force-pushed the issue_273 branch from d5364f5 to f9f7846 Compare February 4, 2023 18:12

nrmancuso requested changes Feb 10, 2023

View reviewed changes

rnveach added 2 commits February 10, 2023 13:02

Issue #273: remove old maven for checkstyle tester

3ff9dd2

Issue #273: converts diff.groovy to use Checkstyle's CLI

b697cb2

rnveach force-pushed the issue_273 branch from f9f7846 to b156dcd Compare February 10, 2023 18:16

Pull #747: adds continueOnError property to diff.groovy

4bc9bf8

rnveach force-pushed the issue_273 branch from b156dcd to 4bc9bf8 Compare February 10, 2023 18:22

nrmancuso requested changes Feb 11, 2023

View reviewed changes

rnveach marked this pull request as draft February 11, 2023 20:36

nrmancuso assigned rnveach and unassigned nrmancuso Feb 16, 2023

rnveach mentioned this pull request Feb 22, 2023

Pull #990: Define variable for non-main-files-suppressions file sevntu-checkstyle/sevntu.checkstyle#990

Merged

This was referenced Jul 2, 2023

Update diff.groovy #782

Closed

Update diff.groovy #783

Closed

rnveach closed this Oct 27, 2024

rnveach deleted the issue_273 branch October 27, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #273: converts diff.groovy to use Checkstyle's CLI #747

Issue #273: converts diff.groovy to use Checkstyle's CLI #747

rnveach commented Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022

rnveach Dec 31, 2022

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

romani commented Jan 2, 2023

rnveach commented Jan 2, 2023

romani left a comment

romani left a comment

romani left a comment

rnveach Feb 4, 2023 •

edited

Loading

nrmancuso left a comment

nrmancuso Feb 10, 2023

rnveach Feb 10, 2023

romani Feb 10, 2023

nrmancuso Feb 11, 2023

romani Feb 11, 2023

nrmancuso Feb 11, 2023

rnveach Feb 11, 2023

romani Feb 12, 2023

nrmancuso left a comment

rnveach commented Feb 11, 2023 •

edited

Loading

rnveach commented Feb 11, 2023 •

edited

Loading

romani commented Jul 2, 2023

rnveach commented Jul 3, 2023

		allowExcludes:allowExcludes,
		continueOnError:continueOnError,

Issue #273: converts diff.groovy to use Checkstyle's CLI #747

Issue #273: converts diff.groovy to use Checkstyle's CLI #747

Conversation

rnveach commented Dec 31, 2022 • edited Loading

rnveach Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

rnveach Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

romani commented Jan 2, 2023

rnveach commented Jan 2, 2023

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

rnveach Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

nrmancuso 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

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 left a comment

Choose a reason for hiding this comment

rnveach commented Feb 11, 2023 • edited Loading

rnveach commented Feb 11, 2023 • edited Loading

romani commented Jul 2, 2023

rnveach commented Jul 3, 2023

rnveach commented Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

rnveach Dec 31, 2022 •

edited

Loading

rnveach Feb 4, 2023 •

edited

Loading

rnveach commented Feb 11, 2023 •

edited

Loading

rnveach commented Feb 11, 2023 •

edited

Loading