eclipse-platform / eclipse.platform

https://eclipse.dev/eclipse/
Eclipse Public License 2.0
84 stars 113 forks source link

Add IContextFunction.ServiceContextKey OSGi component property type #1577

Closed HannesWell closed 1 month ago

HannesWell commented 1 month ago

This annotation simplifies the specification of the 'service.context.key' service property for IContextFunction implementations and makes it type-safe and more robust:

@Component(service = IContextFunction.class)
@IContextFunction.ServiceContextKey(IProgressService.class)
public class ProgressServiceCreationFunction extends ContextFunction {
...
github-actions[bot] commented 1 month ago

Test Results

 1 758 files  ±0   1 758 suites  ±0   1h 29m 23s :stopwatch: - 3m 54s  4 170 tests ±0   4 148 :white_check_mark: ±0   22 :zzz: ±0  0 :x: ±0  13 107 runs  ±0  12 943 :white_check_mark: ±0  164 :zzz: ±0  0 :x: ±0 

Results for commit 12af7b71. ± Comparison against base commit 7fa51b7a.

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

HannesWell commented 1 month ago

@laeubi can you tell why this build has compilation errors and how to fix them?

The existing AdapterTypes, that is the other ComponentPropertyType in the SDK, also seems to compile without any special setup: https://github.com/eclipse-equinox/equinox/blob/19b406bea5b340cee4009cd9f2c358f23b3840da/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/AdapterTypes.java#L44

laeubi commented 1 month ago

The existing AdapterTypes, that is the other ComponentPropertyType in the SDK, also seems to compile without any special setup:

Maybe because it is a top-level class 😛

Beside from that I would assume that enable DS Annotations for the project could solve the issue.

HannesWell commented 1 month ago

The existing AdapterTypes, that is the other ComponentPropertyType in the SDK, also seems to compile without any special setup:

Maybe because it is a top-level class 😛

I expected that answer :P And therefore checked Tycho's DeclarativeServicesClasspathContributor, where I didn't see any sign that the class is scanned at all. Furthermore I assumed that one first has to compile the class to be able to scan it for annotations, unless one operates only on the AST.

Beside from that I would assume that enable DS Annotations for the project could solve the issue.

I thought about this as well, but at least in org.eclipse.equinox.common I didn't find any sign of such configuration, neither in the project files nor in a parent pom. And actually the project does not contain an DS components, it just needs the classpath contribution.

laeubi commented 1 month ago

I thought about this as well, but at least in org.eclipse.equinox.common I didn't find any sign of such configuration, neither in the project files nor in a parent pom. And actually the project does not contain an DS components, it just needs the classpath contribution.

It might pull it in by a transitive dependency (that uses DS annotations), and because DeclarativeServicesClasspathContributor actually looks for such configuration at the moment.

HannesWell commented 1 month ago

I thought about this as well, but at least in org.eclipse.equinox.common I didn't find any sign of such configuration, neither in the project files nor in a parent pom. And actually the project does not contain an DS components, it just needs the classpath contribution.

It might pull it in by a transitive dependency (that uses DS annotations), and because DeclarativeServicesClasspathContributor actually looks for such configuration at the moment.

I still have not found out why it works in o.e.equinox.common, but adding the pde.ds preference file indeed fixes the build failure. 🎉 Thanks for the hints.

Therefore this is now ready for submission. Since we have a 2:1 result pro inner-class in above's discussion I plan to submit this as it is this evening unless somebody has strong new arguments. We can the finally complete https://github.com/eclipse-platform/eclipse.platform.ui/pull/2344.