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
base: master
Are you sure you want to change the base?
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 |
7bb0ae6
to
e94f84f
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? |
The issue is perception. If a project perceives there is a dependency because it is reported by a service like mvnrepository, they will probably just decide not to even evaluate Eclipse Collections for usage. We had a recent situation where someone was unaware of the dual-licensing of Eclipse Collections, and they reported the usage of the EPL 1.0 license as an issue to a project as a Category B license (no, EPL 2.0 doesn't fix this). EDL 1.0 (BSD 3) should have made this a non-issue for the project, but the project decided to not ask any questions, and just remove the dependency. I didn't discover this until after the dependency was already removed, so it was already too late to clarify the issue as a non-issue. If this dependency is reported by an automated scanning tool, even if it is compile time only, then projects will likely (rightly or wrongly) decide to not use Eclipse Collections. |
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. |
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.