Replace content of o.e.osgi.services by osgi-bundles from Maven-Central #44
Conversation
|
This again requires eclipse-platform/eclipse.platform.releng.aggregator#235 to be submitted first. |
| Import-Package: org.osgi.annotation.versioning;version="[1.1.2,1.2.0)", | ||
| org.osgi.service.component.annotations;version="[1.3,1.4)" |
There was a problem hiding this comment.
We don't want runtime import of compile time packages. No bundle exports org.osgi.annotation.versioning. We also don't need to import org.osgi.service.component.annotations since this bundle does not use it in any way.
There was a problem hiding this comment.
I imagine the versioning package is imported here to allow the build to succeed for the remaining source in this bundle. Before we put some project JARs on the project classpath to do this. We could make this import on versioning optional or go back to a project JAR or some other solution.
There was a problem hiding this comment.
I imagine the
versioningpackage is imported here to allow the build to succeed for the remaining source in this bundle. Before we put some project JARs on the project classpath to do this. We could make this import onversioningoptional or go back to a project JAR or some other solution.
That was indeed the reason. I was too fast in removing it and then tried to resolve the compile errors and forgot to fix that afterwards. Thanks for the hint!
I think going back to the jar is the most robust solution, isn't it? And since the days of this bundle are numbered, I would not worry to much in finding a more elegant solution.
Regarding the import of org.osgi.service.component.annotations: I suspect the import was mainly because this bundle is exporting that package? But I removed it now.
There was a problem hiding this comment.
But it is plainly wrong to have an Import-Package in the resulting binary just to make sure source code can compile. This is an example of how PDE conflates build configuration and build output in the Manifest.
There was a problem hiding this comment.
Restoring use of the osgi.annotation binary solves the need for the Import-Package but there should be some other way to get osgi.annotation on the compile path.
There was a problem hiding this comment.
Isn’t there an extra classpath property you can put in the build.properties for such build/compile time dependencies? I think Tycho respects that too.
There was a problem hiding this comment.
There is jars.extra.classpath = lib/osgi.annotation.jar but this now refers to a local binary in the project source. It would be better if it could refer to some artifact in a repository like maven central.
There was a problem hiding this comment.
I think this is fine to have as an incremental improvement. We wouldn't need any of this if we got rid of the org.eclipse.osgi.services bundle altogether. But that takes a longer announcement and deprecation process and even then we will likely get loud objections to the removal. The other option is to simply have the class files sucked into the produced JAR during the build from the available maven artifacts. The disadvantage of that is that this will not work for the PDE project imported into the workspace.
There was a problem hiding this comment.
I agree with everything said in this thread. To me it looks like the root of this problem is that PDE does not support different scopes of dependencies, for example like Maven does. I don't think this is a problem of OSGi because is 'just' the runtime and its PDEs job to assemble a suitable runtime respectively compile-time reactor/container.
PDE uses the same set of dependencies when resolving the PDE-State (the 'Compile-Container') and when launching an Eclipse-Application (the 'Runtime-Container'). But it could use different sets (e.g. compile-time only like for the annotations). To avoid the need ot add the org.osgi.service.component.annotations when using OSGi DS those annotations are treated specially by PDE. If one enables a corresponding setting the annotation-jar with desired version is added implicitly to the classpath at compile-time. So basically PDE already has different Sets of dependencies but in for a very special scenario. If this concept where generalized it could also avoid the need to embed the service.component.annotation jars in PDE.
There could also be another scope for tests (about which I recently had a chat with @laeubi).
However this would be a completely different story (but I think an interesting one) and way out of scope of this change.
Therefore I would like to follow Tom's suggestion and proceed for now with the smaller steps we can do now :) ... if the updated SDK-prereq-TP is published by the platform.aggr build, which fails due to a subsequent change.
HannesWell
force-pushed
the
osgiServicesPackages
branch
from
a3e1a3d
to
3a9a674
Compare
HannesWell
force-pushed
the
osgiServicesPackages
branch
2 times, most recently
from
925e405
to
c8c12c6
Compare
|
The Target-Resolution fails due to the following reasons (the package The Maven-Metadata of org.osgi.service.http do not specify a dependency to a corresponding Artifact so Tycho is not to blame here. So this could either be adjusted in the osgi-bundles (but I assume this will take some time) or we could add |
We could look to use That would also pull in a capability we have been missing for the @rotty3000 Does this sound like a good option? |
|
For folks who are paying attention to the finer details. I opened https://issues.apache.org/jira/browse/FELIX-6525 about the fact that the exports for the various servlet packages does not contain the proper uses directives. |
|
Hey @tjwatson i'd be ok with using the Felix API bundle. We're using it in our product with equinox.http.servlet anyway. |
from Maven-Central This is required for eclipse-equinox/equinox.framework#44
from Maven-Central This is required for eclipse-equinox/equinox.framework#44
So we should simply add Should we then remove Or should we wait until FELIX-6525 is fixed and released to avoid the problems you have described? Relaxing the Package-Import version-range in the osgi-bundle is not an option? I have not yet thought much about it, but my first thought is that a a large as possible version range on the consumer-site gives the greatest flexibility to the resolver. Of course the consumer then must only use the capabilities of the lowest version required and higher major versions then must not break the part of the API that is used. Besides that I suspect that we also have to replace |
|
After I adding So it look like the And the Manifest of org.osgi.service.http.whiteboard contains: I wonder if this is a configuration issue or maybe a bug in Tycho? |
From my current debugging I have the impression that P2 or Tycho does not read the version list correctly. Because it creates only one |
|
Looks like the problem is in |
Just to be clear (and you have already found that out) Tycho do not actually do any resolving/handling here but delegates that work to P2! |
That's right. |
|
If we live with representing each version as a separate capability in the short-term, is there any path forward that would allow us to fix that in the future? I worry about pushing such things so late in the release. |
|
It seems quite challenging to evolve this. With requirements we had IRequirement and IRequiredCapability so we could introduce something new (which had a negative impact on Oomph and the Aggregator). But with IProvidedCapability it seems harder to evolve. One possibility though is that the IProvidedCapability's getProperties could encode additional information and that's already recorded by MetadataWriter.writeProvidedCapability(IProvidedCapability). Perhaps a boolean flag/property that indicates "I am a singleton capability so if there are other capabilities with the same name and namespace, only one of us may be used to resolve a RequiredPropertiesMatch". I have no idea how to implement the latter though. Perhaps such an approach could be used to fix the broken osgi.ee stuff by making the "singleton property" default to true for certain known namespaces... |
Are you referring to completing this PR? If you were referring to fix the change suggested in eclipse-equinox/p2#64, then we could consider to change the model to have a List/Set of versions in Nevertheless if we continue with 'unrolled' single-version capabilities the 'matcher' could be enhanced to assemble the single version capabilities back into a multi-versioned one by merging all capabilities from the same provider with same namespace and properties. I think that should be distinct, shouldn't it? and Which would then be treated equivalent. But I think we should discuss this in eclipse-equinox/p2#64 |
HannesWell
force-pushed
the
osgiServicesPackages
branch
2 times, most recently
from
0c49136
to
4870d9a
Compare
|
As discussed in eclipse-equinox/p2#64 (comment) I reverted the removal of the embedded sources of the I would like to complete this as soon as the master-branch re-opens for the 2022-09 release cycle. |
| org.osgi.service.upnp;version="1.2", | ||
| org.osgi.service.useradmin;version="1.1";uses:="org.osgi.framework", | ||
| org.osgi.service.wireadmin;version="1.0.1";uses:="org.osgi.framework" | ||
| org.osgi.service.log;version="1.5";uses:="org.osgi.framework" |
There was a problem hiding this comment.
We could get rid of the actual classes from the org.osgi.service.log package by placing a mandatory
matching attribute on the package.
org.osgi.service.log;version="1.5";uses:="org.osgi.framework";mandatory:="never.ever.use";never.ever.use="true"
If I recall correctly, at least for the Equinox framework implementation, we consider the Import-Package of a package before allowing its export to be selected. So if a bundle has an Import-Package which can never match its own export then its export will always be dropped. Here the import would never be satisfied by our own export because of the mandatory directive for "never.ever.use". This implies that the empty content from org.eclipse.osgi.services for the org.osgi.service.log package could never be used because it would always be substituted by the import to something real.
We would have to validate that is true.
There was a problem hiding this comment.
That's an interesting suggestion. I will try and validate that tomorrow.
It would be great if we can delete the service.log package too.
There was a problem hiding this comment.
This implies that the empty content from
org.eclipse.osgi.servicesfor theorg.osgi.service.logpackage could never be used because it would always be substituted by the import to something real.We would have to validate that is true.
I just tested it with a test project that contained Require-Bundle: org.eclipse.osgi.services in its manifest and and uses the Logger interface from org.osgi.service.log. The suggested approach worked well with PDE, when build with Tycho and when launched into an Eclipse application. The classes from org.osgi.service.log were available to the compiler respectively could be loaded at runtime. At least at compile-time the mandatory was not necessary. But I think it is better to be save.
I removed the embedded sources from org.osgi.service.log but added a not to not have a empty directory to silence PDE.
HannesWell
force-pushed
the
osgiServicesPackages
branch
from
4870d9a
to
0af6e50
Compare
|
To really use |
We cannot really do that until I do a proper release of Felix scr to have it import the osgi packages. I'll try to get that started tomorrow. |
|
To be more clear we need this commit to be released apache/felix-dev@bb03476 |
HannesWell
force-pushed
the
osgiServicesPackages
branch
from
0af6e50
to
86e4a71
Compare
Roger that. Since we have to wait for the Eclipse release we have some time any way. Since we need it when we want to remove the http packages as well, do you plan to release the change apache/felix-dev@3a9b563 in |
Although the release is not yet available I already created the corresponding PR: eclipse-platform/eclipse.platform.releng.aggregator#282 |
And update to latest version of - org.osgi.service.cm - org.osgi.service.component - org.osgi.service.device - org.osgi.service.event - org.osgi.service.metatype - org.osgi.service.provisioning - org.osgi.service.upnp - org.osgi.service.useradmin - org.osgi.service.wireadmin Discussed in issue: https://github.com/eclipse-equinox/equinox.framework/issues/40
HannesWell
force-pushed
the
osgiServicesPackages
branch
from
86e4a71
to
a0b2dad
Compare
|
Since the prerequisite for this PR, eclipse-platform/eclipse.platform.releng.aggregator#282, will probably be submitted soon: @tjwatson and @bjhargrave could you please renew your review on this PR so that we can submit this as soon all prerequisites are available. Furthermore I build this change locally and got the following test-failure: Do you have an idea why this happens? Maybe this is related to the removal of the log-packages. Or maybe it is just due to the different setup. |
There was a problem hiding this comment.
We recently moved the repository content to https://github.com/eclipse-equinox/equinox would you be so kind to open your PR for the new repository?
Migrated to eclipse-equinox/equinox#30 |
from Maven-Central This is required for eclipse-equinox/equinox.framework#44
As discussed in eclipse-equinox/equinox#18, this replaces the embedded osgi-sources of
org.eclipse.osgi.servicesby corresponding osgi-bundles from Maven-Central.As a side effect this updates the versions of
To comply with the Eclipse deprecation/removal process
org.eclipse.osgi.servicesis not removed entirely but it's content is replaced by requiring and re-exporting the provided osgi-bundles from Maven-Central. Bundles that require-bundleorg.eclipse.osgi.servicesdirectly continue to work.