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

Specifying macros in test profile in umbrella app does not seem to have an effect #1566

Closed
larshesel opened this issue Jun 5, 2017 · 5 comments · Fixed by #1606
Closed

Comments

@larshesel
Copy link

Maybe related to #1477 ?

Environment

$ rebar3 report as test compile
Rebar3 report
 version 3.4.1
 generated at 2017-06-05T11:53:44+00:00
=================
Please submit this along with your issue at https://github.com/erlang/rebar3/issues (and feel free to edit out private information, if any)
-----------------
Task: astestcompile
Entered as:
  astestcompile
-----------------
Operating System: x86_64-unknown-linux-gnu
ERTS: Erlang/OTP 19 [erts-8.3.2] [source] [64-bit] [smp:4:4] [async-threads:0] [hipe] [kernel-poll:false]
Root Directory: /home/opt/erlang/erlang19.3.2
Library directory: /home/opt/erlang/erlang19.3.2/lib
-----------------
Loaded Applications:
bbmustache: 1.3.0
certifi: 0.4.0
cf: 0.2.2
common_test: 1.14
compiler: 7.0.4
crypto: 3.7.4
cth_readable: 1.2.6
dialyzer: 3.1
edoc: 0.8.1
erlware_commons: 1.0.0
eunit: 2.3.2
eunit_formatters: 0.3.1
getopt: 0.8.2
inets: 6.3.7
kernel: 5.2
providers: 1.6.0
public_key: 1.4
relx: 3.23.0
sasl: 3.0.3
snmp: 5.2.5
ssl_verify_fun: 1.1.1
stdlib: 3.3
syntax_tools: 2.1.1
tools: 2.9.1

-----------------
Escript path: /home/opt/bin/rebar3
Providers:
  app_discovery as clean compile compile cover ct deps dialyzer do edoc escriptize eunit get-deps help install install_deps list lock new path pkgs release relup report shell state tar tree unlock update upgrade upgrade upgrade version xref 

Current behaviour

I have an umbrella app and I would like to define a macro with different values
for different profiles, and in particular I'd like to define a specific value
when using the test profile.

I therefore created an umbrella project looking like this:

./rebar.config
./apps
./apps/app1
./apps/app1/src
./apps/app1/src/app1_sup.erl
./apps/app1/src/app1_app.erl
./apps/app1/src/app1.app.src
./apps/app1/rebar.config

I pushed the repo here: https://github.com/larshesel/umbr

with the top-level rebar.config file looking like this:

{deps, []}.

And the app1 rebar.config file looking like this:

{deps, []}.


{profiles,
 [
  {sth,
   [{erl_opts, 
     [{d, 'MYMACRO', sth}]
    }]
  },


  {test,
   [{erl_opts, 
     [{d, 'MYMACRO', test}]
    }]
  }]}.

And in the apps/app1/src/app1_app.erl I reference the macro.

I would then expect that I could then run rebar3 as test compile and the macro
would be correctly be defined, but the profile doesn't seem to be include or is ignored:

$ rebar3 as test compile
===> Verifying dependencies...
===> Compiling app1
===> Compiling apps/app1/src/app1_app.erl failed
apps/app1/src/app1_app.erl:18: undefined macro 'MYMACRO'

apps/app1/src/app1_app.erl:11: function start/2 undefined

On the other hand compiling as the sth profile works:

$ rebar3 as sth compile
===> No entry for profile sth in config.
===> Verifying dependencies...
===> Compiling app1

Which makes me wonder why one profile works and another not?

Expected behaviour

I expected the MYMACRO to be defined when running rebar3 as test compile and the compilation to work.

I was wondering if I might need to try and override the test profile in the umbrella app and tried to do so without getting that to work either. Maybe I was just not doing that correctly? Should it work without overriding or not?

@ferd
Copy link
Collaborator

ferd commented Aug 11, 2017

I had not seen this issue previously, sorry about that. I was just able to reproduce the issue and am investigating.

@ferd
Copy link
Collaborator

ferd commented Aug 11, 2017

I believe the problem is that the test profile auto-inserts an erl_opts option, which gets mixed up with the actual erl_opts from the profile at run-time. At some point the list of options is turned into a dict and only the default options get kept. I'll try to locate where exactly that happens.

@ferd
Copy link
Collaborator

ferd commented Aug 11, 2017

So I think the problem here is that rebar3 automatically inserts a value {profiles, [{test, [{erl_opts, [{d, 'TEST'}]}]}]} in every application. A top-level config of {erl_opts, Whatever} will find itself properly merged when in a test profile.

However, when in an umbrella setting under the test profile, the initial {d, 'TEST'} macro is part of the parent config for the test profile. However, when merging options between applications, profiles are not resolved recursively. This means that special rules used on the top-level config break, and instead of merging both erl_opts lists, the two are seen as distinct duplicate config entries.

At some point later, the list is transformed to a dict, and one of the erl_opts entries for the profile is lost.

I think the same underlying behaviour causes problems in the following issues as well as this one:

@tsloughter how do you feel about us finding a way to make the application of merge_opts rule work fine recursively within each profile? I don't know if it would fix things, but I think it would. See:

rebar3/src/rebar_opts.erl

Lines 119 to 120 in d906476

merge_opt(profiles, NewValue, OldValue) ->
dict:to_list(merge_opts(dict:from_list(NewValue), dict:from_list(OldValue)));

The problem being that when the function is applied recursively, it is applied to each profile (so it will merge on the keys test, prod, etc.) rather than to each of the values of each profile. If we made it so we paired-up profile names as keys in a zipped list, we'd get a recursive application.

I'm afraid this might be some form of breaking change though. Not 100% sure about it.

@ferd
Copy link
Collaborator

ferd commented Aug 11, 2017

Potential fix at #1606

@larshesel
Copy link
Author

larshesel commented Aug 11, 2017

Thanks for taking a look at this and I'm happy to report that your PR seems to fix my issue. I'll test this rebar3 version against VerneMQ and some other projects to see if I can detect any adverse effects.

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

Successfully merging a pull request may close this issue.

2 participants