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

fix rat and checkstyle issue #15530

Merged
merged 2 commits into from
Dec 14, 2023
Merged

fix rat and checkstyle issue #15530

merged 2 commits into from
Dec 14, 2023

Conversation

AlbericByte
Copy link
Contributor

@AlbericByte AlbericByte commented Dec 8, 2023

Fixes #15124

Description

Building Druid with the below command on master/v27.0 branches:

mvn clean package install -DskipTests -Pdist -Prat
the build failed with the below error.

Update the following configuration to fix this issue:

  • checkstyle configurations
  • rat excludes configurations

Fixed the bug ...

Release note


Key changed/added classes in this PR
  • checkstyle-suppressions.xml
  • pom.xml

This PR has:

  • been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021
Copy link
Member

The original issue is not re-produced on the master branch, but got a lots of checkstyle problems when running the command

mvn package install -DskipTests -Pdist -Prat

The checkstyle problems are all for files in the generated-source directory of druid-sql module. I think the change in this PR makes sense.

But I would like to suggest using global match to exclude all checks for the generated source files like the following instead of adding supression rules one by one to the supression file.

<suppress files="[\\/]target[\\/]generated-test-sources[\\/]" checks=".*"/>
<suppress files="[\\/]target[\\/]generated-sources[\\/]" checks=".*"/>

@@ -29,23 +29,50 @@
<!-- Code copied from TestNG to apply a bugfix -->
<suppress checks="AvoidStaticImport" files="[\\/]org[\\/]testng[\\/]" />

<suppress checks="LocalFinalVariableName" files="[\\/]target[\\/]generated-sources[\\/]" />
Copy link
Member

Choose a reason for hiding this comment

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

Remove all rules from this file for generated-source and generated-test-sources and add the following two rules:

Copy link
Member

Choose a reason for hiding this comment

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

The URL in Line 27 of this file is out of date. Please also change it to: https://checkstyle.org/filters/suppressionfilter.html

Copy link
Contributor Author

@AlbericByte AlbericByte Dec 12, 2023

Choose a reason for hiding this comment

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

@FrankChen021 good catch, fixed, Thanks

@FrankChen021
Copy link
Member

This problem is only re-produced when running command 'mvn clean package install'.
The checkstyle runs twice for one module, one is the at the very beginning of the build phase, that is the validate phase, the other is after the 'install' phase.

I don't know why this happens. We can investigate this separately. For GHA, I think there's no need to make changes now until we find out the root cause why the checkstyle runs twice under the 'package install' command

@FrankChen021 FrankChen021 merged commit 0436eda into apache:master Dec 14, 2023
83 checks passed
<exclude>**/jvm.config</exclude>
<exclude>**/*.avsc</exclude>
<exclude>**/*.png</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change required ?

@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Druid build fails with git command '-no-show-signature'
4 participants