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.

Sign up for GitHub

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

[tellstick] Fixes a NoClassDefFoundError that caused the Tellstick addon to not work properly #9634

Merged
merged 6 commits into from
Jan 20, 2021
Merged

[tellstick] Fixes a NoClassDefFoundError that caused the Tellstick addon to not work properly #9634

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a6a869
Fixes a NoClassDefFoundError that caused the Tellstick addon to not w…
kalleboll Jan 1, 2021
adb08c2
Changed maintainer to the repo maintainers team as jarlebh is not act…
kalleboll Jan 17, 2021
8e8c71c
Merge branch 'main' into 7024-Tellstick-addon-not-working-with-the-la…
kalleboll Jan 18, 2021
a273149
Minor fix after code review and fixed some problems with newlines.
kalleboll Jan 18, 2021
4c35e5a
Minor fix after code review and changed the newlines back to the way …
kalleboll Jan 19, 2021
d059a45
Another minor fix after code review.
kalleboll Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@
/bundles/org.openhab.binding.tankerkoenig/ @dolic @JueBag
/bundles/org.openhab.binding.telegram/ @ZzetT
/bundles/org.openhab.binding.teleinfo/ @Nokyyz
/bundles/org.openhab.binding.tellstick/ @jarlebh
/bundles/org.openhab.binding.tellstick/ @openhab/add-ons-maintainers
/bundles/org.openhab.binding.tesla/ @kgoderis
/bundles/org.openhab.binding.tibber/ @kjoglum
/bundles/org.openhab.binding.touchwand/ @roieg
Expand Down
24 changes: 21 additions & 3 deletions bundles/org.openhab.binding.tellstick/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<name>openHAB Add-ons :: Bundles :: Tellstick Binding</name>

<properties>
<bnd.importpackage>!com.luckycatlabs.*,!com.jcraft.jzlib.*,!org.apache.commons.cli.*,!org.eclipse.swt.*</bnd.importpackage>
<bnd.importpackage>!com.luckycatlabs.*,!com.jcraft.jzlib.*,!org.apache.commons.cli.*,!org.eclipse.swt.*,!javax.activation</bnd.importpackage>
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed if I compile locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not needed when compiling but during runtime.

Copy link
Member

Choose a reason for hiding this comment

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

If it's needed during runtime you must not exclude it. Instead you need to add the required dependency. But I believe that is already present in the target platform anyway, otherwise the karaf plugin should fail to resolve the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I meant that the change is not needed to be able to compile but it is there to sort out a runtime issue. So in fact it is the opposite, the new version of async-http-client has javax.activation as a transitive dependency but this addon does not seem to use those parts of the async-http-client library so that is why I excluded javax.activation rather than adding the dependency for it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. Fine if it works. The other option would be to add

mvn:org.apache.servicemix.specs/org.apache.servicemix.specs.activation-api-1.2.1/1.2.1_2

to the feature. That is activation API we use in other bundles. But fine with me if it works with the exclusion. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea maybe that is a nicer solution even though it is not used.
I added that bundle as well and removed the exclude.

<dep.noembedding>netty-transport-native-unix-common,netty-common,netty-transport,netty-transport-native-epoll,netty-buffer,netty-resolver,netty-codec,netty-codec-http,netty-handler</dep.noembedding>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<dep.noembedding>netty-transport-native-unix-common,netty-common,netty-transport,netty-transport-native-epoll,netty-buffer,netty-resolver,netty-codec,netty-codec-http,netty-handler</dep.noembedding>
<dep.noembedding>netty-codec-socks,netty-handler-proxy,netty-transport-native-kqueue,netty-transport-native-unix-common,netty-common,netty-transport,netty-transport-native-epoll,netty-buffer,netty-resolver,netty-codec,netty-codec-http,netty-handler</dep.noembedding>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea sure I can add the dependencies as bundles to the feature file like you wrote above. In fact I had it like that first but I removed it again because I did not see a need for it. Anyway, I have added them back now.
But why would I want to add them to the dep.noembedding as well?
Then what is the point of adding them as a bundle in the first place?
Remember that these are not dependencies that are bundled with the openhab.tp-netty feature so they will not be added that way, like I have commented on an earlier review comment.
Just for the sake of it I tried to add them to the dep.noembedding like you suggested but then I get this error:
2021-01-18 16:54:38.990 [WARN ] [org.apache.felix.fileinstall ] - Error while starting bundle: file:/openhab/addons/org.openhab.binding.tellstick-3.1.0-SNAPSHOT.jar org.osgi.framework.BundleException: Could not resolve module: org.openhab.binding.tellstick [208] Unresolved requirement: Import-Package: io.netty.channel.kqueue; version="[4.1.0,5.0.0)"

I took the opportunity to change all the line endings in this and another xml file as it was causing some weird spotless errors for me and I noticed that no other addon had line endings like these files had.

Copy link
Member

Choose a reason for hiding this comment

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

If you only add the tellstick bundle, it'll not work (the feature is not part of the tellstick bundle but compiled to a feature.xml with all the other bindings. If you decide to install a binding (via addons.cfg or UI) you are not installing the binding-bundle but the binding feature. This contains the bundle and (like in this case) also other bundles or other features).

If you didn't manually install the additonal bundles, the tellstick bundle can't be resolved. That it why it needs to be added to the feature so karaf knows during installation of the tellstick binding it also needs to install the other bundles.

Regarding you other questions: embedding is not the preferred way in an OSGi environment. At best we would have only proper OSGi bundles and don't need to embed bundles at all but add all bundles to the feature. Unfortunately we don't live in such a world. We have some bundles that are not OSGi bundles at all (e.g. javatellstick) or use improper versioning (e.g. incompatible API even if the minor version did not change). Therefore we decided to use embedding for bundles that are either not compatible, very unlikely will be used by other bindings or use different versions than other bindings. To automate that, we embed all bundles with the scope "compile". This scope is also needed for bnd to resolve the bundle in the IDE. That's why we add e.g. netty with scope "compile" to the pom and exclude it from embedding. Netty is used by different bindings (that's why we have the tp.netty feature to keep the used version inline for all binings).

Hope that makes it more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think that makes it a bit clearer.
But I guess it would still technically have worked the way I had it first, even when installing via the UI, as those dependencies was bundled together in the jar file, since the compile scope is used. I mean then it must be on the classpath. But yea I understand if we want to handle "common" dependencies in a special way.

So ok I have now added the three bundles to the dep.noembedding as well.
But after those changes I could not get the binding to work by just dropping it in the addon directory like before.
I read up about Karaf and the OpenHAB console and I managed to get the binding to work by installing the new bundles the binding requires via the console like this:
bundle:install mvn:io.netty/netty-transport-native-kqueue/4.1.42.Final
bundle:install mvn:io.netty/netty-handler-proxy/4.1.42.Final
bundle:install mvn:io.netty/netty-codec-socks/4.1.42.Final

Is this the preferred way to do it? For example if I want someone else to try out an addon before it is released they have to install the required bundles manually first like this?
I searched for some info regarding this but most posts I could find just said to drop the jar in the addon directory but maybe that was for bindings that did not have these kinds of dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

It's ok to do it that way. But: if it's an update to an existing binding, the preferred way would be to use the bundle-update command (e.g. for tellstick):

update org.openhab.binding.tellstick file:///home/myuser/org.openhab.binding.tellstick-3.1.0-SNAPSHOT.jar

If the bundle is available for download, you can also use

update org.openhab.binding.tellstick https://myserver.mydomain/path/to/file/org.openhab.binding.tellstick-3.1.0-SNAPSHOT.jar

That way only the bundle itself is updated and all dependency stay installed (unlike in case of deinstalling the binding, then the whole feature, so the bundle and it's dependencies if not needed by other bundles, are uninstalled).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that is a good to know.
Thanks for the tip.

</properties>

Expand All @@ -36,13 +36,13 @@
<dependency>
<groupId>org.asynchttpclient</groupId>
<artifactId>async-http-client</artifactId>
<version>2.0.19</version>
<version>2.10.4</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.asynchttpclient</groupId>
<artifactId>async-http-client-netty-utils</artifactId>
<version>2.0.19</version>
<version>2.10.4</version>
<scope>compile</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -105,6 +105,24 @@
<version>4.1.42.Final</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-transport-native-kqueue</artifactId>
<version>4.1.42.Final</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
<version>4.1.42.Final</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-codec-socks</artifactId>
<version>4.1.42.Final</version>
<scope>compile</scope>
</dependency>
Comment on lines +108 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you including these dependencies in the binding jar if they are already included in the runtime environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are new dependencies required by the new version of async-http-client.
As far I can tell these are not already included by the openhab.tp-netty feature so I had to add them here specifically.
Not all Netty dependencies are included in that feature and my guess is that only the most used once have been added there so far.
My understanding of the build system is very limited so far by I think I found the openhab.tp-netty definition in this file openhab-core\features\karaf\openhab-tp\src\main\feature\feature.xml. Currently on line 120.
There the included Netty dependencies can be found and they are; netty-buffer, netty-common, netty-codec, netty-codec-http, netty-codec-mqtt, netty-handler, netty-resolver, netty-transport, netty-transport-native-epoll and netty-transport-native-unix-common.

I am quite sure that it will work as well if we add these three dependencies, that I need, to the openhab.tp-netty feature instead but I guess that that will impact all other addons that rely on that feature and it would require a change in the openhab-core. That works for me but as I am unsure of the impact of such a change I think someone with a better knowledge of the build/feature/dependency setup should make that call.

<dependency>
<groupId>org.reactivestreams</groupId>
<artifactId>reactive-streams</artifactId>
Expand Down