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

RAT-369: Add checkstyle and spotbugs to build and generate a report #238

Merged
merged 23 commits into from
May 6, 2024

Conversation

ottlinger
Copy link
Contributor

I verified locally (via ./.buildtools/generateStagingSiteInWebpageRepo) that a report is generated, but the build is not yet configured to fail.

@ottlinger
Copy link
Contributor Author

@Claudenw in case of any errors the build fails. I'm not sure if it's that helpful or if we remove this setting on the branch .....

@ottlinger
Copy link
Contributor Author

ottlinger commented Apr 16, 2024

In order to be able to bring changes into master I'd prefer to configure limits per module with the current threshold, thus we could make sure that no new stuff is being added and can fix the errors&warning in an incremental way - WDYT @Claudenw ?

@Claudenw
Copy link
Contributor

My thought was to turn on the spotbugs and fix any critical errors at that time (the ones with the red circles).
We should then open fixes to remove the yellow ones and/or determine if there are way to ignore them in specific cases (annotations perhaps?) so that we can fix the ones that need fixing and mark the ones that we determine are not an issue.

@ottlinger
Copy link
Contributor Author

@Claudenw the thing is that it either breaks the build or not. One can configure exclusions in an XML file in order to tolerate certain bugs explicitly ..... I'll have a look, but the first report said roughly up to 100 errors per submodule.

@ottlinger
Copy link
Contributor Author

ottlinger commented Apr 21, 2024

Most of the errors relate to the fact that many file operations rely on the default charset. Unfortunately newer version of the JDK provide constructors to specify an encoding, but as RAT still relies on JDK8 these changes will be bigger.

Example:

  • PrintStream
  • FileReader
  • PrintWriter

@Claudenw I changed the configuration to not fail the build temporarily, currently there are 68 errors left.

Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me. Just 2 real changes.

apache-rat-core/spotbugs_ignore.xml Outdated Show resolved Hide resolved
apache-rat-plugin/spotbugs_ignore.xml Outdated Show resolved Hide resolved
apache-rat-tasks/spotbugs_ignore.xml Outdated Show resolved Hide resolved
apache-rat/spotbugs_ignore.xml Show resolved Hide resolved
spotbugs_ignore.xml Show resolved Hide resolved
@ottlinger ottlinger requested a review from Claudenw May 2, 2024 07:52
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. import * is the only real problem

@ottlinger ottlinger changed the title WIP: RAT-369: Add spotbugs to build and generate a report WIP: RAT-369: Add checkstyle and spotbugs to build and generate a report May 4, 2024
@ottlinger
Copy link
Contributor Author

Could we merge this branch and fix the remaining errors step-by-step? Personally I'd prefer to have shorter-living branches ;) @Claudenw

@ottlinger ottlinger changed the title WIP: RAT-369: Add checkstyle and spotbugs to build and generate a report RAT-369: Add checkstyle and spotbugs to build and generate a report May 4, 2024
@ottlinger ottlinger requested a review from Claudenw May 4, 2024 15:00
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. though I don't see any report of spotbugs being generated. I was hoping we could get something that would tell us if the number of bugs was decreasing over time.

spotbugs_ignore.xml Show resolved Hide resolved
@ottlinger
Copy link
Contributor Author

@Claudenw If you do not mind I would merge this one tomorrow :) Thanks for your feedback.

@Claudenw
Copy link
Contributor

Claudenw commented May 5, 2024 via email

@ottlinger ottlinger merged commit 5c091cd into master May 6, 2024
32 checks passed
@ottlinger ottlinger deleted the feature/RAT-369 branch May 6, 2024 21:12
@ottlinger
Copy link
Contributor Author

@Claudenw I filed spotbugs/spotbugs-maven-plugin#804 to maybe get some feedback whether we can use spotbugs in a different way to allow failing the build if new errors are introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants