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

Allow annotation properties to be String instead of Enum #5497

Closed
TomasTokaMrazek opened this issue Jan 10, 2023 · 10 comments
Closed

Allow annotation properties to be String instead of Enum #5497

TomasTokaMrazek opened this issue Jan 10, 2023 · 10 comments
Assignees
Labels
cherry-pick Candidate to cherry-pick to next

Comments

@TomasTokaMrazek
Copy link

Class compilation via javac with enum annotations produce warnings and potentially erros, which break builds, if bnd is not in the classpath. Adding a dependency is not always wanted. See discussion here FasterXML/woodstox#163 and here FasterXML/woodstox#155

@pkriens
Copy link
Member

pkriens commented Feb 20, 2023

I do not think there is a lot we can do here? The mentioned annotations in the woodstox code is very old. We now use the OSGi annotations and they do have the enums. We cannot change those files.

@TomasTokaMrazek
Copy link
Author

Honestly I do not have such deep knowledge about OSGi, I created the ticket as a reference to this comment where talk with BND developers is mentioned. FasterXML/woodstox#155 (comment)

Anyways, as I understand the issue, it's not about what you use, but what you provide. More discussion here. FasterXML/jackson-core#768

The point is, that those libraries cannot use SPI annotations to generate ServiceLoader OSGi metadata. Using enum forces BND dependency, which breakscompatibility, as stated in the comments. Changing Resolution enum to String would apparently not force the BND dependency on library consumers, making it backwards compatible and with proper OSGI metadata to support SPI.

Are there any other possible solutions to this problem?

@kriegfrj
Copy link
Contributor

kriegfrj commented Feb 22, 2023

@pkriens wrote:

I do not think there is a lot we can do here? The mentioned annotations in the woodstox code is very old. We now use the OSGi annotations and they do have the enums. We cannot change those files.

The annotation in question is @ServiceProvider . I do not think that this one has an OSGi equivalent? It's not part of the DS spec, unless I'm mistaken. So it should be possible for us to implement, if we choose.

@TomasTokaMrazek wrote:

Are there any other possible solutions to this problem?

In the case of Jackson, an alternative solution is to configure the pom.xml file to add the necessary metadata instead of using the annotations (as mentioned here). It is slightly less inconvenient to do it this way, but it works fine.

@pkriens
Copy link
Member

pkriens commented Feb 23, 2023

@bjhargrave didnt you already do something similar on the codebase? Can we make it backward compatible?

@bjhargrave
Copy link
Member

70eae7e

were the changes I made for Bnd. I modified the annotations to use Strings with the enum name. You will need to make sure you don't use the enums and use Strings holding the enum names instead.

@pkriens pkriens self-assigned this Feb 24, 2023
@pkriens pkriens removed the waiting label Feb 24, 2023
@pkriens pkriens added the cherry-pick Candidate to cherry-pick to next label Mar 10, 2023
@pkriens
Copy link
Member

pkriens commented Mar 16, 2023

@TomasTokaMrazek could you take a look at PR #5596 and possibly test this code?

@pkriens
Copy link
Member

pkriens commented Mar 16, 2023

I've commit this on the master branch and cherry picked it on the classic branch. When we release 6.5 it will be included

@TomasTokaMrazek
Copy link
Author

@pkriens Quick question, what exactly means that Component relative annotations are removed? Would OSGi DS generation in projects still work? I personally am forced by WSO2 to use DS version 1.2, but AFAIK it was not removed from upstream.

As for test, unfortuantely I don't know a way, how to test it. I probably could update Woodstox with the new String enums and try javac. Hopefully I'll have time on the weekend.

@pkriens
Copy link
Member

pkriens commented Mar 17, 2023

I just committed the classic branch. These are pushed to the snapshots as 6.5.0-SNAPSHOTS if everything works

@pkriens pkriens closed this as completed Mar 17, 2023
@pkriens
Copy link
Member

pkriens commented Mar 20, 2023

We deprecated the Component annotations in the bnd namespace years ago. Let me know. I can restore them for the 6.5 classic branch if you need them. Their processing is still supported by bnd anyway and I do not see a reason to ever remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick Candidate to cherry-pick to next
Projects
None yet
Development

No branches or pull requests

4 participants