Skip to content

HADOOP-17968 Migrate checkstyle module illegalimport to maven enforcer banned-illegal-imports #3584

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.

Sign up for GitHub

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

Merged
tasanuma merged 3 commits into from
Oct 28, 2021
Merged

HADOOP-17968 Migrate checkstyle module illegalimport to maven enforcer banned-illegal-imports #3584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f861cc
HADOOP-17968 Migrate checkstyle module illegalimport to maven enforce…
virajjasani Oct 25, 2021
2dd0c63
keep sun package in checkstyle
virajjasani Oct 25, 2021
202459b
addressing review
virajjasani Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions hadoop-build-tools/src/main/resources/checkstyle/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,8 @@
<!-- Checks for imports -->
<!-- See http://checkstyle.sf.net/config_import.html -->
<module name="IllegalImport">
<property name="regexp" value="true"/>
<property name="illegalPkgs" value="sun, com\.google\.common"/>
<property name="illegalClasses" value="^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.io\.BaseEncoding, ^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.base\.(Optional|Function|Predicate|Supplier), ^org\.apache\.hadoop\.thirdparty\.com\.google\.common\.collect\.(ImmutableListMultimap)"/>
<property name="regexp" value="true"/>
<property name="illegalPkgs" value="sun"/>
</module>
<module name="RedundantImport"/>
<module name="UnusedImports"/>
Expand Down
54 changes: 50 additions & 4 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,23 +189,69 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/x
<reason>Use hadoop-common provided Sets rather than Guava provided Sets</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.collect.Sets</bannedImport>
<bannedImport>com.google.common.collect.Sets</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use hadoop-common provided Lists rather than Guava provided Lists</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.collect.Lists</bannedImport>
<bannedImport>com.google.common.collect.Lists</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use hadoop-common provided VisibleForTesting rather than the one provided by Guava</reason>
<reason>Use hadoop-annotation provided VisibleForTesting rather than the one provided by Guava</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting</bannedImport>
<bannedImport>com.google.common.annotations.VisibleForTesting</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternatives to Guava common classes</reason>
<bannedImports>
<bannedImport>com.google.common.**</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided BaseEncoding</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.io.BaseEncoding</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided Optional</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.base.Optional</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided Function</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.base.Function</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided Predicate</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.base.Predicate</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided Supplier</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.base.Supplier</bannedImport>
</bannedImports>
</restrictImports>
<restrictImports implementation="de.skuzzle.enforcer.restrictimports.rule.RestrictImports">
<includeTestCode>true</includeTestCode>
<reason>Use alternative to Guava provided ImmutableListMultimap</reason>
<bannedImports>
<bannedImport>org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableListMultimap</bannedImport>
</bannedImports>
</restrictImports>
Copy link
Contributor

@amahussein amahussein Oct 25, 2021

Choose a reason for hiding this comment

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

Don't we need to keep "sun" packages? it is in the checkstyle illegal-imports

Copy link
Contributor Author

@virajjasani virajjasani Oct 25, 2021

Choose a reason for hiding this comment

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

Shall we take this separately because sun packages are already in use? Classes already in use are:

sun.misc.Unsafe
sun.misc.Signal
sun.misc.SignalHandler
sun.net.spi.nameservice.NameService

Copy link
Contributor

@amahussein amahussein Oct 25, 2021

Choose a reason for hiding this comment

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

I see!
I think that if the jira is to migrate "guava packages" then we may keep checkstyle.illegal-import entry for sun packages?
Otherwise, if the purpose to remove checkstyle illegal-imports all together, then we can exclude those classes. there is also a flag failBuild if we don't want the build to fail with some patterns.
WDYT?

Copy link
Contributor Author

@virajjasani virajjasani Oct 25, 2021

Choose a reason for hiding this comment

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

I see! I think that if the jira is to migrate "guava packages" then we may keep checkstyle.illegal-import entry for sun packages?

Sounds good, let me put back sun packages.

Copy link
Contributor

@amahussein amahussein Oct 25, 2021

Choose a reason for hiding this comment

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

great!

Copy link
Contributor Author

@virajjasani virajjasani Oct 25, 2021

Choose a reason for hiding this comment

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

Done. Thanks @amahussein

Copy link
Contributor

@amahussein amahussein Oct 25, 2021

Choose a reason for hiding this comment

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

I will wait for the yetus report then merge.

</rules>
Expand Down