-
Notifications
You must be signed in to change notification settings - Fork 136
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
scan all versions in multi-version-jar #61
Conversation
@rPraml : thanks for contributing.
See https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've identified 3 places, where multi-release-jars are not properly handled according to the spec.
These changes should make ecj more javac comatible.
It would be great, if someone can pick up this work and write a test, as I have not much knowledge how to do this in the eclipse ecosystem
if (!Files.exists(this.releasePath)) { | ||
this.releasePath = null; | ||
for (int version = Integer.parseInt(this.compliance); version >= 9; version--) { | ||
this.releasePath = this.fs.getPath("/", "META-INF", "versions", String.valueOf(version)); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to
https://docs.oracle.com/javase/10/docs/specs/jar/jar.html#multi-release-jar-files
all versions down to 9 have to be checked.
this.compliance
should be always a major version accroding to the spec, so parseInt
should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it would be good to parse the compliance only once (same below) and cache it in a field?
ZipEntry entry = zipFile.getEntry(releasePath); | ||
if (entry != null) { | ||
path = releasePath; | ||
for (int version = Integer.parseInt(release); version >= 9; version--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is practically the same code as above.
} catch (Exception e) { | ||
e.printStackTrace(); | ||
// move on to the default | ||
for (int version = Integer.parseInt(this.compliance); version >= 9; version--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here, we search for the module-info.class
, which should be the fix for the bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why It's being done in ClasspathJar instead of org.eclipse.jdt.internal.core.builder.ClasspathMultiReleaseJar ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarthana Do you mean, why I have made this change or why this code is here at all?
IMHO it sounds logical, if this is handled in ClasspathMultiReleaseJar as it does not require to have duplicate code.
(But I'm really not the expert to do such refactoring in this project)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering about having this in the super class ClasspathJar but in case of the compiler.batch package, you have made it in the subclass ClasspathMultiReleaseJar. Haven't looked at the code in detail, but IIRC, we had similar code and hierarchy in both the compiler.batch and core.builder packages.
This PR refers to
https://bugs.eclipse.org/bugs/show_bug.cgi?id=577790