Bug #94892 for JSON-MaybeXS: Add JSON::XS support to MaybeXS

Sun Apr 20 07:36:06 2014 develop [...] traveljury.com - Ticket created

Subject:

Add JSON::XS support to MaybeXS

This patch adds fallback support for JSON::XS. It first checks to see whether Cpanel::JSON::XS or JSON::XS is already loaded and if so, uses that module. Failing that, it tries to load Cpanel::JSON::XS, then JSON::XS then JSON::PP. Currently this module requires Cpanel::JSON::XS and JSON::PP. This should be changed to requiring JSON::PP and recommending Cpanel::JSON::XS and JSON::XS instead (imo).

Subject:

with_JSON_XS.patch

diff -ruN JSON-MaybeXS-1.001000/lib/JSON/MaybeXS.pm JSON-MaybeXS-with-JSON-XS/lib/JSON/MaybeXS.pm --- JSON-MaybeXS-1.001000/lib/JSON/MaybeXS.pm 2013-12-11 02:56:45.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/lib/JSON/MaybeXS.pm 2014-04-20 13:10:16.000000000 +0200 @@ -6,25 +6,28 @@ our $VERSION = '1.001000'; -BEGIN { - our $JSON_Class; +sub _choose_json_module { + return 'Cpanel::JSON::XS' if $INC{'Cpanel/JSON/XS.pm'}; + return 'JSON::XS' if $INC{'JSON/XS.pm'}; - our @err; + my @err; - if (eval { require Cpanel::JSON::XS; 1; }) { - $JSON_Class = 'Cpanel::JSON::XS'; - } else { + return 'Cpanel::JSON::XS' if eval { require Cpanel::JSON::XS; 1; }; push @err, "Error loading Cpanel::JSON::XS: $@"; - if (eval { require JSON::PP; 1; }) { - $JSON_Class = 'JSON::PP'; - } else { - push @err, "Error loading JSON::PP: $@"; - } - } - unless ($JSON_Class) { - die join("\n", "Couldn't load a JSON module:", @err); - } - $JSON_Class->import(qw(encode_json decode_json)); + + return 'JSON::XS' if eval { require JSON::XS; 1; }; + push @err, "Error loading JSON::XS: $@"; + + return 'JSON::PP' if eval { require JSON::PP; 1 }; + push @err, "Error loading JSON::PP: $@"; + + die join( "\n", "Couldn't load a JSON module:", @err ); + +} + +BEGIN { + our $JSON_Class = _choose_json_module(); + $JSON_Class->import(qw(encode_json decode_json)); } our @EXPORT = qw(encode_json decode_json JSON); @@ -43,7 +46,7 @@ =head1 NAME -JSON::MaybeXS - use L<Cpanel::JSON::XS> with a fallback to L<JSON::PP> +JSON::MaybeXS - use L<Cpanel::JSON::XS> with a fallback to L<JSON::XS> and L<JSON::PP> =head1 SYNOPSIS @@ -59,9 +62,10 @@ =head1 DESCRIPTION -This module tries to load L<Cpanel::JSON::XS>, and if that fails instead -tries to load L<JSON::PP>. If neither is available, an exception will be -thrown. +This module first checks to see if either L<Cpanel::JSON::XS> or +L<JSON::XS> is already loaded, in which case it uses that module. Otherwise +it tries to load L<Cpanel::JSON::XS>, then L<JSON::XS>, then L<JSON::PP> +in order, and either uses the first module it finds or throws an error. It then exports the C<encode_json> and C<decode_json> functions from the loaded module, along with a C<JSON> constant that returns the class name @@ -110,8 +114,8 @@ =head2 new -With L<JSON::PP> and L<Cpanel::JSON::XS> you are required to call mutators -to set options, i.e. +With L<JSON::PP>, L<JSON::XS> and L<Cpanel::JSON::XS> you are required to call +mutators to set options, i.e. my $json = $class->new->utf8(1)->pretty(1); diff -ruN JSON-MaybeXS-1.001000/t/cpanel.t JSON-MaybeXS-with-JSON-XS/t/cpanel.t --- JSON-MaybeXS-1.001000/t/cpanel.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/cpanel.t 2014-04-20 13:28:10.000000000 +0200 @@ -0,0 +1,24 @@ +use strict; +use warnings FATAL => 'all'; +use Test::More; +use JSON::MaybeXS; + +unless ( eval { require Cpanel::JSON::XS; 1 } ) { + plan skip_all => 'Cpanel::JSON::XS not installed'; + done_testing; + exit; +} + +is( JSON, 'Cpanel::JSON::XS', 'Correct JSON class' ); + +is( \&encode_json, + \&Cpanel::JSON::XS::encode_json, + 'Correct encode_json function' +); + +is( \&decode_json, + \&Cpanel::JSON::XS::decode_json, + 'Correct encode_json function' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/neither.t JSON-MaybeXS-with-JSON-XS/t/neither.t --- JSON-MaybeXS-1.001000/t/neither.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/neither.t 1970-01-01 01:00:00.000000000 +0100 @@ -1,16 +0,0 @@ -use strict; -use warnings FATAL => 'all'; -use Test::Without::Module 'Cpanel::JSON::XS'; -use Test::Without::Module 'JSON::PP'; -use Test::More; - -ok(!eval { require JSON::MaybeXS; 1 }, 'Class failed to load'); - -# Test::Without::Module always causes 'did not return a true value' errors - -like( - $@, qr{Cpanel/JSON/XS.pm did not.*JSON/PP.pm did not}s, - 'Both errors reported' -); - -done_testing; diff -ruN JSON-MaybeXS-1.001000/t/none.t JSON-MaybeXS-with-JSON-XS/t/none.t --- JSON-MaybeXS-1.001000/t/none.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/none.t 2014-04-20 13:30:50.000000000 +0200 @@ -0,0 +1,17 @@ +use strict; +use warnings FATAL => 'all'; +use Test::Without::Module 'Cpanel::JSON::XS'; +use Test::Without::Module 'JSON::XS'; +use Test::Without::Module 'JSON::PP'; +use Test::More; + +ok(!eval { require JSON::MaybeXS; 1 }, 'Class failed to load'); + +# Test::Without::Module always causes 'did not return a true value' errors + +like( + $@, qr{Cpanel/JSON/XS.pm did not.*JSON/XS.pm did not.*JSON/PP.pm did not}s, + 'All errors reported' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/pp.t JSON-MaybeXS-with-JSON-XS/t/pp.t --- JSON-MaybeXS-1.001000/t/pp.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/pp.t 2014-04-20 13:19:29.000000000 +0200 @@ -1,6 +1,6 @@ use strict; use warnings FATAL => 'all'; -use Test::Without::Module 'Cpanel::JSON::XS'; +use Test::Without::Module 'Cpanel::JSON::XS', 'JSON::XS'; use if !do { require JSON::PP; 1; }, 'Test::More', skip_all => 'No JSON::PP'; use Test::More; use JSON::MaybeXS; diff -ruN JSON-MaybeXS-1.001000/t/preload_cpanel.t JSON-MaybeXS-with-JSON-XS/t/preload_cpanel.t --- JSON-MaybeXS-1.001000/t/preload_cpanel.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/preload_cpanel.t 2013-12-11 02:44:43.000000000 +0100 @@ -0,0 +1,19 @@ +use strict; +use warnings FATAL => 'all'; +use if !do { require Cpanel::JSON::XS; 1; }, 'Test::More', skip_all => 'No Cpanel::JSON::XS'; +use Test::More; +use JSON::MaybeXS; + +is(JSON, 'Cpanel::JSON::XS', 'Correct JSON class'); + +is( + \&encode_json, \&Cpanel::JSON::XS::encode_json, + 'Correct encode_json function' +); + +is( + \&decode_json, \&Cpanel::JSON::XS::decode_json, + 'Correct encode_json function' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/preload_xs.t JSON-MaybeXS-with-JSON-XS/t/preload_xs.t --- JSON-MaybeXS-1.001000/t/preload_xs.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/preload_xs.t 2014-04-20 13:27:56.000000000 +0200 @@ -0,0 +1,12 @@ +use strict; +use warnings FATAL => 'all'; +use if !do { require JSON::XS; 1; }, 'Test::More', skip_all => 'No JSON::XS'; +use Test::More; +use JSON::MaybeXS; + +is( JSON, 'JSON::XS', 'Correct JSON class' ); + +is( \&encode_json, \&JSON::XS::encode_json, 'Correct encode_json function' ); +is( \&decode_json, \&JSON::XS::decode_json, 'Correct encode_json function' ); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/xs.t JSON-MaybeXS-with-JSON-XS/t/xs.t --- JSON-MaybeXS-1.001000/t/xs.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/xs.t 2014-04-20 13:24:48.000000000 +0200 @@ -1,19 +1,19 @@ use strict; use warnings FATAL => 'all'; -use if !do { require Cpanel::JSON::XS; 1; }, 'Test::More', skip_all => 'No Cpanel::JSON::XS'; + +use Test::Without::Module 'Cpanel::JSON::XS'; use Test::More; use JSON::MaybeXS; -is(JSON, 'Cpanel::JSON::XS', 'Correct JSON class'); +unless ( eval { require JSON::XS; 1 } ) { + plan skip_all => 'JSON::XS not installed'; + done_testing; + exit; +} -is( - \&encode_json, \&Cpanel::JSON::XS::encode_json, - 'Correct encode_json function' -); +is( JSON, 'JSON::XS', 'Correct JSON class' ); -is( - \&decode_json, \&Cpanel::JSON::XS::decode_json, - 'Correct encode_json function' -); +is( \&encode_json, \&JSON::XS::encode_json, 'Correct encode_json function' ); +is( \&decode_json, \&JSON::XS::decode_json, 'Correct encode_json function' ); done_testing;

Sun Apr 20 13:15:54 2014 rt-cpan [...] trout.me.uk - Correspondence added

Code-wise, the patch seems excellent. Guess I need to write some verbiage explaining the preference order before it ships. On Sun Apr 20 07:36:06 2014, DRTECH wrote: Show quoted text

> Currently this module requires Cpanel::JSON::XS and JSON::PP. This > should be changed to requiring JSON::PP and recommending > Cpanel::JSON::XS and JSON::XS instead (imo).

That's not quite accurate. What it does is, if it sees a compiler, adds Cpanel::JSON::XS to 'requires', otherwise it only puts a hard dep on JSON::PP. I would be totally happy with an updated version of this patch that changes that behaviour to "if I see a compiler, add Cpanel::JSON::XS to 'requires', unless JSON::XS is already installed, in which case add it to 'recommends'". Too many toolchain setups regard 'recommends' as human-oriented only to want to rely on that for "getting an XS module at all"

Sun Apr 20 13:15:54 2014 The RT System itself - Status changed from 'new' to 'open'

Mon Apr 21 07:50:23 2014 develop [...] traveljury.com - Correspondence added

Show quoted text

> What it does is, if it sees a compiler, adds Cpanel::JSON::XS to > 'requires', otherwise it only puts a hard dep on JSON::PP. > > I would be totally happy with an updated version of this patch that > changes that behaviour to "if I see a compiler, add Cpanel::JSON::XS > to 'requires', unless JSON::XS is already installed, in which case add > it to 'recommends'". Too many toolchain setups regard 'recommends' as > human-oriented only to want to rely on that for "getting an XS module > at all"

EU::MM is a bit of a black box to me. From my brief reading of the docs, the "recommends" section in the META files is fixed when you generate the dist, which is too early to figure out whether the end user will have a compiler or JSON::XS installed. So instead I've kept your existing logic (requiring Cpanel::JSON::XS if a compiler is available) and just added a clause to check whether JSON::XS is installed already, in which case it remos Cpanel from the required list.

Subject:

with_JSON_XS_v2.patch

diff -ruN JSON-MaybeXS-1.001000/lib/JSON/MaybeXS.pm JSON-MaybeXS-with-JSON-XS/lib/JSON/MaybeXS.pm --- JSON-MaybeXS-1.001000/lib/JSON/MaybeXS.pm 2013-12-11 02:56:45.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/lib/JSON/MaybeXS.pm 2014-04-20 13:10:16.000000000 +0200 @@ -6,25 +6,28 @@ our $VERSION = '1.001000'; -BEGIN { - our $JSON_Class; +sub _choose_json_module { + return 'Cpanel::JSON::XS' if $INC{'Cpanel/JSON/XS.pm'}; + return 'JSON::XS' if $INC{'JSON/XS.pm'}; - our @err; + my @err; - if (eval { require Cpanel::JSON::XS; 1; }) { - $JSON_Class = 'Cpanel::JSON::XS'; - } else { + return 'Cpanel::JSON::XS' if eval { require Cpanel::JSON::XS; 1; }; push @err, "Error loading Cpanel::JSON::XS: $@"; - if (eval { require JSON::PP; 1; }) { - $JSON_Class = 'JSON::PP'; - } else { - push @err, "Error loading JSON::PP: $@"; - } - } - unless ($JSON_Class) { - die join("\n", "Couldn't load a JSON module:", @err); - } - $JSON_Class->import(qw(encode_json decode_json)); + + return 'JSON::XS' if eval { require JSON::XS; 1; }; + push @err, "Error loading JSON::XS: $@"; + + return 'JSON::PP' if eval { require JSON::PP; 1 }; + push @err, "Error loading JSON::PP: $@"; + + die join( "\n", "Couldn't load a JSON module:", @err ); + +} + +BEGIN { + our $JSON_Class = _choose_json_module(); + $JSON_Class->import(qw(encode_json decode_json)); } our @EXPORT = qw(encode_json decode_json JSON); @@ -43,7 +46,7 @@ =head1 NAME -JSON::MaybeXS - use L<Cpanel::JSON::XS> with a fallback to L<JSON::PP> +JSON::MaybeXS - use L<Cpanel::JSON::XS> with a fallback to L<JSON::XS> and L<JSON::PP> =head1 SYNOPSIS @@ -59,9 +62,10 @@ =head1 DESCRIPTION -This module tries to load L<Cpanel::JSON::XS>, and if that fails instead -tries to load L<JSON::PP>. If neither is available, an exception will be -thrown. +This module first checks to see if either L<Cpanel::JSON::XS> or +L<JSON::XS> is already loaded, in which case it uses that module. Otherwise +it tries to load L<Cpanel::JSON::XS>, then L<JSON::XS>, then L<JSON::PP> +in order, and either uses the first module it finds or throws an error. It then exports the C<encode_json> and C<decode_json> functions from the loaded module, along with a C<JSON> constant that returns the class name @@ -110,8 +114,8 @@ =head2 new -With L<JSON::PP> and L<Cpanel::JSON::XS> you are required to call mutators -to set options, i.e. +With L<JSON::PP>, L<JSON::XS> and L<Cpanel::JSON::XS> you are required to call +mutators to set options, i.e. my $json = $class->new->utf8(1)->pretty(1); diff -ruN JSON-MaybeXS-1.001000/t/cpanel.t JSON-MaybeXS-with-JSON-XS/t/cpanel.t --- JSON-MaybeXS-1.001000/t/cpanel.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/cpanel.t 2014-04-20 13:28:10.000000000 +0200 @@ -0,0 +1,24 @@ +use strict; +use warnings FATAL => 'all'; +use Test::More; +use JSON::MaybeXS; + +unless ( eval { require Cpanel::JSON::XS; 1 } ) { + plan skip_all => 'Cpanel::JSON::XS not installed'; + done_testing; + exit; +} + +is( JSON, 'Cpanel::JSON::XS', 'Correct JSON class' ); + +is( \&encode_json, + \&Cpanel::JSON::XS::encode_json, + 'Correct encode_json function' +); + +is( \&decode_json, + \&Cpanel::JSON::XS::decode_json, + 'Correct encode_json function' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/neither.t JSON-MaybeXS-with-JSON-XS/t/neither.t --- JSON-MaybeXS-1.001000/t/neither.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/neither.t 1970-01-01 01:00:00.000000000 +0100 @@ -1,16 +0,0 @@ -use strict; -use warnings FATAL => 'all'; -use Test::Without::Module 'Cpanel::JSON::XS'; -use Test::Without::Module 'JSON::PP'; -use Test::More; - -ok(!eval { require JSON::MaybeXS; 1 }, 'Class failed to load'); - -# Test::Without::Module always causes 'did not return a true value' errors - -like( - $@, qr{Cpanel/JSON/XS.pm did not.*JSON/PP.pm did not}s, - 'Both errors reported' -); - -done_testing; diff -ruN JSON-MaybeXS-1.001000/t/none.t JSON-MaybeXS-with-JSON-XS/t/none.t --- JSON-MaybeXS-1.001000/t/none.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/none.t 2014-04-20 13:30:50.000000000 +0200 @@ -0,0 +1,17 @@ +use strict; +use warnings FATAL => 'all'; +use Test::Without::Module 'Cpanel::JSON::XS'; +use Test::Without::Module 'JSON::XS'; +use Test::Without::Module 'JSON::PP'; +use Test::More; + +ok(!eval { require JSON::MaybeXS; 1 }, 'Class failed to load'); + +# Test::Without::Module always causes 'did not return a true value' errors + +like( + $@, qr{Cpanel/JSON/XS.pm did not.*JSON/XS.pm did not.*JSON/PP.pm did not}s, + 'All errors reported' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/pp.t JSON-MaybeXS-with-JSON-XS/t/pp.t --- JSON-MaybeXS-1.001000/t/pp.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/pp.t 2014-04-20 13:19:29.000000000 +0200 @@ -1,6 +1,6 @@ use strict; use warnings FATAL => 'all'; -use Test::Without::Module 'Cpanel::JSON::XS'; +use Test::Without::Module 'Cpanel::JSON::XS', 'JSON::XS'; use if !do { require JSON::PP; 1; }, 'Test::More', skip_all => 'No JSON::PP'; use Test::More; use JSON::MaybeXS; diff -ruN JSON-MaybeXS-1.001000/t/preload_cpanel.t JSON-MaybeXS-with-JSON-XS/t/preload_cpanel.t --- JSON-MaybeXS-1.001000/t/preload_cpanel.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/preload_cpanel.t 2013-12-11 02:44:43.000000000 +0100 @@ -0,0 +1,19 @@ +use strict; +use warnings FATAL => 'all'; +use if !do { require Cpanel::JSON::XS; 1; }, 'Test::More', skip_all => 'No Cpanel::JSON::XS'; +use Test::More; +use JSON::MaybeXS; + +is(JSON, 'Cpanel::JSON::XS', 'Correct JSON class'); + +is( + \&encode_json, \&Cpanel::JSON::XS::encode_json, + 'Correct encode_json function' +); + +is( + \&decode_json, \&Cpanel::JSON::XS::decode_json, + 'Correct encode_json function' +); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/preload_xs.t JSON-MaybeXS-with-JSON-XS/t/preload_xs.t --- JSON-MaybeXS-1.001000/t/preload_xs.t 1970-01-01 01:00:00.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/preload_xs.t 2014-04-20 13:27:56.000000000 +0200 @@ -0,0 +1,12 @@ +use strict; +use warnings FATAL => 'all'; +use if !do { require JSON::XS; 1; }, 'Test::More', skip_all => 'No JSON::XS'; +use Test::More; +use JSON::MaybeXS; + +is( JSON, 'JSON::XS', 'Correct JSON class' ); + +is( \&encode_json, \&JSON::XS::encode_json, 'Correct encode_json function' ); +is( \&decode_json, \&JSON::XS::decode_json, 'Correct encode_json function' ); + +done_testing; diff -ruN JSON-MaybeXS-1.001000/t/xs.t JSON-MaybeXS-with-JSON-XS/t/xs.t --- JSON-MaybeXS-1.001000/t/xs.t 2013-12-11 02:44:43.000000000 +0100 +++ JSON-MaybeXS-with-JSON-XS/t/xs.t 2014-04-20 13:24:48.000000000 +0200 @@ -1,19 +1,19 @@ use strict; use warnings FATAL => 'all'; -use if !do { require Cpanel::JSON::XS; 1; }, 'Test::More', skip_all => 'No Cpanel::JSON::XS'; + +use Test::Without::Module 'Cpanel::JSON::XS'; use Test::More; use JSON::MaybeXS; -is(JSON, 'Cpanel::JSON::XS', 'Correct JSON class'); +unless ( eval { require JSON::XS; 1 } ) { + plan skip_all => 'JSON::XS not installed'; + done_testing; + exit; +} -is( - \&encode_json, \&Cpanel::JSON::XS::encode_json, - 'Correct encode_json function' -); +is( JSON, 'JSON::XS', 'Correct JSON class' ); -is( - \&decode_json, \&Cpanel::JSON::XS::decode_json, - 'Correct encode_json function' -); +is( \&encode_json, \&JSON::XS::encode_json, 'Correct encode_json function' ); +is( \&decode_json, \&JSON::XS::decode_json, 'Correct encode_json function' ); done_testing;

Tue Apr 22 13:46:30 2014 develop [...] traveljury.com - Correspondence added

Hi Matt Any news on this? I'm waiting on a new release of MaybeXS before releasing a new Elasticsearch version. ta

Tue Apr 22 14:02:56 2014 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #94892] Add JSON::XS support to MaybeXS
Date:	Tue, 22 Apr 2014 11:02:42 -0700
To:	Clinton Gormley via RT <bug-JSON-MaybeXS [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Tue, Apr 22, 2014 at 01:46:30PM -0400, Clinton Gormley via RT wrote: Show quoted text

> Hi Matt > Any news on this? I'm waiting on a new release of MaybeXS before releasing a new Elasticsearch version.

If mst asks nicely I could possibly do a release tonight (that's ~7 hours from now) ;) -- I just need a decision on how to structure the logic.

Wed Apr 23 01:48:58 2014 ether [...] cpan.org - Correspondence added

On 2014-04-21 04:50:23, DRTECH wrote: Show quoted text

> So instead I've kept your existing logic (requiring Cpanel::JSON::XS > if a compiler is available) and just added a clause to check whether > JSON::XS is installed already, in which case it remos Cpanel from the > required list.

BTW, the two patches are identical. But I can fix it to do what you describe here, and add to the 'recommends' prereq list as well.

Wed Apr 23 02:22:13 2014 ether [...] cpan.org - Correspondence added

1.002000 is released. Thanks Clinton!

Wed Apr 23 02:22:14 2014 ether [...] cpan.org - Status changed from 'open' to 'resolved'

Wed Apr 23 06:01:12 2014 develop [...] traveljury.com - Correspondence added

On Wed Apr 23 02:22:13 2014, ETHER wrote: Show quoted text

> 1.002000 is released. Thanks Clinton!

And many thanks to you Karen