-
Notifications
You must be signed in to change notification settings - Fork 586
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
Conversation
@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. |
Hi - I think the extender requirements that you have added look fine, but the implementation seems to be missing 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 |
@timothyjward I will check if I can use the |
I adapted the request from @timothyjward and use This also revealed a current issue in the eclipse-collections bundle. The following files are currently included in
|
@nikhilnanivadekar |
@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! |
ad5c9c1
to
104d82c
Compare
After some struggles, I hope I have correctly squashed the commits.
Well, with this PR the p2 update site is created from the 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 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 |
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/ 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. |
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.
|
4a419f9
to
6762424
Compare
@motlin @nikhilnanivadekar @donraab And is there a build result for the PR that could be consumed by @merks to verify if the changes can be consumed easily in the Eclipse environment? @motlin @timothyjward @merks |
I'm familiar with com.google.auto.service.AutoService which seems similar and also has class time retention. What's the difference between then? Does ServiceProvider also power the generation of manifest entries? |
The error message is:
There are two things I find odd about this.
Actually looking at the diffs, it seems that you changed |
e94f84f
to
8671a07
Compare
No, probably it was the last locally working state. I reverted it and you can see the error that comes up then. By accident I noticed that I have pushed the old separate early-access-javadoc build file before, which succeeded. The difference there is that the generator is build before. I have adapted that now. Not sure if it existed before or if I added it with the previous state to get the workflow build to succeed. And it seems I got it fixed again. |
What about the annotation? Is it possible to avoid it? Even compile-time-retention annotation dependencies show up as dependencies when searching sites like mvnrepository.com. We'd prefer to show up as having no dependencies. |
Well technically yes. The annotation triggers generation. If you do everything manually, you can avoid it. I actually tried it at the beginning but got the feedback that I missed several things. If you look at the generated manifest file, you will see that the manual maintenance of it would be a nightmare. The usage of the annotation with the other changes leads to a reduced maintenance effort on your side with regards to support OSGi and therefore Eclipse RCP. Without it, well only very experienced OSGi experts are able to find an issue if you change something in the future. And to be honest, there are not that much experts. A reason for the annotations. BTW I am not aware of any issues with compile time dependencies. Why do you want to avoid them? |
If the artifact does not come in the form of an OSGi bundle then there will also be those who decide not to use it or cannot use it. You just can't please everyone because everyone has different perceptions and needs. VIATRA, a SimRel project, has a dependency on Collections: I've needed to personally make updates to Collections' SimRel contribution: https://github.com/eclipse-simrel/simrel.build/commits/main/collections.aggrcon That's a bit dysfunctional. Orbit is capable of wrapping a non-OSGi artifact as a bundle because m2e provides support for that: That being said, somewhere the BND instructions need to be defined and maintained... I also noticed that Collections is long (years) over due for review: https://projects.eclipse.org/projects/technology.collections |
Agreed. Just answering the question from @fipro78. I can't see a technical issue, but could be a perception issue for the project. I appreciate all the help we have gotten from folks like yourself and @fipro78 on the OSGi front. Thank you! We'd like to make sure projects dependent on OSGi are able to leverage EC without issue, so long as there is not an adverse impact to others using the project. Eclipse Collections had over 800K downloads last month from Maven Central. I can't tell how many downloads there were from Eclipse RCP projects. It's hard for me to tell from the VIATRA project whether they will be able to use Eclipse Collections after 11.1 now, since we have baselined development on Java 11 for the 12.0 release. Most of the pom files in VIATRA seem to be pointing to EC 10.2 as a dependency. Does the VIATRA project still depend on Java 8? |
According to the release notes, the 2.8.x versions of VIATRA will be the last on Java 8. https://projects.eclipse.org/projects/modeling.viatra/releases/2.8.0 |
Hi @fipro78, spending some time trying to understand the details and implications of this PR. A few competing goals we are trying to manage in the project in some sort of priority order.
I'm hoping this PR helps address both 2 and 3, where 2 will have many more potential users of Eclipse Collections who may be impacted. Some of the improvements you have made in this PR look and sound very nice, and I'd like to just shoot some ideas about to see if there is not some way that we can have all three benefits, with minimal pain to existing or future users of Eclipse Collections. I see we had a previous dependency on the I went and looked in mvnrepository for the projects that currently depend on bnd annotaiton. There is very small list for 6.4.1 reported here: https://mvnrepository.com/artifact/biz.aQute.bnd/biz.aQute.bnd.annotation/6.4.1/usages We can see how the bnd annotation dependency shows up for Apache Log4J Parent in mvnrepository here (under provided and managed dependencies): https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j/2.23.1 This is the look we'd like to avoid, since we can't update this page in anyway alleviate any dependency concerns folks may have about dependencies. There is a mechanism that Woodstox (uses 6.4.0) uses for identifying the dependency as "provided" and "optional" which impacts the display in mvnrepository. @motlin @nikhilnanivadekar @prathasirisha @mohrezaei https://github.com/FasterXML/woodstox/blob/master/pom.xml#L191 I took a look at Apache Log4J parent pom, and see they have quite a few bnd and osgi related properties. https://github.com/apache/logging-parent/blob/main/pom.xml Questions:
|
Sorry to step in late here, but isn't this a limitation of maven? Effectively, it has no designation for a dependency that is required during compilation, but not runtime (such as class retention annotations). Wouldn't an alternate approach be to publish a different pom file than the one that's used to compile (without the dependency, because it's not a real dependency)? I'm no maven expert so I'm not sure how to achieve that automatically (I know it's possible manually, as projects that don't even use maven for compilation can publish poms). |
Yes this PR helps address both 2 and 3. Yes I added 6.4.0 because 7.0.0 needs Java 17 for the execution. You can also build using Java 17 and produce Java 11 compatible artifacts, but that would be a bigger change to your build setup that I didn't wanted to start. To your first question, I don't think that is possible, because the annotations need to be known by the compiler. To your second question, I already mentioned that this is possible. But as I also already explained, that would introduce a high risk in the maintenance. If you change anything at any time and don't be serious you will break things without knowing or even knowing how to fix it. And the PR already mentions a place where you have missed to cleanup everything and have incorrect leftovers. No, blaming, just an example why the generation is better in keeping things clean. @mohrezaei But I just want to give you something to think about. What you currently discuss is that you want to use a open source project to help to reduce the technical depth, improve the the maintainability and improve the options to consume your library. But you want to hide this from the rest of the world! Even if it is just a compile time dependency that has technically no impact for consumers of your library. It is just something written in maven central. And it is some kind of credit to the project that you would use to improve your project. Have you ever thought about how the maintainers of that project would think about the effort you put in to hide the usage? I mean we are all working in the OSS space, why not showing that we benefit from each other? |
@fipro78 We have no intention of hiding anything. As I said before, this is a deficiency in maven: it erroneously reports a jar as a dependency. We're trying to provide correct dependency information in our public facing interface. |
I also just re-read maven docs, and I'm guessing from their perspective, this should have a |
There was a similar discussion here. eclipse/microprofile-config#716 Different topic for different annotations, but maybe the provided scope could work for you. |
The Jackson Woodstox project uses @mohrezaei @motlin What do you think about just adding these to the dependency as @fipro78 has it now? |
That would be the closest to correct you can get with maven with a single pom.xml. |
I don't remember the PR very well anymore but in #1175 @donraab you went through some effort to create META-INF/services without adding a dependency on auto-service. I have another project where I use auto-service annotations heavily and like it, but I don't worry about dependencies there. As @mohrezaei says, provided & optional is the closest we can get to expressing the truth with maven's limited set of dependency scopes. It will show up that way in search engines as discussed. Eclipse Collections would be going from 0 dependencies to 1 optional dependency. biz.aQute.bnd.annotation has permissive licenses include Apache 2.0. I feel like it's up to you @donraab! |
I can't unring this particular bell, but it looks like this PR does quite a bit of that anyway with some file removals.
I'm leaning toward the "Let's do this, add the optional dependency, and do an 12.0.0.M4 release and test the heck out of this on multiple projects, in the JPMS and OSGi ecosystems". I'm reading through some of the work and research that @fipro78 has done on the issues he linked in the start of this conversation, and feel like we have a lot of ground we can cover catching up with current recommended practice in the Eclipse Project ecosystem. I can't thank him enough for wading through this. Thank you again! I'd like to hear from @nikhilnanivadekar and @prathasirisha who both did work with getting EC in shape with JPMS and also with sorting through some of the issues we had with ServiceProvider in the 11.0 release that we addressed in the 11.1 release. I feel like 11.1 has been super stable, and I want to know that even with the optional dependency defined, we can continue to consume Eclipse Collections 12.0 without any changes required to clients. |
I added the provided scope and did some further tests. I noticed that without the While testing I also noticed the I created a consumer test project, that builds the applications we where talking about:
You can find the project here: https://github.com/fipro78/ec-consumer-test I am still working on the Eclipse build. But you can start the application after activating the target definition and then start from the product file. But hopefully I can get the build to work and produce a product similar to the other flavours. For Eclipse applications the following changes will be needed:
@merks What do you think about this? Should the consumers be responsible for the bundle start, or should we add the |
@HannesWell has way more knowledge about the SPI Fly stuff than I do. All the product (Platform SDK and EPP) all do this: I'm happy to add it to Orbit as a convince to others... |
@merks |
I figured I was not really answering your question. Sorry for my ignorance. I hope someone else, perhaps @laeubi, will have more insightful answers about this service stuff... |
No reason to apologize. Every input is valuable in that area. :) And the information that spifly is already autostarted in Eclipse products is also important. |
The usual way is to mark the bundle lazy-start, so it will be started whenever a class is loaded from it, this seems the right approach for eclipse-collections as well if it requires to become "started"... |
@laeubi @HannesWell I added the lazy activation policy and tested it with my simple example. Looks good. |
@fipro78 The failing Unit Test Early Access build is not due to anything in your PR, so you don't need to spend time investigating. I reported the issue with the recent Early Access Build of JDK 23 to the OpenJDK Quality Outreach Program. @motlin @mohrezaei @nikhilnanivadekar @prathasirisha |
@donraab @motlin @mohrezaei @nikhilnanivadekar @prathasirisha Of course it is a simple example and does not cover all factory services provided by Eclipse Collections. But it shows that the dependencies are correct, the resolution and build and runtime are correct, and all of the variants are working. The only variant I haven't tested is the creation of a native image using GraalVM. But that is another big topic I would leave out here. |
Hi @fipro78 I don't think there is anything else for you to do. I'd like to give @nikhilnanivadekar and @prathasirisha a chance to have a look and share any feedback. Nikhil and Sirisha, when you get a chance, please have a look at what Dirk has done here and share any concerns about adding the new "provided" and "optional=true" dependency. Thanks! |
Hi @fipro78, good news! I was able to follow up with @nikhilnanivadekar and @prathasirisha and we're all good with the new dependency and the approach using provided and optional. @prathasirisha created two projects to test an issue we had fixed in 11.1 and I was able to clone and build your fork locally and validate the two projects she created and they ran ok. In addition I was able to see the Once you rebase your PR, I will approve and merge. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for all the research, effort, and your patience @fipro78!
Automatic-Module-Name
tojpms-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.
@aQute.bnd.annotation.spi.ServiceProvider
annotation to the factories. This annotation triggers the generation ofRequire-Capability
header for the Service Loader MediatorProvide-Capability
header for all provided servicesMETA-INF/services
META-INF/services
don't contain provider configuration files of services that do not exist anymore (see in a comment below)p2-repository
module which uses EBR.p2-feature
to build a p2 update site via Tycho.Bundle-Developer
in the manifest is also suitable.