From 13bdb75b2961e1ae43e43e7c8d06bf463571d541 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Fri, 11 Aug 2017 13:58:13 -0400 Subject: 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. --- src/rebar3.erl | 4 ++-- src/rebar_opts.erl | 18 +++++++++++++++++- test/rebar_profiles_SUITE.erl | 33 ++++++++++++++++++++++++++++++++- 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), -- cgit v1.1 From bed661aef80d113ae04e8e9da035f904b7c4aec4 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sun, 13 Aug 2017 15:30:03 -0400 Subject: Clarify function to normalise profile pairs --- src/rebar_opts.erl | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/rebar_opts.erl b/src/rebar_opts.erl index e327e31..8863b4c 100644 --- a/src/rebar_opts.erl +++ b/src/rebar_opts.erl @@ -117,8 +117,10 @@ merge_opt(plugins, NewValue, _OldValue) -> merge_opt({plugins, _}, NewValue, _OldValue) -> NewValue; merge_opt(profiles, NewValue, OldValue) -> - ToMerge = fill_profile_gaps(lists:sort(NewValue), - lists:sort(OldValue)), + %% Merge up sparse pairs of {Profile, Opts} into a joined up + %% {Profile, OptsNew, OptsOld} list. + ToMerge = normalise_profile_pairs(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) -> @@ -194,15 +196,25 @@ filter_defines([{platform_define, ArchRegex, Key, Value} | Rest], Acc) -> filter_defines([Opt | Rest], Acc) -> filter_defines(Rest, [Opt | Acc]). -fill_profile_gaps([], []) -> +%% @private takes two lists of profile tuples and merges them +%% into one list of 3-tuples containing the values of either +%% profiles. +%% Any missing profile in one of the keys is replaced by an +%% empty one. +-spec normalise_profile_pairs([Profile], [Profile]) -> [Pair] when + Profile :: {Name, Opts}, + Pair :: {Name, Opts, Opts}, + Name :: atom(), + Opts :: [term()]. +normalise_profile_pairs([], []) -> []; -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 +normalise_profile_pairs([{P,V}|Ps], []) -> + [{P,V,[]} | normalise_profile_pairs(Ps, [])]; +normalise_profile_pairs([], [{P,V}|Ps]) -> + [{P,[],V} | normalise_profile_pairs([], Ps)]; +normalise_profile_pairs([{P,VA}|PAs], [{P,VB}|PBs]) -> + [{P,VA,VB} | normalise_profile_pairs(PAs, PBs)]; +normalise_profile_pairs([{PA,VA}|PAs], [{PB,VB}|PBs]) when PA < PB -> + [{PA,VA,[]} | normalise_profile_pairs(PAs, [{PB, VB}|PBs])]; +normalise_profile_pairs([{PA,VA}|PAs], [{PB,VB}|PBs]) when PA > PB -> + [{PB,[],VB} | normalise_profile_pairs([{PA,VA}|PAs], PBs)]. -- cgit v1.1