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

[depends] bump jsoncpp to 1.8.3 #70

Merged
merged 2 commits into from Nov 20, 2017

Conversation

@Rechi
Copy link
Contributor

commented Nov 14, 2017

No description provided.

@fuzzard

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

id be wary of this. 1.x.y branches have deprecated the older reader/writers and implemented new builders that change behaviour. Anyone that builds from source is now going to receive deprecation warnings. Other addons use far more jsoncpp features than pvr.hdhomerun does(writers and error output)

I'd suggest at this stage to stick to the 0.x.y branch, unless your going to change to the new methods in the codebase.

@Rechi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

Have a look at https://packages.qa.debian.org/libj/libjsoncpp.html and https://launchpad.net/ubuntu/+source/libjsoncpp they use 1.7 in all recent versions.
If you don't want the deprecations warnings disable them via CMAKE_CXX_FLAGS. Also Travis CI only shows one deprecation warning, Windows shows 7.

@fuzzard

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

can i suggest you implement the charreaderbuilder then. Below is a patch for it. I dont have time to do a PR to your branch this morning sorry.

Cant get the diff to display nice.

fuzzard@10c5d24

@Rechi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

@fuzzard done
Before merging this, jsoncpp has to be bumped in pvr.argustv, otherwise this addon will fail building for nightlies in jenkins.

@Rechi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

@FernetMenta just noticed Json::Reader already is marked deprecated in the version you have updated to earlier.

@fuzzard

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

argustv hasnt been building for months. It hasnt been updated to pvr api 5.7.0. I had a brief look about a month ago, but really didnt have the time to work out what it needed to get back up and running, and the maintainer says he no longer has an argustv backend so hasnt really any intention to update it at this point.

just a note regarding the jsoncpp 1.x.y branch, it is considered api incompatible by the maintainer. I had a branch running with 1.8.3 a little while back (hence the quick patch), but i never bothered to fully test everything because of the deprecation warnings (even without deprecated functions being used) on msvc builds. Their are PR's and commits post the 1.8.3 release tag that reduce the deprecations on msvc warnings, but who knows when a new release will be tagged.

@Rechi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2017

argustv doesn't have to build, only jsoncpp version has to be bumped there. Reason, jenkins uses only the dependeny (grouped by name) from the lowest addon id (pvr.a comes before pvr.h).

@Rechi

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2017

Just a note, jenkins already uses jsoncpp 1.8.3 for this binary addon (pvr.argustv was already bumped).

@fuzzard

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2017

probably going to need @ksooo to do the merge. Not sure who else has access, and @zcsizmadia hasnt been around for a couple months.

@ksooo
ksooo approved these changes Nov 20, 2017
Copy link
Member

left a comment

Thanks.

@ksooo ksooo merged commit 3af3e91 into kodi-pvr:master Nov 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Rechi Rechi deleted the Rechi:dependsJsoncpp branch Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.