eclipse.platform icon indicating copy to clipboard operation
eclipse.platform copied to clipboard

Support jakarta.annotation version 3.0 in E4 Injector

Open HannesWell opened this issue 1 year ago • 8 comments

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

HannesWell avatar Sep 23 '24 17:09 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.

HannesWell avatar Sep 23 '24 17:09 HannesWell

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.

merks avatar Sep 23 '24 18:09 merks

Test Results

 1 764 files   1 764 suites   1h 28m 29s ⏱️  4 381 tests  4 357 ✅  24 💤 0 ❌ 13 143 runs  12 976 ✅ 167 💤 0 ❌

Results for commit adc7232a.

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

github-actions[bot] avatar Sep 23 '24 18:09 github-actions[bot]

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.

laeubi avatar Sep 24 '24 04:09 laeubi

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.

image

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.

merks avatar Sep 24 '24 05:09 merks

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.

HannesWell avatar Sep 24 '24 23:09 HannesWell

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

laeubi avatar Sep 25 '24 04:09 laeubi

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 0aad9d22212efb7fbd20190288b2495ce8dddb08 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Fri, 6 Jun 2025 09:00:21 +0000
Subject: [PATCH] Version bump(s) for 4.37 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 29b2880e42..472aed925b 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.600.qualifier
+Bundle-Version: 1.9.700.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.49.0

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

eclipse-platform-bot avatar Sep 26 '24 16:09 eclipse-platform-bot