Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate pde.ds to 'org.osgi.service.component.annotations' from Maven-Central #38

Closed
HannesWell opened this issue Apr 22, 2022 · 12 comments

Comments

@HannesWell
Copy link
Member

In eclipse-equinox/equinox#18 o.e.osgi.services is migrated from embedding the sources of the osgi package org.osgi.service.component.annotations to use the osgi-bundles published to Maven-Central.

The OSGi-bundle org.osgi.service.component.annotations published to Maven-Central is designed to not resolve at runtime (see eclipse-equinox/equinox#18). Therefore and because the contained annotations are designed to be used at compile-time only Plug-ins should not import the package (or worse require the containing bundle).
PDE DS offers to add the annotations implicitly to the classpath to make the compilation in the IDE work.
In the past Tycho did not support this. But @laeubi changed this recently for the upcoming Tycho-3: https://github.com/eclipse/tycho/blob/master/RELEASE_NOTES.md#support-for-pde-declarative-component-annotation-progressing

One (or the only) workaround was/is to import the package org.osgi.service.component.annotations in a Plug-in that uses DS. This worked because the package was supplied by o.e.osgi.services. But since o.e.osgi.services is about to be deprecated for removal (see eclipse-equinox/equinox#18) and the part relevant for DS is replaced by org.osgi.service.component.annotations (which does not resolve so we cannot add it at runtime or to a TP) we should make sure that the mentioned package is not imported anymore. Consequently the PDE-DS builder should mark the import of that package as an error encouraging the user to remove it (best with some explanation).
See also eclipse-equinox/equinox#18, eclipse-equinox/equinox#18, eclipse-equinox/equinox#18 and subsequent.

Besides I would like to check if the org.eclipse.pde.ds.annotations and its sub-plug-ins org.eclipse.pde.ds.lib and org.eclipse.pde.ds1_2.lib can be replaced or at least simplify using 'org.osgi.service.component.annotations' from Maven-Central.
As part of this work the annotation package can be updated to the latest versions as suggested in eclipse-equinox/equinox.framework#36.

@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

But @laeubi changed this recently for the upcoming Tycho-3

I backported this to Tycho 2.7.2 as well : eclipse-tycho/tycho#903

Besides I would like to check if the org.eclipse.pde.ds.annotations and its sub-plug-ins org.eclipse.pde.ds.lib and org.eclipse.pde.ds1_2.lib can be replaced or at least simplify using 'org.osgi.service.component.annotations' from Maven-Central.

Even though it is compatible, if one selects 1.2 only 1.2 should be on the classpath to prevent the usage of 'newer' annotations, so it might not be enough to only have the latest available.

As part of this work the annotation package can be updated to the latest versions as suggested in eclipse-equinox/equinox.framework#36.

Just note that this will require also changes in the DS xml editor and in the handling of the annotations (constructor injection for example). So if you like to invest time, it would most probably be better to first have 1.4 support and then try to get rid of some jars that do not harm anything :-)

@HannesWell
Copy link
Member Author

But @laeubi changed this recently for the upcoming Tycho-3

I backported this to Tycho 2.7.2 as well : eclipse/tycho#903

Great!

Besides I would like to check if the org.eclipse.pde.ds.annotations and its sub-plug-ins org.eclipse.pde.ds.lib and org.eclipse.pde.ds1_2.lib can be replaced or at least simplify using 'org.osgi.service.component.annotations' from Maven-Central.

Even though it is compatible, if one selects 1.2 only 1.2 should be on the classpath to prevent the usage of 'newer' annotations, so it might not be enough to only have the latest available.

Yes that's right. I already expected that all versions have to be included. Unless the PDE-DS builder could parse some version annotations. But I think the compiler does a better job in that. And in the end I assume it's easier to ship all versions (a jar with a few annotations is not large) and put the desired one on the classpath for compilation.

As part of this work the annotation package can be updated to the latest versions as suggested in eclipse-equinox/equinox.framework#36.

Just note that this will require also changes in the DS xml editor and in the handling of the annotations (constructor injection for example). So if you like to invest time, it would most probably be better to first have 1.4 support and then try to get rid of some jars that do not harm anything :-)

Looks like I have opened another big box here. 😅
But I will see what I can achieve (but I will finish my other tasks first).

@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

Looks like I have opened another big box here. sweat_smile

Yep, but having DS 1.4 support would be really great and appreciated ;-)

I even think that we can start with supporting the 1.4 annotations first and simply do the editor later, if annotations work smoothly there is not much need for an editor anyways and we maybe just drop it and replace with a simple XML editor...

@vogella
Copy link
Contributor

vogella commented Apr 22, 2022

FWIW, I think we can retire the non textual part of the DS editor and only support a text editor for it. The "advanced" UI used to be very buggy in the past (when I still tried to use it) and have not been improved since.

@HannesWell
Copy link
Member Author

Looks like I have opened another big box here. sweat_smile

Yep, but having DS 1.4 support would be really great and appreciated ;-)

Understand. I'll try to not postpone it too long.

I even think that we can start with supporting the 1.4 annotations first and simply do the editor later, if annotations work smoothly there is not much need for an editor anyways and we maybe just drop it and replace with a simple XML editor...

FWIW, I think we can retire the non textual part of the DS editor and only support a text editor for it. The "advanced" UI used to be very buggy in the past (when I still tried to use it) and have not been improved since.

I have not yet worked much with DS (but since it looks very promising I hope/expect to use it more in the future). However I was surprised an Editor even exists. As you said I thought the xml is supposed to be fully generated by the tooling. So there should not be any need for it, should it?

So I would support the removal.
How could we conduct it? I suspect the Editor is not part of the API? Can we then simply remove it 'now' and announcement it in the new and noteworthy? Should we discuss it on the pde-dev mailing list or even wider?

@laeubi
Copy link
Contributor

laeubi commented Apr 22, 2022

However I was surprised an Editor even exists

There was a time before annotation support in OSGi and you can still create the XML without using annotations (for whatever reasons). Its also sometimes much nicer to have a UI present things in a structured way that staring at an XML document, especially if your not that familiar with it.

How could we conduct it? I suspect the Editor is not part of the API? Can we then simply remove it 'now' and announcement it in the new and noteworthy?

Don't know but the editor already "knows" what to open so it is disabled for anything > 1.3 already

Should we discuss it on the pde-dev mailing list or even wider?

https://github.com/eclipse-pde/eclipse.pde.ui/discussions

@vogella
Copy link
Contributor

vogella commented Apr 22, 2022

So I would support the removal. How could we conduct it? I suspect the Editor is not part of the API? Can we then simply remove it 'now' and announcement it in the new and noteworthy? Should we discuss it on the pde-dev mailing list or even wider?

We should still support the textual editor, if possible migrate its support to the generic editor. I think it would be fine to remove the other tabs and put this into the N&N.

@HannesWell
Copy link
Member Author

When reworking/enhancing the DS support in PDE, I wonder if we should use BND internally to create the service's xml-files.
Then updates like requested in #36 should be simpler.

I have not yet investigated which module of bnd is doing that, but given that m2e uses bnd.lib any ways it is already in the SimRel.

@laeubi
Copy link
Contributor

laeubi commented Jun 17, 2022

You can take a look at the DS support of tycho to get an idea how to use the BND-lib:

https://github.com/eclipse/tycho/blob/master/tycho-ds-plugin/src/main/java/org/eclipse/tycho/ds/DeclarativeServicesMojo.java

but be aware that generating the xml is only half way down, we also want live validation and error markers and...

@HannesWell
Copy link
Member Author

You can take a look at the DS support of tycho to get an idea how to use the BND-lib:

https://github.com/eclipse/tycho/blob/master/tycho-ds-plugin/src/main/java/org/eclipse/tycho/ds/DeclarativeServicesMojo.java

Thanks for the link.

but be aware that generating the xml is only half way down, we also want live validation and error markers and...

Of course but we would at least have saved one part.

@HannesWell
Copy link
Member Author

This is addressed with the following PRs and can be closed once all are merged:

@HannesWell
Copy link
Member Author

This is addressed with the following PRs and can be closed once all are merged:

* [Always provide the official annotations if present in target #269](https://github.com/eclipse-pde/eclipse.pde/pull/269)

* [Remove the embedded annotation ds-libs #276](https://github.com/eclipse-pde/eclipse.pde/pull/276)

All mentioned PRs are merged, thus is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants