Support jakarta.annotation version 3.0 in E4 Injector

[HannesWell]

Fixes https://github.com/eclipse-platform/eclipse.platform/issues/1565

[HannesWell]

@merks is there a reason to not update to jakarta.annotation version 3 respectively to provide both to allow easy migration?

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/e912d230f7dc8715d5616232236aa60c5bab7898/eclipse.platform.releng.prereqs.sdk/eclipse-sdk-prereqs.target#L805-L810

Of course an application ideally only uses one version and I don't know what happens if two versions are available respectivly if the E4 injector really works then.

[merks]

Supporting a wider range is good. I don’t see why both 2.x and 3.x need to be in the tp. Some other 3rd party dependency might need the 2.x. I’ll have to check with the aggregation analyzer.

[github-actions[bot]]

Test Results

 1 758 files  ±0   1 758 suites  ±0   1h 31m 55s :stopwatch: - 8m 56s  4 170 tests ±0   4 147 :white_check_mark: ±0   22 :zzz: ±0  0 :x: ±0  1 :fire: ±0  13 107 runs  ±0  12 940 :white_check_mark: ±0  164 :zzz: ±0  0 :x: ±0  3 :fire: ±0 

For more details on these errors, see this check.

Results for commit 306174fc. ± Comparison against base commit 1524ff16.

:recycle: This comment has been updated with latest results.

[laeubi]

Of course an application ideally only uses one version and I don't know what happens if two versions are available respectivly if the E4 injector really works then.

Having multiple versions is supported by OSGi and the OSGi resolver should take care of it but the injector will only be able to handle "compatible" bundles then so all have to use 3.x or 2.x you can't mix them (in the current form). So if currently a "client" uses a narrow version range but the injector is bound to a higher range it wont work.

Eclipse Sisu semm to use a quite interesting aproach where they just look at the annotation class name and accept everything that ends with Inject e.g. com.google.Inject, javax.annotation.Inject and jakarta.annotation.Inject can be used together in any version.

[merks]

There are a lot of upper bounds that exclude 3.0.0 which is to be expected because goodness knows what 3.0.0 breaks, which is nothing for this use case, but one doesn't know that before there is a 3.0.0.

So I expect the downstream ecosystem will be in a similar state. Certainly better if only the package name and the package version specified for it via its exporting bundle mattered, regardless of the bundle symbolic name and bundle version.

[HannesWell]

This does not fix DependencyInjectionViewTest for me locally. Instead it crashes with:

Looks like what I have suspected in https://github.com/eclipse-platform/eclipse.platform/issues/1565#issuecomment-2368924990.

instead of having the Annotations as dependency for using java.lang.reflect.AnnotatedElement.isAnnotationPresent(Class<? extends Annotation>) we could simply check the name like: Arrays.stream(element.getDeclaredAnnotations()).anyMatch(a -> a.annotationType().getName().equals(annotationClass.getName())) in org.eclipse.e4.core.internal.di.AnnotationLookup.AnnotationProxy.isPresent(AnnotatedElement) - which solves the test for me

Yes, almost. We just have to use getAnnotations() instead of getDeclaredAnnotations(). The method AnnotatedElement.isAnnotationPresent(Class<? extends Annotation>) also operates on the former.

I just pushed an update.

Eclipse Sisu semm to use a quite interesting aproach where they just look at the annotation class name and accept everything that ends with Inject e.g. com.google.Inject, javax.annotation.Inject and jakarta.annotation.Inject can be used together in any version.

I have to say that I wouldn't be comfortable with that since then even more false positives are possible. This might be only hypothetical, but on the other hand it wouldn't be too difficult to support more annotations if really desired.

There are a lot of upper bounds that exclude 3.0.0 which is to be expected because goodness knows what 3.0.0 breaks, which is nothing for this use case, but one doesn't know that before there is a 3.0.0.

Thanks for the analysis. That's what I have expected and I'm actually happy that proper version-ranges are used, even if this makes migration in this case a bit harder than it would have been without.

[laeubi]

I have to say that I wouldn't be comfortable with that since then even more false positives are possible. This might be only hypothetical, but on the other hand it wouldn't be too difficult to support more annotations if really desired.

What means "even more"? Currently we have not nay false positive at all (only false negatives).

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle. Therefore the following files need a version increment:

runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

``` From 4b8c95c8d50d16b0767d4e6ced4972aa35d860cb Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Fri, 4 Oct 2024 21:59:56 +0000 Subject: [PATCH] Version bump(s) for 4.34 stream diff --git a/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF b/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF index a084435dfd..29b2880e42 100644 --- a/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF +++ b/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF @@ -1,7 +1,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-SymbolicName: org.eclipse.e4.core.di -Bundle-Version: 1.9.500.qualifier +Bundle-Version: 1.9.600.qualifier Bundle-Name: %pluginName Bundle-Vendor: %providerName Bundle-Localization: plugin -- 2.46.1 ```

Further information are available in Common Build Issues - Missing version increments.