Issue #529: Deprecate launch.groovy and merge functionality into diff.groovy #546

nrmancuso · 2021-02-14T14:42:39Z

Issue #529: Deprecate launch.groovy and merge functionality into diff.groovy

Notes:

We should replace all usages of launch.groovy with diff.groovy in single mode, using 'patchBranch' and 'patchConfig' . This isn't a big deal, we are just saving an extra checkstyle run using master branch.
Any usages of launch that are being replaced with diff that DO NOT use -i need to specify --allowExcludes now.
All existing diff.groovy usage needs to be reviewed; no-exception tests can be done in single mode.

Related PRs that must be merged after this for CI in other projects:
checkstyle/checkstyle#9278
sevntu-checkstyle/sevntu.checkstyle#828

A good example of no-exception testing and replacing launch.groovy for many cases is:
https://github.com/checkstyle/checkstyle/blob/492dd22e4730b7902e0af91819da840c2dac7b23/.ci/no-exception-test.sh#L68

rnveach · 2021-02-14T15:44:56Z

Any existing diff usages do not need to change.

You need to review that one I pointed out not too long ago that was converted from launch to diff prematurely before this PR.

nrmancuso · 2021-02-14T15:49:21Z

You need to review that one I pointed out not too long ago that was converted from launch to diff prematurely before this PR.

Yes, you are right, that one can be changed to single mode too, as all no-exceptions testing can. Thanks for the reminder.

nrmancuso · 2021-02-14T20:33:12Z

At this point CI is green for all PR's related to this change in Checkstyle and Sevntu:
checkstyle/checkstyle#9278
sevntu-checkstyle/sevntu.checkstyle#828

~~Still need to change over CI scripts in both to do single mode, instead of diff mode to save CI time.~~
Just need to do checkstyle, sevntu is completed. Examples of single/no-exceptions mode: sevntu-checkstyle/sevntu.checkstyle#828 (comment)

nrmancuso · 2021-02-14T22:35:49Z

Difference in Maven commands between master and this PR

AppVeyor:

DESC=checkstyle-tester (diff.groovy) on guava
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

DESC=checkstyle-tester (diff.groovy with base and patch configs) on guava
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

Travis:

CMD=./.ci/travis.sh checkstyle-tester-launch-groovy
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.failsOnError=false
maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

CMD="./.ci/travis.sh checkstyle-tester-diff-groovy-patch"
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false
maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

CMD=./.ci/travis.sh checkstyle-tester-diff-groovy-patch-only
maven command from master: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

maven command from PR: mvn -e --batch-mode site -Dcheckstyle.config.location=my_check.xml -Dcheckstyle.excludes= -Dcheckstyle.version=8.41-SNAPSHOT -Dcheckstyle.failsOnError=false

nrmancuso · 2021-03-21T14:30:02Z

checkstyle-tester/README.md

+**allowExcludes** (g) - this option tells `diff.groovy` to allow paths and files defined in a
+`projects*.properties` file to be excluded from report generation (optional, default is false).
+


Now we can allow excludes in diff.groovy execution

romani · 2021-03-22T05:08:36Z

Does it make sense to remove launch.groovy ?

nrmancuso · 2021-03-22T15:15:35Z

Does it make sense to remove launch.groovy ?

Probably. I can do it as a separate commit (with corresponding documentation change) in case we need to revert it.

@rnveach what do you think?

rnveach · 2021-03-22T23:53:47Z

Does it make sense to remove launch.groovy ?

This was covered in issue. Removal is a breaking change.
#529

launch.groovy has to remain in repo until Checkstyle AND Sevntu is updated to remove all usage of it. Once Checkstyle AND Sevntu makes the conversion, then launch.groovy can be removed.
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/LAUNCH_GROOVY_README.md should be removed when launch if finally removed.

Even if the PR in all repos are ready to be merged now, not even accounting for time for other admins to review, I think it would be safer to have it in it's own PR to be sure it is ready to be removed.

Once removed, any CIs/PRs trying to use it will fail since it is gone...

nrmancuso · 2021-03-23T00:05:59Z

I think it would be safer to have it in it's own PR to be sure it is ready to be removed.

No complaints here, I can just submit a PR for launch removal once this is merged.

rnveach

First pass questions. Haven't looked at code in depth.

rnveach · 2021-03-23T00:02:48Z

.ci/travis.sh

@@ -69,6 +71,69 @@ checkstyle-tester-diff-groovy-patch-only)
    -pc my_check.xml -p patch-branch -r ../.ci-temp/checkstyle -m single
  ;;

+checkstyle-tester-diff-groovy-regression-single)


The way i understand this, you run regression using Contribution Master and Contribution PR and check if the 2 separate runs produce the same or different files. I assume difference will fail the CI (and are we sure of that?).

If the above is correct, is this planned to be merged in? What happens if we make diff-report-util change the layout. This will then always fail CI even though it is an expected change.

I assume difference will fail the CI (and are we sure of that?).

Yes. https://travis-ci.org/github/checkstyle/contribution/jobs/763804426

If the above is correct, is this planned to be merged in? What happens if we make diff-report-util change the layout. This will then always fail CI even though it is an expected change.

I would guess at that point we would need to use some sort of more complicated awk or perl command to verify that violations match up; right now, this CI job confirms that both the violations and format of the report are consistent.

I wouldn't do anything too complicated. Should we not merge it or is there some alternative for this to not cause issues easily?

I think we should keep these, I have pushed several times in this PR, and these jobs haven't been flaky. It is the best way that I can think of to easily check that the violations, reports, and report formats are all consistent.

I think it is ok to wait and see what the future brings regarding changes to patch-diff-util before doing anything different here.

.ci/travis.sh

checkstyle-tester/README.md

rnveach · 2021-03-23T00:11:43Z

checkstyle-tester/README.md

+`projects*.properties` file to be excluded from report generation (optional, default is false).
+
+**checkstyleVersion** (cv) - used to set the Checkstyle version to be used by maven during report
+generation; this option should mainly be used by satiellite projects


Maybe I lost track. Why do we need to pass in a checkstyle version to override what is being used automatically? Point to example if there is one.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/828/files#r599509434

ok, this is just for no exception testing in sevntu.

These 2 properties are pretty dangerous to use. If the wrong version is set, maven won't fail normally. It will try to find this version installed in the repo or pulled from the server. If it finds a version to use, it will use that and not be noticeable to the user that the wrong version is being regressed on.

Take our own regression, someone active in the repo will always be running local regression. So it is possible they have previous versions installed. If the use this and set an old version, the report will be generated, however, it will not be hitting their new code at all and only hitting old code. This will give a false sense of testing, especially if no changes are to be expected.

I would add some verbage to this to make it clear it is not to be used in normal circumstances. It is for internal use only. It should be close to the first thing read on the property.

Ideally, I think these should be removed but I don't see an alternative to get no exception testing to work in sevntu. If support is added for sevntu, these properties should then be removed.

I can add a check to make sure that these are only used in single mode, too.

Added a check for this:

➜ checkstyle-tester git:(issue-529) ✗ groovy diff.groovy -l projects-to-test-on.properties -r ../../checkstyle -p my-issue --checkstyleVersion "8.41.1" Picked up _JAVA_OPTIONS: -Dawt.useSystemAAFontSettings=gasp -Dawt.useSystemAAFontSettings=gasp WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/usr/share/groovy/lib/groovy-2.5.13.jar) to method java.lang.Object.finalize() WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release Error: Diff mode may not use '--sevntuVersion' or '--checkstyleVersion' arguments! Caught: java.lang.IllegalArgumentException: Error: invalid command line arguments! ... ➜ checkstyle-tester git:(issue-529) ✗ groovy diff.groovy -l projects-to-test-on.properties -r ../../checkstyle -p my-issue --sevntuVersion "8.41.1" Picked up _JAVA_OPTIONS: -Dawt.useSystemAAFontSettings=gasp -Dawt.useSystemAAFontSettings=gasp WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/usr/share/groovy/lib/groovy-2.5.13.jar) to method java.lang.Object.finalize() WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release Error: Diff mode may not use '--sevntuVersion' or '--checkstyleVersion' arguments! Caught: java.lang.IllegalArgumentException: Error: invalid command line arguments! ...

These options have been removed now.

rnveach · 2021-03-23T00:13:14Z

checkstyle-tester/README.md

+generation; this option should mainly be used by satiellite projects
+(optional, default is the latest snapshot).
+
+**sevntuVersion** (sv) - used to set the Sevntu-Checkstyle version to be used by maven during


Same here. We are not saying this supports sevntu regression, right?

We are not saying this supports sevntu regression, right?

Only for single mode, so just for no-exception testing.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/828/files#r599509434

Same here.

What is this comment referring to?

This option has been removed now.

rnveach

2 more connected to documentation as I go through code.

rnveach · 2021-03-23T00:25:15Z

checkstyle-tester/diff.groovy

-        r(longOpt: 'localGitRepo', args: 1, required: true, argName: 'path',
-            'Path to local git repository (required)')
+        r(longOpt: 'localGitRepo', args: 1, required: false, argName: 'path',
+            'Path to local git repository for regression testing (optional)')


Documentation still says this is required. Why did we change this to optional?

Fixed, see
https://gist.github.com/nmancus1/bae2ef94de5d0457c8c59631d5192b88 for new behavior (added checking for both of the mentioned args in diff mode).

This is optional now because of sevntu usage.

This is optional now because of sevntu usage.

I understand but I don't fully agree. This is only because of no sevntu support. We would have to flop this if support is ever given as repo is always required.

~~Can we change sevntu usage to give sevntu repo and just not do anything with it?~~ Edit: I was thinking branch could be left optional, but it was the other item that was required. I am not seeing anything else right now.

Maybe a different alternative. If not, then we will keep it.

sevntu usage should NOT change design on diff.groovy
it is just extension of maven config, if user add something in config he need to make sure that versions are overidable by maven variables.

Reverted this.

rnveach · 2021-03-23T00:25:30Z

checkstyle-tester/diff.groovy

-        p(longOpt: 'patchBranch', args: 1, required: true, argName: 'branch_name',
-            'Name of the patch branch in local git repository (required)')
+        p(longOpt: 'patchBranch', args: 1, required: false, argName: 'branch_name',
+            'Name of the patch branch in local git repository for regression testing (optional)')


Same as above.

checkstyle-tester/diff.groovy

romani

items:

checkstyle-tester/diff.groovy

nrmancuso · 2021-04-07T01:10:12Z

All items have been addressed up to this point, we need to make sure that all CI in other projects is passing. I will share results here.

nrmancuso · 2021-04-07T16:51:04Z

I had to remove the diff report CI job; every time that we change a check we would have to update this job to a new commit, or it will fail. I am leaving the other job, since we are only generating a report for a single commit so that it will be consistent.

… into diff.groovy

romani

This update is huge leap forward, there a lot items to make better, but lets not spend bunch of time on this polishing now, we can improve later on in other PRs.

romani · 2021-04-10T22:25:40Z

I got offline approval from @rnveach to move forward

nrmancuso mentioned this pull request Feb 14, 2021

infra: move all launch.groovy usages in CI to diff.groovy checkstyle/checkstyle#9278

Merged

nrmancuso force-pushed the issue-529 branch 3 times, most recently from f4101ed to 1a82e83 Compare February 14, 2021 15:39

nrmancuso force-pushed the issue-529 branch from 1a82e83 to 2411e55 Compare February 14, 2021 15:47

nrmancuso marked this pull request as draft February 14, 2021 15:53

nrmancuso force-pushed the issue-529 branch 4 times, most recently from df2cb36 to 0321c09 Compare February 14, 2021 19:27

nrmancuso force-pushed the issue-529 branch from 0321c09 to 3fd2575 Compare February 14, 2021 21:43

nrmancuso mentioned this pull request Feb 14, 2021

infra: move wercker no-exception testing to diff.groovy sevntu-checkstyle/sevntu.checkstyle#828

Merged

nrmancuso force-pushed the issue-529 branch from 3fd2575 to c55246a Compare February 15, 2021 14:37

nrmancuso force-pushed the issue-529 branch 7 times, most recently from 71def16 to 57ff5b6 Compare March 21, 2021 13:40

nrmancuso marked this pull request as ready for review March 21, 2021 14:04

nrmancuso commented Mar 21, 2021

View reviewed changes

rnveach reviewed Mar 23, 2021

View reviewed changes

nrmancuso force-pushed the issue-529 branch from 57ff5b6 to cfa7372 Compare March 23, 2021 13:00

This comment has been minimized.

Sign in to view

rnveach reviewed Mar 23, 2021

View reviewed changes

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

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

nrmancuso force-pushed the issue-529 branch 2 times, most recently from 7545f11 to 877c016 Compare April 3, 2021 19:19

romani requested changes Apr 5, 2021

View reviewed changes

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

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

checkstyle-tester/diff.groovy Show resolved Hide resolved

romani self-assigned this Apr 6, 2021

nrmancuso force-pushed the issue-529 branch 2 times, most recently from 845a9ad to f0b4cde Compare April 7, 2021 01:08

nrmancuso force-pushed the issue-529 branch 8 times, most recently from f515a2f to 11d81e9 Compare April 7, 2021 16:48

nrmancuso force-pushed the issue-529 branch from 11d81e9 to 5380019 Compare April 7, 2021 17:15

Issue checkstyle#529: Deprecate launch.groovy and merge functionality…

488e285

… into diff.groovy

nrmancuso force-pushed the issue-529 branch from 5380019 to 488e285 Compare April 7, 2021 18:36

romani approved these changes Apr 10, 2021

View reviewed changes

romani assigned rnveach and unassigned romani Apr 10, 2021

romani merged commit 80943b7 into checkstyle:master Apr 10, 2021

		allowExcludes (g) - this option tells `diff.groovy` to allow paths and files defined in a
		`projects*.properties` file to be excluded from report generation (optional, default is false).

Issue #529: Deprecate launch.groovy and merge functionality into diff.groovy #546

Issue #529: Deprecate launch.groovy and merge functionality into diff.groovy #546

Conversation

nrmancuso commented Feb 14, 2021 • edited Loading

rnveach commented Feb 14, 2021

nrmancuso commented Feb 14, 2021 • edited Loading

nrmancuso commented Feb 14, 2021 • edited Loading

nrmancuso commented Feb 14, 2021 • edited Loading

Difference in Maven commands between master and this PR

AppVeyor:

Travis:

Choose a reason for hiding this comment

romani commented Mar 22, 2021

nrmancuso commented Mar 22, 2021

rnveach commented Mar 22, 2021 • edited Loading

nrmancuso commented Mar 23, 2021

rnveach 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

rnveach Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Apr 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Mar 23, 2021 • 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

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Mar 23, 2021 • 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

Choose a reason for hiding this comment

This comment has been minimized.

romani left a comment

Choose a reason for hiding this comment

nrmancuso commented Apr 7, 2021

nrmancuso commented Apr 7, 2021

romani left a comment

Choose a reason for hiding this comment

romani commented Apr 10, 2021

nrmancuso commented Feb 14, 2021 •

edited

Loading

nrmancuso commented Feb 14, 2021 •

edited

Loading

nrmancuso commented Feb 14, 2021 •

edited

Loading

nrmancuso commented Feb 14, 2021 •

edited

Loading

rnveach commented Mar 22, 2021 •

edited

Loading

rnveach Mar 23, 2021 •

edited

Loading

nrmancuso Apr 3, 2021 •

edited

Loading

nrmancuso Mar 23, 2021 •

edited

Loading

rnveach Mar 23, 2021 •

edited

Loading