search

openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
912 stars 420 forks source link

Multiple bundles for Guava and Jetty #892

Closed 5iver closed 2 years ago

5iver commented 5 years ago

I'm seeing this in S1621...

 22 │ Active │  80 │ 18.0.0                │ Guava: Google Core Libraries for Java
 23 │ Active │  80 │ 21.0.0                │ Guava: Google Core Libraries for Java
 79 │ Active │  80 │ 9.4.11.v20180605      │ Jetty :: Http Utility
 80 │ Active │  80 │ 9.4.12.v20180830      │ Jetty :: Http Utility
 99 │ Active │  80 │ 9.4.11.v20180605      │ Jetty :: Websocket :: Common
100 │ Active │  80 │ 9.4.12.v20180830      │ Jetty :: Websocket :: Common
lolodomo commented 5 years ago

I can confirm.

J-N-K commented 5 years ago

For Guava this might be due to installed bindings which depend on 3rd-Party libraries which in turn depend on different versions of Guava. Not much we can do about that. For jetty we should find out why this happens.

lolodomo commented 5 years ago

Other bundles I see in double:

157 │ Active │  80 │ 2.5.0.201906210315    │ openHAB Core :: Bundles :: REST Interface
158 │ Active │  80 │ 2.5.0.201906210319    │ openHAB Core :: Bundles :: REST Interface

and

211 │ Active │  80 │ 2.3.0                 │ OkHttp
212 │ Active │  80 │ 3.12.1                │ OkHttp
J-N-K commented 5 years ago

OkHttp is definitely due to external libraries. This can‘t be solved. Two REST bundles is wrong, IMO.

5iver commented 5 years ago

I can confirm the multiple REST Interface, but only have one OkHttp (3.12.1).

lolodomo commented 5 years ago

In fact, for REST Interface, this is only a naming problem because the bundles are different:

157 │ Active │  80 │ 2.5.0.201906210315    │ org.openhab.core.io.rest
158 │ Active │  80 │ 2.5.0.201906210319    │ org.openhab.core.io.rest.core
J-N-K commented 5 years ago

@openhab-5iver it depends on the installed bindings. The bundles are only installed when needed.

lolodomo commented 5 years ago

For okhttp, that is fully clear for me:

kaikreuzer commented 5 years ago

In fact, for REST Interface, this is only a naming problem because the bundles are different

I've created https://github.com/openhab/openhab-core/pull/893 for it.

kaikreuzer commented 5 years ago

Wrt Jetty in two versions: @maggu2810 & @wborn, you recently discussed Jetty versions in the context of the Karaf update. Could this be related to Karaf bringing a different version than what our TP features define?

maggu2810 commented 5 years ago

In the long run it would be much simpler to depend on a Karaf feature that provides the Jetty bundles e.g. the standard Karaf jetty one or the Pax Web pax-jetty one. As we already use Pax Web, why not simple depend on pax-jetty? in < 4.2.7 (not released) we will just need to add the jetty-proxy one. In 4.2.7 jetty-proxy will be part of the respective features (I asked JB for the inclusion some weeks before).

wborn commented 5 years ago

We probably still need to update openhab-tp jetty version.

For me with the Demo package installed the Guava 18 bundle is a dependency of the swagger bundles used by the REST Documentation:

openhab> bundle:capabilities 22
com.google.guava_18.0.0 [22] provides:
--------------------------------------
osgi.wiring.bundle; com.google.guava 18.0.0 [UNUSED]
osgi.wiring.host; com.google.guava 18.0.0 [UNUSED]
osgi.identity; com.google.guava 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.net 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.html 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.collect 18.0.0 required by:
   io.swagger.core_1.5.8 [217]
   io.swagger.jaxrs_1.5.8 [218]
osgi.wiring.package; com.google.common.primitives 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.base 18.0.0 required by:
   io.swagger.jaxrs_1.5.8 [218]
...
wborn commented 5 years ago

The issue with the Jetty bundles is fixed on a local build when I update the jetty.version to 9.4.18.v20190429 in both the openhab-core and openhab-tp feature POMs. Would that give issues with your setup @maggu2810 and will it be resolved with the proxy? The versions in these POMs are currently already out of sync.

maggu2810 commented 5 years ago

We probably still need to update openhab-tp jetty version.

No, we would no longer need to specify a Jetty version for our features at all, as we consume what Karaf or to be more precise, the used Jetty feature provides.

maggu2810 commented 5 years ago

Would that give issues with your setup @maggu2810

No, I am already using 4.2.6.

and will it be resolved with the proxy?

Perhaps my comment above has not bean clear.

Jetty Proxy has not been part of any feature repo (Karaf standard and Pax Web) that contains the other jetty bundles.

So, if someone wants to use Jetty Proxy he needs to bundle the respective code in their bundles or install the Jetty Proxy bundle by a custom feature itself (so, create a custom feature).

I requested on the Karaf mailaing list shortly after 4.2.6 has been released, if it would be possible to add that bundle to the existing Jetty features. It has been added and so no custom feature anymore needed.

lolodomo commented 5 years ago

Any progress with Jetty in double?

maggu2810 commented 5 years ago

I don't think so. At least I was not aware of any contribution.

lolodomo commented 5 years ago

Probably to be closed now that #934 is merged ?

maggu2810 commented 5 years ago

@openhab-5iver Is your issue gone?

5iver commented 5 years ago

Using S1645 (the latest distro from 2 days ago), I still see 2 guavas...

 22 │ Active │  80 │ 18.0.0                │ Guava: Google Core Libraries for Java
 23 │ Active │  80 │ 21.0.0                │ Guava: Google Core Libraries for Java
 22 │ Active │  80 │ 18.0.0                │ com.google.guava
 23 │ Active │  80 │ 21.0.0                │ com.google.guava
openhab> bundle:capabilities 22
com.google.guava_18.0.0 [22] provides:
--------------------------------------
osgi.wiring.bundle; com.google.guava 18.0.0 [UNUSED]
osgi.wiring.host; com.google.guava 18.0.0 [UNUSED]
osgi.identity; com.google.guava 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.net 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.html 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.collect 18.0.0 required by:
   io.swagger.core_1.5.8 [233]
   io.swagger.jaxrs_1.5.8 [234]
osgi.wiring.package; com.google.common.primitives 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.base 18.0.0 required by:
   io.swagger.jaxrs_1.5.8 [234]
osgi.wiring.package; com.google.common.escape 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.cache 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.eventbus 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.util.concurrent 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.hash 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.io 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.xml 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.reflect 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.math 18.0.0 [UNUSED]
osgi.wiring.package; com.google.common.annotations 18.0.0 [UNUSED]
openhab> bundle:capabilities 23
com.google.guava_21.0.0 [23] provides:
--------------------------------------
osgi.wiring.bundle; com.google.guava 21.0.0 required by:
   org.eclipse.xtext.util_2.17.0.v20190304-0545 [110]
   org.eclipse.xtext.xbase.lib_2.17.0.v20190304-0518 [113]
   org.eclipse.xtext.common.types_2.17.0.v20190304-0626 [108]
osgi.wiring.host; com.google.guava 21.0.0 [UNUSED]
osgi.identity; com.google.guava 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.annotations 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.base 21.0.0 required by:
   org.eclipse.lsp4j_0.6.0.v20181130-0903 [103]
   org.openhab.core.model.script_2.5.0.201907230308 [171]
   org.openhab.core.model.rule_2.5.0.201907230309 [168]
   org.openhab.core.model.persistence_2.5.0.201907230306 [165]
osgi.wiring.package; com.google.common.cache 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.collect 21.0.0 required by:
   org.openhab.core.model.script.ide_2.5.0.201907230327 [172]
   org.eclipse.lsp4j_0.6.0.v20181130-0903 [103]
   org.openhab.core.model.thing.ide_2.5.0.201907230329 [178]
   org.openhab.core.model.sitemap.ide_2.5.0.201907230328 [175]
   org.openhab.core.model.rule.runtime_2.5.0.201907230310 [170]
   org.openhab.core.model.rule.ide_2.5.0.201907230327 [169]
   org.openhab.core.model.item.ide_2.5.0.201907230325 [162]
   org.openhab.core.model.persistence.ide_2.5.0.201907230326 [166]
   org.openhab.core.io.rest.sitemap_2.5.0.201907230320 [155]
osgi.wiring.package; com.google.common.escape 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.eventbus 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.graph 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.hash 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.html 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.io 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.math 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.net 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.primitives 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.reflect 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.util.concurrent 21.0.0 [UNUSED]
osgi.wiring.package; com.google.common.xml 21.0.0 [UNUSED]
lolodomo commented 5 years ago

IO REST sitemap is using Google Guava here: https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/eclipse/smarthome/io/rest/sitemap/internal/SitemapResource.java#L149 I don't know why MapMaker() was used but I guess we can probably get rid of it in that place ?

maggu2810 commented 5 years ago

See https://github.com/eclipse/smarthome/pull/6449 and https://github.com/eclipse/smarthome/issues/6460

lolodomo commented 5 years ago

Ok so that is ok like that with Guava ? And the doubled jetty has gone.

maggu2810 commented 5 years ago

It is at least known. If someone ever comes with another weak cache usage, I assume we are happy to drop that dependency. I assume that's also part of code that blocks the JAX-RS whitepattern usage currently.

cweitkamp commented 5 years ago

One more dependency (Jackson) with different version:

270 │ Active │  80 │ 2.4.5                 │ Jackson-annotations
271 │ Active │  80 │ 2.4.5                 │ Jackson-core
272 │ Active │  80 │ 2.4.5                 │ jackson-databind
...
286 │ Active │  80 │ 2.9.8                 │ Jackson-annotations
287 │ Active │  80 │ 2.9.8                 │ Jackson-core
288 │ Active │  80 │ 2.9.8                 │ jackson-databind

https://github.com/openhab/openhab-core/blob/b6aae6907f3d247e5d590bb25f6cb009e17584f8/features/karaf/openhab-tp/src/main/feature/feature.xml#L252-L254

and some bindings like Chromecast:

        <bundle dependency="true">mvn:com.fasterxml.jackson.core/jackson-annotations/2.9.8</bundle>
        <bundle dependency="true">mvn:com.fasterxml.jackson.core/jackson-core/2.9.8</bundle>
        <bundle dependency="true">mvn:com.fasterxml.jackson.core/jackson-databind/2.9.8</bundle>

See also https://github.com/openhab/openhab2-addons/search?l=XML&q=jackson-core

J-N-K commented 5 years ago

We need to upgrade Jackson to 2.9.9 (see security advice). I guess that this is easy for the 2.9.x bundles but probably not for 2.4.5.

wborn commented 5 years ago

Can we also use the same latest jackson 2.9.x with all add-ons (and osgiified bundles)?

Syncing the versions would decrease downloads/disk space/KAR files/installation time and fix vulnerabilities. Although there's always the possibility something gets broken.

There's probably also going to be a 2.9.10 release to address even more vulnerabilities. :-|

The most recent Swagger versions also use Jackson 2.9.x so if we'd upgrade to that we may be able to remove Jackson 2.4.5 from core and also fix https://github.com/openhab/openhab-webui/issues/96. But it's probably quite some work.

cweitkamp commented 5 years ago

A first step for simplification is to introduce a feature for Jackson provided by OHC which can be used in other repositories (like we did in #920 for JAXB and JAX-WS).

J-N-K commented 5 years ago

@wborn, did you try whether the current swagger 1.5.8 works with jackson 2.9? The manifest specifies [2.4.3), so technically 2.9 should work. The jackson developers say they try to keep minor version binary compatible, but don't guarantee that. Maybe it worth to try. The first swagger version that requires 2.9 is 1.5.18.

wborn commented 5 years ago

I haven't tested it but the swagger-core release notes state that they use Jackson 2.9.9 nowadays.

kaikreuzer commented 5 years ago

@wborn Would that mean that all related issues such as https://github.com/openhab/openhab-webui/issues/96 could be solved by upgrading swagger-core then?

wborn commented 5 years ago

I think the REST Docs should work again when the Swagger bundles are updated.

A full update may also depend on updating the com.eclipsesource.jaxrs.provider.swagger dependencies (see its MANIFEST.MF). Though that project seems to be no longer active.

If we consider forking osgi-jax-rs-connector we might first want to see if we'd want to use/fix the OpenNMS fork. According to its documentation:

The original work was modified in a way, that it fits the needs at the OpenNMS project and may not work outside of an Apache Karaf container anymore.

There's also a PR for updating Jersey to 2.25.1 in the original repository https://github.com/hstaudacher/osgi-jax-rs-connector/pull/199.

maggu2810 commented 5 years ago

@wborn Wouldn't it make more sense to migrate to the JAX-RS Whiteboard Pattern and use something like that https://github.com/openhab/openhab-core/pull/739#issuecomment-520151528

wborn commented 5 years ago

That would be nice indeed if it all works. There's probably still a lot to be done before that PR can be merged. Another option could be to do a minor bugfix/fork where we just correct the manifest entries so we make sure the Swagger bundles don't start using the Jackson 2.9.x bundles.

cweitkamp commented 4 years ago

FTR: Jackson has been updated to version 2.9.10 in #1128.

J-N-K commented 4 years ago

Are multiple guava bundles really an issue? Usually the import statements are [x.y;<x+1>.0), so a bundle using guava 18 should not have a problem with guava 21 (this was different with jackson, where [2.4;3) matches both 2.4.5 and 2.9.9 which are not binary compatible.

kaikreuzer commented 4 years ago

Guava lib is huge, so we should definitely try to keep the number of versions low. If we have parts in our runtime that require different versions and there isn't any suitable solution to make them use the same, I'd be ok to live with multiple (hopefully <3) bundles of Guava.

lolodomo commented 4 years ago 
212 x Active x  80 x 2.3.0                   x com.squareup.okhttp.okhttp
233 x Active x  80 x 3.8.1.1                 x org.apache.servicemix.bundles.okhttp

I am running OH 2.5.1. I am a little surprised to see version 3.8.1.1 which is the version used by the openhabclould binding. As you can see above in this issue, it was previously 3.12.1. What was the reason to reduce the version ?

I found 5 bindings depending on okhttp:

Do you think, it will be possible and a good idea to move all (except netatmo) to the same verison 3.12.5 ?

J-N-K commented 2 years ago

As far as I can see there is nothing we can do in core in that regard ATM. We still have two versions of Guava (on my system 27/30), but since these are incompatible and are added because 3rd party code depends on them, we have to live with that.