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

Removed dependency on 'org.apache.commons.lang' #1433

Merged
merged 2 commits into from Apr 22, 2020

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Apr 19, 2020

  • Removed dependency on org.apache.commons.lang

  • bom/compile/pom.xml

  • bom/compile/pom.xml

  • features/karaf/openhab-tp/src/main/feature/feature.xml

  • Resolve itest.bndrun

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

@cweitkamp cweitkamp added work in progress A PR that is not yet ready to be merged infrastructure API breaking labels Apr 19, 2020
@cweitkamp cweitkamp force-pushed the feature-remove-apache-commons-lang branch 2 times, most recently from 04f0ca2 to bc24a35 Compare April 20, 2020 08:18
@cweitkamp cweitkamp added this to In progress in openHAB 3 Issue Tracking Apr 20, 2020
@cweitkamp cweitkamp force-pushed the feature-remove-apache-commons-lang branch from bc24a35 to 510049f Compare April 20, 2020 10:48
@cweitkamp cweitkamp changed the title [WIP] Removed dependency on 'org.apache.commons.lang' Removed dependency on 'org.apache.commons.lang' Apr 20, 2020
@cweitkamp cweitkamp removed the work in progress A PR that is not yet ready to be merged label Apr 20, 2020
@cweitkamp
Copy link
Contributor Author

OHC code is clean now. But it will completely break openHAB add-ons. Thus I did not yet remove the import from BOM nor resolved the itests.

@cweitkamp cweitkamp added this to the 3.0 milestone Apr 20, 2020
@wborn
Copy link
Member

wborn commented Apr 20, 2020

The task list suggests you want to keep working on this but you did remove the WIP label?

5iver pushed a commit to openhab-scripters/openhab-helper-libraries that referenced this pull request Apr 20, 2020
@5iver
Copy link

5iver commented Apr 20, 2020

ScriptUtils is only used in one JS example script in any of the helper libraries, which I have created a PR to remove. I'm doubtful there will be much of an impact to peoples' scripts. I'm sure there is a good reason, but I'm curious why this is being removed.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp cweitkamp force-pushed the feature-remove-apache-commons-lang branch from 510049f to 46e6b85 Compare April 22, 2020 08:22
@cweitkamp
Copy link
Contributor Author

cweitkamp commented Apr 22, 2020

The task list suggests you want to keep working on this but you did remove the WIP label?

It is more or less a reminder. In general I wanted to ask for your opinion. How should we proceed? Remove everything in OHC right now and break OH3 add-ons build or try to find a solution for add-ons. Do we currently continue to allow apache.commons.lang over there or not?

I'm sure there is a good reason, but I'm curious why this is being removed.

One of our open items on the roadmap for OH3 is to remove dependency on apache.commons in OHC. Imo nearly everything is possible to solve with plain Java 11 and the library itself is quite huge (size in MB) an a heavy payload for the framework.

@J-N-K
Copy link
Member

J-N-K commented Apr 22, 2020

We need to refactor addons anyway to remove commons-lang, right? So we could do it now, it shpuld not break anything that faces the user, so it is fully backward-compatible.

@wborn
Copy link
Member

wborn commented Apr 22, 2020

We can already replace all usages in the core and keep the Maven dependency for now so the OH3 add-ons port keeps working.

For some changes such as using String.isBlank Java 11 is required. So that change can't be done until the OH2 add-ons are ported and Java 11 features can be used. But other replacements that don't depend on Java 11 features can already be made on the OH2 add-ons. E.g. replacing StringUtils.join by utilizing streams.

That way this PR won't collect merge conflicts and we can slowly progress to fully removing the dependency.

@cweitkamp
Copy link
Contributor Author

That was exactly my idea and the purpose of leaving BOM and features untouched. Resolving the itest.bndrun files can be done in scope of this PR without problems.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks! As the changes show, the library no longer provides us with a lot of added value.

@wborn wborn merged commit d371a34 into openhab:master Apr 22, 2020
openHAB 3 Issue Tracking automation moved this from In progress to Done Apr 22, 2020
@cweitkamp cweitkamp deleted the feature-remove-apache-commons-lang branch April 22, 2020 12:41
@cweitkamp
Copy link
Contributor Author

Great. I will try to continue with org.apache.commons.io and org.apache.commons.logging.

@kaikreuzer
Copy link
Member

Can you help me with which change org.apache.commons.codec has been removed?
It seems that something broke the add-ons port build: https://ci.openhab.org/job/openHAB-Addons-port/108/

@cweitkamp
Copy link
Contributor Author

I think org.apache.commons.codec was a dependency of org.apache.commons.httpclient (see #1436). We never included it ourselves. Was probably resolved by mvn.

@cweitkamp cweitkamp removed this from Done in openHAB 3 Issue Tracking Apr 23, 2020
@kaikreuzer
Copy link
Member

Hm, ok, thanks - interesting side-effect.
Hopefully not too many bindings are hit by that.

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Apr 23, 2020

A quick search reveals three: freebox, loxone and millheat. They are using methods to decode/encode hex strings to byte arrays. We can replace them by OHC HexUtils.

@kaikreuzer
Copy link
Member

"We can" - I assume you are talking in majestic plural and am looking forward to your PR - thanks! 🥇

@cweitkamp
Copy link
Contributor Author

"We" created a PR to fix it openhab/openhab-addons#7460

@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/guide-binding-development-changes-for-openhab-3-from-2-5-x/104134/4

@cweitkamp cweitkamp moved this from In progress to Done in openHAB 3 Issue Tracking Aug 30, 2020
@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-3-0-milestone-2-discussion/107564/131

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
GitOrigin-RevId: d371a34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants