From 88b251a364b0d1ec9624bcc7e9bb4f99dcb6678c Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Fri, 3 Jun 2016 23:43:13 -0400 Subject: [PATCH 1/3] Regression test showing the bug for umbrella apps When the operation for an unlock takes place in the config of a umbrella application, the `unlock' provider does not see the dependency in the `deps' value of the config (since it only includes the deps at the root of the project) and ignores these. --- test/rebar_upgrade_SUITE.erl | 52 +++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index cfe1d8a3..e7651a12 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -11,7 +11,7 @@ groups() -> triplet_a, triplet_b, triplet_c, tree_a, tree_b, tree_c, tree_c2, tree_cj, tree_ac, tree_all, delete_d, promote, stable_lock, fwd_lock, - compile_upgrade_parity]}, + compile_upgrade_parity, umbrella_config]}, {git, [], [{group, all}]}, {pkg, [], [{group, all}]}]. @@ -66,6 +66,18 @@ end_per_testcase(_, Config) -> meck:unload(), Config. +setup_project(Case=umbrella_config, Config0, Deps, UpDeps) -> + DepsType = ?config(deps_type, Config0), + NameRoot = atom_to_list(Case)++"_"++atom_to_list(DepsType), + Config = rebar_test_utils:init_rebar_state(Config0, NameRoot++"_"), + AppDir = filename:join([?config(apps, Config), "apps", NameRoot]), + rebar_test_utils:create_app(AppDir, "Root", "0.0.0", [kernel, stdlib]), + TopDeps = rebar_test_utils:top_level_deps(Deps), + TopConf = rebar_test_utils:create_config(AppDir, [{deps, []}]), + RebarConf = rebar_test_utils:create_config(AppDir, [{deps, TopDeps}]), + [{rebarconfig, TopConf}, + {rebarumbrella, RebarConf}, + {next_top_deps, rebar_test_utils:top_level_deps(UpDeps)} | Config]; setup_project(Case, Config0, Deps, UpDeps) -> DepsType = ?config(deps_type, Config0), Config = rebar_test_utils:init_rebar_state( @@ -437,7 +449,12 @@ upgrades(compile_upgrade_parity) -> [], {"", [{"A","1"}, "D", "J", "E", {"I","1"}, {"B","1"}, "F", "G", - {"C","1"}, "H"]}}. + {"C","1"}, "H"]}}; +upgrades(umbrella_config) -> + {[{"A", "1", []}], + [{"A", "2", []}], + ["A"], + {"A", [{"A","2"}]}}. %% TODO: add a test that verifies that unlocking files and then %% running the upgrade code is enough to properly upgrade things. @@ -570,9 +587,36 @@ compile_upgrade_parity(Config) -> ?assertEqual(CompileLockData1, CompileLockData2), ?assertEqual(CompileLockData1, UpgradeLockData). +umbrella_config(Config) -> + apply(?config(mock, Config), []), + {ok, TopConfig} = file:consult(?config(rebarconfig, Config)), + %% Install dependencies before re-mocking for an upgrade + rebar_test_utils:run_and_check(Config, TopConfig, ["lock"], {ok, []}), + {App, Unlocks} = ?config(expected, Config), + ct:pal("Upgrades: ~p -> ~p", [App, Unlocks]), + Expectation = case Unlocks of + {error, Term} -> {error, Term}; + _ -> {ok, Unlocks} + end, + + meck:new(rebar_prv_upgrade, [passthrough]), + meck:expect(rebar_prv_upgrade, do, fun(S) -> + apply(?config(mock_update, Config), []), + meck:passthrough([S]) + end), + _NewRebarConf = rebar_test_utils:create_config(filename:dirname(?config(rebarumbrella, Config)), + [{deps, ?config(next_top_deps, Config)}]), + %% re-run from the top-level with the old config still in place; + %% detection must happen when going for umbrella apps! + rebar_test_utils:run_and_check( + Config, TopConfig, ["upgrade", App], Expectation + ), + meck:unload(rebar_prv_upgrade). + run(Config) -> apply(?config(mock, Config), []), - {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)), + ConfigPath = ?config(rebarconfig, Config), + {ok, RebarConfig} = file:consult(ConfigPath), %% Install dependencies before re-mocking for an upgrade rebar_test_utils:run_and_check(Config, RebarConfig, ["lock"], {ok, []}), {App, Unlocks} = ?config(expected, Config), @@ -587,7 +631,7 @@ run(Config) -> apply(?config(mock_update, Config), []), meck:passthrough([S]) end), - NewRebarConf = rebar_test_utils:create_config(?config(apps, Config), + NewRebarConf = rebar_test_utils:create_config(filename:dirname(ConfigPath), [{deps, ?config(next_top_deps, Config)}]), {ok, NewRebarConfig} = file:consult(NewRebarConf), rebar_test_utils:run_and_check( From 3cb67ff753d68212ba613587bceb806244d795c5 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sat, 4 Jun 2016 00:01:37 -0400 Subject: [PATCH 2/3] Account for umbrella apps' deps in upgrades --- src/rebar_prv_upgrade.erl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index c5c43e43..87d01d4e 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -45,7 +45,13 @@ init(State) -> do(State) -> {Args, _} = rebar_state:command_parsed_args(State), Locks = rebar_state:get(State, {locks, default}, []), - Deps = rebar_state:get(State, deps, []), + %% grab the parsed deps, and then add in the deps found in each of the + %% umbrella apps' configs (which are in {deps, default} when added by + %% `rebar_app_discover'). This later set however includes locks, so + %% we remove them to get a full picture. + TopDeps = rebar_state:get(State, deps, []), + ProfileDeps = rebar_state:get(State, {deps, default}, []), + Deps = TopDeps ++ ((ProfileDeps -- TopDeps) -- Locks), % top deps > profile deps Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args, <<"">>)), Locks), DepsDict = deps_dict(rebar_state:all_deps(State)), case prepare_locks(Names, Deps, Locks, [], DepsDict) of From 3038aae4ba91c48066e5fc71dfdbb2a06679ec92 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Mon, 6 Jun 2016 11:25:40 -0400 Subject: [PATCH 3/3] Rework patch to work without accident The previous iteration of the patch worked somewhat by accident. After digging in and figuring out why the two dep sources are the way they are, the patch is now properly working with a well-documented explanatiion inline. --- src/rebar_prv_upgrade.erl | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index 87d01d4e..18c307be 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -45,13 +45,29 @@ init(State) -> do(State) -> {Args, _} = rebar_state:command_parsed_args(State), Locks = rebar_state:get(State, {locks, default}, []), - %% grab the parsed deps, and then add in the deps found in each of the - %% umbrella apps' configs (which are in {deps, default} when added by - %% `rebar_app_discover'). This later set however includes locks, so - %% we remove them to get a full picture. + %% We have 3 sources of dependencies to upgrade from: + %% 1. the top-level rebar.config (in `deps', dep name is an atom) + %% 2. the app-level rebar.config in umbrella apps (in `{deps, default}', + %% where the dep name is an atom) + %% 3. the formatted sources for all after app-parsing (in `{deps, default}', + %% where the reprocessed app name is a binary) + %% + %% The gotcha with these is that the selection of dependencies with a + %% binary name (those that are stable and usable internally) is done with + %% in the profile deps only, but while accounting for locks. + %% Because our job here is to unlock those that have changed, we must + %% instead use the atom-based names, both in `deps' and `{deps, default}', + %% as those are the dependencies that may have changed but have been + %% disregarded by locks. + %% + %% As such, the working set of dependencies is the addition of + %% `deps' and `{deps, default}' entries with an atom name, as those + %% disregard locks and parsed values post-selection altogether. + %% Packages without versions can of course be a single atom. TopDeps = rebar_state:get(State, deps, []), ProfileDeps = rebar_state:get(State, {deps, default}, []), - Deps = TopDeps ++ ((ProfileDeps -- TopDeps) -- Locks), % top deps > profile deps + Deps = [Dep || Dep <- TopDeps ++ ProfileDeps, % TopDeps > ProfileDeps + is_atom(Dep) orelse is_atom(element(1, Dep))], Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args, <<"">>)), Locks), DepsDict = deps_dict(rebar_state:all_deps(State)), case prepare_locks(Names, Deps, Locks, [], DepsDict) of