minor: add openjdk21 and remove openjdk17 from projects file for report generation by mahfouz72 · Pull Request #878 · checkstyle/contribution

mahfouz72 · 2024-07-14T00:15:17Z

No description provided.

mahfouz72 · 2024-07-15T15:30:59Z

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project sample: Error generating maven-checkstyle-plugin:3.1.1:checkstyle report: Failed during checkstyle execution: There is 1 error reported by Checkstyle 10.18.0-SNAPSHOT with my_check.xml ruleset. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project sample: Error generating maven-checkstyle-plugin:3.1.1:checkstyle report

please help with this I don't understand the failure. I need this update to use the default list. We should have openjdk21 in the default projects now.

rnveach · 2024-07-15T15:32:33Z

Provide us a link to what is being executed before this diff.groovy call so we can see the context of the job.

mahfouz72 · 2024-07-15T15:35:15Z

https://app.circleci.com/pipelines/github/checkstyle/contribution/363/workflows/cbafbb70-b92e-446d-aeec-82ca8f4ba40e/jobs/2516

rnveach · 2024-07-15T15:37:42Z

I meant the source code. You need know what is the context of this job and what it is trying to do to understand what the failure is.

mahfouz72 · 2024-07-15T15:53:44Z

This job is trying to generate reports right? what I see is that it clones checkstyle repo and the report generation failed with this message #878 (comment). So what source code do you refer to? I see no extra info in this CI failure

rnveach · 2024-07-15T15:56:49Z

So what source code do you refer to?

Looking at https://app.circleci.com/pipelines/github/checkstyle/contribution/363/workflows/cbafbb70-b92e-446d-aeec-82ca8f4ba40e/jobs/2516

each section has a title. The title of section with the job failure is ./.ci/validation.sh checkstyle-tester-diff-groovy-patch. That is the job being executed, so we will need to look at it's source to see what it is doing and trying to understand the failure.

mahfouz72 · 2024-07-15T16:41:27Z

I tried to run this script on local, build was successful

Creation of configuration report is started.
Creation of diff html site is started.
Creation of the result site succeed.
patch-diff-report-tool execution finished.
Running command: java -jar /home/mahfouz/contribution/patch-diff-report-tool/target/patch-diff-report-tool-0.1-SNAPSHOT-jar-with-dependencies.jar --patchReport reports/patch-branch/checkstyle-sonar/checkstyle-result.xml
                        --output reports/diff/checkstyle-sonar --patchConfig my_check.xml --baseReport reports/master/checkstyle-sonar/checkstyle-result.xml --baseConfig my_check.xml
patch-diff-report-tool execution started.
XML parsing is started.
Creation of configuration report is started.
Creation of diff html site is started.
Creation of the result site succeed.
patch-diff-report-tool execution finished.
Diff report generation finished ...
Starting creating report summary page ...
Creating report summary page finished...

mahfouz72 · 2024-07-15T17:32:19Z

CI is green idk why it failed before please consider merging

rnveach · 2024-07-15T17:43:00Z

Can you provide the link to the source of the job?

mahfouz72 · 2024-07-15T17:57:16Z

contribution/.ci/validation.sh

Lines 35 to 46 in fb4a65b

    
           checkstyle-tester-diff-groovy-patch) 
        
             checkout_from https://github.com/checkstyle/checkstyle 
        
             cd .ci-temp/checkstyle 
        
             git checkout -b patch-branch 
        
             cd ../../checkstyle-tester 
        
             cp projects-to-test-on.properties ../.ci-temp/projects-to-test-on.properties 
        
             sed -i'' 's/^guava/#guava/' ../.ci-temp/projects-to-test-on.properties 
        
             sed -i'' 's/#checkstyle/checkstyle/' ../.ci-temp/projects-to-test-on.properties 
        
             groovy diff.groovy -l ../.ci-temp/projects-to-test-on.properties \ 
        
               -c my_check.xml -b master -p patch-branch -r ../.ci-temp/checkstyle -h 
        
             ;;

rnveach · 2024-07-15T18:32:47Z

So this test regression on checkstyle's master against itself and runs regression on checkstyle project only. The default config file is really set up to produce no violations. It is basically a no harms test of the diff groovy.

It uses projects-to-test-on.properties but you modified projects-to-test-on-for-github-action.properties, so yes, this test has no connection to your changes.

It didn't specify what the error was, so we won't know what the issue was. So its failure does seem unrelated.

rnveach · 2024-07-15T18:34:17Z

Since this says for github actions, I assume this means in main repo. Is this file and openjdk17 used anywhere in main repo that we have switch it out once this is merged?

mahfouz72 · 2024-07-15T19:11:51Z

I don't think so. This file is not in the main repo. Updating this file in this PR is enough. It is used in diff-report.yml
https://github.com/checkstyle/checkstyle/blob/d335962fa743408b6823236cd5c352eb7ea7965a/.github/workflows/diff-report.yml#L18-L20

I just don't know why we have those files:

They are not used anywhere why are we keeping them?

rnveach · 2024-07-15T22:28:56Z

This file is not in the main repo. Updating this file in this PR is enough.

It is only enough if the main repo does not make use of this file or this property you changed. If we made use of this in main repo, we would most likely get a failure after merge since openjdk17 no longer exists.

It is used in diff-report.yml

So this is only used for default configuration for regression. Users can supply an override and all projects in this file should be uncommented.

rnveach · 2024-07-15T22:29:57Z

@romani @nrmancuso

don't know why we have those files:

Are we keeping these files for some reason? They were added over a year ago and they just say minor: New Projects added for Diff regression.

rnveach · 2024-07-15T22:30:42Z

checkstyle-tester/projects-to-test-on-for-github-action.properties

 # RequireThis usage
 spring-integration|git|https://github.com/spring-projects/spring-integration|main|

-# openjdk 17 requires lots of excludes, list here should be consistent with file filters at https://github.com/checkstyle/checkstyle/blob/master/.ci/openjdk17-excluded.files


@nrmancuso This line says it must have excludes consistent with main repo. Are you sure we can remove all excludes?

This was added in f8728f0 almost 2 1/2 years ago.

Related discussions:

#584

checkstyle/checkstyle#10956

#594

I tried to catch up with discussions

We used to have an exclude list (for non-compilable files) for all projects in this file. but we then removed all excludes from all projects because it turned out that these excluded files were ignored. We removed them to be consistent with previous reports.

After that, we added support for exclude option with --allowExcludes. But we never returned the exclude list to the projects list.

Then we added this openjdk17 with this huge exclude list.

I can confirm that this file (projects-to-test-on-for-github-action.properties) isn't used by any other task like no-exception. It is only used as a DEFAULT_PROJECTS_LINK in diff-report.yml. So, removing this exclude list and openjdk17 will not affect any other task.

now the question should we add an exclude list for the new openjdk21? If yes, then why don't we add an exclude list for the other projects?

Should we add an exclude list for the new openjdk21?

I don't think we gain any thing if we add it. we have 2 cases :

non-compilable file and can't be parsed by CS: This will get ignored due to the skipFileOnJavaParseException new treewalker property

non-compilable file but can be parsed (example): This will have no parse exception so will not be skipped and it will appear in the report. Is this acceptable? for me, I don't think we will lose anything by not excluding those files On the contrary they might help ensure regression (example for a non-compilable file that shows that the check update is as expected from this PR)

I am not sure if we should add files filters for these files, though, since they are how I confirmed logic of check update.

@nrmancuso
Please correct me if anything is wrong I tried to go quickly over the blame of this file. You must have strong opinion because all these PR are created by you :)

non-compilable file but can be parsed (example): This will have no parse exception so will not be skipped and it will appear in the report. Is this acceptable? for me, I don't think we will lose anything by not excluding those

Thanks a lot for such detailed post.
There is a nuance. If file is not compile able by javac, but parse able by checkstyle - target Check will execute and produce some violations or no violations.
It will be very confusing, as in report it will looks like diff in behavior and will be red flag for all reviewers. It will be not easy to catch a fact that this file is not legitimate to run on.

Removal of excludes is prone to cause bunch of false investihations for author of PR or bad requests from maintainers to investigate differences.

I think we need to keep exclude list for open jdk.

How do we even know these files?

it was hard work of @nrmancuso .

can we find any way to know not compilable by javac, but parse able by checkstyle files

only by pratice and spotting something weird in diff report.
We can make a script to find all, but I am not sure that it is reasonable to do this at summer.

In this case, the excludes is for non-parseable files only which can be completely ignored by the treewalker property

should be skipped completely for diff report.
Treewalker can handle all, this list is for known files that can give weird behavior and confuse readers of diff report.

@nrmancuso , please step in.

These exclusions are to avoid having to analyze files that don't matter (noncompilable) or that we don't support parsing. It is generated by bash/python from here.

Let's just generate these and move on, we can debate the value of having these in another issue

Look like @nrmancuso , agree to keep this list but content might be different.

@nrmancuso @romani done please approve pr
issue :
#881

romani · 2024-07-16T01:05:07Z

Files are added in this PR
#785 (review)

Unfortunately no reference to why we need this.
Sounds like there were some testing activities on extended list of projects, and most likely me asked to preserve list of projects.
But I don't see anything reference them in PR descriptions , looks like they never used.

mahfouz72 · 2024-07-16T01:13:28Z

Unfortunately no reference to why we need this.

Then we should have a good reference in this PR about why we did this update before merge to help future us :)

I tried to explain here #878 (comment)

romani

I placed reason to keep excludes in comment above

rnveach · 2024-07-16T14:38:24Z

@mahfouz72 Since there is too much going on in this issue to possibly get lost, please move #878 (comment) to a new issue.

We should have an issue if we are going to remove them anyways, otherwise, we will document why we need the files and possibly add the reason to the file(s).

mahfouz72 · 2024-07-17T01:07:29Z

We should have an issue if we are going to remove them anyways, otherwise, we will document why we need the files and possibly add the reason to the file(s).

#880

…rt generation

romani

Ok to merge.

Looks like we regenerated exclude list that is good.

romani · 2024-07-17T14:11:00Z

test-configs will be updated to have openjdk21 checkstyle/test-configs#124

mahfouz72 force-pushed the jdk21 branch 4 times, most recently from 98ee02e to ad0a031 Compare July 15, 2024 17:21

rnveach approved these changes Jul 15, 2024

View reviewed changes

rnveach assigned nrmancuso Jul 15, 2024

rnveach requested a review from nrmancuso July 15, 2024 22:31

romani requested changes Jul 16, 2024

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Jul 16, 2024

romani assigned nrmancuso and unassigned romani Jul 16, 2024

mahfouz72 mentioned this pull request Jul 17, 2024

checkstyle-tester: unused project.properties files #880

Open

minor: add openjdk21 and remove openjdk17 from projects file for repo…

549e79d

…rt generation

mahfouz72 force-pushed the jdk21 branch from ad0a031 to 549e79d Compare July 17, 2024 01:53

mahfouz72 mentioned this pull request Jul 17, 2024

Remove exclude list from openjdk projects #881

Open

nrmancuso approved these changes Jul 17, 2024

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Jul 17, 2024

nrmancuso mentioned this pull request Jul 17, 2024

minor: add openjdk21 to projects file for diff report generation #821

Closed

mahfouz72 mentioned this pull request Jul 17, 2024

new project list template checkstyle/test-configs#123

Closed

romani approved these changes Jul 17, 2024

View reviewed changes

romani merged commit 8d09811 into checkstyle:master Jul 17, 2024

romani mentioned this pull request Jul 17, 2024

Use openjdk21 in projects lists checkstyle/test-configs#124

Closed

Comments

Conversation

mahfouz72 commented Jul 14, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

rnveach commented Jul 15, 2024

Uh oh!

rnveach Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

romani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

romani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

romani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

nrmancuso Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

romani Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani commented Jul 16, 2024

Uh oh!

mahfouz72 commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

rnveach commented Jul 16, 2024

Uh oh!

mahfouz72 commented Jul 15, 2024 •

edited

Loading

mahfouz72 commented Jul 15, 2024 •

edited

Loading

rnveach Jul 15, 2024 •

edited

Loading

mahfouz72 Jul 15, 2024 •

edited

Loading

mahfouz72 Jul 16, 2024 •

edited

Loading

mahfouz72 Jul 17, 2024 •

edited

Loading

mahfouz72 commented Jul 16, 2024 •

edited

Loading