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

Parses META-INF/services files for annotations #5912

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

pkriens
Copy link
Member

@pkriens pkriens commented Dec 1, 2023

It is possible to add addnotions in the comments of the service files of the Service Loader.

Fixes #5903


Signed-off-by: Peter Kriens [email protected]

It is possible to add addnotions in the comments of
the  service files of the Service Loader.

Fixes bndtools#5903

---
 Signed-off-by: Peter Kriens <[email protected]>

Signed-off-by: Peter Kriens <[email protected]>
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot for working on this.

Adding the annotation comments to the services file is a clever idea to still have control about which services are processed and to handle the non default cases.

Just for clarification, in the simplest case I could to something like

META-INF/services/com.example.ServiceType:
   #import aQute.bnd.annotation.spi.ServiceProvider
   #@ServiceProvider
   com.example.impl.Impl2
   #@ServiceProvider
   com.example.impl.Impl1

or

META-INF/services/com.example.ServiceType:
   #@aQute.bnd.annotation.spi.ServiceProvider.ServiceProvider
   com.example.impl.Impl2
   #@aQute.bnd.annotation.spi.ServiceProvider.ServiceProvider
   com.example.impl.Impl1

and would have Impl1 and Impl2 registered?

And if I would do

META-INF/services/com.example.ServiceType:
   #@aQute.bnd.annotation.spi.ServiceProvider.ServiceProvider
   com.example.impl.Impl2
   com.example.impl.Impl1

... I only would have Impl2 registered respectively if the annotation were only above Impl1 only Impl1 would be registered?

static final Pattern IMPORT_P = Pattern
.compile("#import\\s+(?<fqn>" + FQN_S + ")\\s*;?\\s*$");
static final Pattern ANNOTATION_P = Pattern
.compile("#@(?<fqn>" + FQN_S + ")\\((?<attrs>.*)" + "\\)\\s*;?\\s*$");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make the braces optional if no attributes are specified for an annotation?
And naming the capturing group type or just name would avoid possible confusion with the IMPORT_P Pattern.

Suggested change
.compile("#@(?<fqn>" + FQN_S + ")\\((?<attrs>.*)" + "\\)\\s*;?\\s*$");
.compile("#@(?<type>" + FQN_S + ")(\\((?<attrs>.*))?" + "\\)\\s*;?\\s*$");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this, but if you change 'fqn', don't you then also not have to change the code where we get the group?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course the code below has to be adjusted.
In fact I mainly noticed this when reading the code below where it was first read like it would look up a fully-qualified name in the imports (which is of course not necessary), when it actually just looked up the class-name.

I just wanted to keep the suggestion short. :)

/**
* Get the comments that preceded this implementation definition
*/
public Optional<String> getComments() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used?

Copy link
Member Author

@pkriens pkriens Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, but it is important information for use. I always make my code handle a use case but try to stay away from the specifics. I compare it with drawing a rectangle around a messy curvy perimeter of requirements.

I find it very important that all code is refective so outsiders can use it in many different ways. Notice that this class has no clue about Service Provider

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I just wanted to make sure it is not left-over from early development steps and ended up here accidentally.

@pkriens
Copy link
Member Author

pkriens commented Dec 2, 2023

I've played with the idea of automatically repeating the annotations but decided against it. I think it is confusing and it would require a new mechanism to stop the repetition.

And this is a rather bad thing anyway. Using the Java annotation is so much cleverer. It gets you all the IDE help, it adds your type to the services file, and the compiler verifies, you can use constants, etc. It is pretty dumb to use this over the real annotations. The only reason I worked on this is because there are a lot of people out there that prefer to cut off their nose to spite their face :-(

It would be nice if we had a good article somewhere to explain what the annotations do and how useful they are outside OSGi. After all, most open source programs actually use the bnd mvn plugin already. Why not use it to maintain your META-INF/services directory?

@pkriens
Copy link
Member Author

pkriens commented Dec 2, 2023

The key advantage of using the annotations is that the the actual manifest code as orchestrated by the ServiceProvider and its Capability and Requirement annotations is used.

You realize that you can make a custom application annotation in a similar way?

@HannesWell
Copy link
Contributor

I've played with the idea of automatically repeating the annotations but decided against it. I think it is confusing and it would require a new mechanism to stop the repetition.

That's right.
When requesting it in #5903, I only wanted to cover the most basic use-cases where all services should have the @ServiceProvider annotation, without any attributes different from the default well knowing that it would have limitations (all services would be processed and not attribute configuration possible).
Your approach is a good idea to make this much more powerful.

And this is a rather bad thing anyway. Using the Java annotation is so much cleverer. It gets you all the IDE help, it adds your type to the services file, and the compiler verifies, you can use constants, etc. It is pretty dumb to use this over the real annotations. The only reason I worked on this is because there are a lot of people out there that prefer to cut off their nose to spite their face :-(

Fully agree, but I can only speculate about why other projects are reluctant to use them.
Maybe it is because OSGi is not the hottest topic anymore or maybe because they don't know BND and it is often in the human nature to have some distrust in foreign things. I don't know.

It would be nice if we had a good article somewhere to explain what the annotations do and how useful they are outside OSGi. After all, most open source programs actually use the bnd mvn plugin already. Why not use it to maintain your META-INF/services directory?

Absolutely. That would be great. I'm trying to contribute OSGi metadata to the libraries and projects I'm getting in touch (slowly but steadily) and therefore it would be great to have something to reference instead of explaining it again and again.

You realize that you can make a custom application annotation in a similar way?

I didn't, but you are right. But can you tell a use-case where I would need that? I would need my own processor of these annotations to use it, wouldn't I?

@laeubi
Copy link
Contributor

laeubi commented Dec 3, 2023

It would be nice if we had a good article somewhere to explain what the annotations do and how useful they are outside OSGi. After all, most open source programs actually use the bnd mvn plugin already. Why not use it to maintain your META-INF/services directory?

Actually there are a few approaches already, for example https://metainf-services.kohsuke.org/ and also some maven-plugin was to do it here and there.

What mostly bugs people I think is that these are bnd specific and the don't (and don't want to) know about OSGi and all that stuff. So it might help to have these @MetaInfServices like one or similar a separate artifact so they don't need to reference full bnd lib to use the annotation, if then there would be a tiny maven plugin similar to service-generator that just do the annotation processing it could be another pice in the puzzle

---
 Signed-off-by: Peter Kriens <[email protected]>

Signed-off-by: Peter Kriens <[email protected]>
@pkriens
Copy link
Member Author

pkriens commented Dec 4, 2023

@pkriens pkriens merged commit 58b8818 into bndtools:master Dec 4, 2023
10 checks passed
@HannesWell
Copy link
Contributor

Thank you for this PR. :)

@pkriens
Copy link
Member Author

pkriens commented Dec 5, 2023

Can you take a look at the article, suggest improvements if anything is missing? If you let me know if it is ok, I will send a post on X.

chrisrueger added a commit to chrisrueger/bnd that referenced this pull request Oct 3, 2024
this is an experiment based on an idea mentioned in bndtools#6304

it leverages the recent addition of PR bndtools#5912 that allowed to add annotations in comments of files in META-INF/services. But in this PR we basically pretent and add a aQute.bnd.annotation.spi.ServiceProvider annotation artificially. this causes bnd to generate Provide-Capability manifest headers for the services

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

Signed-off-by: Christoph Rueger <[email protected]>
chrisrueger added a commit to chrisrueger/bnd that referenced this pull request Oct 7, 2024
it leverages the recent addition of PR bndtools#5912 that allowed to add annotations in comments of files in META-INF/services. But in this PR we basically pretent and add a aQute.bnd.annotation.spi.ServiceProvider annotation artificially. this causes bnd to generate Provide-Capability manifest headers for the services

Signed-off-by: Christoph Rueger <[email protected]>
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.

Derive serviceloader capabilities and requirements from hand-crafted services files
3 participants