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

OSGi import/export headers #242

Open
ascertrobw opened this issue Sep 8, 2021 · 14 comments · May be fixed by #247
Open

OSGi import/export headers #242

ascertrobw opened this issue Sep 8, 2021 · 14 comments · May be fixed by #247
Labels
for: team-attention An issue we need to discuss as a team to make progress

Comments

@ascertrobw
Copy link

Realise this is a very niche/uncommon area these days. Would be handy though to have OSGi import/export declarations in the Manifest so that the library can be used in an OSGi runtime framework with other libs that create these. Can supply examples if it is of interested to be added.

@mp911de
Copy link
Member

mp911de commented Sep 8, 2021

I'm not sure we have someone amongst our maintainers that is an OSGi expert. I'm reluctant to these additions as we have not means to properly test such metadata nor do we have the expertise to maintains OSGi declarations. Such arrangements typically ask for constant trouble and maintenance.

@mp911de mp911de added the for: team-attention An issue we need to discuss as a team to make progress label Sep 8, 2021
@ascertrobw
Copy link
Author

Yeah - I get that, it is a bit of a fringe area these days. In fact, looking at what the BND tool produces, I don't think it's too hard in your case for just the SPI JAR. Here's how the Manifest comes out once we push it through the "auto BND" in our library re-build stage

Manifest-Version: 1.0
Bundle-Category: bundle
Bundle-ContactAddress: support@ascert.com
Bundle-Description: r2dbc-spi packaged for OSGi
Bundle-ManifestVersion: 2
Bundle-Name: r2dbc-spi
Bundle-SymbolicName: com.ascert.repackaged.r2dbc-spi
Bundle-Vendor: Ascert, LLC
Bundle-Version: 0.8.5.RELEASE
Export-Package: io.r2dbc.spi;version="0.8.5.RELEASE"
Import-Package: javax.annotation.meta;resolution:=optional,javax.annot
 ation
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

Most of the above is boilerplate our re-build creates - the active parts really are the Import-Package / Export-Package headers. We made the javax.annotation.meta optional which may not be strictly correct for your code, but since we only need the bundle to resolve (we're not using the underlying classes) it handles things our side.

@lukaseder
Copy link
Contributor

Such arrangements typically ask for constant trouble and maintenance.

I've added these meta data to jOOQ early on using the Apache Felix Maven plugin, and surprisingly, I hardly ever had any need to maintain them, which can only mean two things:

  1. No one is using OSGi
  2. The meta data that most OSGi users need really is stable

@ascertrobw
Copy link
Author

OSGi is a rarer usage case for sure - I couldn't speak to the wider usage, but Apache Felix is certainly used and maintained which is how we use our OSGi framework. I believe the Eclipse IDE itself was OSGi framework based at one stage, although we mostly used Netbeans or VScode these days.

I guess the imports must have changed with jOOQ 3.15, since this issue only arises then. Presumably r2dbc-spi was either not an import in 3.14, or was an optional one.

@lukaseder
Copy link
Contributor

Presumably r2dbc-spi was either not an import in 3.14, or was an optional one.

It was not

@ascertrobw
Copy link
Author

BTW - I'm happy to submit a pull request with the extra headers if of help. Not a MVN expert, as we're gradle users, but pretty sure it's a small change to the current pom.xml:

 <configuration>
                    <archive>
                        <manifestEntries>
                            <Automatic-Module-Name>r2dbc.spi</Automatic-Module-Name>
                        </manifestEntries>
                    </archive>

The essentials as per the following example

Bundle-Category: bundle
Bundle-ManifestVersion: 2
Bundle-Name: r2dbc-spi
Bundle-SymbolicName: io.r2dbc-spi
Bundle-Version: 0.8.5.RELEASE
Export-Package: io.r2dbc.spi;version="0.8.5.RELEASE"
Import-Package: javax.annotation.meta;resolution:=optional,javax.annot
 ation
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"


Optional, but not really required being:

Bundle-ContactAddress: 
Bundle-Description: 
Bundle-Vendor: 

@mp911de
Copy link
Member

mp911de commented Sep 8, 2021

A PR sounds good, with having Bundle-Version and Export-Package / version=… parametrized it should be a rather small change. javax.annotation is only required during compilation and is mostly there for IDE support. Frameworks like Spring can additionally introspect API for nullability annotations, but I'm not sure whether there are OSGi-based tools for nullability introspection. Since all this can be an enhancement for existing pom config and we don't need a plugin here, it's less of an hassle than initially anticipated.

One thing to note, R2DBC SPI is a component that interacts with Java's ServiceLoader to load driver classes. Is this something to be considered in OSGi metadata?

@ascertrobw
Copy link
Author

ascertrobw commented Sep 8, 2021

Cool - I'll take a look. I'm not a Git expert, so forgive me if I fumble a bit on the local fork / PR creation process. Always have to remind myself on the GitHub process for that

One thing to note, R2DBC SPI is a component that interacts with Java's ServiceLoader to load driver classes. Is this something to be considered in OSGi metadata?

You have me there - I'll have to go digging into some Docs. Haven't looked into how OSGi and ServiceLoader co-operate and if additional metadata is needed.

Update: this document looks like the relevant spec, and seems to be involve extra manifest declarations on the provider side and consumer side. I'm guessing r2dbc will need the consumer side ones?

@mp911de
Copy link
Member

mp911de commented Sep 8, 2021

I'm guessing r2dbc will need the consumer side ones?

I think so. We call ServiceLoader.load(io.r2dbc.spi.ConnectionFactoryProvider, …).

ascertrobw added a commit to ascertrobw/r2dbc-spi that referenced this issue Sep 8, 2021
Fix for r2dbc#242 - Inclusion of basic OSGI headers.

Note, further work may be needed for full OSGi ServiceLoader handling :  https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html
@ascertrobw
Copy link
Author

Submitted pull request for the basic headers.

Might be handy if feasible to also merge into the 0.8.x release branch.

@mp911de
Copy link
Member

mp911de commented Sep 9, 2021

Thanks a lot, the PR looks pretty decent. Can you add the ServiceLoader entries as well? Once that's in place, we can merge the change for both, the main and 0.8.x branches.

@mp911de
Copy link
Member

mp911de commented Sep 9, 2021

Partly because I have no test case or environment in which I can prove they actually work - but also because I have a feeling they may well trigger knock on set of dependencies.

Exactly the same here, even worse, I never really got in touch with OSGi details.

I'd rather defer on the ServiceLoader entries for this commit if we can

We don't have many attempts to get things right. We should do it in a single step. I cannot oversee how much or little impact we will have with the metadata already so I'm inclined to wait with the addition until we can deliver the full set of metadata.

@ascertrobw
Copy link
Author

ascertrobw commented Sep 9, 2021

I'd rather defer on the ServiceLoader entries for this commit if we can

We don't have many attempts to get things right. We should do it in a single step. I cannot oversee how much or little impact we will have with the metadata already so I'm inclined to wait with the addition until we can deliver the full set of metadata.

Let me see if OSGi supports the concept of an "optional" ServiceLoader requirement on the consumer side. If not "right" becomes a bit debatable. With the present headers, the bundle will load and not prevent the OSGi framework from starting if no service providers are present. That would be right for any bundle that needs the SPI classes to resolve, but may not actually use the underlying service - which often times with OSGi SPI or API bundles, is as many or more cases than those who use the provided service.

ascertrobw added a commit to ascertrobw/r2dbc-spi that referenced this issue Sep 9, 2021
Inclusion of optional ServiceLoader provider requirement
@ascertrobw
Copy link
Author

OK - added an extra commit to the PR for the ServiceLoader headers.

ascertrobw added a commit to ascertrobw/r2dbc-spi that referenced this issue Sep 9, 2021
ascertrobw added a commit to ascertrobw/r2dbc-spi that referenced this issue Sep 9, 2021
…iceLoader provider requirement.

Signed-off-by: RobWalker <4760572+ascertrobw@users.noreply.github.com>
rotty3000 added a commit to rotty3000/r2dbc-spi that referenced this issue Oct 5, 2021
- <developers><developer><id> element added to silence a complaint from bnd about well formed entries
- let bnd generate default manifest spec entries, as well as setting all other headers
- added BND SPI annotations to add the necessary parts to make ServiceLoader work in OSGi (passively)

fixes r2dbc#242

Signed-off-by: Raymond Augé <raymond.auge@liferay.com>
@rotty3000 rotty3000 linked a pull request Oct 5, 2021 that will close this issue
3 tasks
rotty3000 added a commit to rotty3000/r2dbc-spi that referenced this issue Oct 5, 2021
- <developers><developer><id> element added to silence a complaint from bnd about well formed entries
- let bnd generate default manifest spec entries, as well as setting all other headers
- added BND SPI annotations to add the necessary parts to make ServiceLoader work in OSGi (passively)

fixes r2dbc#242

Signed-off-by: Raymond Augé <raymond.auge@liferay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants