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

Bump Kotlin, okhttp, okio, and java-telegram-bot-api libraries #16458

Merged
merged 6 commits into from
Mar 29, 2024

Conversation

Skinah
Copy link
Contributor

@Skinah Skinah commented Feb 25, 2024

Update various libraries to their latest versions, and also make it easier to keep multiple bindings in sync with the latest and same version. No need to use 3+ different versions of the same library.

Updating java-telegram-bot-api to tackle some of the Telegram bugs and feature requests.

Signed-off-by: Matthew Skinner <matt@pcmus.com>
@Skinah Skinah requested review from GiviMAD, lujop, cpmeister and a team as code owners February 25, 2024 06:54
@stefan-hoehn
Copy link
Contributor

out of curiosity: why do we have kotlin in our pom.xml? AFAIK Kotlin as a language itself not allowed to be used in OH. Are there libraries that we use and require kotlin?

@Skinah
Copy link
Contributor Author

Skinah commented Feb 25, 2024

I am not the person to ask as I do not know enough about Java.... However I can point toward this info:
https://square.github.io/okhttp/changelogs/upgrading_to_okhttp_4/
The okhttp3 lib has not had any updates in 4 years now as the lib has moved to Kotlin and any bug and security fixes are only being made in the okhttp4 Kotlin branch. To keep everything compatible they kept the package called okhttp3 even in V4 branch.

The Telegram binding uses...
https://github.com/pengrad/java-telegram-bot-api
And this lib then uses the okhttp3 lib, which in turn then needs the kotlin-stdlib.

Having looked at the code the past week, the Pengrad lib is very annoying to me when I have to keep referring to a json API for documentation reasons and then learn it all a second time in a java lib that is not documented fully. I believe it would be better to just get rid of it, and implement the Telegram API directly using Jetty and Gson libs. I may regret that statement, but I wont know until I try and do it directly. Anyone's thoughts on this?

@GiviMAD
Copy link
Member

GiviMAD commented Feb 25, 2024

out of curiosity: why do we have kotlin in our pom.xml? AFAIK Kotlin as a language itself not allowed to be used in OH. Are there libraries that we use and require kotlin?

In the case of the Jellyfin add-on, it uses the official Jellyfin SDK which is developed in Kotlin.

Copy link
Member

@GiviMAD GiviMAD left a comment

Choose a reason for hiding this comment

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

LGTM for the Jellyfin and Watson add-ons, both seem to work correctly with the upgraded dependencies.

@jlaur
Copy link
Contributor

jlaur commented Feb 25, 2024

This PR will supersede #15903 and by that fix https://github.com/openhab/openhab-addons/security/dependabot/57.

@jlaur jlaur added enhancement An enhancement or new feature for an existing add-on security labels Feb 25, 2024
@lsiepel lsiepel requested a review from stefan-hoehn February 25, 2024 13:20
@Skinah
Copy link
Contributor Author

Skinah commented Feb 26, 2024

I believe it will also supersede #15902
I have tested the Telegram binding with the changes and it works.

@stefan-hoehn
Copy link
Contributor

@lsiepel You requested a review from me but I doubt I can give any valuable input here. 🤷🏻‍♂️

@Skinah Skinah changed the title update Kotlin okhttp okio libraries Bump Kotlin, okhttp, okio, and java-telegram-bot-api libraries Feb 26, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Mar 2, 2024

@lsiepel You requested a review from me but I doubt I can give any valuable input here. 🤷🏻‍♂️

I thought you did some testing for dbquery in the past for similar updates. Anyway, LGTM. @Skinah did you perform some testing with influxdb ? That is the only one left with a mayor release change.

@Skinah
Copy link
Contributor Author

Skinah commented Mar 4, 2024

I do not use that bundle so I built it successfully with mvn clean install and then dropped it in the addons folder. I got an error in the logs that was stating I did not have it setup or have any strategies setup.

@lsiepel
Copy link
Contributor

lsiepel commented Mar 4, 2024

I do not use that bundle so I built it successfully with mvn clean install and then dropped it in the addons folder. I got an error in the logs that was stating I did not have it setup or have any strategies setup.

Are you planning to tests this a bit further? I think this part needs some basic testing before it can be merged. Maybe someone else that uses influx? (personally i have never used it)

@Skinah
Copy link
Contributor Author

Skinah commented Mar 7, 2024

I have a pi3 setup for testing, so I can do this sometime next week... hopefully. If someone has it already setup it would save me a lot of time. I agree this should be tested further then a basic compile.

@lsiepel lsiepel added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Mar 8, 2024
@justauser0815
Copy link

justauser0815 commented Mar 8, 2024

I could try the addon in my RPi4 environment with latest milestone release installed and influxdb running, if this would help. But as I'm only a semi-professional end-user, I sadly can't build the addon myself.

Signed-off-by: Matthew Skinner <matt@pcmus.com>
@Skinah
Copy link
Contributor Author

Skinah commented Mar 10, 2024

OK after more then 6 hours of getting influxdb installed, working and then trying to work out the dependancies, I finally got it working with this log output showing its connecting and talking to the Database...

2024-03-10 07:26:34.841 [DEBUG] [rnal.influx1.InfluxDB1RepositoryImpl] - database status is OK, version is 1.8.10
2024-03-10 07:26:34.844 [INFO ] [.influxdb.InfluxDBPersistenceService] - InfluxDB persistence service started.

Release notes for v7.0.0 of the influxdb-client-java library that lists the dependencies it uses...
https://github.com/influxdata/influxdb-client-java/blob/master/CHANGELOG.md#700-2024-01-30

@justauser0815
Your welcome to try it out from my personal server below, but I would caution you against it unless your capable of backing up and restoring the database if something goes wrong, or like myself you are testing it on a spare raspberry pi with a load you do not care about. This has updated multiple libraries from very outdated ones v4.3.0 [date released 2022-02-18] to 7.0.0 this is a very big jump and multiple libs have been bumped in their versions. It would be great to have another person test it due to the number of large changes the libraries have probably gone through.

If your having issues or bad performance with influxdb, then it is worth trying it for sure. I am only saying the above as a disclaimer it should work just fine.

http://pcmus.com/openhab/org.openhab.persistence.influxdb-4.2.0-SNAPSHOT.jar

Skinah added 2 commits March 10, 2024 18:07
Signed-off-by: Matthew Skinner <matt@pcmus.com>
Signed-off-by: Matthew Skinner <matt@pcmus.com>
@justauser0815
Copy link

Thank you very much! I will definitely give it a try. I have an extra raspberry for testing. Is your updated version of the telegram binding already available somewhere for testing?

@Skinah
Copy link
Contributor Author

Skinah commented Mar 10, 2024

I put the telegram changes on hold till this gets merged. Its a single file I can drop back in once this is sorted to get the changes and then compile.

@Skinah
Copy link
Contributor Author

Skinah commented Mar 10, 2024

@lsiepel I cancelled the build as it was already at 4 hours and did not look like it was progressing. Is it a problem with the build system as it is building fine here?

@lsiepel lsiepel added the rebuild Triggers Jenkins PR build label Mar 10, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Mar 10, 2024

Maybe @wborn has a clue as i think he is an expert on this matter. I also cancelled the build after 4:20.

Signed-off-by: Matthew Skinner <matt@pcmus.com>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-2-milestone-discussion/154316/45

@jlaur
Copy link
Contributor

jlaur commented Mar 16, 2024

I cancelled the build as it was already at 4 hours and did not look like it was progressing. Is it a problem with the build system as it is building fine here?

I think there is a problem with full PR builds for the time being. Jenkins builds time out after 4 hours and GitHub Actions builds after 6 hours. However, I just saw one full Jenkins build succeed today, so maybe there is hope: #16526.

@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 16, 2024
@jlaur
Copy link
Contributor

jlaur commented Mar 18, 2024

@Skinah - just to be sure: Was commit 0941c20 due to build failing, or some other issue? The build now succeeded.

Signed-off-by: Matthew Skinner <matt@pcmus.com>
@Skinah
Copy link
Contributor Author

Skinah commented Mar 22, 2024

@jlaur Yes I dropped the okio jar back to the older version because that one passed an earlier build before something must have changed on the build system. I have now updated them all to the latest versions as of this point in time, and it is now passing, thanks to whatever and who made the change. I certainly understand why we end up with so many different version of the same libraries now.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-2-milestone-discussion/154316/60

@Skinah Skinah removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Mar 29, 2024
@Skinah
Copy link
Contributor Author

Skinah commented Mar 29, 2024

Removing additional testing tag as I have tested and now a user has confirmed it also fixes an issue on their system in the thread that is linked just above.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM

@lsiepel lsiepel merged commit 09132d9 into openhab:main Mar 29, 2024
3 checks passed
@lsiepel lsiepel added this to the 4.2 milestone Mar 29, 2024
adr001db pushed a commit to adr001db/openhab-addons that referenced this pull request May 12, 2024
…ab#16458)

* update libs

Signed-off-by: Matthew Skinner <matt@pcmus.com>
Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
@Skinah Skinah deleted the telegram branch July 14, 2024 07:07
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
…ab#16458)

* update libs

Signed-off-by: Matthew Skinner <matt@pcmus.com>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…ab#16458)

* update libs

Signed-off-by: Matthew Skinner <matt@pcmus.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
…ab#16458)

* update libs

Signed-off-by: Matthew Skinner <matt@pcmus.com>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
…ab#16458)

* update libs

Signed-off-by: Matthew Skinner <matt@pcmus.com>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants