search

kodi-pvr / pvr.nextpvr

Kodi's NextPVR client addon
GNU General Public License v2.0
22 stars 33 forks source link

Conversion to tinyxml-2 #142

Closed emveepee closed 4 years ago

emveepee commented 4 years ago

Test build for tinyxml-2 Mirror all backend calls to see that returned xml parses correctly. Replace one function to see how it works. The intent of the PR will be to replace tinyxml .

emveepee commented 4 years ago

Ok my test build is good so this should be a straight forward upgrade. @AlwinEsch @phunkyfish In another thread there were issues identified that tinyxml is not being maintained but I see that Kodi only has an older version of tinyxml-2 http://mirrors.kodi.tv/build-deps/sources/ How do I get a newer version included in build-deps?

Can there anything more confusing then the version numbers for tinyxml and tinyxml2? V1 is tinyxml-2.6.2_2.tar.gz and v2 is tinyxml2-6.2.0.tar.gz

AlwinEsch commented 4 years ago

You can use the Link from here https://github.com/leethomason/tinyxml2/releases/tag/8.0.0 / https://github.com/leethomason/tinyxml2/archive/8.0.0.tar.gz

Can also upload it to our mirror, but by github URL's not direct needed as them fast enough.

AlwinEsch commented 4 years ago

About versioning, yes thats there really stupid.

emveepee commented 4 years ago

You can use the Link from here https://github.com/leethomason/tinyxml2/releases/tag/8.0.0 / https://github.com/leethomason/tinyxml2/archive/8.0.0.tar.gz

Can also upload it to our mirror, but by github URL's not direct needed as them fast enough.

Ok thanks I assume I just put the URL in depends with the correct sha256 It seems to be working.

I thought the version number originally was bad. The github file that gets downloaded is just 8.0.0.tar.gz

emveepee commented 4 years ago

Ok, I converted everything to tinyxml2 and removed tinyxml from the build and everything seems in order. @AlwinEsch thanks for the help, the azure pipeline error continues on tinyxml2.

@phunkyfish could you please review. After you review PR 138 I will ask @sub3 to merge it and then resolve conflicts with this PR, update the changelog and merge both as 6.1.0

emveepee commented 4 years ago

You can use the Link from here https://github.com/leethomason/tinyxml2/releases/tag/8.0.0 / https://github.com/leethomason/tinyxml2/archive/8.0.0.tar.gz

@AlwinEsch I saw that tinxyxml-dev was in debian/control and took it out but I don't know how to test that is it correct now, could you review? I also removed kodi-platform since I assume that was only there for tinxyxml.

phunkyfish commented 4 years ago

pvr.vbox uses tinyxml2.

emveepee commented 4 years ago

pvr.vbox uses tinyxml2.

Yes but it just includes old source files not using find etc. pvr.dvblink is closer and it uses find but it also adds libtinyxml2-dev to the debian/control file and I don't know if that is needed or not.

phunkyfish commented 4 years ago

I would assume it is required. There would need to be some reference to tiny xml 2 in that file.

emveepee commented 4 years ago

I would assume it is required. There would need to be some reference to tiny xml 2 in that file.

My concern is @AlwinEsch suggested 8.0.0 and that version of libtinyxml2-dev isn't even exist in Ubuntu 20.04. It seems to be compile without it. Alternatively I can revert to the older kodi version that probably is more supported.

I notice there is also a line in the debian/control file that maybe I need to add

Recommends: inputstream-ffmpegdirect

phunkyfish commented 4 years ago

Unless you know if the API changes in 8.0.0 don't effect what you use it would be safer to the use a version that is at least compatible with 20.04.

I'm sure that version 7 will be new enough.

emveepee commented 4 years ago

I'm sure that version 7 will be new enough.

Seems there was a critical bug in 7.0.1 with homebrew so I updated this to 7.0.1

This is one reason I feel it is a good idea to keep common libraries in kodi-platform. 3 pvr'ssknow use different versions of tinyxml2 and Kodi uses another.

phunkyfish commented 4 years ago

I meant to use the latest stable version of v7.

Having this as a common library would be worse. Then for every bump each addon would need to be retested. Very hard to do when those addons may not have maintainers.

emveepee commented 4 years ago

I meant to use the latest stable version of v7.

Bumped to 7.1.0

phunkyfish commented 4 years ago

@emveepee why remove the Doxygen comments in XMLUtils.h?

Leaving them there (and possible updating them) makes it far more likely your work on this will get reused by other addons ;)

emveepee commented 4 years ago

@emveepee why remove the Doxygen comments in XMLUtils.h?

Leaving them there (and possible updating them) makes it far more likely your work on this will get reused by other addons ;)

As it is I don't think it is a good base. From the first commit pvr.nextpvr was loose mixing node and element names which has always confused me and I had to change some calls. I will look at cleaning that up first and if I can get rid of the ambiguity and convert nodes to nodes and elements to elements then I can add the comments back.

Edit it should be closer now, probably better to use new style doxygen

emveepee commented 4 years ago

Final review for release requested.