File name too long error

[rickie]

I use Linux Ubuntu 20.04.1 on my machine and my file system is encrypted with eCryptfs. As a result, there is a maximum allowed filename size for folders and files. For context, see this StackOverflow question.

If I do a mvn clean install on Checkstyle master, I end up with the following error:

[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /home/rick/repos/oss/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.java:[14,13] error while writing com.puppycrawl.tools.checkstyle.checks.modifier.redundantmodifier.InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.PublicInnerInterface.PublicInnerInnerInterface.PublicInnerClassInNestedPublicInterfaces: /home/rick/repos/oss/checkstyle/target/test-classes/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers$PublicInnerInterface$PublicInnerInnerInterface$PublicInnerClassInNestedPublicInterfaces.class: File name too long

The problem is that the names of all the inner classes and interfaces are chained together with a $ in between for the file names. This results in fairly long file names as can be seen in the example.

I realize this is quite an uncommon case and asking the following feels like a long shot, but here is my question.
Would you be open to fixing this problem? If yes, I'm open to creating a PR for it based on your decision on how to best fix this. The simple way would of course be renaming some of the classes and interfaces, to prevent this problem from happening. However, I can see how this is not an ideal way of fixing this.

To give you a potential solution, see this diff.

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheckTest.java
index af4601e7f..c7a01f7c6 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/modifier/RedundantModifierCheckTest.java
@@ -135,7 +135,7 @@ public class RedundantModifierCheckTest
        };

        verifyWithInlineConfigParser(
-            getPath("InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.java"),
+            getPath("InputRedundantModifierNestedClassInInt.java"),
            expected);
    }
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInInt.java
similarity index 60%
rename from src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.java
rename to src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInInt.java
index 03a0cb906..bafe3ca16 100644
--- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers.java
+++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/modifier/redundantmodifier/InputRedundantModifierNestedClassInInt.java
@@ -8,7 +8,7 @@ tokens = (default)METHOD_DEF, VARIABLE_DEF, ANNOTATION_FIELD_DEF, INTERFACE_DEF,

package com.puppycrawl.tools.checkstyle.checks.modifier.redundantmodifier;

-public interface InputRedundantModifierNestedClassInPublicInterfaceRedundantModifiers {
+public interface InputRedundantModifierNestedClassInInt {
    interface PublicInnerInterface {
        interface PublicInnerInnerInterface {
            class PublicInnerClassInNestedPublicInterfaces {
@@ -16,14 +16,14 @@ public interface InputRedundantModifierNestedClassInPublicInterfaceRedundantModi
            }
        }
    }
-    class PublicClassInsideInterface {
-        private interface PrivateNestedInterface {
-            class ClassInPrivateNestedInterface {
-                public ClassInPrivateNestedInterface() { } // violation
+    class PublicClassInsideInt {
+        private interface PrivateNestedInt {
+            class ClassInPrivateNestedInt {
+                public ClassInPrivateNestedInt() { } // violation
            }
-            public interface PrivateNestedInterfaceWithPublicModifier { // violation
-                class ClassInPrivateNestedInterface {
-                    public ClassInPrivateNestedInterface() { } // violation
+            public interface PrivateNestedIntWithPublicModifier { // violation
+                class ClassInPrivateNestedInt {
+                    public ClassInPrivateNestedInt() { } // violation
                }
            }
        }
@@ -33,13 +33,13 @@ public interface InputRedundantModifierNestedClassInPublicInterfaceRedundantModi
            }
        }
        protected interface PublicInnerInnerProtectedInterface {
-          class PublicInnerClassInNestedProtectedInterfaces {
-           public PublicInnerClassInNestedProtectedInterfaces() { } // violation
+          class PublicInnerClassInNestedProtectedInt {
+           public PublicInnerClassInNestedProtectedInt() { } // violation
          }
        }
    }
-    class PublicNestedClassInInterfaceWithPublicConstructor {
-        public PublicNestedClassInInterfaceWithPublicConstructor() { } // OK in public class
+    class PublicNestedClassInInterfaceWithPublicConst {
+        public PublicNestedClassInInterfaceWithPublicConst() { } // OK in public class
        private class PrivateClassInPublicNestedClass {
            public class PublicInPrivateClass {
                public PublicInPrivateClass() { } // violation

This was identified by working with our integration-test framework from Error Prone Support, which also identified this issue: #14042.

Let me know if I need to clarify something or elaborate.

[nrmancuso]

@rickie issue is approved, please send a PR. We might ultimately change naming from what is proposed above, but we can work something out. Do you have an idea of how we can prevent this from happening in the future? If we don’t have a check that can currently do this, it might be good to propose one if this is a common issue.

[romani]

@rickie, may be we should apply your tool to our codebase and we will benefit from some extra code validations, we try use use all code analysis tools in world, if you look in our pom, you will see it.
It will be part of our "mvn verify" and may be our CI will catch such problems at the beginning.

[nrmancuso]

@romani I am not sure if encrypting an entire file system in CI is reasonable

[romani]

I just try to activate this tool first. On our code base. We will be in benefit and maybe some CI will catch problems that picnic team has, not specifically this, but maybe be others.

[rickie]

@rickie issue is approved, please send a PR.

Nice, I'll open a PR in a bit.

And w.r.t. testing, I'm not really sure. Internally at Picnic I've had this problem only once... It's not a common thing so I'm not sure how far we should go with preventing this. I'm still kind of surprised no one ran into this issue as my setup is not that uncommon (I suppose).

@rickie, may be we should apply your tool to our codebase and we will benefit from some extra code validations, we try use use all code analysis tools in world, if you look in our pom, you will see it.
It will be part of our "mvn verify" and may be our CI will catch such problems at the beginning.

That will be really cool to do! I'm totally up for that, we should think about an approach for this though. Error Prone Support contains a large set of rules that will be interesting for Checkstyle, but also a few more "Picnic-opinionated" rules that might be to strict. Also, enabling all these rules in one go will result in a very large diff. In the integration test we apply all the rules on Checkstyle and we end up with a git diff of 62.000+ lines, see here. Mainly because there are a few rules that have a big impact on the codebase, like the JUnitMethodDeclaration and StaticImport check.

I'd say it makes most sense to create few different PRs for different checks such that the review process is a lot easier.

Till now, I haven't seen an easy way of enabling only a small amount of the Error Prone rules that we have (I probably have to dig a bit deeper in this).

So my question to you all is; what is the desired approach to this? I'm open to discussing options 😄.

[romani]

We can create instance of https://checkstyle.org/checks/regexp/regexponfilename.html to our config to violate file name above some limit, I hope regexp can do this.

[romani]

but also a few more "Picnic-opinionated" rules that might be to strict

I hope we can disable some rules that we/checkstyle team decide to not be enforced.

Also, enabling all these rules in one go will result in a very large diff.

I hope we can enforce rule by rule, in separate PRs, to have smaller PRs and are not painful to review.

So my question to you all is; what is the desired approach to this?

We can create new issue and simply list rules, and discuss each of the one by one, and merge PRs if we got to agreement.
We usually enable tool and disable all rules, each PR enable single rule or if too much does it in few PRs. And we slowly, but firmly moving to enforcement of all.