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

Adds Feature #1568 - [OSGi] Opting in to Service Loader Mediator #1569

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fipro78
Copy link
Contributor

@fipro78 fipro78 commented Mar 27, 2024

  • Update bnd-maven-plugin to 6.4.0 (not directly to 7.0.0 as that requires Java 17 for the build)
  • Change the Automatic-Module-Name to jpms-module-info
    With this the module-info.class gets generated by Bndtools to the bundle file. This should resolve Getting rid of Automatic Modules? #1451 then.
  • Added the @aQute.bnd.annotation.spi.ServiceProvider annotation to the factories. This annotation triggers the generation of
    • The Require-Capability header for the Service Loader Mediator
    • The Provide-Capability header for all provided services
    • The service provider configuration files in META-INF/services
  • Remove the static service provider configuration files and the generation of those files for the primitive collections, as they are now generated and don't need to be maintained manually anymore. This way the META-INF/services don't contain provider configuration files of services that do not exist anymore (see in a comment below)
  • Change the p2 update site build (should fix Update Eclipse Tycho and replace EBR dependency with Orbit #1501)
    • disable p2-repository module which uses EBR.
    • reactivate p2-feature to build a p2 update site via Tycho.
      • The p2 update site build is not included in the main build anymore and needs to be triggered separated.
      • It uses the artifacts from Maven as dependencies and provide them via p2 update site, instead of generating a new wrapped bundle of API and Impl. This makes it necessary to include a Service Loader Mediator Implementation when using Eclipse Collections in the future.
  • Minor update in the parent pom.xml
    • Updated some versions
    • Removed the indentation in the description which partly makes PR Clean up BND instructions #1468 obsolete. Only thing that is missing for Clean up BND instructions #1468 is the warning about missing developer id's. Where actually I would suggest to add the id to the developers in the parent pom.xml to avoid the warning. But that is a project decision how they want to deal with this. The suggestion from @Bananeweizen to have a general Bundle-Developer in the manifest is also suitable.
  • Minor updates in the generator files for the primitive factories, to get rid of the warnings in the Javadoc creation process.

@fipro78
Copy link
Contributor Author

fipro78 commented Mar 27, 2024

@timothyjward As an OSGi expert, could you please at least verify if my assumption and approach for the fix is correct? I tested it locally in an Maven based OSGi-application, and it works fine. But getting feedback from you would be very beneficial.

@timothyjward
Copy link

timothyjward commented Mar 27, 2024

@timothyjward As an OSGi expert, could you please at least verify if my assumption and approach for the fix is correct?

Hi - I think the extender requirements that you have added look fine, but the implementation seems to be missing osgi.serviceloader capabilities for the implementations that it wants to register. This is described in the specification at https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.loader.html#d0e106889 and has an example at https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.loader.html#d0e107018

For reference I think you would need entries for each of the implementations registered in https://github.com/eclipse/eclipse-collections/tree/master/eclipse-collections/src/main/resources/META-INF/services

There are a lot of these, so you may wish to consider having bnd generate all this for you (including META-INF/services files) using the @ServiceProvider annotation https://bnd.bndtools.org/chapters/240-spi-annotations.html#serviceprovider

@fipro78
Copy link
Contributor Author

fipro78 commented Mar 28, 2024

@timothyjward
Thanks for the feedback. From the specification point of view, you are totally right that the osgi.serviceloader capabilities need to be added. Interestingly it doesn't need to be required at runtime. SPI Fly seems to inspect META-INF/services and process everthing in there, even without the capabilities.

I will check if I can use the @ServiceProvider annotation. That would even reduce the maintenance effort on the project side, as the provider configuration files don't need to be maintained manually anymore.

@fipro78
Copy link
Contributor Author

fipro78 commented Mar 28, 2024

I adapted the request from @timothyjward and use @ServiceProvider for the generation of the SPI resources. This way also the provider configuration files are generated. I therefore also removed the generation of those resources.

This also revealed a current issue in the eclipse-collections bundle. The following files are currently included in META-INF/services but the related implementations do not exist anymore. I suppose they were dropped in the meanwhile, but the services files are still generated:

org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanBooleanMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanByteMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanCharMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanDoubleMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanFloatMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanIntMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanLongMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanShortMapFactory

org.eclipse.collections.api.factory.map.primitive.MutableBooleanBooleanMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanByteMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanCharMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanDoubleMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanFloatMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanIntMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanLongMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanShortMapFactory

org.eclipse.collections.api.factory.set.primitive.ImmutableBooleanSetFactory

@fipro78
Copy link
Contributor Author

fipro78 commented Apr 3, 2024

@nikhilnanivadekar
Sorry that this PR now got that big. But it was necessary to be able to build an update site with Tycho once the meta information in the MANIFEST files get generated.

@nikhilnanivadekar
Copy link
Contributor

@fipro78 trying to understand the PR. Is it possible to logically squash the commits? Multiple commits are ok, but some of them are overwritten and reviewing would be cumbersome.

Also, if you wouldn't mind explaining what will be the new process for p2 update site and to contribute to the Simrel repo? I am not an expert in this area. So, I will need (a lot of) help as I learn. Thanks!

@fipro78 fipro78 force-pushed the master branch 2 times, most recently from af243ee to ad5c9c1 Compare April 4, 2024 07:06
@fipro78
Copy link
Contributor Author

fipro78 commented Apr 4, 2024

@fipro78 trying to understand the PR. Is it possible to logically squash the commits? Multiple commits are ok, but some of them are overwritten and reviewing would be cumbersome.

After some struggles, I hope I have correctly squashed the commits.

Also, if you wouldn't mind explaining what will be the new process for p2 update site and to contribute to the Simrel repo? I am not an expert in this area. So, I will need (a lot of) help as I learn. Thanks!

Well, with this PR the p2 update site is created from the p2-feature project, and not the p2-repository anymore. And the p2 update site is not created on every build, but needs to be triggered manually. That means, the Jenkins job that publishes the p2 update site needs to be changed to build the p2-feature and publish the results from the p2-feature/org.eclipse.collections.repository/target.

With regards to contribute to the Simrel repo, this PR actually has no impact. What this PR does is, it ensures that the artifacts, that are published to Maven Central, can also be used in an OSGi context. This was not really possible as of now, because the API retrieves services from the Impl, but they reside in different bundles, and therefore we have classloader issues. That is why the previous EBR approach in p2-repository creates a new wrapped bundle that combines API and Impl. The wrapping should be not needed anymore, if a Service Loader Mediator implementation is available in the runtime.

Whether Eclipse Collections become part of Simrel or will be included in Eclipse Orbit, is out of the scope of this PR. But without additional build steps, it should be easy to include the artifacts from Maven Central there. It would be only needed to consume those artifacts, as they are OSGi bundles.

Of course that need to be validated again, to be sure that it is really working. I suppose @merks asked whether there is a Maven repository to consume the artefacts before they are actually released, so it can be verified how easy it would be. And maybe he can then also give some more information with regards to including Eclipse Collections to Orbit or Simrel. In the end, the question is, who is responsible for the publishing of the p2 update site. And IMHO that depends on how easy it becomes to consume the artifacts from Maven Central, rather then needing to create a wrapped bundle.

@merks
Have I explained that correctly? Do you want to add something? Because Simrel and Orbit is definitely your domain. ;-)

@merks
Copy link

merks commented Apr 4, 2024

PDE, with m2e's help, as well as Tycho, support specifying Maven dependencies in a *.target. Orbit uses that specifically here:

https://github.com/eclipse-orbit/orbit-simrel/blob/main/maven-osgi/tp/Maven.target

As background, this file is generated/updated automatically by looking at the *.target of various SimRel projects and looking at Maven Central for new versions. As part of that analysis, the following target is also considered:

https://github.com/eclipse-orbit/orbit-simrel/blob/main/maven-osgi/tp/other/MavenSupplement.target

So if Eclipse Collections publishes well-formed OSGi artifacts to Maven Central we can trivially include it in MavenSupplement.target so that it's generated into Maven.target and in the end, is available in these Orbit p2 repositories:

https://download.eclipse.org/tools/orbit/simrel/maven-osgi/
https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/

Note that is is a convenience for the consumers because the consumers can include such dependencies directly in their own *.target, in which case they are also responsible for PGP signing those artifacts, which Orbit also does as an additional convenience.

Once that's in place, the Eclipse Collection's SimRel contribution should be removed and any other project relying on those bundles, must contribute them via the p2 repository of their own SimRel contribution.

So I'm happy with this path forward. The overhead for Orbit is trivial and is just done once, after which updates will be automatically generated.

@donraab
Copy link
Contributor

donraab commented Apr 11, 2024

Hi @fipro78, thanks for spotting this. I don't believe these classes ever existed. IIRC, we had a discussion a long time ago around the benefit of having boolean to anything Map and decided it wasn't worth coding or generating. These must have been mistakenly added to the services.

I adapted the request from @timothyjward and use @ServiceProvider for the generation of the SPI resources. This way also the provider configuration files are generated. I therefore also removed the generation of those resources.

This also revealed a current issue in the eclipse-collections bundle. The following files are currently included in META-INF/services but the related implementations do not exist anymore. I suppose they were dropped in the meanwhile, but the services files are still generated:

org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanBooleanMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanByteMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanCharMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanDoubleMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanFloatMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanIntMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanLongMapFactory
org.eclipse.collections.api.factory.map.primitive.ImmutableBooleanShortMapFactory

org.eclipse.collections.api.factory.map.primitive.MutableBooleanBooleanMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanByteMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanCharMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanDoubleMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanFloatMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanIntMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanLongMapFactory
org.eclipse.collections.api.factory.map.primitive.MutableBooleanShortMapFactory

org.eclipse.collections.api.factory.set.primitive.ImmutableBooleanSetFactory

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.

Update Eclipse Tycho and replace EBR dependency with Orbit Getting rid of Automatic Modules?
5 participants