Support jakarta.annotation version 3.0 in E4 Injector #1566

HannesWell · 2024-09-23T17:23:12Z

HannesWell · 2024-09-23T17:25:37Z

@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 · 2024-09-23T18:02:40Z

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 · 2024-09-23T18:25:11Z

Test Results

1 758 files ±0 1 758 suites ±0 1h 31m 55s ⏱️ - 8m 56s
4 170 tests ±0 4 147 ✅ ±0 22 💤 ±0 0 ❌ ±0 1 🔥 ±0
13 107 runs ±0 12 940 ✅ ±0 164 💤 ±0 0 ❌ ±0 3 🔥 ±0

For more details on these errors, see this check.

Results for commit 306174f. ± Comparison against base commit 1524ff1.

♻️ This comment has been updated with latest results.

laeubi · 2024-09-24T04:40:25Z

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 · 2024-09-24T05:46:43Z

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.

jukzi

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

MESSAGE bundle org.eclipse.pde.ds.tck:1.0.0.qualifier (397) Component descriptor entry 'OSGI-INF/testServiceSingleton.xml' not found
WARNING: Annotation classes from the 'javax.inject' or 'javax.annotation' package found.
It is recommended to migrate to the corresponding replacements in the jakarta namespace.
The Eclipse E4 Platform will remove support for those javax-annotations in a future release.
To suppress this warning, set the VM property: -Declipse.e4.inject.javax.warning=false
To disable processing of 'javax' annotations entirely, set the VM property: -Declipse.e4.inject.javax.disabled=true


!ENTRY org.eclipse.e4.core.di 2 0 2024-09-24 09:19:11.740
!MESSAGE Possbible annotation mismatch: method "void org.eclipse.e4.ui.internal.workbench.ResourceHandler.init()" annotated with "jakarta.annotation-api:2.1.1:jakarta.annotation.PostConstruct" but was looking for "jakarta.annotation-api:3.0.0:jakarta.annotation.PostConstruct
 or jakarta.annotation-api:1.3.5:javax.annotation.PostConstruct"

!ENTRY org.eclipse.e4.ui.workbench 4 0 2024-09-24 09:19:11.744
!MESSAGE Unable to load resource platform:/plugin/org.eclipse.platform/LegacyIDE.e4xmi
!STACK 0
java.lang.NullPointerException: Cannot invoke "org.eclipse.emf.ecore.resource.ResourceSet.getResource(org.eclipse.emf.common.util.URI, boolean)" because "this.resourceSet" is null
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.getResource(ResourceHandler.java:287)
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.loadResource(ResourceHandler.java:263)
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.loadMostRecentModel(ResourceHandler.java:178)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.loadApplicationModel(E4Application.java:368)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.createE4Workbench(E4Application.java:244)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:568)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:58)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1454)

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

HannesWell · 2024-09-24T23:42:55Z

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

Looks like what I have suspected in #1565 (comment).

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 · 2024-09-25T04:34:03Z

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

jukzi · 2024-09-25T06:02:42Z

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

@@ -15,7 +15,7 @@ Export-Package: org.eclipse.e4.core.di;version="1.7.0",
 org.eclipse.e4.core.internal.di.osgi;x-internal:=true,
 org.eclipse.e4.core.internal.di.shared;x-friends:="org.eclipse.e4.core.contexts,org.eclipse.e4.core.di.extensions.supplier"
 Require-Bundle: org.eclipse.e4.core.di.annotations;bundle-version="[1.4.0,2.0.0)";visibility:=reexport
-Import-Package: jakarta.annotation;version="[2,3)",
+Import-Package: jakarta.annotation;version="[2,4)",


Do we need any import at all? Should be possible to use class names only as String.

Initially I wanted to keep this change minimal, but if this is started it should be done completely.
So in theory yes, but while implementing it I noticed that obtaining the values of jakarta/javax.inject.Named is not trivial because I think we want a more performant solution than using reflection all the time.
I think I'll try it with cached MethodHandles or alike.

I think we want a more performant solution than using reflection all the time

Have you measured the performance impact? Injection frameworks always require reflection and the JVM should be quite good at that so I'm curious if it is really a performance impact (in practical application).

My suspicion would be that in the overall scheme of things the performance here is not relevant (especially base I've debugged all the injection stuff that happens a framework like Xtext that's using guice).

eclipse-platform-bot · 2024-09-26T16:29:38Z

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 <[email protected]>
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.

stale

jukzi · 2024-10-02T08:41:32Z

...ime/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/AnnotationLookup.java

-	public static record AnnotationProxy(List<Class<? extends Annotation>> classes) {
-		public AnnotationProxy {
-			classes = List.copyOf(classes);
+	static record AnnotationProxy(List<Class<? extends Annotation>> classes, List<String> classNames) {


Has to be public to make org.eclipse.e4.core.internal.services.MessageFactoryServiceImpl.processPostConstruct(Object, Class<?>) compile.

i would like to see this PR merged soon. Even if it may not be the final solution it still helps to solve the current situation

Yes.

Even if it may not be the final solution it still helps to solve the current situation

The problem with the first approach was that it only worked in multiple versions for some kind of annotations and not for all kind of annotations processed by the injector. And while partial progress is certainly better than no progress inconsistent behavior can be really bad.

But I think I have found a good approach now that works with all kind of annotation and I hope to complete it within the next days.

jukzi · 2024-10-03T06:48:08Z

...ime/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/AnnotationLookup.java

-		return classes;
+	private static final Map<Class<? extends Annotation>, Function<Annotation, String>> NAMED_ANNOTATION2VALUE_GETTER2 = new ConcurrentHashMap<>();
+	private static final Set<String> NAMED_ANNOTATION_CLASSES = Set.of("jakarta.inject.Named", "javax.inject.Named"); //$NON-NLS-1$//$NON-NLS-2$
+	// TODO: warn about the javax-class?


remove TODO

jukzi · 2024-10-03T06:48:43Z

...ime/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/AnnotationLookup.java

+	// TODO: warn about the javax-class?
+
+	private static List<String> getAvailableClasses(String jakartaClass, String javaxClass) {
+		return javaxClass != null && canLoadJavaxClass(javaxClass) //


why is it needed to check if javax is loadable?

This methods checks if the javax-classes are even available in the application in any version and if it's therefore worth to check for them. And, maybe even more important, it emits the warning about javax annotations being 'deprecated'.

But in the past it was asked to improve the implementation because the message is to coarse. So this can definitively improved. OTOH, if support for javax annotation does not require their classes in the build-system we can probably support it much easier and longer and it could be consider to un-deprecate their support.

If it can be supported by reflection for very little ongoing maintenance overhead, that seems like a good thing for the consumers.

The most work would be testing it. Currently we have the test-suite duplicated once for javax and once for jakarta.
But if we just use the class-names permanently to support e.g. multiple versions of a class eventually, the injector would even be simpler without all the logic for the deprecation warnings etc.

The main question is probably for how long we want to explicitly test javax annotations.
At the same time it might not even be necessary to explicitly test with all supported class names, as long as we make sure that the injector just cares about class-names and can handle multiple Class-objects with the same name.

and specifically support jakarta.annotation version 3.0. Fixes eclipse-platform#1565

HannesWell mentioned this pull request Sep 23, 2024

@PostConstruct sometimes not working when using higher jakarta inject version #1565

Open

HannesWell force-pushed the fix-1565 branch from 2f91a24 to ac33327 Compare September 23, 2024 17:54

jukzi previously requested changes Sep 24, 2024

View reviewed changes

HannesWell force-pushed the fix-1565 branch from ac33327 to 8783197 Compare September 24, 2024 23:38

jukzi reviewed Sep 25, 2024

View reviewed changes

HannesWell force-pushed the fix-1565 branch from 8783197 to b65cc75 Compare September 26, 2024 16:21

HannesWell mentioned this pull request Sep 26, 2024

Set up workflow for automated version increments in pull-requests #1568

Merged

jukzi reviewed Oct 2, 2024

View reviewed changes

HannesWell force-pushed the fix-1565 branch 2 times, most recently from b4d65c0 to 3811491 Compare October 2, 2024 22:21

jukzi reviewed Oct 3, 2024

View reviewed changes

HannesWell marked this pull request as draft October 3, 2024 09:34

Support multiple versions of annotation classes in E4 Injector

995deaf

and specifically support jakarta.annotation version 3.0. Fixes eclipse-platform#1565

HannesWell force-pushed the fix-1565 branch from 0c71052 to 995deaf Compare October 4, 2024 21:55

Version bump(s) for 4.34 stream

306174f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support jakarta.annotation version 3.0 in E4 Injector #1566

Support jakarta.annotation version 3.0 in E4 Injector #1566

HannesWell commented Sep 23, 2024

HannesWell commented Sep 23, 2024

merks commented Sep 23, 2024

github-actions bot commented Sep 23, 2024 •

edited

Loading

laeubi commented Sep 24, 2024

merks commented Sep 24, 2024

jukzi left a comment

HannesWell commented Sep 24, 2024

laeubi commented Sep 25, 2024

jukzi Sep 25, 2024

HannesWell Sep 30, 2024

laeubi Oct 1, 2024

merks Oct 1, 2024

eclipse-platform-bot commented Sep 26, 2024 •

edited

Loading

jukzi Oct 2, 2024

HannesWell Oct 2, 2024

jukzi Oct 3, 2024

jukzi Oct 3, 2024

HannesWell Oct 3, 2024

merks Oct 3, 2024

HannesWell Oct 3, 2024

Support jakarta.annotation version 3.0 in E4 Injector #1566

Are you sure you want to change the base?

Support jakarta.annotation version 3.0 in E4 Injector #1566

Conversation

HannesWell commented Sep 23, 2024

HannesWell commented Sep 23, 2024

merks commented Sep 23, 2024

github-actions bot commented Sep 23, 2024 • edited Loading

Test Results

laeubi commented Sep 24, 2024

merks commented Sep 24, 2024

jukzi left a comment

Choose a reason for hiding this comment

HannesWell commented Sep 24, 2024

laeubi commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclipse-platform-bot commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2024 •

edited

Loading

eclipse-platform-bot commented Sep 26, 2024 •

edited

Loading