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

Make Checkstyle an (automatic) module #14120

Open
future2r opened this issue Dec 8, 2023 · 6 comments
Open

Make Checkstyle an (automatic) module #14120

future2r opened this issue Dec 8, 2023 · 6 comments

Comments

@future2r
Copy link
Contributor

future2r commented Dec 8, 2023

Checkstyle 10.x requires JDK 11 to run. Wouldn't it be the right time to finally make it a Java module (introduced with JDK 9)?

I have implemented some Checks for Checkstyle by my own. My project complains about an 'unstable module name derived from the file name'. Even Maven warns about not uploading my project to a public repository, since the build is far from being stable.

Since the filename is something like checkstyle-10.12.6.jar, Java ends up with the module name checkstyle. Usually, a good naming would be the name of the main package, in this case com.puppycrawl.tools.checkstyle. This is more expressive and more unique.

You may achieve this just by adding a module-info.java into your source root. For the beginning you may export all your packages and open those, that require reflection.

If this is not an option, please add AutomaticModuleName to the MANIFEST.MF. In this case, at least the module name is no longer constructed from the file name.

https://dzone.com/articles/automatic-module-name-calling-all-java-library-maintainers

jlink requires module-info.java, see details at #14666

@romani
Copy link
Member

romani commented Dec 8, 2023

Even Maven warns about not uploading my project to a public repository, since the build is far from being stable.

Please share details. We use maven for long time, never had a problem.

Since the filename is something like checkstyle-10.12.6.jar

I thought all maven artifacts are named like this, org name is in path/folders of maven repository. Please share details/examples that some other old projects are migrating.

Usually, a good naming would be the name of the main package, in this case com.puppycrawl.tools.checkstyle

Did you miss version and extension?


Please share link to your project with extension of checkstyle


What benefits our project will have from this?
What benefits our users will have? What problems they have now?

We have two plugins in our org ( sonar and eclipses), how it will help them?

Is it somehow breaking compatibility issue for some users? To let us plan it accordingly.

@future2r
Copy link
Contributor Author

It is not about Maven, it is about the Java 9 module system.

This is the module-info.java of the project with my implemented checks:

module com.qualitype.modaplex.checkstyle {

    requires java.xml;
    requires transitive checkstyle;

    exports com.qualitype.modaplex.checkstyle;
}

Java Compiler (since Java 9):
"Name of automatic module 'checkstyle' is unstable, it is derived from the module's file name."

Maven:
"[WARNING] Required filename-based automodules detected: [checkstyle-10.12.6.jar]. Please don't publish this project to a public artifact repository!"

Please check this for more background: https://dzone.com/articles/automatic-module-name-calling-all-java-library-maintainers

@romani
Copy link
Member

romani commented Dec 13, 2023

I am not sure I can find time to investigate all of this.
Can you contribute module definition to us ?
Maybe by example and initial implementation we will be onboarder quicker. I will invite plugin maintainers to double check that we will not affect them.

@future2r
Copy link
Contributor Author

Sure, I can try.

For the beginning, the simplest way would be to make Checkstyle a named automatic module. All you need to do is, add this line to your MANIFEST.MF:

AutomaticModuleName=com.puppycrawl.tools.checkstyle

This should change nothing in the behavior when used in a non-modularized environment. However, when used in a modularized environment, all those warnings should disappear.

Depending on your project setup this can be done manually or as a configuration in the packaging in the pom.xml. It will try this in a branch and let you know about the result.

The next step would be to create a proper module-info.java. This may be a bit harder, depending on how much of your code is considered as a public API and how much reflection is used. Furthermore, it depends on the state of modularization of your dependencies. I will try this in another branch,

This will be my first contribution to an open source project. So please be patient with me.

Best regards,
Frank

future2r added a commit to future2r/checkstyle that referenced this issue Dec 13, 2023
future2r added a commit to future2r/checkstyle that referenced this issue Dec 13, 2023
@future2r
Copy link
Contributor Author

future2r commented Dec 13, 2023

I did my very best. Hope this will help you. #14139

future2r added a commit to future2r/checkstyle that referenced this issue Dec 14, 2023
romani pushed a commit to future2r/checkstyle that referenced this issue Dec 28, 2023
BitC3t pushed a commit to BitC3t/checkstyle that referenced this issue Jan 7, 2024
@rnveach
Copy link
Member

rnveach commented Feb 1, 2024

@future2r When working on the backport release ( rnveach/checkstyle-backport-jre8#13 ), I received the following error:

Caused by: org.apache.maven.plugin.MojoExecutionException: Error assembling JAR
	at org.apache.maven.plugins.jar.AbstractJarMojo.createArchive(AbstractJarMojo.java:281)
	at org.apache.maven.plugins.jar.AbstractJarMojo.execute(AbstractJarMojo.java:305)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:126)
	at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2(MojoExecutor.java:342)
	... 25 more
Caused by: org.codehaus.plexus.archiver.jar.ManifestException: Invalid automatic module name: 'com.puppycrawl.tools.checkstyle-backport-jre8'
	at org.apache.maven.archiver.MavenArchiver.createArchive(MavenArchiver.java:702)
	at org.apache.maven.plugins.jar.AbstractJarMojo.createArchive(AbstractJarMojo.java:274)
	... 28 more

When I run verify test -e with PMD/Jacoco skips, the job seems to work fine. However, when I run the same command with skipping tests, I then get the above failure. This does not happen with the same commands in the main repo. Reverting 8ac4fcd fixes the run in backport.

Do you happen to know why this is? Is this because this project is for Java 8 only and this automatic module is more Java 9+?

Edit: I see now. It is because the artifact name has -. When I switch them to _, it resolves the issue. It is odd that it only occurs when skipping tests though.

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

No branches or pull requests

3 participants