[MCOMPILER-485] Incorrect internal string format in generated package-info.class files on Windows

[sos-jira-migration-bot]

Daniel Widdis opened MCOMPILER-485 and commented

In 3.10.0, a new feature to generate package-info classes was added in [MCOMPILER-205]

Unfortunately, the binary class file is incorrectly generated using Java's file toString() which uses a platform dependent file separator character: on Windows, these Strings contain a backslash.  See this line which passes the platform-dependent file name (minus the root path and .java extension) to the ClassWriter.

The Java class file specification (4.2.1) specifies the internal format: "In this internal form, the ASCII periods (.) that normally separate the identifiers which make up the binary name are replaced by ASCII forward slashes (/)."

This invalid class file format causes problems in downstream plugins, e.g.,

• The bnd-maven-plugin fails with an error: Classes found in the wrong directory: {oshi/jna/platform/unix/package-info.class=oshi\jna\platform\unix\package-info ... (and hundreds more) ... }
• The JDK's classFileParser will throw java.lang.NoClassDefFoundError: com\puppycrawl\tools\checkstyle\ant\package-info (wrong name: com/puppycrawl/tools/checkstyle/ant/package-info

Steps to reproduce:

1. Perform a build on any Windows machine using maven-compiler-plugin 3.10.0
2. Inspect any of the generated package-info.class files in target/classes using a text editor

Expected behavior:

Internal strings representing package names use forward slashes

Observed behavior:

Internal strings representing package names use backward slashes

 

---

Affects: 3.10.0

Issue Links:

• MCOMPILER-205 incremental compilation broken for package-info classes
  ("is caused by")

Remote Links:

• GitHub Pull Request #95
  
• AspectJ compilation problem caused by this bug
  

3 votes, 8 watchers

[sos-jira-migration-bot]

Olivier Lamy commented

good catch. by chance can you provide a simple project to reproduce it? so we can easily integrate this as an IT test and work faster on this issue. thanks

[sos-jira-migration-bot]

Daniel Widdis commented

I'll give it a shot on a day that's not Valentine's day.  

Also for anyone stumbling on this, a workaround is to disable the on-by-default behavior, e.g.,

mvn clean test "-Dmaven.compiler.createMissingPackageInfoClass=false"

[sos-jira-migration-bot]

Olivier Lamy commented

I'll give it a shot on a day that's not Valentine's day.  

oh yes definitely not the good day, we definitely don't want to cause such huge trouble!!

[sos-jira-migration-bot]

Daniel Widdis commented

Here's a quick reproducer project:  dbwiddis/foo: Minimal repository for reproducing bugs (github.com)

1. Clone the project
2. On windows, run ./mvnw.cmd clean verify

Result:

[INFO] --- maven-compiler-plugin:3.10.0:compile (default-compile) @ foo ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 2 source files to C:\Users\dawiddis.REDMOND\git\foo\target\classes
[INFO]
[INFO] --- bnd-maven-plugin:6.1.0:bnd-process (default) @ foo ---
[ERROR] C:\Users\dawiddis.REDMOND\git\foo\pom.xml [0:0]: Classes found in the wrong directory: {foo/bar/package-info.class=foo\bar\package-info}
[

[sos-jira-migration-bot]

Daniel Widdis commented

Added a class reading the bytes from the object file:
Path packageInfo = Paths.get("target/classes/foo/bar/package-info.class");
byte[] binaryObject = Files.readAllBytes(packageInfo);
String s = new String(binaryObject, StandardCharsets.US_ASCII);
System.out.println("Bad class format:" + s.contains("foo\\bar\\package-info"));
System.out.println("Look at the evil backslashes: " + s);

Output:
Bad class format: true
Look at the evil backslashes: ???? 4 ?? ?package-info.java? ?foo\bar\package-info? ?? ?java/lang/Object? ??SourceFile? ? ?

 

[sos-jira-migration-bot]

Daniel Widdis commented

Suggested fix (possibly wrapped in a conditional): 

javaFile.replaceAll(File.separator, "/");

 

[sos-jira-migration-bot]

Olivier Lamy commented

OMG it looks you've trashed a Valentine's day 🤣

[sos-jira-migration-bot]

Daniel Widdis commented

I had a few hours' break for a romantic dinner. ;)

[sos-jira-migration-bot]

Daniel Widdis commented

A few other comments and then I think I'll leave it to the experts:

1. Existing IT just has "assert packageInfoClassFile.exists()".  Could probably be modified to check the contents of the file for the expected dummy/package-info and not dummy\package-info.
2. Docs for https://asm.ow2.io/javadoc/org/objectweb/asm/ClassVisitor.html indicate the the visit() call should be before visitSource(). I have no idea if this has any impact on this issue, but was one thing I noted in my debugging tour.

[sos-jira-migration-bot]

Olivier Lamy commented

thanks for your help, comments and provided test

And enjoy your romantic dinner 😜

[sos-jira-migration-bot]

Daniel Widdis commented

Submitted a PR with proposed fix:  [MCOMPILER-485] Fixes internal string format in generated package-info.class by dbwiddis · Pull Request #95 · apache/maven-compiler-plugin (github.com)

[sos-jira-migration-bot]

Olivier Lamy commented

PR merged.

Thanks!

[sos-jira-migration-bot]

Jesse Glick commented

Will there be some notification when the fix is released to Central?

[sos-jira-migration-bot]

Olivier Lamy commented

Yes I have to plan to do it this week.

need to release and wait for the required vote period.

But the hope is to have dependabot triggering thousands of PRs by the end of the week (maybe for the weekend :))

[sos-jira-migration-bot]

Alexander Kriegisch commented

Just in case other AspectJ users also get weird errors due to this problem: I never would have expected this to be an MCOMPILER problem, unaware of the fact that the plugin actually creates byte code for package infos using ASM instead of relying on Javac or ECJ functionality. I even implemented a woraround in the AspectJ compiler for this, because I thought that Javac itself creates bogus byte code for this corner case. Only later I noticed that Maven Compiler itself was the culprit. See eclipse-aspectj/aspectj#176. This was an expensive bug.

Lesson learned (I hope): I think you should have used Java compiler functionality in order to create those files, namely -Xpkginfo:always on Javac and rely on ECJ to create those class files by default anyway. No own implementation, no own bug.