openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
3.94k stars 3.45k forks source link

14.07: libmodbus regression from AA #292

Closed karlp closed 9 years ago

karlp commented 10 years ago

AA had libmodbus 3.0.2, as seen here http://git.openwrt.org/?p=12.09/packages.git;a=blob_plain;f=libs/libmodbus/Makefile;hb=HEAD

trunk, up to and beyond the branch for 14.07 had 3.0.2. Master has the updated and patched 3.0.6, but in https://github.com/openwrt/packages/commit/98d5c3a15acb46ffa557afbda30f2760ad66168c the for-14.07 branch was reverted back to v2.0.4, ostensibly to allow collectd plugins to build.

Those collectd plugins would have been failing for the entire lifetime of AA, so it feels unhelpful at best to massively downgrade the library for this. collectd might have been the only in feed application using libmodbus, but it most certainly isn't the only app in the world using libmodbus, and shipping BB with a very very out of date library, and changing the version during the RC cycle seems inappropriate to say the least. Please can we put this back to 3.0.2 as before. (or 3.0.6 even to get dot release bugfixes)

Surely this is a collectd problem, not a libmodbus problem.

champtar commented 10 years ago

@karlp, do you have a fix for the compilation problem

karlp commented 10 years ago

I don't have a fix for collectd no, I don't use collectd, and there's a package that hasn't been compiling with the feed version of libmodbus for the entire life of AA doesn't work, I feel strongly that the "fix" is to simply remove the packaging of those collectd plugins. That was the whole point of this new packages thing right? to have things that are maintained?

karlp commented 10 years ago

From the "news" page of collectd, version 5 (back in 2011-03-28) added support for libmodbus 2.9.x, (which is at least API compatible with 3.0.x, but still very old) In version 5+, there's some support for using both new and old libmodbus versions, (but nothing for libmodbus 3.1+, but that's a future story! :)

OpenWrt still has collectd 4.10, which was released in 2011-03-26. I'm test building a patch that simply removes collectd-mod-modbus. If/when OpenWrt moves to collectd v5+, and if someone wants libmodbus support again, they can fix it, but it hasn't worked for a couple of years, so I doubt anyone has been using it anyway.

hnyman commented 10 years ago

Libmodbus plugin for collectd is simply marked broken in trunk, so wouldn't it be enough to just revert the For-14.07 branch-specific downgrade commit https://github.com/openwrt/packages/commit/98d5c3a15acb46ffa557afbda30f2760ad66168c and the other commit from the same author https://github.com/openwrt/packages/commit/12de0b9bae0d90fca343f8dece1f0c0bbf950f5a that removed the "broken" definition from the modbus plugin in collectd.

There is also the collectd update discussion at #290

karlp commented 10 years ago

Unfortunately 4.10.9 still doesn't update enough to fix the modbus plugin, but if, as #290 suggests, that master gets updated to collectd v5.4+, then that's great, as the master collectd still has the modbus plugin enabled presently.

karlp commented 10 years ago

@mhei You may want to have a read of these threads, if you've missed it

danwrt commented 10 years ago

I am using collectd with libmodbus and committed the downgrade to 2.0.4 as it was not obvious to me why it was ok to upgrade it if that breaks the only known/public user of that library. Given that it was me originally submitting the original package build back then, it felt right to also keep things intact: https://lists.openwrt.org/pipermail/openwrt-devel/2011-March/010079.html https://lists.openwrt.org/pipermail/openwrt-devel/2011-March/010080.html https://lists.openwrt.org/pipermail/openwrt-devel/2011-March/010081.html

My intention was to restore the functionality which was broken by the upgrade of libmodbus some time ago -- I didn't intervene immediatly because I wasn't anywhere near my test-setup (I must admit that I use another patch on top of collectd which I wrote in order to support Modbus-RTU, however, I'll happily publish that if there is demand for that).

If you are aware of other applications able to build and run on OpenWrt and making use of libmodbus, probably the best would be you'd add them to the packages feed as well.

danwrt commented 10 years ago

+1 for the collectd upgrade, if anyone other than me fixes the existing LuCI stuff I'm all for it :) I'd also happily fix support for more recent versions of libmodbus in recent versions of collectd.

karlp commented 10 years ago

Upgrading collectd to something modern would make us all happy surely. Not all packages that run on openwrt are in public openwrt feed repositories, let alone this repository. Also, please note that by "some time ago" we're talking about before Attitude Adjustment was even released :)

@danwrt the recent collectd seems to already support recent libmodbus, but I've not tried it personally, just looked at the codebase.

danwrt commented 10 years ago

The point is that upgrading collectd (with all it's many users and user-made scripts around it) within the current release cycle seems impossible to me. Fixing a broken-build of a single plugin by down-grading a library without any other users within the packages being included for the release was realistic and better than not doing it imho. In the long-term, of course, collectd should be upgrade in the master branch, then the modbus plugin could also be fixed/ported to which-ever version of libmodbus. Given that third-parties already need to ship their application binaries, why can't they also ship their own version of libmodbus or even use static linking?

karlp commented 10 years ago

My major issue with this downgrade is that the entire life of AA shipped with libmodbus3.x, as did all of BB up to your change. So the collectd-mod-modbus plugin was broken for the entire last 2.5 years. So I feel it's highly inappopriate to downgrade libmodbus now. (fwiw, debian, fedora and ubuntu all dropped libmodbus v2.x further back than I know how to find on the web)

I would suggest that for 1407:

For master:

An alternative, which is kinda gross, but it's how debian/ubuntu et al handled it, back in the dim dark past when they still bothered supporting libmodbus v2.x, was two packages, libmodbus, and libmodbus5.

danwrt commented 10 years ago

I get your point, but for now I'm only able to speculate about your excact requirements regarding the upcoming release. My feeling is still that it'd be sad to break a free (as in free software) feature for the sake of not statically linking libmodbus (or shipping it along) with a supposedly proprietary application. Though it lacks any technical beauty, I can see that the alternative you suggested (having packages for both, libmodbus and libmodbus3) is kinda the best option, as both sets of requirements/expectations will be satisfied then. This, however, will still require some re-packaging so to avoid filename collisions between libmodbus and libmodbus3. I guess the best would be to rename libmodbus2 binaries and headers and patch collectd accordingly, as we'd then maintain compatibity for 3rd party applications already depending on libmodbus==libmodbus3.

karlp commented 10 years ago

Still, AA shipped with libmodbus 3. You're asking BB to ship with libmodbus 2, which no other linux distro even offers as an option just so you can ship a plugin for an obsolete version of collectd, where said plugin hasn't even built for the last 2.5 years, with no complaints.

And again, AA shipped with libmodbus 3. You're asking BB to ship with libmodbus 2.

(If you must keep libmodbus v2, at least let's use the libmodbus5 name for libmodbus 3+ that the other linux distros used)

danwrt commented 10 years ago

Personally, I don't mind having my own build in order to be able to continue running ancient collectd with ancient libmodbus, so from my point of view there is no /must/ or anything strict like that. I see this as a matter of consistentency and QA-kinda thoughts (i.e. provide the version of a library another package depends on rather than breaking things for the sake of including a newer version which is not used by anything part of the release). If you believe that for any reason you and other users would really benefit from including a lonely shared library rather than a working feature in BB, go ahead if you are sure about what you are doing, I promise it won't be me complaining about it, I said what I wanted to say about it.

mhei commented 10 years ago

Sorry for my late reply. I think both of you have very good arguments for your points of view and I thank you very much for such an objective discussion.

However, I would follow Karls argumentation:

mhei commented 10 years ago

In other words, the technically correct way -as describe by @danwrt-, that means maintaining various ABI versions of a library in the feed, is this a requirement of our (OpenWrt) users? libmodbus might be only a simple starting point for this discussion. What about other libraries?

mhei commented 10 years ago

@danwrt: I've hacked a patch for collectd to upgrade to libmodbus 3.0.x. However, I've no setup to test this at the moment :-( Do you have one ready? See https://github.com/mhei/packages/tree/collectd-modbus-fix-for-14.07

danwrt commented 10 years ago

@mhei: That's great news! I'll hack-up my modbus-rtu patch and hope to be somewhere near a testable setup (sunsaver mppt) within the next couple of weeks. I'll also be playing with libmodbus Thursday-Friday anyway as I was planning to hack support for the Tracer MT-5 protocol (i.e. data-link quirks/preamble/sync/... , custom functions 0xA0, 0xAA, 0xAD, ... with complex return values, ...) into libmodbus and got hardware for testing that. The goal is to replace some crude socat hackery throwing csv at collectd I currently use for that. Thus this will include porting my collectd modbus-rtu-support patch to libmodbus-3 and adding support for the status register data returned by the Tracer in collectd. See https://gitorious.org/tracertools/

hnyman commented 9 years ago

collectd for trunk has been updated to 5.4.1 today by https://github.com/openwrt/packages/commit/7d44d48c6698d2ee711392e3933f09375e11c2f4.