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

update avahi, nss-mdns and jsoncpp #2138

Merged
merged 3 commits into from Nov 22, 2017
Merged

Conversation

CvH
Copy link
Member

@CvH CvH commented Oct 24, 2017

changed the url from OE mirros to actually maintained repos

  • just changed to the same version with identical files at the archives
  • avahi, jsoncpp and nss-mdns are partly horrible outdated, I didn't updated due very likely changed behaviour (no idea how to test it)
  • nss-mdns 0.10 files at github are not identical to the released 0.10 so I used the "old" package url (they missing the buildscripts)

@CvH CvH added the LE 9.0 label Oct 24, 2017
@fuzzard
Copy link

fuzzard commented Oct 25, 2017

The 0.x.y branch of jsoncpp is feature compatible with the 0.5.0 used from my testing. I looked at updating a pvr addon (pvr.hdhomerun) to use newer updated versions, and the latest 0.10.6 release (dated April 2016) had no regressions. That branch only receives minor backports from master every now and again.
The 1.x.y branch is completely incompatible as they have changed the reader/writers, and would require major rework for all code.

This is more just to let you know my experiences. I havent looked at anything LE ships that uses jsoncpp, so obviously it would require some thorough testing.

@MilhouseVH
Copy link
Contributor

@fuzzard many thanks for the info.

Unfortunately all of the pvr addons that use jsonccp (the only jsoncpp consumers in LE, as far as I know) are using 0.5.0, and for the sake of continued support it would be better to continue using the same version in LE - using a different version increases the risk of errors/bugs being pushed back on us because we're not using the same library version as the author/maintainer supports, etc.

What should happen is that the upstream projects switch to using a more modern jsoncpp, and then we can follow suit.

The PVR addons using jsoncpp are:

pvr.argustv
pvr.filmon
pvr.hdhomerun
pvr.octonet
pvr.pctv
pvr.stalker

pvr.filmon is basically dead, and pvr.argustv is on life support.

Ping @ksooo - time to update jsoncpp in the remaining addons? Maybe even to 1.x.y if possible?

@fuzzard
Copy link

fuzzard commented Oct 25, 2017

Fully understand, just wanted to pass on info i had at hand.

I have a working branch of pvr.hdhomerun running on the 1.x.y branch, but the most annoying thing with that is the build on win machines has deprecated warning spam. Even if no deprecated methods are being used or referenced. There was a PR to jsoncpp to minimise the deprecation warnings to only when affected functions were used, but last i checked (couple weeks back) the behaviour hadn't changed.

I have a PR at pvr.hdhomerun to pull it up to 0.10.6 with the end goal to take it up to the 1.x.y branch, but maintaner has gone a little awol. I figure if we can get one of the pvr clients up to date as a reference, it might make it easier on the others to change. All this isnt really relevant to LE though. Again, just info at hand that might be useful at a later date.

@CvH
Copy link
Member Author

CvH commented Oct 25, 2017

added 3 commits to bump to latest versions

  • it builds for generic
  • I have no idea if this is right
  • nss-mdns installs this files (even the old version) to image, likely we only need libnss_mdns.so.2 - but no idea how to test it runtime
libnss_mdns.so.2
libnss_mdns4.so.2
libnss_mdns6.so.2
libnss_mdns_minimal.so.2
libnss_mdns4_minimal.so.2
libnss_mdns6_minimal.so.2

@awiouy
Copy link
Collaborator

awiouy commented Oct 25, 2017

According to docs and https://github.com/CvH/LibreELEC.tv/blob/76d000231af7df69ce4079992f82c44f38c16898/packages/network/nss-mdns/config/nsswitch.conf only libnss_mdns_minimal is needed.

I will also have a look at librespot later tonight.

IIRC to runtime test, resolve libreelec.local (or another device on your subnet, eg a routee) in libreelec.

@MilhouseVH
Copy link
Contributor

A brief test on Generic is working OK (absolute basics - booting into Kodi, pinging www.google.co.uk, Samba is working, but no PVR tests)

However if @ksooo (or anyone else) has no plans to update jsoncpp in the upstream PVR addons then I'd prefer that we do not bump to a newer version of jsoncpp and potentially introduce unexpected behaviour.

I'll include this as-is in test builds as it will help confirm there are no immediate jsoncpp incompatibilities and improve the chances of an upstream bump, but unless it happens I don't think we should merge a jsoncpp bump ahead of upstream.

@MilhouseVH
Copy link
Contributor

I chatted @ksooo on Slack and he has no plans to update jsoncpp on Kodi-PVR, but PRs are welcome.

Until upstream is updated (and really, all the PVR addons need to be updated to be on the same version otherwise it gets messy), we should drop the jsoncpp bump.

@fuzzard
Copy link

fuzzard commented Oct 27, 2017

Ive had a quick glance at those other 5 pvr addons Millhouse (not including pvrhdhomerun).

Octonet is the only one that currently compiles against Leia Head. Ill do a PR to octonet for a version bump, would you be able to throw that into test builds to try and get some actual testing done against it? I dont have the backends for any of these pvr addons for testing.

DigitalDevices/pvr.octonet#15

The other 4 all use instances of iChannelNumber and strStreamUrl which were removed as part of ksooo's work over the past couple months. As you have already been excluding them in your builds, im not sure whats best to do here. Ill have a look through and see if i can make the necessary changes to compile them in a minimalist fashion. But the maintainers (if there are any) will probably need to look at some of the other changes ksooo has done lately anyway.

@MilhouseVH
Copy link
Contributor

Yes, currently only pvr.hdhomerun and pvr.octonet are building - if nobody fixes the other addons to bring them up to date with latest Kodi PVR API then they'll likely be removed at some point. It might still be worth submitting PRs (assuming it's not too much work) on the off chance someone does get around to fixing them up.

The upstream PRs are not strictly relevant to LibreELEC as we build with an "external" jsoncpp, so the last couple of Milhouse builds have included pvr.hdhomerun and pvr.octonet with jsoncpp 0.10.6 (which is bumped in this PR) regardless of whatever version the upstream currently uses.

Hopefully the addons that do compile will merge your PRs in which case we can then align LibreELEC with the commit already in this PR. Many thanks for taking care of this!

@fuzzard
Copy link

fuzzard commented Oct 27, 2017

Hi MillHouse,

Sorry for bringing this up in this thread, but im not sure whats the best way/place to ask this.

Ive done an initial pass on pvr.filmon to bring upto PVR API 5.7.0 as a bit of a test to see what may be required for the others. This includes a bump to jsoncpp 0.10.6

kodi-pvr/pvr.filmon#76

Could you possibly build with this, and request any feedback. If it functions, ill look to pull the rest up to the bare minimum, but i cant test it at all, so its a blind fix. It compiles, and runs in Leia Head, but thats as far as i can get. Ive honestly no idea what the user base is, or even if filmon is used by anyone, so dont know where i can get any feedback.

@MilhouseVH
Copy link
Contributor

Thanks - I'll try building pvr.filmon later this and give you feedback on the PR.

@fuzzard
Copy link

fuzzard commented Oct 28, 2017

Another PR this time for pvr.pctv. The only thing ive noticed in regards to jsoncpp (and this is the same for argustv) is they use a deprecated function (getFormatedErrorMessages). The only reason this method has been deprecated is because of the typo in the function name (new method getFormattedErrorMessages).

Edited to add pvr.stalker

kodi-pvr/pvr.pctv#52
kodi-pvr/pvr.stalker#100

Im wondering if theres any point updating some of these addons, or if we/i should just let them die. Theres lots of non implemented ToDo's running back to the timer changes done for API 2.0.0/1.9.7
I've only been doing the bare minimum to get them compiling, and they SHOULD function, but again, i dont have any of these backends to test/verify.

At a brief glance, argustv is going to require a bit more in depth knowledge of the code. Ill have a look when i get some more time, but i may have to leave that one for someone else.

@MilhouseVH
Copy link
Contributor

Thanks, I'll include pvr.pctv and pvr.stalker in tonight's build so long as they build (won't know for another hour or so). pvr.filmon will also be included as we know that builds.

Killing addons is never an easy decision, but if they're not maintained it does become a little easier.

@MilhouseVH
Copy link
Contributor

pvr.pctv and pvr.stalker have both built successfully - thanks!

@CvH
Copy link
Member Author

CvH commented Nov 14, 2017

@MilhouseVH I guess it works for your builds ? If so I should squash the commits I guess?

@MilhouseVH
Copy link
Contributor

@Rechi has just submitted PRs for the PVR addons that bumps jsoncpp to 1.8.3:

kodi-pvr/pvr.argustv#74
kodi-pvr/pvr.filmon#79
kodi-pvr/pvr.hdhomerun#70
kodi-pvr/pvr.pctv#54
kodi-pvr/pvr.stalker#102
DigitalDevices/pvr.octonet#17

I'm not seeing any code changes though - I thought jsoncpp 1.x.y was incompatible with jsoncpp 0.x.y? @fuzzard are the PVR changes sufficinet for jsconcpp 1.8.3?

@fuzzard
Copy link

fuzzard commented Nov 14, 2017

your going to see a shit ton of deprecation spam in your builds. The old functions are still in, but are all marked as deprecated. Should work for now, but dont expect good things in the future. i would suggest sticking with the 0.x.y branches at this stage personally

@MilhouseVH
Copy link
Contributor

@fuzzard As upstream has now moved to 1.8.3 I think doing the same is the best option, regardless of the deprecation spam - if we remain on 0.x.y while upstream uses 1.x,y any differences in behaviour that we experience (both good and bad) could be the result of being on the older library.

Presumably the deprecation changes are in the pipeline?

@CvH could you add another commit that bumps jsconpp to 1.8.3 and I'll try some test builds.

@fuzzard
Copy link

fuzzard commented Nov 14, 2017

yeah, feel free to bump if they are going to do that. ive put a comment on the pvr.hdhomerun PR to ask if he is aware of the changes, but if they want to push to it, thats up to them i guess.

@MilhouseVH
Copy link
Contributor

Many thanks, @fuzzard!

@CvH CvH force-pushed the 9.0-oe-links branch 2 times, most recently from 8f01f02 to 9cf6128 Compare November 14, 2017 20:00
@MilhouseVH
Copy link
Contributor

Thanks that builds ok. These are the jsoncpp-related deprecation warnings for all PVR addons: http://sprunge.us/APZD

@fuzzard
Copy link

fuzzard commented Nov 14, 2017

I've put up a patch to the pvr.hdhomerun addon to change to the new methods. The spam comes from the windows side of builds. GCC/Clang handles the deprecations differently, with only 1 message per instance, but windows seems to show it regardless with multiple.

Seems Rechi is wanting to go ahead with it regardless, and i understand why. Just would have been nice to update the addons to remove the deprecated methods rather than blindly do a version bump, but all good.

@fuzzard
Copy link

fuzzard commented Nov 16, 2017

Another off topic, but @MilhouseVH ive just pushed a PR to pvr.argustv kodi-pvr/pvr.argustv#75
Any chance you can build and see if we get any feedback from users. As with the other addons, completely blind fixes, with no runtime testing.

@MilhouseVH
Copy link
Contributor

Any chance you can build and see if we get any feedback from users.

Will do. I can at least say they all build OK.

@CvH CvH changed the title removed OE mirror links update avahi, nss-mdns and jsoncpp Nov 22, 2017
@CvH
Copy link
Member Author

CvH commented Nov 22, 2017

rebased

@MilhouseVH MilhouseVH merged commit fa8f13d into LibreELEC:master Nov 22, 2017
@CvH CvH deleted the 9.0-oe-links branch November 22, 2017 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants