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

Multiple bundles for Guava and Jetty #892

Closed
5iver opened this issue Jun 23, 2019 · 39 comments
Closed

Multiple bundles for Guava and Jetty #892

5iver opened this issue Jun 23, 2019 · 39 comments

Comments

@5iver
Copy link

5iver commented Jun 23, 2019

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
Copy link
Contributor

I can confirm.

@J-N-K
Copy link
Member

J-N-K commented Jun 23, 2019

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
Copy link
Contributor

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
Copy link
Member

J-N-K commented Jun 23, 2019

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

@5iver
Copy link
Author

5iver commented Jun 23, 2019

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

@lolodomo
Copy link
Contributor

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
Copy link
Member

J-N-K commented Jun 23, 2019

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

@lolodomo
Copy link
Contributor

For okhttp, that is fully clear for me:

  • 2.3.0 is a dependency for the Netatmo binding
  • 3.12.1 is a dependency for the openHAB Cloud connector

@kaikreuzer
Copy link
Member

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

I've created #893 for it.

@kaikreuzer
Copy link
Member

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
Copy link
Contributor

maggu2810 commented Jun 23, 2019

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
Copy link
Member

wborn commented Jun 23, 2019

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
Copy link
Member

wborn commented Jun 23, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

lolodomo commented Jul 1, 2019

Any progress with Jetty in double?

@maggu2810
Copy link
Contributor

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

wborn added a commit to wborn/openhab-core that referenced this issue Jul 19, 2019
Resolves the Jetty bundle duplicates as reported in openhab#892.

Signed-off-by: Wouter Born <[email protected]>
cweitkamp pushed a commit that referenced this issue Jul 22, 2019
Resolves the Jetty bundle duplicates as reported in #892.

Signed-off-by: Wouter Born <[email protected]>
@lolodomo
Copy link
Contributor

Probably to be closed now that #934 is merged ?

@maggu2810
Copy link
Contributor

@openhab-5iver Is your issue gone?

@5iver
Copy link
Author

5iver commented Jul 25, 2019

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
Copy link
Contributor

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
Copy link
Contributor

@lolodomo
Copy link
Contributor

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

@maggu2810
Copy link
Contributor

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
Copy link
Contributor

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

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

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
Copy link
Member

J-N-K commented Aug 10, 2019

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
Copy link
Member

wborn commented Aug 10, 2019

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 openhab/openhab-webui#96. But it's probably quite some work.

@cweitkamp
Copy link
Contributor

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
Copy link
Member

J-N-K commented Aug 29, 2019

@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
Copy link
Member

wborn commented Aug 29, 2019

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

@kaikreuzer
Copy link
Member

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

@wborn
Copy link
Member

wborn commented Sep 3, 2019

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 hstaudacher/osgi-jax-rs-connector#199.

@maggu2810
Copy link
Contributor

@wborn Wouldn't it make more sense to migrate to the JAX-RS Whiteboard Pattern and use something like that #739 (comment)

@wborn
Copy link
Member

wborn commented Sep 4, 2019

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
Copy link
Contributor

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

@J-N-K
Copy link
Member

J-N-K commented Oct 17, 2019

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
Copy link
Member

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
Copy link
Contributor

lolodomo commented Feb 9, 2020

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:

  • netatmo => 2.3.0
  • openhabcloud => 3.8.1.1
  • ambientweather => 3.8.1.1
  • telegram => 3.12.5
  • (WP) linky => 3.12.5

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
Copy link
Member

J-N-K commented Apr 29, 2022

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.

@J-N-K J-N-K closed this as completed Apr 29, 2022
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this issue Jul 11, 2023
Resolves the Jetty bundle duplicates as reported in openhab#892.

Signed-off-by: Wouter Born <[email protected]>
GitOrigin-RevId: 657d63f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants