search

checkstyle / contribution

some useful sources that should not stay in main repo but it is good to host them
GNU Lesser General Public License v2.1
49 stars 131 forks source link

minor: add openjdk21 and remove openjdk17 from projects file for report generation #878

Closed mahfouz72 closed 4 months ago

mahfouz72 commented 4 months ago 
[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 commented 4 months ago

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

mahfouz72 commented 4 months ago

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

rnveach commented 4 months ago

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 commented 4 months ago

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 https://github.com/checkstyle/contribution/pull/878#issuecomment-2228784742. So what source code do you refer to? I see no extra info in this CI failure

rnveach commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

CI is green idk why it failed before please consider merging

rnveach commented 4 months ago

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

mahfouz72 commented 4 months ago

https://github.com/checkstyle/contribution/blob/fb4a65b361fd6c6887444eb0e89a9191ec89d789/.ci/validation.sh#L35-L46

rnveach commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

romani commented 4 months ago

Files are added in this PR https://github.com/checkstyle/contribution/pull/785#pullrequestreview-1455790371

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 commented 4 months ago

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 https://github.com/checkstyle/contribution/pull/878#discussion_r1678624190

rnveach commented 4 months ago

@mahfouz72 Since there is too much going on in this issue to possibly get lost, please move https://github.com/checkstyle/contribution/pull/878#issuecomment-2229543870 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 commented 4 months ago

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

https://github.com/checkstyle/contribution/issues/880

romani commented 4 months ago

test-configs will be updated to have openjdk21 https://github.com/checkstyle/test-configs/issues/124