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

changing cjson dependency to another json library #6

Open
msva opened this issue Oct 2, 2015 · 9 comments
Open

changing cjson dependency to another json library #6

msva opened this issue Oct 2, 2015 · 9 comments

Comments

@msva
Copy link

msva commented Oct 2, 2015

Hi there!
Original lua-cjson library seems to be abandoned by @mpx (he has no activity for more that year and all PRs are ignored), and the one in @openresty fork has no Lua5.2/5.3 compatibilty too (and also has no PRs for fixing another bugs).

So, how do you think about migrating to another json library?

@calio
Copy link
Contributor

calio commented Oct 2, 2015

Do you have a particular one in mind?

@msva
Copy link
Author

msva commented Oct 2, 2015

Actually, I've not benchmarked them, but https://github.com/harningt/luajson looks at least alive ;)
Although, I've anyway already sent a 5.2/5.3-compat PR to @openresty's cjson fork

@catwell
Copy link

catwell commented Nov 6, 2015

@msva Why do you say cjson is not 5.3 compatible? There is a version in LuaRocks, called lua-cjson, which works well with 5.3 for me. All raven-lua tests pass using it.

That being said, luajson is almost completely compatible with cjson. All tests pass using it as well. Supporting it in addition to cjson would be a good idea, especially for environments where you cannot use C modules (luajson is pure Lua).

This can easily be done by replacing local json = require "cjson" by:

local json
do
   local ok
   ok, json = pcall(require, "cjson")
   if not ok then json = require "json" end
end

@msva
Copy link
Author

msva commented Dec 5, 2015

@catwell I didn't say cjson isn't compatible with 5.3.
I only said that both cjson repos: original (@mpx) and @openresty one aren't (at the moment of that comment, at least). As for me, I using my own cjson fork. But since I maintaining lua packages ibn the distro, it'd be nice if there will be kinda "official" repo with "compatible" cjson ;)

@catwell
Copy link

catwell commented Dec 5, 2015

Actually the one in LuaRocks is the one from @mpx and it is compatible with 5.3, except that it decodes integers to Lua floating point numbers, but that isn't such a big issue IMO.

@msva
Copy link
Author

msva commented Dec 9, 2015

@catwell doubt it:

  1. it is on "luarocks" account, but not on @mpx,
  2. @mpx has neither commited in github repo nor released new releases on homepage for about 4 years.

So, there is definitely some fork.

@calio, btw, I've found https://github.com/bungle/lua-resty-libcjson (same on LR). Looks neat.

@catwell
Copy link

catwell commented Dec 9, 2015

@msva Being on the "luarocks" account just means it wasn't adopted by its author since the move to moonrocks. The rockspec makes it clear it is not a fork.

The release is from March 2012 but it still works with Lua 5.3.

$ lua -v
Lua 5.3.1  Copyright (C) 1994-2015 Lua.org, PUC-Rio
$ luarocks list | grep -A1 cjson
lua-cjson
   2.1.0-1 (installed) - /usr/lib/luarocks/rocks-5.3
$ make
Loaded testsuite with 12 tests in 3 testcases.

[ERROR] Failed to send to Sentry:   connection refused      {"level":"error","message":"IF YOU ARE READING THIS IT IS CORRECT; THIS TEST SHOULD GENERATE AN ERROR.","server_name":"undefined","timestamp":"2015-12-09T08:32:33","event_id":"d9d72cb5635c41ad8b239b598efcf148","culprit":"tests\/test_exceptions.lua:17","logger":"root","platform":"lua"}
    .[ERROR]    Failed to send to Sentry:   connection refused      {"level":"error","timestamp":"2015-12-09T08:32:33","culprit":"error","platform":"lua","event_id":"111e94a4a18f4e9a8c5cff848028d291","message":"tests\/test_exceptions.lua:28: bad","exception":[{"stacktrace":{"frames":[{"lineno":-1,"filename":"[C]"},{"lineno":11,"filename":"stdin"},{"lineno":540,"filename":"\/usr\/share\/lua\/5.3\/lunit.lua"},{"function":"runtest","lineno":522,"filename":"\/usr\/share\/lua\/5.3\/lunit.lua"},{"function":"callit","lineno":504,"filename":"\/usr\/share\/lua\/5.3\/lunit.lua"},{"function":"mypcall","lineno":108,"filename":"\/usr\/share\/lua\/5.3\/lunit.lua"},{"function":"xpcall","lineno":-1,"filename":"[C]"},{"lineno":28,"filename":"tests\/test_exceptions.lua"},{"function":"xpcall","lineno":-1,"filename":"[C]"},{"lineno":28,"filename":"tests\/test_exceptions.lua"},{"function":"error","lineno":-1,"filename":"[C]"}]},"value":"tests\/test_exceptions.lua:28: bad"}],"server_name":"undefined","logger":"root"}
...........

60 Assertions checked.

Testsuite finished (12 passed, 0 failed, 0 errors).

@msva
Copy link
Author

msva commented Dec 9, 2015

Talking about original message in the issue, I said "@mpx one is abandoned, and @openresty fork - has no 5.2/5.3 compatibility) :)

// btw, by default, CMake there, in @mpx repo, has issues with finding (and linking against) luajit. Which I (kinda) fixed.

@catwell
Copy link

catwell commented Dec 9, 2015

The OpenResty fork has the same 5.3 compatibility as the @mpx version: it just does not have special code for integers and considers them as doubles. So it works exactly the same as with 5.2.

The only differences between the OpenResty fork and the original are:

  • an option to encode empty JSON tables as arrays which is not used by raven-lua;
  • better precision when encoding very large numbers (more than 14 decimal places).

If the mpx version is abandonned we will just replace it with the OpenResty version in LuaRocks. Also it looks like OpenResty will merge your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants