Fix recursive profile merging in umbrella apps

When a config file exists at the root of a project, defines a given
configuration value for a given profile, and that a sub-application
(umbrella app) also has the same profile defined with the same key (but
different values), the configuration values of the sub-application's
profile would get silently dropped.

The problem being that when the function to merge profiles 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.

This patch reworks the profile merging so that the current behaviour is
respected overall (a profile cannot be cancelled by a subdep's
non-existant profile since its value should have been ignored), but
ensures that sub-deps' profiles are otherwise applied recursively with
the proper rules:

- dependencies favor prior values
- plugins favor new values
- erl_first_files combine the lists
- relx uses the tuple merge algorithm
- erl_opts has its own custom merge as well
- otherwise the new value takes precedence

A test has also been added.

There is a risk of breakage in some applications that may have relied on
the buggy behaviour to work, though at this time we are aware of none of
them.

3 files changed, 51 insertions, 4 deletions

diff --git a/src/rebar3.erl b/src/rebar3.erl
index eec8968..2b24bca 100644
--- a/src/rebar3.erl
+++ b/src/rebar3.erl
@@ -396,8 +396,8 @@ safe_define_test_macro(Opts) ->
     %% defining a compile macro twice results in an exception so
     %% make sure 'TEST' is only defined once
     case test_defined(Opts) of
-       true  -> [];
-       false -> [{d, 'TEST'}]
+       true  -> Opts;
+       false -> [{d, 'TEST'}|Opts]
     end.
 
 test_defined([{d, 'TEST'}|_]) -> true;
diff --git a/src/rebar_opts.erl b/src/rebar_opts.erl
index 589dbb8..e327e31 100644
--- a/src/rebar_opts.erl
+++ b/src/rebar_opts.erl
@@ -117,7 +117,10 @@ merge_opt(plugins, NewValue, _OldValue) ->
 merge_opt({plugins, _}, NewValue, _OldValue) ->
     NewValue;
 merge_opt(profiles, NewValue, OldValue) ->
-    dict:to_list(merge_opts(dict:from_list(NewValue), dict:from_list(OldValue)));
+    ToMerge = fill_profile_gaps(lists:sort(NewValue),
+                                lists:sort(OldValue)),
+    [{K,dict:to_list(merge_opts(dict:from_list(New), dict:from_list(Old)))}
+     || {K,New,Old} <- ToMerge];
 merge_opt(erl_first_files, Value, Value) ->
     Value;
 merge_opt(erl_first_files, NewValue, OldValue) ->
@@ -190,3 +193,16 @@ filter_defines([{platform_define, ArchRegex, Key, Value} | Rest], Acc) ->
     end;
 filter_defines([Opt | Rest], Acc) ->
     filter_defines(Rest, [Opt | Acc]).
+
+fill_profile_gaps([], []) ->
+    [];
+fill_profile_gaps([{P,V}|Ps], []) ->
+    [{P,V,[]} | fill_profile_gaps(Ps, [])];
+fill_profile_gaps([], [{P,V}|Ps]) ->
+    [{P,[],V} | fill_profile_gaps([], Ps)];
+fill_profile_gaps([{P,VA}|PAs], [{P,VB}|PBs]) ->
+    [{P,VA,VB} | fill_profile_gaps(PAs, PBs)];
+fill_profile_gaps([{PA,VA}|PAs], [{PB,VB}|PBs]) when PA < PB ->
+    [{PA,VA,[]} | fill_profile_gaps(PAs, [{PB, VB}|PBs])];
+fill_profile_gaps([{PA,VA}|PAs], [{PB,VB}|PBs]) when PA > PB ->
+    [{PB,[],VB} | fill_profile_gaps([{PA,VA}|PAs], PBs)].
\ No newline at end of file
diff --git a/test/rebar_profiles_SUITE.erl b/test/rebar_profiles_SUITE.erl
index 9ffaf98..11aca6a 100644
--- a/test/rebar_profiles_SUITE.erl
+++ b/test/rebar_profiles_SUITE.erl
@@ -7,6 +7,7 @@
          all/0,
          profile_new_key/1,
          profile_merge_keys/1,
+         profile_merge_umbrella_keys/1,
          explicit_profile_deduplicate_deps/1,
          implicit_profile_deduplicate_deps/1,
          all_deps_code_paths/1,
@@ -33,7 +34,8 @@
 -include_lib("kernel/include/file.hrl").
 
 all() ->
-    [profile_new_key, profile_merge_keys, all_deps_code_paths, profile_merges,
+    [profile_new_key, profile_merge_keys, profile_merge_umbrella_keys,
+     all_deps_code_paths, profile_merges,
      explicit_profile_deduplicate_deps, implicit_profile_deduplicate_deps,
      same_profile_deduplication, stack_deduplication,
      add_to_profile, add_to_existing_profile,
@@ -118,6 +120,35 @@ profile_merge_keys(Config) ->
                                                                  ,{dep, "a", "1.0.0"}
                                                                  ,{dep, "b", "2.0.0"}]}).
 
+profile_merge_umbrella_keys(Config) ->
+    AppDir = ?config(apps, Config),
+    ct:pal("Path: ~s", [AppDir]),
+    Name = rebar_test_utils:create_random_name("profile_merge_umbrella_keys"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    SubAppDir = filename:join([AppDir, "apps", Name]),
+
+    RebarConfig = [{vals, [{a,1},{b,1}]},
+                   {profiles,
+                    [{ct,
+                      [{vals, [{a,1},{b,2}]}]}]}],
+    
+    SubRebarConfig = [{vals, []},
+                       {profiles, [{ct, [{vals, [{c,1}]}]}]}],
+
+    rebar_test_utils:create_app(SubAppDir, Name, Vsn, [kernel, stdlib]),
+    rebar_test_utils:create_config(SubAppDir, SubRebarConfig),
+    {ok, RebarConfigRead} = file:consult(rebar_test_utils:create_config(AppDir, RebarConfig)),
+
+    {ok, State} = rebar_test_utils:run_and_check(
+        Config, RebarConfigRead, ["as", "ct", "compile"], return
+    ),
+
+    [ProjectApp] = rebar_state:project_apps(State),
+    ?assertEqual(Name, binary_to_list(rebar_app_info:name(ProjectApp))),
+    Opts = rebar_app_info:opts(ProjectApp),
+    ?assertEqual([{a,1},{b,2},{b,1},{c,1}], dict:fetch(vals, Opts)),
+    ok.
+
 explicit_profile_deduplicate_deps(Config) ->
     AppDir = ?config(apps, Config),