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

[marytts] Add bnd to make serviceloader aware of marytts impl #14064

Merged
merged 5 commits into from
Feb 19, 2023

Conversation

holgerfriedrich
Copy link
Member

This is required to for the Java serviceloader to work in OSGI environment. Allows for properly reloading the binding without issues.

Fixes #14056.

Signed-off-by: Holger Friedrich mail@holger-friedrich.de

@holgerfriedrich holgerfriedrich added the bug An unexpected problem or unintended behavior of an add-on label Dec 26, 2022
@lolodomo
Copy link
Contributor

I am not enough expert in that stuff to review and merge that change but I already ask the question if this is something to include in the future 3.4 patch ?

@holgerfriedrich
Copy link
Member Author

From my perspective: should be included, as users experience it during install of the add-on via UI.

But still we need someone with deeper knowledge in OSGI to check if this is the correct way to do it.

bundles/org.openhab.voice.marytts/bnd.bnd Outdated Show resolved Hide resolved
bundles/org.openhab.voice.marytts/bnd.bnd Outdated Show resolved Hide resolved
@holgerfriedrich
Copy link
Member Author

@wborn The PR no longer adds the file bnd.bnd; I added the configuration for Bnd to pom.xml. Starting with the defaults from

<bnd><![CDATA[Bundle-SymbolicName: ${project.artifactId}
, and adding

Require-Capability:
    osgi.extender:=
      filter:="(osgi.extender=osgi.serviceloader.processor)",
    osgi.serviceloader:=
      filter:="(osgi.serviceloader=marytts.config.MaryConfig)";
      cardinality:=multiple
SPI-Provider: marytts.config.MaryConfig
SPI-Consumer: java.util.ServiceLoader#load(java.lang.Class[marytts.config.MaryConfig])

it seems to work.

We have some duplication of code now. I did not manage to configure any of the mvn plugins to do it right.
The solution above is at least closer to how we build other bindings.

Bnd SPI annotation works fine for my PR for KNX to create the metadata file for the service loader.

@holgerfriedrich holgerfriedrich requested a review from a team as a code owner December 31, 2022 15:02
@holgerfriedrich holgerfriedrich force-pushed the pr-marytts-serviceloader branch 2 times, most recently from d0f60cb to c50e91d Compare December 31, 2022 15:09
@holgerfriedrich
Copy link
Member Author

holgerfriedrich commented Dec 31, 2022

@wborn I introduced a variable for Bnd defaults in main pom.xml. This can be used for all plugins which need to extend the Bnd configuration, e.g. for SPI. Hope it does not break anything else.

@holgerfriedrich holgerfriedrich marked this pull request as draft December 31, 2022 15:59
@holgerfriedrich holgerfriedrich added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 31, 2022
This is required to for the Java serviceloader to work in OSGI
environment. Allows for properly reloading the binding without issues.

Fixes openhab#14056.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Instructions for bnd now part of pom.xml.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@holgerfriedrich holgerfriedrich force-pushed the pr-marytts-serviceloader branch 7 times, most recently from 418cd2f to db4aa58 Compare January 1, 2023 21:19
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@holgerfriedrich holgerfriedrich force-pushed the pr-marytts-serviceloader branch from db4aa58 to 9f5e4cf Compare January 1, 2023 21:33
@holgerfriedrich holgerfriedrich marked this pull request as ready for review January 1, 2023 22:01
@holgerfriedrich holgerfriedrich added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 1, 2023
@holgerfriedrich holgerfriedrich force-pushed the pr-marytts-serviceloader branch from 9f5e4cf to ad3e27a Compare January 2, 2023 00:09
@holgerfriedrich holgerfriedrich force-pushed the pr-marytts-serviceloader branch from ad3e27a to bb5a165 Compare January 2, 2023 00:10
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@holgerfriedrich
Copy link
Member Author

Comment above still valid, my suggestion would be to merge it the way it is implemented now.

Any other suggestions for improvement?

@wborn
Copy link
Member

wborn commented Jan 14, 2023

I'll have a look if it is possible to reduce the amount of bnd instruction duplication.

@holgerfriedrich
Copy link
Member Author

@wborn this issue is still open, may I ask you to look into it again? Maybe we should go with the suggested approach, as currently MaryTTS (since 3.4) and KNX binding (for the coming 4.0M1) are affected by the SPI issue.
There is no duplicate code in the pom files anymore as a variable was introduced in the main pom file. Bnd instructions will be a bit more for the two affected bindings. Including the SPI capabilities could also be put into a macro, but I'm not sure if this helps as we need it only in two bindings....

If you see a better way using annotations which does not need additional Bnd configuration in pom, this of course would be better.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

I remember seeing some duplication. But it seems gone now. 🙂 Let's give it a test. 🚀

@wborn wborn merged commit 1da2694 into openhab:main Feb 19, 2023
@wborn wborn added this to the 4.0 milestone Feb 19, 2023
@holgerfriedrich holgerfriedrich deleted the pr-marytts-serviceloader branch February 19, 2023 14:43
@holgerfriedrich
Copy link
Member Author

Thanks, @wborn! We will see....

nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
…b#14064)

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
…b#14064)

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
…b#14064)

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[marytts] Fails to install on 3.4.0 release
3 participants