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

[kalleboll]

Fixes #7024
The problem was caused by the asynchttpclient dependency that seemed to require another version of the Netty dependency than what is added by the openhab.tp-netty feature.
This caused some of the classes in the ssl package to not be found during initialization.
I fixed it by upgrading the asynchttpclient dependency to a version that use the same version of Netty that is bundled(4.1.42.Final).

Signed-off-by: Jörgen Nilsson mail.jnilsson@gmail.com

[kalleboll]

A test build of the addon that includes the fix can be found here:
/~https://github.com/kalleboll/beta-test-binaries/raw/main/org.openhab.binding.tellstick-3.1.0-SNAPSHOT.jar

[cpmeister]

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

[kalleboll]

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.

[jannegpriv]

To my knowledge @jarlebh is no longer active in the openHAB community.
Code owner should be changed on this binding.

[kalleboll]

To my knowledge @jarlebh is no longer active in the openHAB community.
Code owner should be changed on this binding.

Ok.
Do you know how to initiate that?
Who should I talk to about it (If I should)?

[fwolter]

If @jarlebh isn't active anymore and you want to take over, you can simply change it in the CODEOWNERS file at the root of this repo.

[J-N-K]

I would prefer not to use <scope>compile</scope> here but <scope>provided</scope> and add the bundles to the feature.xml. The will be available at runtime then but could be shared with othe bindings requiring them. compile should be used for bundles that are unlikely to be shared by others or require a version that is incompatible with the one provided by the framework. This is not the case here. These are aditional bundles.

[kalleboll]

I would prefer not to use <scope>compile</scope> here but <scope>provided</scope> and add the bundles to the feature.xml. The will be available at runtime then but could be shared with othe bindings requiring them. compile should be used for bundles that are unlikely to be shared by others or require a version that is incompatible with the one provided by the framework. This is not the case here. These are aditional bundles.

I am not sure if I misunderstood you maybe, but are you sure? Because I couldn't find a single addon that has a dependency set to provided except for org.openhab.addons.bundles.. dependencies. I guess that is used for addons that depend on other addons.
Anyway I tried to change the dependencies I added to provided and add them as bundles to the feature file instead but then my addon does not load and I get errors like this:
`2021-01-17 03:01:29.695 [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)"

    at org.eclipse.osgi.container.Module.start(Module.java:444) ~[org.eclipse.osgi-3.12.100.jar:?]
    at org.eclipse.osgi.internal.framework.EquinoxBundle.start(EquinoxBundle.java:383) ~[org.eclipse.osgi-3.12.100.jar:?]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundle(DirectoryWatcher.java:1260) [bundleFile:3.6.4]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.startBundles(DirectoryWatcher.java:1233) [bundleFile:3.6.4]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.startAllBundles(DirectoryWatcher.java:1221) [bundleFile:3.6.4]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.doProcess(DirectoryWatcher.java:515) [bundleFile:3.6.4]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.process(DirectoryWatcher.java:365) [bundleFile:3.6.4]
    at org.apache.felix.fileinstall.internal.DirectoryWatcher.run(DirectoryWatcher.java:316) [bundleFile:3.6.4]

`

[J-N-K]

<feature name="openhab-binding-tellstick" description="Tellstick Binding" version="${project.version}">
		<feature>openhab-runtime-base</feature>
		<feature>openhab-transport-serial</feature>
		<feature dependency="true">openhab.tp-netty</feature>
		<feature dependency="true">openhab.tp-jaxb</feature>
		<bundle dependency="true">mvn:net.java.dev.jna/jna/4.5.2</bundle>
		<bundle dependency="true">mvn:net.java.dev.jna/jna-platform/4.5.2</bundle>
		<bundle dependency="true">mvn:io.netty/netty-transport-native-kqueue/4.1.42.Final</bundle>
		<bundle dependency="true">mvn:io.netty/netty-handler-proxy/4.1.42.Final</bundle>
		<bundle dependency="true">mvn:io.netty/netty-codec-socks/4.1.42.Final</bundle>
		<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.tellstick/${project.version}</bundle>
	</feature>

[J-N-K]

This is not needed if I compile locally?

[kalleboll]

No it is not needed when compiling but during runtime.

[J-N-K]

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.

[kalleboll]

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.

[J-N-K]

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.

[kalleboll]

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

[J-N-K]

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>

[kalleboll]

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.

[J-N-K]

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.

[kalleboll]

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.

[J-N-K]

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).

[kalleboll]

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

[J-N-K]

The provided thing is not working out as expected. I check that this does resolve the feature. Can you check and adapt? I'll fix the missing pom for the tellstick library in the repo.

[kalleboll]

<feature name="openhab-binding-tellstick" description="Tellstick Binding" version="${project.version}">
		<feature>openhab-runtime-base</feature>
		<feature>openhab-transport-serial</feature>
		<feature dependency="true">openhab.tp-netty</feature>
		<feature dependency="true">openhab.tp-jaxb</feature>
		<bundle dependency="true">mvn:net.java.dev.jna/jna/4.5.2</bundle>
		<bundle dependency="true">mvn:net.java.dev.jna/jna-platform/4.5.2</bundle>
		<bundle dependency="true">mvn:io.netty/netty-transport-native-kqueue/4.1.42.Final</bundle>
		<bundle dependency="true">mvn:io.netty/netty-handler-proxy/4.1.42.Final</bundle>
		<bundle dependency="true">mvn:io.netty/netty-codec-socks/4.1.42.Final</bundle>
		<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.tellstick/${project.version}</bundle>
	</feature>

[kalleboll]

The provided thing is not working out as expected. I check that this does resolve the feature. Can you check and adapt? I'll fix the missing pom for the tellstick library in the repo.

I accidently closed this PR but it is opened again now.
What 'thing' it not working? If you mean that the Jenkins build failed, then yea I noticed that. It seems to have failed due to some unrelated tests that does no seem to work. I have merged the latest from main so hopefully it will build successfully this time.
I have made some small changes according to the comments above.

[J-N-K]

I meant the provided scope I suggested earlier.

[J-N-K]

Did you check it works? If so, I‘ll merge tomorrow.

[kalleboll]

Did you check it works? If so, I‘ll merge tomorrow.

Yes I tested it after the last change as well and it seems to be working.