sile / jsone

Erlang JSON library
MIT License
291 stars 72 forks source link

jsone fails to compile when setting overrides to enable HiPE #32

Closed nalundgaard closed 6 years ago

nalundgaard commented 6 years ago

In the README's instructions for compiling jsone with HiPE, it says:

If you want to use HiPE compiled version, please add following code to your rebar.config.

{overrides,
  [
    {override, jsone, [{erl_opts, [{d, 'ENABLE_HIPE'}, inline]}]}
  ]}.

However, this has the unfortunate side effect of erasing the existing erl_opts in the rebar.config, including the platform_define settings, which make it very likely that the compilation will fail on pre-OTP 21 versions of Erlang.

Can the README be updated to provide instructions for how to enable HiPE compilation in the context of jsone as a dependency?

Note: when I filed this issue, I did not realize that it was because of my overrides, so the problem statement was incomplete. Here is the text of the original issue (for context): As of the latest changes, it seems that jsone is no longer compatible with compiling on OTP 20 (and probably older releases as well) when it is added as a dependency:

./rebar3 compile
===> Verifying dependencies...
===> Compiling jsone
===> Compiling _build/default/lib/jsone/src/jsone.erl failed
_build/default/lib/jsone/src/jsone.erl:304: illegal pattern
_build/default/lib/jsone/src/jsone.erl:305: variable 'Reason' is unbound
_build/default/lib/jsone/src/jsone.erl:305: variable 'StackItem' is unbound
_build/default/lib/jsone/src/jsone.erl:305: variable '__StackTrace' is unbound
_build/default/lib/jsone/src/jsone.erl:353: illegal pattern
_build/default/lib/jsone/src/jsone.erl:354: variable 'Reason' is unbound
_build/default/lib/jsone/src/jsone.erl:354: variable 'StackItem' is unbound
_build/default/lib/jsone/src/jsone.erl:354: variable '__StackTrace' is unbound

This seems related to 0e42866. I am not sure what happened. It looks like it is written right, and it compiles when used directly (rebar3 compile from inside the jsone folder itself), but when it's a dependency of my project, it fails.

jadeallenx commented 6 years ago

Seems like the define isn't set correctly when rebar3 is going over the dependencies. Is this possibly a rebar3 problem?

nalundgaard commented 6 years ago

That does seem to be the case. If so, it's in the current rebar 3.6.0.

sile commented 6 years ago

In my environment, the following commands worked successfully.

$ rebar3 --version
rebar 3.6.0+build.4110.ref4cb15b19 on Erlang/OTP 20 Erts 9.3.3

$ rebar3 new app foo
$ cd foo/
$ echo '{deps, [jsone]}.' > rebar.config

$ rebar3 compile
===> Verifying dependencies...
===> Fetching jsone ({pkg,<<"jsone">>,<<"1.4.6">>})
===> Version cached at /root/.cache/rebar3/hex/default/packages/jsone-1.4.6.tar is up to date, reusing it
===> Compiling jsone
===> Compiling foo

$ cat rebar.lock
{"1.1.0",
[{<<"jsone">>,{pkg,<<"jsone">>,<<"1.4.6">>},0}]}.
[
{pkg_hash,[
 {<<"jsone">>, <<"644D6D57BEFB22C8E19B324DEE19D73B1C004565009861A8F64C68B7B9E64DBF">>}]}
].

Is it possible to tell me the content of your rebar.config file?

nalundgaard commented 6 years ago

🤦‍♂️ I found the problem, and it was on our side, from following the README:

{overrides, [
    {override, jsone, [{erl_opts, [{d, 'ENABLE_HIPE'}, inline]}]}
]}.

Is there a better way to do this?

nalundgaard commented 6 years ago

I am going to update the issue title according to actually understanding the problem: that customizing erl_opts for jsone through rebar3 overrides breaks compilation by steamrolling the platform_define-set macros.

nalundgaard commented 6 years ago

As an update to this, I did a little more digging today, and I noticed that rebar3 supports add in the overrides list, like so:

{overrides, [{add, app_name(), [{atom(), any()}]}]}.

add sure sounds like what I want: I want to add this additional erl_opts flag to whatever the defaults are for jsone So I tried the following:

$ rebar3 --version
rebar 3.6.0 on Erlang/OTP 20 Erts 9.1.5

$ rebar3 new app jsone_test

$ cd jsone_test

$ cat << EOF > rebar.config
{deps, [jsone]}.
{overrides, [
    {add, jsone, [{erl_opts, [{d, 'ENABLE_HIPE'}, inline]}]}
]}.
EOF

This results in the following issue:

$ DEBUG=1 rebar3 compile
===> Load global config file ~/.config/rebar3/rebar.config
===> Expanded command sequence to be run: [{default,app_discovery},
                                                  {default,install_deps},
                                                  {default,lock},
                                                  {default,compile}]
===> Provider: {default,app_discovery}
===> Provider: {default,install_deps}
===> Verifying dependencies...
===> Provider: {default,lock}
===> Provider: {default,compile}
===> Compiling jsone
===> run_hooks("~/src/jsone_test/_build/default/lib/jsone", pre_hooks, compile) -> no hooks defined

===> run_hooks("~/src/jsone_test/_build/default/lib/jsone", pre_hooks, erlc_compile) -> no hooks defined

===> erlopts [debug_info,
                     {d,'ENABLE_HIPE'},
                     inline,
                     {d,'ENABLE_HIPE'},
                     warnings_as_errors,warn_export_all,warn_untyped_record,
                     inline,
                     {d,'NO_DIALYZER_SPEC'},
                     {d,'FUN_STACKTRACE'}]
===> files to compile ["~/src/jsone_test/_build/default/lib/jsone/src/jsone_encode.erl",
                              "~/src/jsone_test/_build/default/lib/jsone/src/jsone_decode.erl",
                              "~/src/jsone_test/_build/default/lib/jsone/src/jsone.erl"]
===> Compiling _build/default/lib/jsone/src/jsone_encode.erl failed
_build/default/lib/jsone/src/jsone_encode.erl:none: redefining macro 'ENABLE_HIPE'

===> Compilation failed: {error,
                                    [["_build/default/lib/jsone/src/jsone_encode.erl:none: redefining macro 'ENABLE_HIPE'\n"]],
                                    []}

It seems like {d,'ENABLE_HIPE'} (and, strangely, inline) are both getting added 2 times in this case.

@ferd @tsloughter does this sound like a rebar3 problem to you all? should I file an issue?

ferd commented 6 years ago

We have a ticket open for that at https://github.com/erlang/rebar3/issues/1801

sile commented 6 years ago

@nalundgaard

I fixed this problem at jsone-v1.4.7.

The up-to-date jsone can be compiled in a non OTP-21 environment even if rebar.config contains the following entry:

{overrides,
  [
    {override, jsone, [{erl_opts, [{d, 'ENABLE_HIPE'}, inline]}]}
  ]}.
jadeallenx commented 6 years ago

Thanks!!

nalundgaard commented 6 years ago

Thanks, that is awesome! Much appreciated workaround.