8000 Add multi-release compilation support to JDT by laeubi · Pull Request #3900 · eclipse-jdt/eclipse.jdt.core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

laeubi
Copy link
Contributor
@laeubi laeubi commented Apr 5, 2025

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:

  1. One can set a release attribute in the classpath of a source folder, this can look like this:
<?xml version="1.0" encoding="UTF-8"?>
<classpath>
	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11">
		<attributes>
			<attribute name="module" value="true"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="src" path="src"/>
	<classpathentry kind="src" path="src9">
		<attributes>
			<attribute name="release" value="9"/>
		</attributes>
	</classpathentry>
	<classpathentry kind="output" path="bin"/>
</classpath>
  1. if such attribute is found JDT enables "multi-release-output" as per specification to place the compiled output of this source folder to <outputfolder>/META-INF/versions/<release>

The next step would be to further pass the release option to the compiler itself.

@laeubi laeubi marked this pull request as draft April 5, 2025 09:09
@laeubi
Copy link
Contributor Author
laeubi commented Apr 6, 2025

I now also passed this down to the compiler, inspecting the generated class files shows that the target is correctly applied, next step would be to add a testcase that checks this.

@laeubi laeubi force-pushed the add_multi_release_support branch from 9fc7614 to 1acafa7 Compare April 7, 2025 05:24
@laeubi laeubi marked this pull request as ready for review April 7, 2025 05:24
@laeubi
Copy link
Contributor Author
laeubi commented Apr 7, 2025

I now also added a test-case to show the new feature, so I think this is ready for review.

@iloveeclipse
Copy link
Member

Do you plan to add UI support for this?

@laeubi
Copy link
Contributor Author
laeubi commented Apr 7, 2025

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

@laeubi laeubi force-pushed the add_multi_release_support branch 2 times, most recently from ed4fefe to a310892 Compare April 7, 2025 09:48
@laeubi
Copy link
Contributor Author
laeubi commented Apr 7, 2025

@iloveeclipse tests are all green now can you review/merge?

Copy link
Member
@iloveeclipse iloveeclipse left a 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 class ForRelease9, 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.

8000
@iloveeclipse
Copy link
Member

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.

@laeubi
Copy link
Contributor Author
laeubi commented Apr 7, 2025

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.

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.

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.

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.

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.

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.

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?

A "release" folder might contain more classes / methods, you just can't call any code here from the "default"
If we can add some tooling support on this that would be great, but this is something I would propose as a follow up as it is not that trivial.

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.

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 class ForRelease9, it shouldn't be resolvable from "SomeInnocentClassForRelease8"?

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 ForRelease11 class what will be able to see ForRelase9 class

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.

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.

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?

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.

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?

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.

What is about indexer? Which version do we see on index? Both? Only one?

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.

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?

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.

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.

I would suspect this to be implemented as part of the regular MR-support in JDT.

@laeubi
Copy link
Contributor Author
laeubi commented May 4, 2025

@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.

@iloveeclipse
Copy link
Member

@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?

@stephan-herrmann
Copy link
Contributor

I pulled the change and played with it resulting in this test project:
MR24.zip

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:

  • a clean build tells me that src9/p/B.java cannot see class A and src24/p/B.java can see neither class A nor C
  • editing src9/p/B.java triggers an incremental build that let's all errors vanish
  • even when the Problems view is clean, the editor for D.java complains "The method m() is undefined for the type B"
  • saving D.java makes that last message appear in the problems view, too.
  • in the editor of D.java select B and press F3: navigation takes as to src/p/B.java, which is wrong.

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?

  • if yes please explain
  • otherwise we need to go back to the drawing board

@laeubi
Copy link
Contributor Author
laeubi commented May 7, 2025

@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).

@laeubi laeubi force-pushed the add_multi_release_support branch from 4bc1b79 to 7912455 Compare May 7, 2025 09:44
@laeubi
Copy link
Contributor Author
laeubi commented May 7, 2025

@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 B introduces the risk of breaking the contract of Multirelease jar).

I could need some hints on how it is supposed to work with crossreferences between different source folders, as I can see A is compiled and present in the output folder, something still prevents it from being found when compiling src9 + src24, so any place where I should specifically look at or put a break point to?

@stephan-herrmann
Copy link
Contributor

@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 B introduces the risk of breaking the contract of Multirelease jar).

I could need some hints on how it is supposed to work with crossreferences between different source folders, as I can see A is compiled and present in the output folder, something still prevents it from being found when compiling src9 + src24, so any place where I should specifically look at or put a break point to?

Which classes are found is initially determined by the NameEnvironment, see, e.g., AbstractImageBuilder.nameEnvironment. During compilation, the Compiler stores the internal representations (ast and/or bindings) in its LookupEnvironment, which is alive as long as the Compiler is alive. This implies that you cannot use the same Compiler instance for source folders with different visibility scopes.

For an example how these issues can be resolved see how each AbstractImageBuilder is created for a CompilationGroup (MAIN or TEST). See in particular that JavaBuilder has two distinct name environments: nameEnvironment and testNameEnvironment.

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?

@laeubi
Copy link
Contributor Author
laeubi commented May 9, 2025

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.

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?

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.

@laeubi
Copy link
Contributor Author
laeubi commented May 9, 2025

I now debugged it a bit and the cause for this is that compile call in the end LookupEnvironment.reset() so when the second call to compile is performed all previous know types are lost. I tried to inject these into the NameEnvironment after the compile as "extra" units but it seems this is not sufficient (or I did it wrong), so if one would be able to inject a "backup" if a type is not found from the previous round this would bring us one step forward...

@laeubi laeubi force-pushed the add_multi_release_support branch from 7912455 to 67933b8 Compare May 9, 2025 15:25
@laeubi
Copy link
Contributor Author
laeubi commented May 9, 2025

@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!

@laeubi
Copy link
Contributor Author
laeubi commented Jun 24, 2025

Here is

I'm looking into it now ...

make a whitespace change to trigger incremental build, to get the project error-free

I think here it is where thing go wrong first, this should give an error always like after project clean as the Type D is not accessible at the default project code.

Run with Java 9

So this is kind of expected, see above, will need a new testcase for this.

Run with Java 17

This seems a bit expected here as the project is not using org.eclipse.jdt.core.compiler.release=enabled, so it is the same behavior I get with a plain JDT project when using a Java 24 VM and disable that option. What makes me think one probably should force org.eclipse.jdt.core.compiler.release=enabled for all multi-release enabled folders (and most likely one wants to use that for the main project sources as well but that's of course the choice of the user).

Run with Java 24

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 null).

@stephan-herrmann
Copy link
Contributor

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.

Do you think PDE API tools could bring the desired functionality here?

@laeubi
Copy link
Contributor Author
laeubi commented Jun 24, 2025

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 AbstractImageBuilder .. the spec is quite open here:

The run-time system will not verify this property, but tooling can and should detect such API compatibility issues, and a library method may also be provided to perform such varification (for example on java.util.jar.JarFile)

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 jar tool or whatever packages the final jar file (e.g. Tycho) ...

@laeubi
Copy link
Contributor Author
laeubi commented Jun 24, 2025

I now added a testcase for the case where the project settings are disabled the target option but for some cases changes in this are not taken into account. If I change the project settings to enable it before hands, this is actually working and I get the desired error. Looking through references of CompilerOptions.target only gives references in get/setMap and resetDefaults, so it seems I'm probably looking at the wrong item here... or something is cached before I can change it :-\

@stephan-herrmann
Copy link
Contributor

I now added a testcase for the case where the project settings are disabled the target option but for some cases changes in this are not taken into account. If I change the project settings to enable it before hands, this is actually working and I get the desired error. Looking through references of CompilerOptions.target only gives references in get/setMap and resetDefaults, so it seems I'm probably looking at the wrong item here... or something is cached before I can change it :-\

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?

@laeubi
Copy link
Contributor Author
laeubi commented Jun 25, 2025

@stephan-herrmann I only found that ClasspathMultiReleaseJar uses the compliance option and though it might be evaluated during compilation but that's not the case instead it is determined when the classpath is first build so it does not change anything. Same for the target option and ClasspathJrtWithReleaseOption ... so long story short, I need to make ClasspathLocation itself release aware. What probably is a good thing because it is needed anyways and maybe even help in make one of the filtering obsolete. It might even help with redhat-developer/vscode-java#3319 on the long run and might even contribute to #3168 later on.

@laeubi
Copy link
Contributor Author
laeubi commented Jun 25, 2025

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));
Copy link
Contributor

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.

Copy link
Contributor Author
@laeubi laeubi Jun 25, 2025

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.

Copy link
Contributor Author

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.

@stephan-herrmann
Copy link
Contributor

@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.

  • when building a given project we initialize all classpath locations so that lookup can rely on optimal cached data
    • during this process a ClasspathJrtWithReleaseOption is initialized for one specific release version
  • now that we agree that different release folders need different release views of the JDK, this boils down to separate instances of ClasspathJrtWithReleaseOption

I don't see how those multiple instances of ClasspathJrtWithReleaseOption could be harmonized with the overall builder design.

We do have a precedent for multiple builder invocations with different configurations (see testImageBuilder in JavaBuilder.buildAll()). We do not have a precedent for changing the configuration during one builder invocation. Maybe there's a reason for that?

@laeubi
Copy link
Contributor Author
laeubi commented Jun 25, 2025

@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:
I have tried to my best to keep the impact for the common case (no multi-release) negligible for the price of having maybe a little less performance in the case of Multi-Release compile because this is a very rare case and if you do it correctness is more important than performance, don't expect people are writing large parts of their code in a multi-release way that's simply not something you want to do, I think the spec explain it quite good:

Library and framework maintainers can gradually migrate to and support new features while still carrying around support for the old features

So the ultimate goal would always be to keep the amount of code that is MR as possible.

@laeubi
Copy link
Contributor Author
laeubi commented Jun 26, 2025

@stephan-herrmann I now found two more places that needs adjustments in LookupEnvironment when the project itself uses a modular JVM version ( > 1.8 ) so this one works now as well (I need to add a testcase for this).

@laeubi
Copy link
Contributor Author
laeubi commented Jun 26, 2025

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 ClasspathJrtWithReleaseOption is likely not needed anymore as with this enhancement it is just working without any specialization...

I also think there should be a warning (or even error) if the user don't have enabled --release for their main project sources as the risk to getting things wrong is much to high as it requires a higher JVM than the compliance settings to be used and then one can easily use newer API form the JDK (what should be avoided).

@laeubi
Copy link
Contributor Author
laeubi commented Jun 27, 2025

when the project itself uses a modular JVM version ( > 1.8 ) so this one works now as well (I need to add a testcase for this).

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 NameEnvironment is caching the module names from the Release 9 so it later can not find the type form the Release 11 module.

@laeubi
Copy link
Contributor Author
laeubi commented Jun 28, 2025

In the end the solution seems quite obvious ... instead of trying to figure out the release after the fact, use a NameEnvironment per release and let it carry the target release itself. This seems like a middle ground between what @stephan-herrmann suggested to use a complete own compilation groups (MAIN vs TEST) and what I used here initially. Unfortunately some of the interesting work I done here to allow lookups based on a release parameter are now likely not needed anymore (for this feature) and can be handled much more straight forward up in the chain and even with less influence on the standard compilation case.

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.

@laeubi laeubi force-pushed the add_multi_release_support branch 5 times, most recently from d4d881d to 95b94a9 Compare June 29, 2025 03:32
@laeubi
Copy link
Contributor Author
laeubi commented Jun 29, 2025

@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 SearchableEnvironment ...

laeubi added 2 commits June 29, 2025 07:33
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
@laeubi laeubi force-pushed the add_multi_release_support branch from 11da262 to 00b4208 Compare June 29, 2025 05:34
@laeubi laeubi requested a review from stephan-herrmann June 29, 2025 05:34
@laeubi
Copy link
Contributor Author
laeubi commented Jun 29, 2025

@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 IReleaseAwareNameEnvironment entirely.

This should now be ready for a next round of review!

@laeubi
Copy link
Contributor Author
laeubi commented Jul 9, 2025

@stephan-herrmann anything you want me to do here before we can proceed?

Copy link
Contributor
@stephan-herrmann stephan-herrmann left a 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;
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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...?

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double indent

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Comment on lines +729 to +734
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeIfAbsent() ? :)

Copy link
Contributor Author

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())
Copy link
Contributor

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)? :)

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use constants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author
@laeubi laeubi left a 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");
Copy link
Contributor Author

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;
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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;
Copy link
Contributor Author

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()) {
Copy link
Contributor Author

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();
Copy link
Contributor Author

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines +729 to +734
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);
}
Copy link
Contributor Author

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())
Copy link
Contributor Author

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

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

Successfully merging this pull request may close these issues.

4 participants
0