auto register capabilities for META-INF/services

[chrisrueger]

In this PR

this is an idea based on #6304 by @royteeuwen

it leverages the recent addition of PR #5912 that allowed to add annotations in comments of files in META-INF/services.
See https://github.com/bndtools/bnd/blob/master/docs/_chapters/230-manifest-annotations.md#meta-infservices-annotations

In this PR we basically add a aQute.bnd.annotation.spi.ServiceProvider annotation automatically if a service doesn't specify one. this causes bnd to generate Provide-Capability manifest headers for files in META-INF/services which was the requirement in the #6304

example

bnd.bnd file and a groovy-3.0.22.jar in a the lib folder

-buildpath: biz.aQute.bnd.annotation;version='7.0'
-includeresource: @lib/groovy-3.0.22.jar
-metainf-services: auto

created the following manifest headers

Provide-Capability                      osgi.service;objectClass:List<String>="org.codehaus.groovy.transform.ASTTransformation";effective:=active
                                        osgi.serviceloader;osgi.serviceloader="org.codehaus.groovy.transform.ASTTransformation";register:="groovy.grape.GrabAnnotationTransformation"
Require-Capability                      osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
                                        osgi.extender;filter:="(&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0)))"

Note: -metainf-services: auto is a new instruction needed to activate this new behavior (see comment below)

make sure biz.aQute.bnd.annotation is on the buildpath which contains the 'aQute.bnd.annotation.spi.ServiceProvider' annotation.

Let's use this as a base for discussion if it makes sense at all, or needs configurability etc.

[pkriens]

I think this is an ok idea and the code is quite suitable for this.

However, I think the decision can better be done in MetaInfServiceParser. The MetaInfService code is gathering the information of the services directory. It is generally not a good idea then to mix in strategy. I.e. you can now no longer detect if the service implementation was marked or not on the higher level. You never know when you need that. You could also not leverage the project setup to treat it special. Plus it is would be a gratuitous API change.

In MetaInfServiceParser we do policy. We have access to the project and workspace properties so we can make decisions on the actual project setup, unlike MetaInfService who is a worker bee.

Here we can detect that a given service implementation is not annotated and treat it special. We might add an instruction like -metainf-services = auto to the bnd.bnd file to then create the annotation on an empty Implementation if it is set.

[chrisrueger]

Thanks for the feedback @pkriens , I will move the code higher up to MetaInfParse.

-metainf-services = auto

Wondering what should be possible values and what should be the default? What about:

• annotation -> the default if not set, which is current behaviour, which only processes annotations
• auto -> as discussed above which processes annotations and also create the annotation on an empty Implementation if it is set.
• none (maybe do disable processing of META-INF/services completely. Not sure anybody would want that. )

As I have no experience with this topic just out of curiosity: What would be the effect if "auto" would be the default on libraries out there in the wild?

Update: I moved the code to MetaInfServiceParser and added -metainf-services: auto instruction.

[chrisrueger]

@royteeuwen could you test this PR with your bundle?

[royteeuwen]

I tried it on my branch (see referenced commit) but it doesnt produce the headers yet. Anything I can do to debug?

[chrisrueger]

Start using an experimental feature to automatically create the corre…

Hmm...

1. I see -includeresource: @*.jar;lib:=true

When I do this bnd then it seems biz.aQute.bndlib is being wrapped (I assume because it is on the -buildpath)

If I do it with one specific jar it works:
-includeresource: @lib/groovy-3.0.22.jar;lib:=true

IF it works with a specific jar file then we need to find out the correct syntax for -includeresource to do this with *.jar

[royteeuwen]

it would be nice, because as you can see in my referenced branch, I'm wrapping like 20 jars in the same time using pluginManagement, making sure there is always only one dependency (namely the wrappable non-osgi jar), which avoids me from hardcoding the thing. If there is another syntax to achieve the same, I'm completely fine with it of course!

[chrisrueger]

Anything I can do to debug?

Have you built my branch locally too?
Just wondering where you are pulling in the changes of my PR here?

[royteeuwen]

Of course :)

git clone [email protected]:bndtools/bnd.git                                                                                                                                                                        
 gh pr checkout 6309  
./gradlew :build
./mvnw install

First time I have checked out the repo, so there is definitely not another SNAPSHOT in my .m2 ;p

[royteeuwen]

@chrisrueger I do think its still not using the correct bnd, I'm debugging locally by using mvnDebug and then attaching in the project and the lines don't match, trying to figure out why, will get back to you

[chrisrueger]

@chrisrueger I do think its still not using the correct bnd, I'm debugging locally by using mvnDebug and then attaching in the project and the lines don't match, trying to figure out why, will get back to you

Just guessing:

Maybe you also need to reference the latest bndlib 7.1.0-SNAPSHOT in the -buildpath?

-buildpath: biz.aQute.bndlib;version='7.1.0-SNAPSHOT'

Or maybe try removing the -buildpath and see what happens.
I am not familar with the maven plugin and how it behaves. Maybe stuff is automatically on the buildpath.
I am testing with a bnd workspace from within Eclipse and when I did not have -buildpath: biz.aQute.bndlib, this code was not finding the Annotation class and ignored it https://github.com/bndtools/bnd/blob/master/biz.aQute.bndlib/src/aQute/bnd/osgi/AnnotationHeaders.java#L332

[chrisrueger]

I'm wrapping like 20 jars in the same time using pluginManagement, making sure there is always only one dependency (namely the wrappable non-osgi jar), which avoids me from hardcoding the thing. If there is another syntax to achieve the same, I'm completely fine with it of course!

On that one:
If your jar filenames start with a common prefix e.g. opentele then you could do
-includeresource: @opentele*.jar;lib:=true

This avoids that bndlib itself ends up in the resulting jar.

[royteeuwen]

OK, I found the issue. You have to have the bnd.annotations jar still on the classpath:

                        <dependency>
                            <groupId>biz.aQute.bnd</groupId>
                            <artifactId>biz.aQute.bnd.annotation</artifactId>
                            <version>7.1.0-SNAPSHOT</version>
                        </dependency>

Else you get the following issue in the aQute.bnd.osgi.AnnotationHeaders:

[chrisrueger]

OK, I found the issue. You have to have the bnd.annotations jar still on the classpath:

                        <dependency>
                            <groupId>biz.aQute.bnd</groupId>
                            <artifactId>biz.aQute.bnd.annotation</artifactId>
                            <version>7.1.0-SNAPSHOT</version>
                        </dependency>

Else you get the following issue in the aQute.bnd.osgi.AnnotationHeaders:

Yes I got the same issue:

Oh you are right.
It's biz.aQute.bnd.annotation

it seems biz.aQute.bndlib also contains the annotations, that's why it worked for me too.

I tested with biz.aQute.bnd.annotation and it works fine as well and is much better because smaller.
But I think this dependency is just needed for build / compile time and not at runtime.

So you get the Provide-Capability headers now?
Have you tried with a prefix-includeresource: @opentele*.jar;lib:=true

[royteeuwen]

@chrisrueger well it seems that the bnd-maven-plugin doesn't include the annotations by default though, so definitely a remark that will be required in the documentation :)! Adding it through the -buildpath property doesn't seem to do anything in maven, there I have to explicitly add it in <dependencies>

Yep, I get the Provide-Capability 🎉

I'm adding some extra maven properties and doing it like this:

-includeresource: @${opentelemetry.artifactId}-${opentelemetry.java.version}.jar;lib:=true
-metainf-services: auto

[royteeuwen]

I'll test the actual bundle tomorrow, but seeing as it uses already existing functionality, I guess it should work out ;). Thanks a lot so far!

[chrisrueger]

Yeah, and thanks for your feedback.

well it seems that the bnd-maven-plugin doesn't include the annotations by default though, so definitely a remark that will be required in the documentation :)!

Ok good point.
Since I am not familiar with the maven plugin, could you maybe suggest a paragraph which could be added to the docs e.g. to the files I touched here?
https://github.com/bndtools/bnd/pull/6309/files#diff-6c05b50a9d4a861349bca27733122a0ea6c002f2d62086c9a857c310b2da2d41

as a code comment would be fine for me, I can work it in.
I just would like to avoid giving maven recommendations for stuff I am not familiar with.

[royteeuwen]

@chrisrueger everything seems to work on my side :) i already validated it in a real life OSGi env that uses the opentelemetry jars. What would be required to make this into a release? Very nice work :)

By the way, another nice feature that would also be nice and currently isn't done yet is that it also detects when a ServiceLoader.load is used in a jar and also create the Require-Capability: osgi.extender;filter:="(osgi.extender=osgi.serviceloader.processor)" headers as well. Now it only does the work for the Provider, not the Consumer bundle.
Not sure how feasible that is though, because that's based on the actual code instead of files inside the META-INF 😄

[chrisrueger]

@royteeuwen great news: I had a little discussion with @pkriens about why bnd.annotations needs to be a dependency on the classpath to use the textual ServiceProvider annotation in services files or via -metainfo-services: auto It seems that was a a small flaw in PR #5912

So he was so kind to work on a fix. b206734

Long story short:

The following dependency is NOT required in your pom.xml anymore:

<dependency>
    <groupId>biz.aQute.bnd</groupId>
    <artifactId>biz.aQute.bnd.annotation</artifactId>
    <version>7.1.0-SNAPSHOT</version>
</dependency> 

I will update the docs again later and then I think this PR is ready to merge.

[chrisrueger]

Hmm, I don't know what changed, but a lot of tests are failing when compiling now.

Yeah sorry. I was a bit too fast. It was the change to make 'auto' the default.
Try to take care of that tomorrow.

[royteeuwen]

Hmm, I don't know what changed, but a lot of tests are failing when compiling now.

Yeah sorry. I was a bit too fast. It was the change to make 'auto' the default. Try to take care of that tomorrow.

OK 👍 , let me know when I should retest

I see for example that the Require-Capability in my wrapped bundle changed from

Require-Capability: osgi.extender;filter:="(&(osgi.extender=osgi.servi
 celoader.registrar)(version>=1.0.0)(!(version>=2.0.0)))",osgi.ee;filt
 er:="(&(osgi.ee=JavaSE)(version=1.8))"

to

Require-Capability: osgi.extender;filter:="(&(osgi.extender=osgi.servi
 celoader.registrar)(version>=1.0.0)(!(version>=2.0.0)))",osgi.ee;filt
 er:="(&(osgi.ee=JavaSE)(version=17))"

(notice the Java version)

[chrisrueger]

@royteeuwen I reverted for now. so 'annotation' is the default again and you need to have -metainf-services: auto in your <bnd>

I will investigate the failing tests with 'auto' default in the next days

[royteeuwen]

@chrisrueger I see you already pushed some commits in the meantime and all builds are green except the rebuild jdk17, is that still something you have to look at or should I already retest?

[chrisrueger]

@chrisrueger I see you already pushed some commits in the meantime and all builds are green except the rebuild jdk17, is that still something you have to look at or should I already retest?

Yeah the rebuild is on my list. Haven't managed to reproduce that locally.

If it's not too much hazzle you could retest. But i could also ping you when i know more about the rebuild.

[pkriens]

hmm. Maybe making this the default is breaking too much. Hate to do this to you but I guess when you contacted me last night I had a glass of wine and felt more bravado than today.

Wdyt, shall we make the default no automatic annotations? Sorry for the work :-(

[chrisrueger]

hmm. Maybe making this the default is breaking too much. Hate to do this to you but I guess when you contacted me last night I had a glass of wine and felt more bravado than today.

@pkriens I managed to fix the tests and build (https://github.com/bndtools/bnd/actions/runs/11235758156).
I squashed all my relevant commits into cfbef6f

The last and maybe important change was to make the new auto-registered annotation resolution: optional

Wdyt, shall we make the default no automatic annotations? Sorry for the work :-(

Could you please review this commit above and then let's decide if we change the default or not?

@royteeuwen could you retest too? do you see a problem with the Require-Capability haviing resolution: optional to fix errors in tests like Unresolved requirements: osgi.extender; (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0))) ?

[royteeuwen]

do you see a problem with the Require-Capability haviing resolution: optional to fix errors in tests like Unresolved requirements: osgi.extender; (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0))) ?

Not at all, seems even logical. Using the service loaders should be pluggable, if you don't need them you should be able to disable it :)

I'll see if I can get a test done tomorrow!

[chrisrueger]

@royteeuwen had a review with @pkriens
A few updates are about to come. So maybe wait with your test.

[chrisrueger]

Just sidenote for me how to build locally like build is done via Github Actions (at least that is what I figured)

./gradlew :build
./gradlew :gradle-plugins:build
./.github/scripts/rebuild-build.sh ; ./.github/scripts/rebuild-build.sh

[chrisrueger]

@royteeuwen I think I'm done and would appreaciate your test.
One little glitch:
you have to use -metainf-service: auto explicitly again.
Why? After a longer discussion we agreed that we are unsure if this is considered a too big (potentially breaking) change for some projects out there if they start switching to bnd 7.1.0, given the effect it might have on bundle resolution. So we decided to play it safe, so that -metainf-service: annotation is the default (if not set). This at least requires a concious decision by a developer to annotate the services files.
If you want 'auto' convenience you have to enable it.

There is still the option to make it the default in a future version of bnd if experience shows this options works.
At least it is minimal, e.g.

# bnd.bnd
-includeresource: @lib/groovy-3.0.22.jar
-metainf-services: auto

can be enough.

Note: biz.aQute.bnd.annotation is NOT required as a depency in you pom.xml. bnd automatically knows about the @ServiceProvider annotation under the hood.

@pkriens I (think) I implemented everything we talked about today.
See commit ca16c50 and the commits afterwards from today.

Regarding avoiding mixed-services we talked about:
I splitted the services into two parts:

• services with annotations
• and services without

1e130b2#diff-c50541e76ba2146c1d0ee544d6a8df58d45e1a7aaf8f3e1628143d08016e9bb0R58

This might not be the most elegant implementation of splitting, but all I could come up with today. I am open for suggestions.

[pkriens]

LGTM, let's merge

[royteeuwen]

@chrisrueger just tested it and works as expected, nice! Thanks a lot!

Any idea on when a new release would happen for the bnd plugin? I see the last release was already a year ago, so I'm not sure how often it happens?

[pkriens]

We had talked about a release before the OCX conference but that is not happening. We should have a release in the next few months.

[royteeuwen]

@pkriens @chrisrueger ok sad to hear, I also got a talk at adaptTo conference next week where I wanted to use the wrapped jars in the demo: https://adapt.to/2024/schedule/observability-in-aemaacs-with-opentelemetry

Guess I'll have to try and find a way to release them with the snapshot version of bnd 😅

[pkriens]

I am trying to find some funding for this work. Since I have no more customers in this area, there is a lack of customer pressure and financial incentive.