minor: test PR for new projects file and removal of jxr

[nrmancuso]

Related to checkstyle/contribution#527

No need for review on this PR; I have opened it to check that the removal of the maven jxr plugin from checkstyle-tester won't cause any problems with checkstyle CI. In order to determine where to test the changes in checkstyle/contribution#527, I did:

➜  checkstyle git:(remove-jxr) grep -R 'launch.groovy\|diff.groovy'
.github/workflows/diff_report.yml:           groovy diff.groovy -r $REPO -p $REF -pc new_module_config.xml -m single\
.github/workflows/diff_report.yml:           groovy diff.groovy -r $REPO -b $BASE_BRANCH -p $REF -bc diff_config.xml\
.github/workflows/diff_report.yml:           groovy diff.groovy -r $REPO -b $BASE_BRANCH -p $REF -c diff_config.xml\
.ci/no-exception-test.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/no-exception-test.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/no-exception-test.sh:  groovy ./launch.groovy --listOfProjects ../../../.ci/openjdk-projects-to-test-on.config \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-for-wercker.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects projects-to-test-on.properties \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects $PROJECTS --config $CONFIG \
.ci/wercker.sh:  groovy ./launch.groovy --listOfProjects $PROJECTS --config $CONFIG \
.ci/shippable.sh:  groovy launch.groovy --listOfProjects projects-for-circle.properties \
.ci/shippable.sh:  groovy launch.groovy --listOfProjects projects-for-circle.properties \
.ci/shippable.sh:  groovy launch.groovy --listOfProjects projects-for-circle.properties \
.ci/shippable.sh:  groovy launch.groovy --listOfProjects projects-for-circle.properties \
.ci/shippable.sh:  groovy launch.groovy --listOfProjects projects-to-test-on.properties \
➜  checkstyle git:(remove-jxr) 

@rnveach Is there anything else we should look at to verify that it is safe to remove the JXR plugin from checkstyle-tester?

Note that I have used the github mirror for openjdk(in the projects file) that has failed to run in the past due to the JXR plugin, to show that this mirror will parse now that JXR is removed.

Diff Regression projects: https://gist.githubusercontent.com/nmancus1/97cf95e0aed0c5789d5e0fc23ef82ff3/raw/1d4d102d59e7cfbc0a4d770fc55de3e04fee3b4d/projects-to-test-on.properties

Diff Regression config: https://gist.githubusercontent.com/nmancus1/f5579d1ad36c5ebe441738ad9bf71d4a/raw/74a7356c0a6fc322b11013055046333136f0f3ae/NoWhitespaceBefore.xml

[nrmancuso]

Github, generate report

[github-actions]

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/438540410

[nrmancuso]

Github, generate report

[github-actions]

Report generation job failed on phase "make_report", step "Download files".
Link: https://github.com/checkstyle/checkstyle/actions/runs/438630169

[nrmancuso]

Github, generate report

[rnveach]

Is there anything else we should look at to verify that it is safe to remove the JXR plugin from checkstyle-tester?

I would post reports re-running diff groovy and launch groovy with some change in master showing the reports with differences/violations. Show that the 2 programs don't need JXR and will continue to function properly without them and provide us the necessary reports. You could show the comparison of the complete folders created between JXR and non-JXR runs.

IMO, I think launch groovy will fail as it looks like it makes references to HTML files JXR produces. It doesn't have reference or use the diff-report-util. https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch.groovy#L250
If this is indeed the case, we then need to look at if our CI looks for and tries to use these files or are we completely ok without them.

If we swapped all our CIs to use diff groovy instead of launch, then we can say launch is deprecated and just need to make sure diff groovy still fully works.

[nrmancuso]

launch groovy will fail

I tried a few of these locally, such as .ci/./wercker.sh no-exception-struts and this passed, just stating that the report wasn't available; we will see what results in CI.

[rnveach]

I wouldn't rely on CI for full validation of launch.

CI passes now before this PR, meaning no violations. I think an issue (or not) will only show itself when launch reports violations.

I didn't create a lot of the CI items, nor have a full knowledge of what they need for validation. If needs be, you can point to the logic showing CI doesn't care about launch HTML reports as output. If validation requires HTML reports and it has failsafe when no HTML is found, CI won't report this as an issue.

[nrmancuso]

CI passes now before this PR, meaning no violations. I think an issue (or not) will only show itself when launch reports violations.

Good point. Thanks for explaining.

some change in master showing the reports with differences/violations

What would be a good change to make to show violations? Removing a token from a check?

[rnveach]

I would do a change already existing in master and not make your own change, but thats just me. An existing PR would have difference reports already created for possible additional comparisons. Just create a branch from this commit and pretend it's your "PR" for the reports.

https://github.com/checkstyle/checkstyle/commits/master
Commit: 92be3fb
PR and diff: #8711 (comment)

This is one commit that would be a good example for differences. Running launch doesn't matter so much on differences as you just want it to production violations, so any violation from the check, or another check is fine.

[github-actions]

Report generation job failed on phase , step .
Link: https://github.com/checkstyle/checkstyle/actions/runs/438638350

[rnveach]

Just another thought.

The CI here is suppose to validate checkstyle only. It doesn't and shouldn't fully validate changes to external tools like the groovy scripts. Our CI is already overflowed with items, adding ones for external tools will just bloat it up more.

This is why this CI shouldn't be fully used to validate changes in the groovy scripts from contribution repo.

Our CI in contribution, https://github.com/checkstyle/contribution/blob/master/.travis.yml , should do all the extra validation needed for the groovy scripts. I see it as beneficial if we could make it show how the groovy scripts will behave with your new changes in checkstyle/contribution#527 .

We can do without any changes to contribution CI for now if its an issue, but its just going to make everyone (probably just talking more about myself) more comfortable with these changes for a script that is a big part of our regression suite.

[nrmancuso]

@rnveach thanks a lot for sharing your thoughts; let me clarify what my intentions are, and perhaps you can provide a bit of guidance. I would like to clean up/ consolidate the tools in contribution to make contribution easier, and to be able to create reports for the newer openjdk repos on github (the highest version of java hosted on the mercurial repo is 10) without hacks like extraMvnOptions if we can help it. The mercurial repos frequently go down, and are fairly slow as well.

My motivation for wanting to make these changes is that it is tough to find good repos that use the newer jdk features that Checkstyle supports; we should really be parsing the openjdk repos, since they use all the new language features, often in challenging and unusual ways.

My thought was that it would be good to first enable us to (1)parse the openjdk github repos; this only requires the removal/ ignoring/ modification of the maven jxr plugin. Then, (2)migrate all launch.groovy usage in Checkstyle to use diff.groovy. After that, begin picking off issues in contribution such as checkstyle/contribution#273 and checkstyle/contribution#367.

I see it as beneficial if we could make it show how the groovy scripts will behave with your new changes

Can you recommend the best way to do this?

Edit: would it be better to move all launch.groovy usage in Checkstyle first? I have some time in the coming weeks to work on these items, but I am not sure where the best place to start is.

[rnveach]

Yes, it sound like we need a plan here and some things you pointed to helps. I will create issues for each of our ideas to track them and better plan. Feel free to contact me in telegram for any 1 on 1s that can be faster.

[rnveach]

Here is my thoughts on order. Let me know if I missed anything.

checkstyle/contribution#530 - forgo tests. Looking at https://github.com/checkstyle/checkstyle/blob/master/.ci/no-exception-test.sh#L26-L27, it doesn't look like CI cares if HTML is created or not. It just requires maven to fail the build if it fails. Launch will eventually be removed too.
checkstyle/contribution#524 and checkstyle/contribution#527 - finish PR for new github repos.
checkstyle/contribution#523 - first part of merging launch into diff. lets make this easy and just do copy/paste of what we can. ¿Maybe look into creating groovy tests?
checkstyle/contribution#529 - Complete merging.
checkstyle/contribution#367 - start some removals.
checkstyle/contribution#273 - complete removals.

Can you recommend the best way to do this?

https://groovy-lang.org/testing.html
Testing is going to be new for groovy. This also brings up a thought brought up before. Should we keep this as groovy or is it becoming so complex we should switch it to a Java project.

[nrmancuso]

Should we keep this as groovy or is it becoming so complex we should switch it to a Java project.

I was thinking exactly this today. In my opinion, diff.groovy is getting pretty heavy for a script.

[rnveach]

@nmancus1 Minor addition, Sevntu also uses launch.groovy . See https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/.ci/wercker.sh#L35 . I am making updates to existing issues as I see/remember things we need to be mindful of in this revamp.

[nrmancuso]

Since we have determined that this PR is unnecessary, all discussion has been moved to checkstyle/contribution#531. I am closing this issue.