Revapi JMPS compilation support - #301
IvanAndresFritzler wants to merge 7 commits into
Conversation
metlos
left a comment
There was a problem hiding this comment.
First of all, thank you very much for all your effort, the changes look great!
I have some minor stylistic comments but more importantly I am not sure this approach works (different API of MissingTypeAwareDelegatingElements) or that we're not dropping test-coverage of split packages (but then I have no idea how important it is it nowadays).
That said, I've not written any Java for a good number of years now so my understanding of things has surely waned. I am also not sure how important it is to keep support for Java8 (even though it is still 5 years away from EOL by Oracle if my google-fu doesn't fail me).
| } | ||
|
|
||
| @Override | ||
| public ModuleElement getModuleOf(Element e) { |
There was a problem hiding this comment.
Does this work? According to the spec, the public API of the classes across all the versions must be exactly the same and here we're adding a new method. See https://docs.oracle.com/en/java/javase/23/docs/specs/jar/jar.html#multi-release-jar-files.
There was a problem hiding this comment.
This actually works (we are being able to build and use both java 8 and java 9 versions). Both tooling and runtime are not making any extensive API check so this can compile and run. Even when you add a class that does not supersede any class of the original code it still compile and run, son I assume no checks at all actually.
Given that the one extending it's API is a JDK class here we have no option but to implement it this way or thinking about a bigger refactor. Let me think about this and I'll let you know.
There was a problem hiding this comment.
@metlos Do you consider the release of a major version that drops java 8 support an option? Going that way we could evaluate more additions too (I will still try an alternate implementation that respects the "same API" specification of the mrjars).
| @@ -0,0 +1,241 @@ | |||
| /* | |||
There was a problem hiding this comment.
Instead of duplicating the whole class and changing just the way the compiler options are composed, I think it'd be better to introduce a sort of package-private base class that would implement all of the common logic (i.e. basically a copy of what Compiler class currently is) and just define a new package-private abstract method for getting the compiler options that each of the Compiler classes would implement according to the java compiler version requirements (i.e. the Compiler in the main sources would use the classpath the old way and the Compiler in 9 would use the module path as you changed it).
There was a problem hiding this comment.
Yes, I agree. We will refactor this in order to implement it in that way.
| SUP_PACKAGE_PATH + "B$T$1$TT$1.class") | ||
| .addAsResource(compRes.compilationPath.resolve(SUP_PACKAGE_PATH + "B$T$2.class").toFile(), | ||
| SUP_PACKAGE_PATH + "B$T$2.class") | ||
| // Change to D because this is used in the api in SupplementaryJarsTest (split package fixes) |
There was a problem hiding this comment.
I think this was here exactly because Java8- allowed split packages (and I think Java8+ still supports it when only classpath is used).
There was a problem hiding this comment.
Yes, that's right. But the original coverage is kept in java 8.
There was a problem hiding this comment.
We can readd that runs only for java 8 to cover that case.
There was a problem hiding this comment.
@metlos readded a test that runs only in java 8 for verifying split package allowed case with supplementary jars.
|
I just had an idea. We could move on from java 8 by creating new plugins for every java version that brings new "API" features. The current This would ideally come with some refactoring so that we can reuse as much code as possible but I think long term it could be a more "clean" solution than relying on the undocumented behavior of the multi-release class-loading (by which I mean the fact that it works even if the API of the classes changes between releases, despite of what it says in the docs). |
Hello!
This PR is the smallest change that we found in order to make RevAPI include the jpms ModuleElement as part of the AST.
With this changes, the following code returns the jpms module element for any revapi JavaModelElement whose archive includes a jpms module definition (including automatic modules).
ModuleElement moduleElement = element.getTypeEnvironment().getElementUtils().getModuleOf(element.getDeclaringElement());This allowed us to implement a revapi filter that uses the module definition in order to compute the API boundaries.
We used a multi release approach where the change only applies for java 9+ because we saw it was used for an initial java9 support.
The test fixes are maily split packages (something that cannot be done with modules) and something at the ClasspathScanner that we think it was not triggering any issue because of the classpath setup but it becomes necessary when you use a module path.