From 2d430bf3e2766ad98e9a859a0de246abfd4eee3e Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sun, 22 Mar 2015 21:39:36 -0400 Subject: Preserve profile order on mergeable tuple/opts Rather than using the stdlib lists:umerge, we expand it to allow fuzzy matching on tuples vs. vals (`key` vs. `{key,val}`) with an overriden sort order so that two tuples or values comparing equal get a priority on the newest profile. This is a partial fix for #287 -- this current patch should be followed by a relx update to take options in order (as if they were a proplist) to complete it. --- src/rebar_state.erl | 58 ++++++++++++++++++++++++++++++++++++++++++- test/rebar_profiles_SUITE.erl | 11 ++++++-- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/rebar_state.erl b/src/rebar_state.erl index 008f202..eced383 100644 --- a/src/rebar_state.erl +++ b/src/rebar_state.erl @@ -240,7 +240,7 @@ merge_opts(NewOpts, OldOpts) -> true -> NewValue; false -> - lists:umerge(lists:sort(NewValue), lists:sort(OldValue)) + tup_umerge(lists:sort(NewValue), lists:sort(OldValue)) end; (_Key, NewValue, _OldValue) -> NewValue @@ -362,3 +362,59 @@ add_hook(pre, {PreHooks, PostHooks}, Hook) -> {[Hook | PreHooks], PostHooks}; add_hook(post, {PreHooks, PostHooks}, Hook) -> {PreHooks, [Hook | PostHooks]}. + +%% Custom merge functions. The objective is to behave like lists:umerge/2, +%% except that we also compare the merge elements based on the key if they're a +%% tuple, such that `{key, val1}' is always prioritized over `{key, val0}' if +%% the former is from the 'new' list. +%% +%% This lets us apply proper overrides to list of elements according to profile +%% priority. +tup_umerge([], Olds) -> + Olds; +tup_umerge(News, Olds) -> + [ENew|ENews] = expand(News), + EOlds = expand(Olds), + unexpand(lists:reverse(umerge(ENews, EOlds, [], ENew))). + +%% Expand values, so they `key' is now `{key, key}', and so that +%% `{key, val}' is now `{key, {key, val}'. This allows us to compare +%% possibly only on the total key or the value itself. +expand([]) -> []; +expand([Tup|T]) when is_tuple(Tup) -> [{element(1, Tup), Tup} | expand(T)]; +expand([H|T]) -> [{H,H} | expand(T)]. + +%% Go back to unexpanded form. +unexpand(List) -> [element(2, X) || X <- List]. + +%% This is equivalent to umerge2_2 in the stdlib, except we use the expanded +%% value/key only to compare +umerge(News, [{KOld,_}=Old|Olds], Merged, {KCmp, _} = Cmp) when KCmp =< KOld -> + umerge(News, Olds, [Cmp | Merged], Cmp, Old); +umerge(News, [Old|Olds], Merged, Cmp) -> + umerge(News, Olds, [Old | Merged], Cmp); +umerge(News, [], Merged, Cmp) -> + lists:reverse(News, [Cmp | Merged]). + +%% Similar to stdlib's umerge2_1 in the stdlib, except that when the expanded +%% value/keys compare equal, we check if the element is a full dupe to clear it +%% (like the stdlib function does) or otherwise keep the duplicate around in +%% an order that prioritizes 'New' elements. +umerge([{KNew,_}=New|News], Olds, Merged, _CmpMerged, {KCmp,_}=Cmp) when KNew =< KCmp -> + umerge(News, Olds, [New | Merged], New, Cmp); +umerge([{KNew,_}=New|News], Olds, Merged, {KCmp,_}=CmpMerged, Cmp) when KNew == KCmp -> + if New == CmpMerged -> + umerge(News, Olds, Merged, New); + New =/= CmpMerged -> % this is where we depart from the stdlib! + umerge(News, Olds, [New | Merged], New, Cmp) + end; +umerge([New|News], Olds, Merged, _CmpMerged, Cmp) -> % > + umerge(News, Olds, [Cmp | Merged], New); +umerge([], Olds, Merged, {KCmpM,_}=CmpMerged, {KCmp,_}=Cmp) when KCmpM =:= KCmp -> + if CmpMerged == Cmp -> + lists:reverse(Olds, Merged); + CmpMerged =/= Cmp -> % We depart from stdlib here too! + lists:reverse(Olds, [Cmp | Merged]) + end; +umerge([], Olds, Merged, _CmpMerged, Cmp) -> + lists:reverse(Olds, [Cmp | Merged]). diff --git a/test/rebar_profiles_SUITE.erl b/test/rebar_profiles_SUITE.erl index a4f926e..ab37255 100644 --- a/test/rebar_profiles_SUITE.erl +++ b/test/rebar_profiles_SUITE.erl @@ -100,12 +100,17 @@ profile_merges(_Config) -> {test2, "hello"}, {test3, [key3]}, {test4, "oldvalue"}, + {test5, [{key5, true}]}, + {test6, [{key6, false}]}, {profiles, [{profile1, [{test1, [{key3, 5}, key1]}]}, {profile2, [{test2, "goodbye"}, {test3, []}, - {test4, []}]}]}], + {test4, []}, + {test5, [{key5, false}]}, + {test6, [{key6, true}]} + ]}]}], State = rebar_state:new(RebarConfig), State1 = rebar_state:apply_profiles(State, [profile1, profile2]), @@ -118,7 +123,9 @@ profile_merges(_Config) -> %% Check that a newvalue of []/"" doesn't override non-string oldvalues [key3] = rebar_state:get(State1, test3), - [] = rebar_state:get(State1, test4). + [] = rebar_state:get(State1, test4), + [{key5, false}, {key5, true}] = rebar_state:get(State1, test5), + [{key6, true}, {key6, false}] = rebar_state:get(State1, test6). add_to_profile(_Config) -> RebarConfig = [{foo, true}, {bar, false}], -- cgit v1.1 From 3e7a58ccd6da147ea4ccf835a115512ab5304f0f Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sun, 22 Mar 2015 21:55:03 -0400 Subject: Reverse relx args order for config With the new priority order, and knowing Relx processes things in reverse already (possibly building a dict internally), we should flip our options around to keep them correct. --- src/rebar_prv_release.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rebar_prv_release.erl b/src/rebar_prv_release.erl index 766bfe4..db012d5 100644 --- a/src/rebar_prv_release.erl +++ b/src/rebar_prv_release.erl @@ -47,7 +47,7 @@ do(State) -> ,{caller, Caller}], AllOptions); Config -> relx:main([{lib_dirs, LibDirs} - ,{config, Config} + ,{config, lists:reverse(Config)} ,{output_dir, OutputDir} ,{caller, Caller}], AllOptions) end, -- cgit v1.1 From 81969f1cf68f9509ee1bb18a15695b610fd33bd7 Mon Sep 17 00:00:00 2001 From: Kelly McLaughlin Date: Sun, 22 Mar 2015 22:09:27 -0600 Subject: Test cases to exercise the dev_mode release option Add one test case to verify the dev_mode option for a release and another to verify overriding the dev_mode option in a profile for a release. Verification of proper dev_mode functioning is done in the rebar_test_utils:check_results/2 function by checking if all the directories in the release lib dir are symlinks or not and comparing that result to the dev_mode expectation passed as input to the check_results function. --- test/rebar_release_SUITE.erl | 46 +++++++++++++++++++++++++++++++++++++++++--- test/rebar_test_utils.erl | 11 ++++++++++- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/test/rebar_release_SUITE.erl b/test/rebar_release_SUITE.erl index 92219a5..3809106 100644 --- a/test/rebar_release_SUITE.erl +++ b/test/rebar_release_SUITE.erl @@ -3,7 +3,10 @@ -include_lib("common_test/include/ct.hrl"). -include_lib("eunit/include/eunit.hrl"). -all() -> [release, tar]. +all() -> [release, + dev_mode_release, + profile_dev_mode_override_release, + tar]. init_per_testcase(Case, Config0) -> Config = rebar_test_utils:init_rebar_state(Config0), @@ -33,9 +36,46 @@ release(Config) -> rebar_test_utils:run_and_check( Config, RebarConfig, ["release"], - {ok, [{release, list_to_atom(Name), Vsn}]} + {ok, [{release, list_to_atom(Name), Vsn, false}]} ). +dev_mode_release(Config) -> + AppDir = ?config(apps, Config), + Name = ?config(name, Config), + Vsn = "1.0.0", + {ok, RebarConfig} = + file:consult(rebar_test_utils:create_config(AppDir, + [{relx, [{release, {list_to_atom(Name), Vsn}, + [list_to_atom(Name)]}, + {lib_dirs, [AppDir]}, + {dev_mode, true}]}])), + rebar_test_utils:run_and_check( + Config, RebarConfig, + ["release"], + {ok, [{release, list_to_atom(Name), Vsn, true}]} + ). + + +profile_dev_mode_override_release(Config) -> + AppDir = ?config(apps, Config), + Name = ?config(name, Config), + Vsn = "1.0.0", + {ok, RebarConfig} = + file:consult(rebar_test_utils:create_config(AppDir, + [{relx, [{release, {list_to_atom(Name), Vsn}, + [list_to_atom(Name)]}, + {lib_dirs, [AppDir]}, + {dev_mode, true}]}, + {profiles, + [{ct, + [{relx, [{dev_mode, false}]}]}]}])), + rebar_test_utils:run_and_check( + Config, RebarConfig, + ["as", "ct", "release"], + {ok, [{release, list_to_atom(Name), Vsn, false}]} + ). + + tar(Config) -> AppDir = ?config(apps, Config), Name = ?config(name, Config), @@ -48,5 +88,5 @@ tar(Config) -> rebar_test_utils:run_and_check( Config, RebarConfig, ["tar"], - {ok, [{release, list_to_atom(Name), Vsn}, {tar, Name, Vsn}]} + {ok, [{release, list_to_atom(Name), Vsn, false}, {tar, Name, Vsn}]} ). diff --git a/test/rebar_test_utils.erl b/test/rebar_test_utils.erl index 7c52a18..c035b91 100644 --- a/test/rebar_test_utils.erl +++ b/test/rebar_test_utils.erl @@ -217,7 +217,7 @@ check_results(AppDir, Expected) -> ?assertEqual(iolist_to_binary(Vsn), iolist_to_binary(LockVsn)) end - ; ({release, Name, Vsn}) -> + ; ({release, Name, Vsn, ExpectedDevMode}) -> ct:pal("Release: ~p-~s", [Name, Vsn]), {ok, Cwd} = file:get_cwd(), try @@ -228,6 +228,15 @@ check_results(AppDir, Expected) -> {ok, RelxState2} = rlx_prv_app_discover:do(RelxState1), {ok, RelxState3} = rlx_prv_rel_discover:do(RelxState2), + LibDir = filename:join([ReleaseDir, Name, "lib"]), + {ok, RelLibs} = file:list_dir(LibDir), + IsSymLinkFun = + fun(X) -> + ec_file:is_symlink(filename:join(LibDir, X)) + end, + DevMode = lists:all(IsSymLinkFun, RelLibs), + ?assertEqual(ExpectedDevMode, DevMode), + %% throws not_found if it doesn't exist rlx_state:get_realized_release(RelxState3, Name, Vsn) catch -- cgit v1.1