sile / jsone

Erlang JSON library
MIT License
291 stars 72 forks source link

fix compatibility with Erlang OTP 21 #34

Closed benoitc closed 6 years ago

benoitc commented 6 years ago

On Erlang OTP 21 it will uses the new syntax to retrieve the stacktrace from the try catch. With other Erlang erlang:get_stacktrace/0 will be used.

fix #33

codecov-io commented 6 years ago

Codecov Report

Merging #34 into master will decrease coverage by 0.28%. The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #34      +/-   ##
=========================================
- Coverage   88.38%   88.1%   -0.29%     
=========================================
  Files           3       3              
  Lines         267     269       +2     
=========================================
+ Hits          236     237       +1     
- Misses         31      32       +1
Impacted Files Coverage Δ
src/jsone.erl 31.25% <40%> (+2.67%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9a5ee50...c989d7e. Read the comment docs.

benoitc commented 6 years ago

I'm not sure how you can trick this coverage tool to handle the usage of this new macro. The code is working there. If it needs some changes let me know as I would like to have a working version of it in hex asap :)

benoitc commented 6 years ago

Other solution would be using stacktrace_compat but I was not sure you would be OK to use a dependancy. Just let me know I can provide a PR for it.

sile commented 6 years ago

It seems great to use OTP RELEASE macro for determining whether the Erlang version is OTP-21 or later However, OTP-21 already has been supported since https://github.com/sile/jsone/commit/42125d3c96ad138683221b29a1c80b7c6b23b149. So, I want to know the reason why site:master does not work well in your environment (maybe the same reason as #32 ?).

Other solution would be using stacktrace_compat ...

stacktrace_compat is a good library, but I would like to keep jsone have no dependencies as much as possible.

benoitc commented 6 years ago

I don’t think it’s the same issue. Using latest rebar3 to only compile jsone failed the same way.

sile commented 6 years ago

Hmm...

In my environment (OTP-21), jsone can be compiled as follows:

$ erl -eval 'halt().'
Erlang/OTP 21 [erts-10.0] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1]

$ rebar3 --version
rebar 3.6.1 on Erlang/OTP 21 Erts 10.0

$ git clone git://github.com/sile/jsone
$ cd jsone
$ git rev-parse HEAD
9a5ee505278a824e5d30195ef99e977972acd93a
$ rebar3 compile
===> Verifying dependencies...
===> Compiling jsone

$ rebar3 shell
> jsone:decode(<<"{}">>).
#{}

In addition, the latest CI also succeeded (it includes OTP-21 as a target environment). So I do not know how to reproduce the warnings reported in #33 ...

benoitc commented 6 years ago

OK. So, I've started a new build from scratch and sone alone pass. But not as a dependency that indeed has its own erl_opts same version of rebar3.

benoitc commented 6 years ago

here is the rebar.config that use jsone a deep:

%% == Erlang Compiler ==

{erl_opts, [
  warn_unused_vars,
  %warnings_as_errors,
  warn_export_all,
  warn_shadow_vars,
  %warn_unused_import,
  warn_unused_function,
  warn_bif_clash,
  warn_unused_record,
  warn_deprecated_function,
  warn_obsolete_guard,
  strict_validation,
  warn_export_vars,
  warn_exported_vars,
%%  warn_missing_spec,
%%  warn_untyped_record,
  debug_info,
  {d, 'DEFAULT_PORT', 7080},

  {parse_transform, lager_transform}
]}.

%% == Dependencies ==

{deps, [

  %% uuid lib
  {uuid, {pkg, uuid_erl}},
  {jsx, "2.8.1"},
  {jsone, "1.4.3"},

  %% client and http deps
  {cowboy, "2.4.0"},

  {jsx, "2.8.1"},
  {hackney, "1.8.5"}
]}.
benoitc commented 6 years ago

I am curious where is documented this __Stacktrace usage; If you prefer I can revisit the patch using 'OTP_RELEASE' instead of the define. Thoughts?

benoitc commented 6 years ago

bump. Actually sone Is unusable without the patch I provided, let me know how to proceed next..

benoitc commented 6 years ago

following my comment in #33 I think the PR should be closed until you want to use OTP_RELEASE instead. thoughts?

sile commented 6 years ago

I will create a new ticket for using OTP_RELEASE ( #33 )

This is welcome. I prefers to use OTP_RELEASE macro instead of FUN_STACKTRACE for this purpose.

sile commented 6 years ago

Clould you modify this PR based on the current master implementation (i.e., use CAPTURE_STACKTRACE and GET_STACKTRACE macros instead of TRY_FUN) and then replace FUN_STACKTRACE by OTP_RELEASE ?

sile commented 6 years ago

At the commit https://github.com/sile/jsone/commit/775b3d02eb483ddec2a98b09c0670ecc0ddde5fa , I modified the code to use the OTP_RELEASE macro instead of self defined FUN_STACKTRACE. I will close this PR, but thank you for letting me know the existence of the macro.