-
Notifications
You must be signed in to change notification settings - Fork 155
Add multi-release compilation support to JDT #3900
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
base: master
Are you sure you want to change the base?
Conversation
I now also passed this down to the compiler, inspecting the generated class files shows that the |
9fc7614
to
1acafa7
Compare
I now also added a test-case to show the new feature, so I think this is ready for review. |
Do you plan to add UI support for this? |
Yes if this is available then next step would be to add UI in Java Build Path configuration page |
ed4fefe
to
a310892
Compare
@iloveeclipse tests are all green now can you review/merge? |
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 reviewed the test only and commented on test code only because I've realized there are too many open questions.
Some concept parts are unclear to me and I believe before we will check other code changes we should have a clear picture of the proposed concept.
Is the description of #265 ticket up-to-date with this PR? It doesn't look like it is, please provide there a full picture of proposed concept.
Topics to cover:
- effects on class output directories
- effects on extra compilation checks (errors/warnings) needed
- effects on inter-project compilation
- effects on indexer / search / call hierarchy / UI
For example, just few questions I had in mind:
- It seem to me https://openjdk.org/jeps/238 expects that the "release" contains exactly a subset of "default" sources and no new API / classes. They say
By this scheme, it is possible for versions of a class designed for a later Java platform release to override the version of that same class designed for an earlier Java platform release.
For me it means, in the IDE we shouldn't allow any class in the "release" directory that doesn't exist in "default" source directory? What is our take on it? If this is not the case, I assume classes from "release" source folders shouldn't be available for compilation of "default" source folders? Means, if you have classForRelease9
, it shouldn't be resolvable from "SomeInnocentClassForRelease8"? I don't see it is solved yet, at least incremental build allows to see all classes from "release" source folder in "default" source folder. - Should the "release" source folder be compiled last, after "default" source folder? My assumption is that "release" source folder is just "mirroring" a subset of classes from default source folder, so in order to resolve all dependencies to the default sources the order matters?
- If there are multiple "release" source folders, are they compiled in some specific oder? Does it matter?
- Given projects A (multi-release 8/21) and B (Java 21, uses A), the "release 21" class version from A should be used to compile project B in the IDE. This seem to be not implemented yet?
- What is about indexer? Which version do we see on index? Both? Only one?
- The "search" / "call hierarchy" etc resolve classes for single JDK release only, but they would need to resolve depending on the source folder where they were executed?
- The https://openjdk.org/jeps/238 "Classpath and modulepath" section explains that depending on where the multi-release jar is placed, different classes will be visible (because of exported packages per module). This need to be understood / implemented regarding multiple projects case like A/B above.
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/MultiReleaseTests.java
Show resolved
Hide resolved
Note: I'm not against the idea of supporting multi-release compilation in the IDE in general, so don't see my negative review as a sign that the idea will never be accepted, it just needs more work. |
Thanks for looking into this, I hope I can at least shade some light on some parts. I just want to point out, that the main goal is to allow multi-release-compilation this does not aim to a full verification of the multi-release-jar specification as such tool currently does not exits as far as I know.
It is mostly, but I have now choose a classpath attribute instead (as it is an existing concept) over a dedicated (new) attribute on the top level. This also seems like other concepts regarding such features are already implemented.
The general contract is that every "multi-release-class" must be present in "default" and has the exact same "API" as the one in the "release" variant, because java will load one class for the other, so all calls from there must be matching.
A "release" folder might contain more classes / methods, you just can't call any code here from the "default" The intend here is to support the feature up to the level of compilation, if that actually works as intended at runtime is something that is currently out of scope and a bit as if one uses reflection-api, it can only fully proven at runtime.
Basically yes this will be the case, this is similar to "test" classes not visible from "main. It becomes more interesting if you add a
I designed it that classes are compiled from lowest (default) to highest release, as this is how classes will be visible later on. There might cases where (technically) things are still visible, but I think this is nothing that has to be solved in the compiler, the use-case here is really small and focused, a MR jar has to be carefully designed and one won't write "big code" in a MR way, it is more like a service method that is implemented in two different ways. I once added an example here: https://github.com/bndtools/bnd/pull/5581/files it show evolution of reading an URL form java 8 (using commons I/O) to Java 9 (using readallbytes) to Java 11 (httpclient) invoked from a Main Again if we can add such support this would be perfect, but I don't want to kill this feature by complexity now that no one actually needs (now). If the feature is actually adopted and found to be useful in the larger scale It could be adopted.
At least that is how it is usually be done, so default > X > X+n but of course this is no where described how such a classfile is produced. From runtime perspective there is of course a hierarchy so that more recent release can use anything from an older release. So it seems to me quite natural to have this reflected in compile order.
Even though it (technically) would work I don't see that this is something we should actually support here, a MR class is likely nothing one would ever share between modules/component but are internal behavior. Given that all call-sites must be binary compatible, it should even be not relevant what actual class file is used during compilation.
I haven't checked the current implementation, but as JDT itself supports MR-jars as a binary already there, I would say it should work here as with an already build MR-jar. So if anything is there to be improved it should not depend on this feature in general.
As I can have two classes with the same name and package in two distinct projects, I would assume it is working like these two source folder where two projects, but that is something that has to be done on a higher level I think.
I would suspect this to be implemented as part of the regular MR-support in JDT. |
723b2c8
to
4bc1b79
Compare
@iloveeclipse I now have hopefully addressed the concerns and added more tests. It would be great if you can review again so this can be considered for M2 or at least M3 and I can work on the UI parts. |
@laeubi : seeing the date of M3 set to 16th of May I cannot guarantee that, I'm very busy right now, and this is a feature not not a regression for which I have the highest priority. @eclipse-jdt/eclipse-jdt-committers : could someone please take a look? |
I pulled the change and played with it resulting in this test project: While class files are produced in the expected output structure, compilation produces several unexpected errors. What's more: the set of reported errors depends on how exactly the build is triggered:
From browsing the code, I couldn't find any concepts that would define visibility between source folders. @laeubi do you have plans how such inter-folder references are supposed to be resolved?
|
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/State.java
Outdated
Show resolved
Hide resolved
@stephan-herrmann I'll look into your example, please note that some problems with code navigation are cause because JDT currently seem to not handle MR jars correct see: Beside that, as explained I do not want to solve everything in this PR but allow to at least enable basic MR compilation support. So more things will likely be to discovered (and need to solved), but at least the basic things should work (and people use this usually in a basic way and not create complex inter-dependencies between packages). |
4bc1b79
to
7912455
Compare
@stephan-herrmann I have now added your testcase and can confirm that it fails to compile even though I would expects it working (even though adding package private members / public methods to a MR type I could need some hints on how it is supposed to work with crossreferences between different source folders, as I can see |
Which classes are found is initially determined by the For an example how these issues can be resolved see how each Before you start adopting that strategy, it would be good to know, what is the expectation when a project has version dependent source folders as well as one or more test source folders: what should be the exact visibilities of each folder? |
I see al about to compiled files are put into the environment before starting the compiles so I'm a bit confused the name is not found but maybe something is cleared between the steps and I find it sometimes hard to track down where the exact error is generated in the compiler. My idea was to maybe not have an own NameEnvironment/builder but maybe it needs to be the case here will investigate.
At laest for the maven case I'm not aware that test sources are handled differently, as from the outside (that is the testable interface) all releases must look the same. So what one would do is run the same test for each release on the given JVM with the same compiled test classes. |
I now debugged it a bit and the cause for this is that compile call in the end |
7912455
to
67933b8
Compare
@stephan-herrmann thanks for the hints, I finally sorted it out, it was the other way round than I initially thought. Because the about to compiled names are set the compiler ignores to lookup these types from the compiled binary locations! So what was needed instead is to limit the names to the types currently compiled for the current target level! Now your provided testcase passes without a problem! |
I'm looking into it now ...
I think here it is where thing go wrong first, this should give an error always like after project clean as the Type
So this is kind of expected, see above, will need a new testcase for this.
This seems a bit expected here as the project is not using
This clearly violates the "binary compatible API contract" here and I'm not sure the compiler can detect this situation without complex analysis of all existing types and call sites. I would rather not implement this at this stage, of course such in deep analysis would be a great benefit to the user but that's more something one would perform outside compilation and given that depending on how it is used it might or might not work (a bit similar like null analysis where the compiler gives a hint a value might be |
Do you think PDE API tools could bring the desired functionality here? |
Maybe API tools do similar, but JDT would be a better place here as it is not really related to plugin development, maybe one can do something in the
But as of today I'm not aware of such "library method provided", one might argue that the compiler does not build the jar and such check has to come from the |
I now added a testcase for the case where the project settings are disabled the |
In order to help I would need more specific pointers ... in particular: what are you trying to achieve here? Generate byte code for an unspecified version? What errors should be controlled using that target option? Are you sure you understand this set of options: source, target, compliance, release? |
@stephan-herrmann I only found that |
FYI @stephan-herrmann It is just a bit clumsy and needs more cleanup but I pushed it here just to show that in general it works as the test now passes. I enhanced the test in a way that it also check if in a higher java release one can use the method that is not available at the lower one. Sadly the code is a little bit hard to extend I tried to make it more clear here: but it seem to fail some one test, have to try it in smaller batches maybe. What leads me to the question if you prefer I should try to make some code reorganization as separate PRs so they can be merged independently or keep them in the PR (that already has grown a bit). |
//TODO TEMP properly merge with ClasspathJrtWithReleaseOption | ||
if (releaseNumber>= IReleaseAwareNameEnvironment.FIRST_MULTI_RELEASE) { | ||
try { | ||
ClasspathJrtWithReleaseOption withReleaseOption = new ClasspathJrtWithReleaseOption(this.zipFilename, this.accessRuleSet, this.externalAnnotationPath ==null?null:IPath.fromOSString(this.externalAnnotationPath), Integer.toString(releaseNumber)); |
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 don't think we can afford to instantiate a ClasspathJrtWithReleaseOption
for each lookup. To much caching inside that would go to waste.
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.
Don't worry this was just a quick one to show it could work, now I need to make it not only work by being well integrated and nicely cached. With this change at least your described case with Java 17 using an API only available in Java 20+ will work (or more formally give an compile error) while it will be compile successful if you use it in a Java 20+ release folder.
By the way I don't see that actually much is cached in ClasspathJrtWithReleaseOption
itself there are some caches about read modules in the CtSym
but both are cached outside that class already in a global way.
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.
@stephan-herrmann I now was able to demangle the ClasspathJrtWithReleaseOption
and the ClasspathJrt
to support a generic (cached) lookup of release based classe where the first one now is more a lightweight redirect to the super classes.
@laeubi I have been surprised by your progress so far on a path that I hadn't expected. Unfortunately, I'm not sure that path will lead to success, because it doesn't seem to go well with the existing strategy of data caching.
I don't see how those multiple instances of We do have a precedent for multiple builder invocations with different configurations (see |
@stephan-herrmann actually I'm quite confident that this all will work out and haven't come to a conclusion that it is the wrong way its more feels like every test-case and the corresponding fix just reveals one more piece of the puzzle to be solved. As I haven't worked with all these JDT internal before starting this adventure I still need to learn many things but I'm already quite satisfied by the result as already much more things got supported that will help developers that previously was more some kind of good-luck and a good test suite with different JDKs. One thing about performance:
So the ultimate goal would always be to keep the amount of code that is MR as possible. |
@stephan-herrmann I now found two more places that needs adjustments in |
It seems it is working way to good now and some test fail because they compile against latest Java API by not using the target option, will add an additional check for that... But this also show that I also think there should be a warning (or even error) if the user don't have enabled |
I now added a testcase (and found a way to break it ...) I'll investigate this as it is a quite tricky case. The main sources uses e.g. java 9 and then in Java 11 a new module was added. But the |
In the end the solution seems quite obvious ... instead of trying to figure out the release after the fact, use a Because of this, I now have made a backup of my branch and will revert / rework some of the parts, the good thing is that on the way there where added some very good test-cases now so that changes will be easier to test and verify. If all works well I plan to squash all changes into a second commit and rebase it on master for a next round of review so one can easily see what changed compared to the first in-deep review. |
d4d881d
to
95b94a9
Compare
@stephan-herrmann with these changes I now was able to get rid of the releaseaware environment for the compilation path entirely so the changes here are now quite minimal. For the reconciler case it is a bit trickier, I'm currently toying around a bit to choose a similar approach for the |
Currently JDT (like other IDEs) lacks good support for compilation of so called multi-release-jar setups. Maven has quite good support for this but often needs some ugly workarounds due to missing support in IDEs. This is an attempt to bring multi-release-compilation to JDT to finally have this feature supported at least in one of the major IDEs and make this powerful feature more widely adopted.
- Use a NameEnvironment per target option - revert ClasspathLocation changes - Filter output location for given release and sort output folders - Set release option for reconciler tests - NameEnvironment must not implement MultiReleaseAware anymore - don't let SearchableEnvironment implement release aware - revert LookupEnvironment changes - Remove IReleaseAwareNameEnvironment
11da262
to
00b4208
Compare
@stephan-herrmann I now have rebased and squashed into two commits (one the previous state and one my rework), this now needs no changes in the compiler part and therefore I deleted the This should now be ready for a next round of review! |
@stephan-herrmann anything you want me to do here before we can proceed? |
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 completed another round of source level review.
I still plan to do some more black box testing, next week, hopefully.
@@ -91,17 +91,19 @@ public class SearchableEnvironment | |||
private long timeSpentInFindTypes; | |||
|
|||
private List<IPackageFragmentRoot> unnamedModulePackageFragmentRoots; | |||
private int release; |
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.
For a moment I was confused: source folders specify a release, so how should "release" be a property of an environment that may refer to multiple source folders with different "release" value.
I guess some doc comment here and/or on the constructor could clarify that this environment is intended for searching sources that are appropriate for the given release, am I correct?
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.
If a source folder is compiled, and it has a release property, then it gets an own SearchableEnvironment for a specific release option, I added some javadoc now.
} | ||
/** | ||
* Creates a SearchableEnvironment on the given project | ||
*/ | ||
public SearchableEnvironment(JavaProject project, org.eclipse.jdt.core.ICompilationUnit[] workingCopies, boolean excludeTestCode) throws JavaModelException { | ||
public SearchableEnvironment(JavaProject project, org.eclipse.jdt.core.ICompilationUnit[] workingCopies, boolean excludeTestCode, int release) throws JavaModelException { |
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.
Most call paths into this ctor don't provide a release. Could you document, which scenarios are covered, which are TODO, and perhaps for some scenarios the release is never relevant? Otherwise it's hard to assess the status of this change.
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.
It is fine to provide NO_RELEASE
here, I mostly did this to retain backward compatibility and only changed the relevant parts. Other parts might benefit from specify a release (or compliance level) but that has to be investigated deeper and would far more complicate this to do it right now.
this.notifier.checkCancel(); | ||
boolean warnAboutMissingReleaseFlag = false; |
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.
should this be called hasWarned...
?
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.
done
for (SourceFile source : entry.getValue()) { | ||
IProject project = source.sourceLocation.sourceFolder.getProject(); | ||
createProblemFor(project, null, | ||
NLS.bind(Messages.AbstractImageBuilder_target_required, |
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.
Several issues:
- why does this message say "target", it looks more like complaining about "release"?
- the message template does not use any of the arguments provided.
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.
Fixed.
} | ||
} | ||
if (!oldRelease && !warnAboutMissingReleaseFlag) { | ||
for (SourceFile source : entry.getValue()) { |
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.
double indent
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.
Fixed.
new Object[] { release, e.getStatus()}), | ||
JavaCore.ERROR); | ||
} | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this com 10000 ment to others. Learn more.
so, when CoreException
was thrown, nothing from this source folder is getting compiled? We should try really hard, to avoid that.
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.
Yes it seems better to have an error marker on one source folder and let the user fix that than break everything in the middle and not compile even more things. If a fatal error occurs (see above) I'm not sue what one can do better than not proceed with compilation?
INameEnvironment environment = this.releaseSpecificEnvironments.get(release); | ||
if (environment == null) { | ||
environment = new NameEnvironment(this.workspaceRoot, this.javaProject, this.binaryLocationsPerProject, | ||
this.notifier, CompilationGroup.MAIN, release); | ||
this.releaseSpecificEnvironments.put(release, environment); | ||
} |
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.
computeIfAbsent()
? :)
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 would happily use that if the thing would not throw CoreException
... and JDK devs has decided that such methods can not throw any checked exceptions :-\
@@ -203,13 +216,25 @@ private void computeClasspathLocations( | |||
} | |||
bLocation.patchModuleName = patchedModuleName; | |||
} else { | |||
String release = Arrays.stream(entry.getExtraAttributes()) |
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.
After seeing several new implementations of this lookup, may I suggest org.eclipse.jdt.internal.core.ClasspathEntry.getExtraAttribute(IClasspathEntry, String)
? :)
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.
As we have org.eclipse.jdt.core.IClasspathEntry.isTest()
I could of course ad something similar for release as well, I just try to keep the API footprint low, just let me know and I'll add something.
mustSortOutput = true; | ||
finalOutputFolder = outputFolder.getFolder(new Path(String.format("META-INF/versions/%s", release))); //$NON-NLS-1$ | ||
} | ||
int sourceTargetRelease = release==null?JavaProject.NO_RELEASE:Integer.parseInt(release); |
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 can't help, but sourceTargetRelease
sounds contradictory to me.
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.
renamed
} | ||
} | ||
"""); | ||
mrproject.setOption("org.eclipse.jdt.core.compiler.release", "enabled"); |
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.
please use constants
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.
Done
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 have now hopefully addressed the concerns!
} | ||
} | ||
"""); | ||
mrproject.setOption("org.eclipse.jdt.core.compiler.release", "enabled"); |
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.
Done
@@ -91,17 +91,19 @@ public class SearchableEnvironment | |||
private long timeSpentInFindTypes; | |||
|
|||
private List<IPackageFragmentRoot> unnamedModulePackageFragmentRoots; | |||
private int release; |
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.
If a source folder is compiled, and it has a release property, then it gets an own SearchableEnvironment for a specific release option, I added some javadoc now.
} | ||
/** | ||
* Creates a SearchableEnvironment on the given project | ||
*/ | ||
public SearchableEnvironment(JavaProject project, org.eclipse.jdt.core.ICompilationUnit[] workingCopies, boolean excludeTestCode) throws JavaModelException { | ||
public SearchableEnvironment(JavaProject project, org.eclipse.jdt.core.ICompilationUnit[] workingCopies, boolean excludeTestCode, int release) throws JavaModelException { |
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.
It is fine to provide NO_RELEASE
here, I mostly did this to retain backward compatibility and only changed the relevant parts. Other parts might benefit from specify a release (or compliance level) but that has to be investigated deeper and would far more complicate this to do it right now.
this.notifier.checkCancel(); | ||
boolean warnAboutMissingReleaseFlag = false; |
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.
done
this.compiler.compile(units); | ||
Map<Integer, List<SourceFile>> collect = Arrays.stream(units) | ||
.collect(Collectors.groupingBy(sf -> sf.sourceLocation.release, TreeMap::new, Collectors.toList())); | ||
for (Entry<Integer, List<SourceFile>> entry : collect.entrySet()) { |
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 tried to use a better name now.
} | ||
if (!oldRelease && !warnAboutMissingReleaseFlag) { | ||
for (SourceFile source : entry.getValue()) { | ||
IProject project = source.sourceLocation.sourceFolder.getProject(); |
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.
Fixed
for (SourceFile source : entry.getValue()) { | ||
IProject project = source.sourceLocation.sourceFolder.getProject(); | ||
createProblemFor(project, null, | ||
NLS.bind(Messages.AbstractImageBuilder_target_required, |
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.
Fixed.
INameEnvironment environment = this.releaseSpecificEnvironments.get(release); | ||
if (environment == null) { | ||
environment = new NameEnvironment(this.workspaceRoot, this.javaProject, this.binaryLocationsPerProject, | ||
this.notifier, CompilationGroup.MAIN, release); | ||
this.releaseSpecificEnvironments.put(release, environment); | ||
} |
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 would happily use that if the thing would not throw CoreException
... and JDK devs has decided that such methods can not throw any checked exceptions :-\
@@ -203,13 +216,25 @@ private void computeClasspathLocations( | |||
} | |||
bLocation.patchModuleName = patchedModuleName; | |||
} else { | |||
String release = Arrays.stream(entry.getExtraAttributes()) |
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.
As we have org.eclipse.jdt.core.IClasspathEntry.isTest()
I could of course ad something similar for release as well, I just try to keep the API footprint low, just let me know and I'll add something.
mustSortOutput = true; | ||
finalOutputFolder = outputFolder.getFolder(new Path(String.format("META-INF/versions/%s", release))); //$NON-NLS-1$ | ||
} | ||
int sourceTargetRelease = release==null?JavaProject.NO_RELEASE:Integer.parseInt(release); |
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.
renamed
Currently JDT (like other IDEs) lacks good support for compilation of so called multi-release-jar setups. Maven has quite good support for this by setting a
${release}
property per source folder and enable an output flag but often needs some ugly workarounds due to missing support in IDEs.This is an attempt to bring multi-release-compilation to JDT to finally have this feature supported at least in one of the major IDEs and make this powerful feature more widely adopted.
In its current form it does the following:
release
attribute in the classpath of a source folder, this can look like this:<outputfolder>/META-INF/versions/<release>
The next step would be to further pass the release option to the compiler itself.