Support jakarta.inject / jakarta.annotation in E4 as an alternative

[laeubi]

The javax.annotation / javax.inject are effectively deprecated as they are now supersede by the jakarta.inject / jakarta.annotation 2.0 API.

This adds support for E4 to optionally process these annotations as well so users can migrate their code away from the old namespace.

See eclipse-platform/eclipse.platform.ui#1129

Lets see if this breaks anything....

[github-actions]

Test Results

       39 files   -        3         39 suites   - 3   51m 41s ⏱️ - 4m 40s
  3 778 tests ±       0    3 775 ✔️ ±       0    3 💤 ±0  0 ❌ ±0 
10 092 runs   - 1 245  10 066 ✔️  - 1 244  26 💤  - 1  0 ❌ ±0 

Results for commit 6e4c6df. ± Comparison against base commit 229beea.

♻️ This comment has been updated with latest results.

[HannesWell]

First of all, please split this into two commits, where the first adds support for jakarta annotations and the second one migrates existing code.

Regarding the implementation: With the current approach the javax and jakarta API bundles are mandatory in the runtime as long as both are supported. This is bad for scenarios where only the jakarta or javax annotations are used. The injector should not mandate the precense of any, this requirement should come from the consuming bundles.
Therfore the e4.core.di bundle should import the annotation packages only optionally and should be prepared that the annotation-classes cannot be loaded.
To achieve this in an elegant way I thought about creating something like an AnnotationProcessor interface that has methods like isPresent(AnnotatedElement) (and more if necessary).
We can then have multiple implementations as inner static classes that each handles one Annotation class.
During the initialization of the InjectorImpl class it should be attempted to create an instance of each AnnotationProcessor implementation and collect them in lists grouped by type:

List<AnnotationProcessor> injectProcessors = ...
List<AnnotationProcessor> namedProcessors = ...

The important part is to wrap each constructor call in a try-catch-block where a potential NoClassDefFoundError is catched so that the corresponding processor can be ignored if the annotation class is not available.

This would not mandate the javax and jakarta annotations in the runtime, but at the same time would avoid the need for reflection. At build time of course both still would be necessary.

Alternatively it could be handled reflectively by just calling Class.forName() with the annotation's class string. But IMHO this would be less elegant and would not require the annotation classes to be present at compile-time and therefore would not require the Imports in the Manifest at compile-time.

[laeubi]

With the current approach the javax and jakarta API bundles are mandatory in the runtime as long as both are supported.

This is intentional.

This is bad for scenarios where only the jakarta or javax annotations are used.

Why is it bad? Consumers can still only use one and that the goal here, to allow for consumers to choose not to save a single standalone bundle of a few bytes (aka javax.inject).

The injector should not mandate the precense of any, this requirement should come from the consuming bundles.

This will greatly complicate the things starting with hard to read/debug code and ending in possible class-space inconsistencies and is most likely a variant of YAGNI (de)

So I won't invest any time in regards to making anything optional here because its unlikely to be ever needed and is not a goal of the implementation. Apache Felix for example supports "old" servlet API and Jakarta in parallel for a time and decides to drop old from a specific release, Platform might (unlikely) later decide to do the same in an unforeseeable future.

[akurtakov]

I agree both javax and jakarta implementations should be mandatory so extenders can be sure that whatever they use will work proper.
Decision to drop javax could be made when nothing in the SDK/tests/examples uses/relies on javax* with the standard deprecation notice/warning of 2 years.

[laeubi]

The compilation errors are currently cause by the following Tycho bug:

• Fix DependencyComputer missing bundles when they have different versions eclipse-tycho/tycho#2841

[akurtakov]

@laeubi What is the status here?

[laeubi]

I plan to release Tycho 4 bugfix end of month and then we can proceed here, if I have not missed something I then have converted all platform code to jakarta and both annotation-types should be supported.

[HannesWell]

I agree both javax and jakarta implementations should be mandatory so extenders can be sure that whatever they use will work proper.

Both of course should work properly, but at least those that have migrated everything to Jakarta should be awarded to not need the javax jar anymore.
This also allows to find 'hidden' dependencies (e.g. hard coded Strings for Class.forName or similar) early and not only when support for javax is dropped one day.

[HannesWell]

if I have not missed something I then have converted all platform code to jakarta and both annotation-types should be supported.

Could you please split the migartion in a second commit?

[laeubi]

@HannesWell I would prefer to keep this in one commit as this is strongly related.
@akurtakov the build now succeeds except one test failure but this looks like a false positive as the github checks all succeed.

[HannesWell]

Sorry I missed the notification, I'll review tomorrow.

I agree both javax and jakarta implementations should be mandatory so extenders can be sure that whatever they use will work proper.

Both of course should work properly, but at least those that have migrated everything to Jakarta should be awarded to not need the javax jar anymore. This also allows to find 'hidden' dependencies (e.g. hard coded Strings for Class.forName or similar) early and not only when support for javax is dropped one day.

Does anybody have a remark on this?

[laeubi]

Does anybody have a remark on this?

The remark is that making things more complex, hard to maintain and error prone just for not having an (unused) jar in a product does not really outweigh the disadvantages.

[akurtakov]

Any solution now must ensure that both javax and jakarta are available now in the RCP for the transition perion. Ability to remove one or the other if someone explicitly wants that is fine if someone invest the time for it.

[laeubi]

Any solution now must ensure that both javax and jakarta are available now in the RCP for the transition perion. Ability to remove one or the other if someone explicitly wants that is fine if someone invest the time for it.

That's why I suggest to go for the current solution and not invest time into something that maybe never will be needed. If it will become important once in the future one can add on this later on.

So please let me know if it can be consider in this (type safe and classlaoder safe) form as otherwise I would like to close / drop this to keep my task list clean, this is defiantly not a priority for me and was more to empower people to start the transition ...

[akurtakov]

"Perfect is the enemy of good"! Please go with this one so migration could happen elsewhere too. Follow-up work and improvements could be taken at any time (if/when someone finds time) but shouldn't delay this first step.

Requested Feature not desired by others.

[laeubi]

Then lets see what happens!

[akurtakov]

@HannesWell I have to clarify - it's not that the feature is not desired or nice to have. It's just that it is not mandatory for step 1.

[vogella]

Please add to N&N

[mickaelistria]

Looks like those test failures https://download.eclipse.org/eclipse/downloads/drops4/I20231011-1110/compilelogs/platform.doc.isv.javadoc.txt are related. Can someone please investigate (not that we need an immediate fix, but at least evaluate whether this change is the root cause)?

[laeubi]

@mickaelistria I think this needs an extra requirement now for jakarta...

[mickaelistria]

OK, that makes sense. Can you please take care of it?

[laeubi]

I have created

• Add jakarta.inject-api / jakarta.annotation-api to isv build eclipse.platform.releng.aggregator#1436

[HannesWell]

Please add to N&N

Besides that I thinking we should deprecate the javax-annotstions Support for removal now in order to be able to remove it in two/three years.

[akurtakov]

Please add to N&N

Besides that I thinking we should deprecate the javax-annotstions Support for removal now in order to be able to remove it in two/three years.

Yeah, that's a must if we want to ever drop support for it.

[HannesWell]

In order to make the javax-annotations optional, I have created #792 as a follow-up.