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

Remove the embedded annotation ds-libs #276

Merged
merged 1 commit into from Aug 11, 2022

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Aug 9, 2022

Now we fetch the annotation jars from the target, we can get rid of the embedded items.

Lets see if there are more references ...

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm in favor of this change, I think we have to check if this removal has to go through the regular 2-years deprecation process, although no API is removed.

Correct me if I'm wrong, with this change Eclipse-users have to add the o.o.service.component to their TP if they update their development Eclipse to 2022-09, don't they? The annotation bundle is not installed into Eclipse anymore respectively not accessible, unless one has the pde-feature in the TP?
While this is doable with maven-targets or the component-annotation bundle in the eclipse-update repo, I wonder if we should give users some time in advance? On the other hand, the question would be if one would make the change earlier if it is not yet necessary?
@vogella, @vik-chand what do you think?

That being said you have to remove the removed plug-ins from the pde-feature as well.

A side-note, if I didn't miss anything we provide o.o.service.component-1.5.0 in the eclipse-upates repo, which is pulled in if the pde-feature is in the TP and put on the classpath, but PDE-DS can only process up to 1.3. So it could happen that 1.5. features are used but cannot be processed?

@laeubi
Copy link
Contributor Author

laeubi commented Aug 10, 2022

if this removal has to go through the regular 2-years deprecation process, although no API is removed.

As its not API no API deprecation is required ... otherwise all these "move X from orbit to maven" would have to go through the process as well.

Correct me if I'm wrong, with this change Eclipse-users have to add the o.o.service.component to their TP if they update their development Eclipse to 2022-09, don't they?

Depends on configuration and/or how users are using it, keeping the the pde specific annotation bundles won't change it and as we ship the o.o.service.component it should not be much of a problem.

I wonder if we should give users some time in advance

For what? Actually time has proven that users actually don't change if they don't need to, so not doing it now will simply lead to that people will complain after the grace period that something has changed, if one absolutely needs this it could be consumed from an older update-site as these are retained "forever"

A side-note, if I didn't miss anything we provide o.o.service.component-1.5.0 in the eclipse-upates repo, which is pulled in if the pde-feature is in the TP and put on the classpath, but PDE-DS can only process up to 1.3. So it could happen that 1.5. features are used but cannot be processed?

This is already the case today.

Now we fetch the annotation jars from the target, we can get rid of the
embedded items
@laeubi
Copy link
Contributor Author

laeubi commented Aug 10, 2022

Just the usual API tools failures, so it seems there are no more other references in PDE itself...

@HannesWell
Copy link
Member

HannesWell commented Aug 10, 2022

Correct me if I'm wrong, with this change Eclipse-users have to add the o.o.service.component to their TP if they update their development Eclipse to 2022-09, don't they?

Depends on configuration and/or how users are using it, keeping the the pde specific annotation bundles won't change it and as we ship the o.o.service.component it should not be much of a problem.

I meant for the case where somebody uses a TP without the running-platform that does not contain the annotation-bundle at all. I assumed that at the moment the PDE-DS Plug-in adds the annotation-bundle from the running platform (e.g. like it is for example done with the junit-runtime if none is in the TP and one launches a Plug-in Test. But when looking at the DSAnnotationClasspathContributor (which is deleted with this change) then I don't think that this was ever possible.
If I did not oversaw anything actually nothing changes because the PDE-feature still pulls in a anno-bundle.

I wonder if we should give users some time in advance

For what? Actually time has proven that users actually don't change if they don't need to, so not doing it now will simply lead to that people will complain after the grace period that something has changed, ...

Yes, you are probably right. And I suspected that already, which I tried to indicate with my last sentence in that paragraph. :)

A side-note, if I didn't miss anything we provide o.o.service.component-1.5.0 in the eclipse-upates repo, which is pulled in if the pde-feature is in the TP and put on the classpath, but PDE-DS can only process up to 1.3. So it could happen that 1.5. features are used but cannot be processed?

This is already the case today.

That's right. It was more to discuss if we should downgrade to 1.3 until 1.4 and 1.5 are supported. I don't know if that happens in time for the september release.

@HannesWell
Copy link
Member

Actually with the removal of the bundles the annotation-version preferences is obsolete as well? Or does it still has any effect?

@laeubi
Copy link
Contributor Author

laeubi commented Aug 10, 2022

I meant for the case where somebody uses a TP without the running-platform that does not contain the annotation-bundle at all.

Then it won't work anyways, if the annotations are not present at all, so either people are already consuming it from some official source or because they have the SDK in their target platform referenced somewhere

I assumed that at the moment the PDE-DS Plug-in adds the annotation-bundle from the running platform (e.g. like it is for example done with the junit-runtime if none is in the TP and one launches a Plug-in Test. But when looking at the DSAnnotationClasspathContributor (which is deleted with this change) then I don't think that this was ever possible.
If I did not oversaw anything actually nothing changes because the PDE-feature still pulls in a anno-bundle.

PDE-DS and junit-runtime work differently, junit resolve from the current Eclipse-Installation and as you already found out it actually will contain the bundle if PDE is installed or SDK is in the target.

That's right. It was more to discuss if we should downgrade to 1.3 until 1.4 and 1.5 are supported

I don't think this bring any value here as mentioned before, the Support for a DS version is and was never provided by the classpath entry but by PDE code!

Actually with the removal of the bundles the annotation-version preferences is obsolete as well? Or does it still has any effect?

This controls what constructs are allowed and the version in the XML, but my long term plan is more to simply decide this on the features used, for the moment we can simply keep this as-is.

@HannesWell
Copy link
Member

I meant for the case where somebody uses a TP without the running-platform that does not contain the annotation-bundle at all.

Then it won't work anyways, if the annotations are not present at all, so either people are already consuming it from some official source or because they have the SDK in their target platform referenced somewhere

OK, then I was overconcerned. Sorry for that.
So basically nothing changes. Unless somebody has referenced the removed bundles in a product or category.xml. But I think that is not very common and something we can accept, just like for the other Orbit-to-Maven transitions.

That's right. It was more to discuss if we should downgrade to 1.3 until 1.4 and 1.5 are supported

I don't think this bring any value here as mentioned before, the Support for a DS version is and was never provided by the classpath entry but by PDE code!

Yes of course. But now one can see new elements from 1.5 in the annotations and could falsely assume they are supported, while nothing happens. Of course this could already have happened in an older release if one has referenced the anno-bundles from Maven version 1.5. But now this is the case for the default shipped with PDE.

But maybe this is over-concerning too and we should better add support for that instead of worrying too much.

Actually with the removal of the bundles the annotation-version preferences is obsolete as well? Or does it still has any effect?

This controls what constructs are allowed and the version in the XML, but my long term plan is more to simply decide this on the features used, for the moment we can simply keep this as-is.

Maybe you mean that, but wouldn't it be ideal to control the version simply by the version available in the TP?
PDE-DS then simply has to support the latest version and supports what the user uses. And if a user want's to restrict him/herself because e.g. an older OSGi version is targeted, just an older version of the annotations can be selected?

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side this is fine. Thank you.

I think it should be mentioned as well, that the annotation-bundles can now also be referenced from maven (although this is actually due to the predecessor of this PR).

@laeubi laeubi merged commit 9a6d7bb into eclipse-pde:master Aug 11, 2022
@akurtakov
Copy link
Member

Build failure looks related https://ci.eclipse.org/releng/job/I-build-4.25/135/console . If the bundle wrongly requires it please submit PR for org.eclipse.debug.ui.launchview . I'm not sure what is the correct thing here otherwise would have done it myself.

@akurtakov
Copy link
Member

Thanks for the fast reaction :). Merged and new I-build running.

HannesWell added a commit to HannesWell/eclipse.platform.releng.aggregator that referenced this pull request Aug 17, 2022
Because the 'o.e.pde.ds.lib' bundle has been removed from PDE and to be
replaced by the official OSGi o.o.service.component.annotations bundles
from Maven-Central [1] but the change that PDE-DS understands the new
bundle-symbolic names [2] is only available as I-build this temporarily
adds 'o.e.pde.ds.lib' to the Platform-TP in order to keep DS-annotation
processing working until [2] is available in a release.

[1] - eclipse-pde/eclipse.pde#276
[2] - eclipse-pde/eclipse.pde#269
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

Successfully merging this pull request may close these issues.

None yet

3 participants