Ticket #11609 (closed Bugs: Fixed)

Opened 3 years ago

Last modified 3 years ago

Can't compile xbmc (git) with yajl 2.0

Reported by: [vEX] Owned by: topfs2
Priority: 4 - Normal Milestone: 11.0
Component: Other (un-categorized if does not fit anywhere else) Version: GIT
Severity: Normal Keywords: yajl yajl_parse_complete
Cc: topfs2, montellese, sraue, Blocked By:
Blocking: Platform: All
Revision: 1e703b3780de1dd19053

Description

There was a few changes between yajl 1.x and 2.0 which makes it compilation fail if using yajl 2.0.

The issues at hand are:

  • the function "yajl_parse_complete" was renamed to "yajl_complete_parse" in yajl 2.0.
  • the configuration mechanism changed, don't pass config struct to alloc use yajl_gen_config()/ yajl_config() instead.

(full ChangeLog here:  https://github.com/lloyd/yajl/blob/dbcba50b7785170d89bfeddaf2b41fe3f3ffacf4/ChangeLog)

Attachments

yajl_2.0_support.patch Download (6.5 KB) - added by [vEX] 3 years ago.
Add yajl 2.0 support to JSONVariantParser/JSONVariantWriter
0001-Ticket-11609-Can-t-compile-xbmc-git-with-yajl-2.0.patch Download (7.6 KB) - added by [vEX] 3 years ago.
0001-JSON-fix-compilation-with-YAIL-2.x-Ticket-11609.patch Download (9.2 KB) - added by PaulePanter 3 years ago.
JSON: fix compilation with YAIL 2.x (Ticket #11609)
0001-JSON-fix-compilation-with-YAJL-2.x-Ticket-11609.patch Download (9.2 KB) - added by PaulePanter 3 years ago.
0001-JSON-fix-compilation-with-YAJL-2.x-Ticket-11609.patch; s/YAIL/YAJL/ in commit message
0001-Add-YAJL_MAJOR-1-define-for-windows.patch Download (544 bytes) - added by topfs2 3 years ago.
0001-Add-support-for-YAJL-2.x-Ticket-11609.patch Download (7.8 KB) - added by [vEX] 3 years ago.
Made the commit message more elaborate.
0001-Add-support-for-YAJL-2.x-fixes-11609.patch Download (8.1 KB) - added by Montellese 3 years ago.
Rebased patch with necessary changes for win32
0001-Add-support-for-YAJL-2.x-fixes-11609.2.patch Download (8.0 KB) - added by topfs2 3 years ago.

Change History

comment:1 Changed 3 years ago by [vEX]

I've been hacking away at a patch for yajl 2.0, but I'm very rusty at C/C++. And I can't figure out how to pass "this" as the context when using yajl_gen(). It seems like I have to pass in the yajl_gen enum as the context instead. Someone who's more awake should look at the docs 'cause I must be blind.

And how do I test the functionallity after a successful compile? What part of XBMC uses yajl?

comment:2 Changed 3 years ago by Montellese

  • Cc topfs2, montellese added

The problem with this patch is that it breaks support for yajl 1.0.12 which is used on win32 and osx. A "better" approach would be to use #ifdef to distinguish between the yajl versions on compile time but I'm not sure if that's possible as it depends on whether yajl provides some #define with a version info.

If you want to test it yajl is used in jsonrpc.

comment:3 Changed 3 years ago by [vEX]

Yeah, this isn't the best patch, I thought I'd just offer it to those who want to use yajl 2.0. Now I'm starting to think I should just have create a package of yajl 1.x for my distro and used that instead.

It appears that there are more changes than I first thought. Not just the renamed function and new config mechanism. But also some function prototypes for the callbacks have changed so I'm not sure how to handle that.

And yajl doesn't seem to provide any version info to #ifdef on either. I will keep hacking away at it and see if I can come up with something.

comment:4 Changed 3 years ago by [vEX]

The attach patch will apply cleanly and compile without any hitches against yajl 2.0.2.

I'm not 100% sure on how to verify the JSON-RPC functionallity but the following seems to run just fine:

$ curl -i -X POST -d '{"jsonrpc": "2.0", "method": "VideoLibrary.ScanForContent", "id": "1"}' localhost:8080/jsonrpc HTTP/1.1 200 OK Content-Length: 40 Content-Type: application/json Date: Mon, 06 Jun 2011 12:07:54 GMT

{"id":"1","jsonrpc":"2.0","result":"OK"}

comment:5 follow-up: ↓ 16 Changed 3 years ago by topfs2

Seems like there is no version tag in the older yajl, and the yajl_version.h does not exist either. I wouldn't be surprised if pkg-config can tell you the version though. I would have preferred going for yajl 2.x if it wasn't for ubuntu not packaging it.

If available in pkg-config you could define HAVE_YAJL_VERSION or something so that the compile works for older also (and assume 1.x on older).

Thanks for looking into it.

comment:6 Changed 3 years ago by [vEX]

I will have a look at pkg-config and see if I can whip something up, might take a few days though.

comment:7 Changed 3 years ago by [vEX]

automake/autoconf/pkg-config seems to be quite the beast, could something as simple as this (stuffed into configure.in) be enough:

+# yajl version check (yajl_version.h was added in yajl 2.0) +AC_CHECK_HEADERS([yajl/yajl_version.h], [], [] [ +#ifndef YAJL_VERSION_H +#define YAJL_VERSION_H +#define YAJL_MAJOR 1 +#endif +]) +

comment:8 Changed 3 years ago by topfs2

Yeah something like that atleast, AC_CHECK_HEADERS will define HAVE_YAJL_VERSION_H or something like that if found, so then you could just do ifdef YAJL_VERSION_H #include <yajl/version.h> else define YAJL_MAJOR 1

Sounds like the easiest way to go about it, otherwise one can use the pkg-config and set forced version but sounds like this is simpler tbh.

Changed 3 years ago by [vEX]

Add yajl 2.0 support to JSONVariantParser/JSONVariantWriter

comment:9 Changed 3 years ago by [vEX]

Fix some typos in the patch and now configure doesn't complain. Would be great if someone could test this on yajl 1.x and 2.x to make sure I didn't screw things up completely.

comment:10 Changed 3 years ago by topfs2

I can test on yajl 1.x atleast, I can push it if it works. Looks correct to me.

Could you please provide it as a proper git patch, that would help me a lot.

Here is a very quick git walkthrough, ignore if you know how already :)

Essentially do a git clone, git add the files, git commit (add your e-mail and such if it asks you) and git format-patch HEAD~1

If you accidently do more than one patch please do git rebase HEAD~X -i (X is patches you have) and put f infront of all but the first.

If you have any questions please feel free to ask

Cheers, Tobias

comment:11 Changed 3 years ago by topfs2

Some thoughts regarding the patch. AC check header will define HAVE_YAJL_VERSION_H by itself so skip the define of that and use the given one. With that i think it should be fine for inclusion, needs win32 love but i can add that.

comment:12 Changed 3 years ago by [vEX]

There, I hope this should be exactly what you wanted. Thanks for the instructions.

Cheers, Fredrik

comment:13 Changed 3 years ago by topfs2

  • Cc sraue, added

comment:14 Changed 3 years ago by topfs2

Ok I took a more thorough look and tried the patch, it seems like you have switched the logic a bit.

In JSONVariantWriter.h l26 and JSONVariantParser.h l25 it should be ifdef HAVE_YAJL_VERSION_H and not ifndef You also need to peg on #include "system.h" on the top of both of those files so it will include config.h properly.

Otherwise it does seem to generate a proper config.h for me, i.e. without HAVE_YAJL_VERSION_H and with YAJL_MAJOR 1

So if you just fix those two I'll be happy to test again and if working I'll push. Thanks for the contributions

comment:15 follow-up: ↓ 19 Changed 3 years ago by [vEX]

I'm terrible sorry, for some dumb reason I decided not to compile test before submitting that patch. I've now fixed the issues you pointed out as well as some other minor issues and compile tested.

Now this should hopefully be exactly what you want.

Cheers, Fredrik

comment:16 in reply to: ↑ 5 Changed 3 years ago by PaulePanter

Replying to topfs2:

[…]

I would have preferred going for yajl 2.x if it wasn't for ubuntu not packaging it.

So if that problem was known from the beginning why was the version XBMC needed not mentioned anywhere? At least git grep yajl and all commits containing yajl do not contain any hint that YAJL 1.0.x is needed. XBMC needs to improve in this kind of area! And the first thing is to mention everything in the commit message in my opinion.

[…]

Thanks for looking into it.

That is definitely appreciated.

comment:17 follow-up: ↓ 20 Changed 3 years ago by topfs2

a) We do this for fun b) Main linux development platform is ubuntu, which uses yajl 1.x c) I got sign-off from all supported platforms before the merge, all where fine or wanted to use 1.x, thus we aimed for it

Yes, perhaps we should have put that in the commit message, but we are in the middle of a dev cycle, we are bound to have a few speed bumps. We would have put down hard dependency in README on yajl 1.x before stable release if this patch hadn't arrived.

comment:18 Changed 3 years ago by topfs2

  • Owner set to topfs2
  • Status changed from new to accepted

comment:19 in reply to: ↑ 15 Changed 3 years ago by PaulePanter

Replying to [vEX]:

[…]

Now this should hopefully be exactly what you want.

If you still have some energy/time/motivation it would be great if you could improve the commit message. I am attaching my proposal. I am aware that a lot of developers do not like elaborate commit messages. But in my opinion quality assurance demands this.

Changed 3 years ago by PaulePanter

JSON: fix compilation with YAIL 2.x (Ticket #11609)

comment:20 in reply to: ↑ 17 Changed 3 years ago by PaulePanter

Replying to topfs2:

a) We do this for fun

I know. But that is not an excuse for everything and not to improve the development process. And as far as I know some people also do or try to do XBMC development professionally and you also try to get XBMC deployed in “commercial” products.

I am grateful for your contributions and I am also doing free software work in my free time.

b) Main linux development platform is ubuntu, which uses yajl 1.x

As you wrote yourself you knew about the problems.

c) I got sign-off from all supported platforms before the merge, all where fine or wanted to use 1.x, thus we aimed for it

I never criticized that decision. It should just have been mentioned somewhere.

Yes, perhaps we should have put that in the commit message, but we are in the middle of a dev cycle, we are bound to have a few speed bumps.

I know. But unfortunately people trying to follow development and trying to help out by compile testing – also in my free time by the way – waste a lot of time by these simple documentation things. In comparison to for example the Linux kernel the commit messages should be improved a lot to describe the problem and to explain the fix. Right now over half of them are worthless. It would take the developer maximum five minutes more to compose a good commit message but waste up to half an hour of *each* tester to understand the code changes.

Good commit messages also improve the quality, since reviewers can compare the intention of the committer and the actual change.

We would have put down hard dependency in README on yajl 1.x before stable release if this patch hadn't arrived.

In my opinion documentation should be up to date and a commit should contain such information.

Changed 3 years ago by PaulePanter

0001-JSON-fix-compilation-with-YAJL-2.x-Ticket-11609.patch; s/YAIL/YAJL/ in commit message

comment:21 follow-up: ↓ 26 Changed 3 years ago by [vEX]

Of course I can change the commit message, how elaborate do you want it?

And what do you want the first line to say, "Add support for YAJL 2.x [Ticket#11609]" or something similiar?

comment:22 follow-ups: ↓ 28 ↓ 37 Changed 3 years ago by topfs2

That commit message your suggesting PaulePanter is IMO way way to informative. The one given by patch owner is more than enough. (The new one you suggest is also fine, its not that important, it links to trac ticket which is the important part)

As said, I use ubuntu as this is the main development platform for linux. I looked around for libraries with json parsing/writing, sudo apt-get installed them and chose the one most interesting. When I had made the patch I never had cared about version, this popped up later when dealing with the other platforms (i.e. not me) and I felt no reason to change the commit messages at that point.

Point being, the alteration being done was to move to yajl, we never intended it to restrict the version, this cropped up later. Version is usually never interesting in commit messages, the dependencies of version should be in the README. And as said, the intent was not to restrict to 1.x but to just yajl. We are in the middle of a development cycle, if packagers want to follow now they need to understand that they might hit bumps. Overall our team is extremely active on IRC and if you had asked about yajl in there I (or someone) else would have answered within minutes most likely. When stable is released the needed information would be in README.platform.

I'm not interested in arguing this point any further with you, the developers value small and to the point commit messages mostly. Its meant to be between devs and not for the general public.

@sraue, if you please. could you verify it with yajl 2.x, it works fine for 1.x ubuntu atleast so its valid to be pushed with your sign-off. @Montellese could you please try on windows, I bet it would need my added patch (which I plan to amend in before pushing)

Changed 3 years ago by topfs2

comment:23 Changed 3 years ago by topfs2

I can agree that referencing interesting commit, i.e. 3565ad579e9b80f4be57db901d3fe44d9bfac87d could be useful if we want to be anal, its ok if omitted though

comment:24 Changed 3 years ago by topfs2

@vEX "Add support for YAJL 2.x [Ticket #11609]" seems like a valid message

comment:25 Changed 3 years ago by [vEX]

How's this for a commit message then (first line being the subject), it's short and to the point and has all the references:

Add support for YAJL 2.x (Ticket #11609)

Commmit 3565ad57 replaced jsoncpp with yajl, however there are some API differences between YAJL 1.x and 2.x. This implementation adds support for using YAJL 2.0 (using version guards). I've also added a call to yajl_free() in the destructor to clean up the handler.

comment:26 in reply to: ↑ 21 Changed 3 years ago by PaulePanter

Replying to [vEX]:

Of course I can change the commit message, how elaborate do you want it?

Please take a look at my proposal I attached.

And what do you want the first line to say, "Add support for YAJL 2.x [Ticket#11609]" or something similiar?

I took »JSON: fix compilation with YAJL 2.x (Ticket #11609)« but your suggestion is fine too. »Add« would suggest that originally only YAJL 1.x was supported as topfs2 suggested.

But to be honest I do not know any style guide for XBMC suggesting how to format and compose commit messages. Some developers seem to start the commit message with »fixed« or »added« although I do not like that much.

In the end topfs2 will decide as the developer.

Changed 3 years ago by [vEX]

Made the commit message more elaborate.

comment:27 Changed 3 years ago by [vEX]

Instead of dragging this on and on let's make it easy. topfs2, just change the commit message as you see fit, just as long as the support is added to XBMC I am happy.

comment:28 in reply to: ↑ 22 Changed 3 years ago by PaulePanter

Replying to topfs2:

That commit message your suggesting PaulePanter is IMO way way to informative. The one given by patch owner is more than enough. (The new one you suggest is also fine, its not that important, it links to trac ticket which is the important part)

So I need to open a Web browser to read what is the problem? Very time consuming instead of searching for the error message in the commit log.

As said, I use ubuntu as this is the main development platform for linux. I looked around for libraries with json parsing/writing, sudo apt-get installed them and chose the one most interesting. When I had made the patch I never had cared about version, this popped up later when dealing with the other platforms (i.e. not me) and I felt no reason to change the commit messages at that point.

This reads for me as if it is no problem for me, than I do not care.

Point being, the alteration being done was to move to yajl, we never intended it to restrict the version, this cropped up later.

So your commit was not perfect. Why cannot you accept that and try to argument around that?

Version is usually never interesting in commit messages, the dependencies of version should be in the README.

It should be in the commit message too so the reviewer can check if the README has been updated or if the updated README was intentional.

And as said, the intent was not to restrict to 1.x but to just yajl. We are in the middle of a development cycle, if packagers want to follow now they need to understand that they might hit bumps.

Why do you want to make it difficult for outsiders and possible developers?

Overall our team is extremely active on IRC and if you had asked about yajl in there I (or someone) else would have answered within minutes most likely. When stable is released the needed information would be in README.platform.

Please read my answers above.

I'm not interested in arguing this point any further with you, the developers value small and to the point commit messages mostly.

Sorry I did not understand the sentence.

Its meant to be between devs and not for the general public.

That is a very wrong assumption in my opinion!

comment:29 Changed 3 years ago by topfs2

I've said it multiple times already. I coded support for yajl, never intended it to be restricted to 1.x, this was made clear after the commit. I have said that perhaps it should have been clear in the README but as said, it was shown after the commit was made. And I think this is a silly discussion as we are in a development cycle, stuff is in flux and all documentation is not always up to date.

Commit messages are meant to be viewable from git log and are meant to be easy to read through fast. Besides, a single commit is supposed to be a small localized fix or alteration, usually you don't need a 50 line commit message for that, nor would we ever want that big message.

And its not an assumption that the commit messages are for developers, its a decision we have made... They are meant to be used by developers (and packagers) not general population. And I see nothing in your suggested message which would help aspiring developers to be frank. What helps aspiring developers are an active IRC and when they ask questions they get answers.

You are welcome to start a discussion in forum if you want to try and get developers to be more uniform and start discussion about a strict format (and guidelines). Its extremely out of scope of this ticket which is why I wish not to argue the point further.

Anyways please can we please leave the off-topic discussion now? If you have anything remotely on topic for this particular ticket please continue, otherwise move the discussion elsewhere.

comment:30 Changed 3 years ago by Montellese

I just applied the latest patch to latest master and tested it on win32 and it still works for yajl 1.* but win32 doesn't need the manual definition of YAJL_MAJOR 1 as there is a yajl_version.h on win32 which already defines that value.

One suggestion for the commit message: If you write "(fixes #11609)" instead of "(Ticket #11609)" it will auto-close this bug report ticket as soon as your commit is pushed to xbmc master.

comment:31 Changed 3 years ago by Montellese

OK I just looked a bit closer at it and it worked on win32 but simply because YAJL_MAJOR is undefined which is != 2 but YAJL_MAJOR isn't actually defined. We have to add the HAS_YAJL_YAJL_VERSION_H define on win32 as well. I updated the last patch and rebased it so it should also contain the changes necessary for win32 now.

Just out of curiosity: why is it HAS_YAJL_YAJL_VERSION_H containing YAJL twice? looks a bit odd at a quick glance ;-)

Changed 3 years ago by Montellese

Rebased patch with necessary changes for win32

comment:32 Changed 3 years ago by [vEX]

That I don't know, I'm guessing automake/autoconf just performs a simple substitution on the string "yajl/yajl_version.h". So that the first YAJL is the directory and the second one is the filename.

Changed 3 years ago by topfs2

comment:33 Changed 3 years ago by topfs2

I removed the AC_DEFINE of HAVE_YAJL_YAJL_VERSION_H_ you added in failure of finding the header, I'm not sure if there was a reason why you added it? If there is a reason I'll push Montellese's version otherwise I will push my version, so would love your thoughts vEX :)

Thanks for the contribution!

comment:34 Changed 3 years ago by [vEX]

I though I had already removed that one after you mentioned it previously. Sorry, guess I managed to mix up the patch on my side when doing the git patch.

If you define "HAVE_YAJL_YAJL_VERSION_H" in system.h, won't it break for YAJL 1.x since it will try to include "yajl/yajl_version.h"?

comment:35 Changed 3 years ago by Github Janitor

  • Status changed from accepted to closed
  • Resolution set to Fixed

Add support for YAJL 2.x (fixes #11609)

Commmit 3565ad57 replaced jsoncpp with yajl, however there are some API differences between YAJL 1.x and 2.x. This implementation adds support for YAJL 2.0 (using version guards).

I've also added a call to yajl_free() in the destructor to clean up the handler.

Changeset: 71f5fa909908ac68f002ccb3868246341c9e4a08 By: Fredrik Wendel

comment:36 Changed 3 years ago by topfs2

  • Milestone changed from Future / Pending to 11.0

comment:37 in reply to: ↑ 22 Changed 3 years ago by sraue

Replying to topfs2:

@sraue, if you please. could you verify it with yajl 2.x, it works fine for 1.x ubuntu atleast so its valid to be pushed with your sign-off.

builds fine here, run time test will follow :-) thanks much!

if still needed: Signed-off-by: Stephan Raue <stephan@…>

Note: See TracTickets for help on using tickets.