Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rnveach opened this issue Dec 25, 2017 · 10 comments
Open

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

rnveach opened this issue Dec 25, 2017 · 10 comments

Comments

@rnveach
Copy link
Member

rnveach commented Dec 25, 2017

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.

@romani
Copy link
Member

romani commented Dec 25, 2017

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 .

@rnveach
Copy link
Member Author

rnveach commented Dec 25, 2017

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.

@romani
Copy link
Member

romani commented Dec 25, 2017

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.

@rnveach
Copy link
Member Author

rnveach commented Dec 25, 2017

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
Copy link
Member Author

rnveach commented Dec 26, 2017

@romani If you have any other doubts, see #274 .
I was able to update my utility with no problems and didn't need any 3rd party dependencies or maven.

nrmancuso added a commit to nrmancuso/contribution that referenced this issue Nov 28, 2020
nrmancuso added a commit to nrmancuso/contribution that referenced this issue Nov 28, 2020
nrmancuso added a commit to nrmancuso/contribution that referenced this issue Nov 28, 2020
nrmancuso added a commit to nrmancuso/contribution that referenced this issue Nov 28, 2020
nrmancuso added a commit to nrmancuso/contribution that referenced this issue Nov 28, 2020
nrmancuso added a commit to nrmancuso/contribution that referenced this issue Dec 15, 2020
@rnveach
Copy link
Member Author

rnveach commented Dec 24, 2020

If this issue is completed, close #255

@romani
Copy link
Member

romani commented Feb 7, 2021

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.

@nrmancuso
Copy link
Member

nrmancuso commented Feb 8, 2021

@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.

@romani
Copy link
Member

romani commented Feb 10, 2021

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

@rnveach
Copy link
Member Author

rnveach commented Jan 2, 2022

-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 added a commit to rnveach/contribution that referenced this issue Dec 31, 2022
rnveach added a commit to rnveach/contribution that referenced this issue Dec 31, 2022
rnveach added a commit to rnveach/contribution that referenced this issue Dec 31, 2022
rnveach added a commit to rnveach/contribution that referenced this issue Dec 31, 2022
rnveach added a commit to rnveach/contribution that referenced this issue Dec 31, 2022
rnveach added a commit to rnveach/contribution that referenced this issue Jan 2, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 2, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 3, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 9, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 9, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 9, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 9, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 10, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 10, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 10, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 10, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 27, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Jan 27, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Feb 4, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Feb 4, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Feb 10, 2023
rnveach added a commit to rnveach/contribution that referenced this issue Feb 10, 2023
Mansi8874 added a commit to Mansi8874/contribution that referenced this issue May 20, 2023
Issue checkstyle#273: converts diff.groovy to use Checkstyle's CLI
Mansi8874 added a commit to Mansi8874/contribution that referenced this issue May 20, 2023
Issue checkstyle#273: converts diff.groovy to use Checkstyle's CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants