contribution icon indicating copy to clipboard operation
contribution copied to clipboard

Launch/Diff Groovy should remove use of maven-checkstyle-plugin

Open rnveach opened this issue 7 years ago • 10 comments

See https://issues.apache.org/jira/browse/MCHECKSTYLE-346 . We need to deprecate all usage of maven-checkstyle-plugin so we can start implementing backward breaking changes.

The groovy scripts are our heaviest scripts as we use them for all types of regression, so they should be the first ones converted.

Instead of installing checkstyle and then running mvn site, we should package the project into the all jar and run the CLI checkstyle program and produce the XML violation file. This file and the sources will be what is fed into patch-diff-report-tool to produce the final report.

We should not need to change patch-diff-report-tool. Using the CLI allows us to run Checkstyle on the repo directory directly instead of copying the files to another location temporarily. This will also allow us to run regression on non-Java files which is what maven-jxr-plugin forced on us.

rnveach avatar Dec 25 '17 03:12 rnveach

I am ok create issue on Checkstyle to generate HTML report, I think we already have such issue, and raised this in discussions several times. Or users/me can use xsl to convert xml to html (very old fashioned approach).

Html generation is better to be done html template libraries but it is new dependency .... so it is ok to generate html by old-fashioned way (StringBuilder.append)

maven-jxr-plugin

this is plugin that we benefit a lot, to convert sources to html. It(as a tool) might be used separately to just convert folder of source to folder of html .

romani avatar Dec 25 '17 18:12 romani

I am ok create issue on Checkstyle to generate HTML repor

For regression, we don't need this. patch-diff-report-tool creates the final HTML report for us. We don't use the output of maven-jxr-plugin for anything. So HTML generation from Checkstyle would have to be a separate need then this.

rnveach avatar Dec 25 '17 18:12 rnveach

we don't use the output of maven-jxr-plugin for anything.

this is result of maven-jxr-plugin - https://djydewang.github.io/diffReport_Issue4981/findbugs-with-excldues/xref/home/bbg/project/contribution/checkstyle-tester/repositories/findbugs-with-excldues/eclipsePlugin/src/edu/umd/cs/findbugs/plugin/eclipse/quickfix/util/SourceLineVisitor.java.html#L26 , without generation of such pages, html report is usless for us , to my mind. We should be able to run it on sources (with fake pom.xml) to generate folder of html pages.

romani avatar Dec 25 '17 18:12 romani

this is result of maven-jxr-plugin

patch-diff-report-tool generates this via JavaCodeTransform and TextTransform. This is why we have these code transforms in the tool. https://github.com/checkstyle/contribution/blob/master/patch-diff-report-tool/src/main/java/com/github/checkstyle/site/XrefGenerator.java#L136-L160 If you look at the tool's test resource folder, we only give it Java files as inputs, not html files of the generated source. https://github.com/checkstyle/contribution/tree/master/patch-diff-report-tool/src/test/resources/run . We don't verify the results of the html generation for the Java sources.

If you at look inputs to patch-diff-report-tool at https://github.com/checkstyle/contribution/blob/master/patch-diff-report-tool/README.md refFiles is given the src/main/java folder, not the target folder. The tool takes the xml checkstyle violation file as input, which has direct links to the source files that caused violations.

rnveach avatar Dec 25 '17 19:12 rnveach

@romani If you have any other doubts, see https://github.com/checkstyle/contribution/pull/274 . I was able to update my utility with no problems and didn't need any 3rd party dependencies or maven.

rnveach avatar Dec 26 '17 21:12 rnveach

If this issue is completed, close https://github.com/checkstyle/contribution/issues/255

rnveach avatar Dec 24 '20 03:12 rnveach

I still see usage of plugin in pom.xml - https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/pom.xml#L59 I think site plugin can be removed for sure as we use HTML generation by our diff tool.

@nmancus1 , please make summary of where we are now.

romani avatar Feb 07 '21 14:02 romani

@nmancus1 , please make summary of where we are now.

We need to generate reports from checkstyle directly, but this issue is last on the list of improvements to checkstyle-tester. I have left a general update here.

nrmancuso avatar Feb 08 '21 19:02 nrmancuso

reminder: checkstyle do not have html reports, chekcstyle can generate xml report and diff.groovy can convert it to HTML.

romani avatar Feb 10 '21 14:02 romani

-Dassembly.skipAssembly=true has to be added to the assembly of the all jar to skip creating the extra files that are not needed.

rnveach avatar Jan 02 '22 23:01 rnveach