From b94afdfa2d44b0cb1489234c7746cd72f85a167d Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Tue, 20 Jan 2015 01:39:43 +0000 Subject: [PATCH 1/8] WIP Test that all the correct locks are set for an upgrade run. Now to actually re-run the install deps and prove it works --- src/rebar_prv_install_deps.erl | 2 +- src/rebar_prv_lock.erl | 2 +- src/rebar_prv_upgrade.erl | 48 ++++++--- src/rebar_state.erl | 8 +- test/rebar_deps_SUITE.erl | 1 - test/rebar_test_utils.erl | 12 ++- test/rebar_upgrade_SUITE.erl | 185 +++++++++++++++++++++++++++++++++ 7 files changed, 238 insertions(+), 20 deletions(-) create mode 100644 test/rebar_upgrade_SUITE.erl diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl index 025d32ac..49aa65c7 100644 --- a/src/rebar_prv_install_deps.erl +++ b/src/rebar_prv_install_deps.erl @@ -287,7 +287,7 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, handle_update(AppInfo, UpdateName, UpdateLevel, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> Name = rebar_app_info:name(AppInfo), - {_, _, _, DepLevel} = lists:keyfind(Name, 1, Locks), + {_, _, DepLevel} = lists:keyfind(Name, 1, Locks), case UpdateLevel < DepLevel orelse Name =:= UpdateName of true -> diff --git a/src/rebar_prv_lock.erl b/src/rebar_prv_lock.erl index 2f26a7fd..8a694343 100644 --- a/src/rebar_prv_lock.erl +++ b/src/rebar_prv_lock.erl @@ -41,7 +41,7 @@ do(State) -> ,rebar_app_info:dep_level(Dep)} end, AllDeps), Dir = rebar_state:dir(State), - file:write_file(filename:join(Dir, "rebar.lock"), io_lib:format("~p.~n", [Locks])), + file:write_file(filename:join(Dir, ?LOCK_FILE), io_lib:format("~p.~n", [Locks])), {ok, State}. -spec format_error(any()) -> iolist(). diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index c0a56c36..4609f57c 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -12,7 +12,10 @@ -include("rebar.hrl"). -define(PROVIDER, upgrade). --define(DEPS, []). +-define(DEPS, [lock]). +%% Also only upgrade top-level (0) deps. Transitive deps shouldn't be +%% upgradable -- if the user wants this, they should declare it at the +%% top level and then upgrade. %% =================================================================== %% Public API @@ -38,19 +41,36 @@ init(State) -> do(State) -> {Args, _} = rebar_state:command_parsed_args(State), Name = ec_cnv:to_binary(proplists:get_value(package, Args)), - Locks = rebar_state:get(State, locks, []), - case lists:keyfind(Name, 1, Locks) of - {_, _, _, Level} -> - Deps = rebar_state:get(State, deps), - case lists:keyfind(binary_to_atom(Name, utf8), 1, Deps) of - false -> - {error, io_lib:format("No such dependency ~s~n", [Name])}; - Dep -> - rebar_prv_install_deps:handle_deps(State, [Dep], {true, Name, Level}), - {ok, State} - end; - _ -> - {error, io_lib:format("No such dependency ~s~n", [Name])} + Locks = rebar_state:lock(State), + %% TODO: optimize by running the find + unlock in one sweep + case find_app(Name, Locks) of + {AppInfo, 0} -> + %% Unlock the app and all those with a lock level higher than + %% it has + NewLocks = unlock_higher_than(0, Locks -- [AppInfo]), + {ok, rebar_state:lock(State, NewLocks)}; + {_AppInfo, Level} when Level > 0 -> + {error, transitive_dependency}; + false -> + {error, unknown_dependency} + end. + +find_app(_Name, []) -> false; +find_app(Name, [App|Apps]) -> + case rebar_app_info:name(App) of + Name -> {App, rebar_app_info:dep_level(App)}; + _ -> find_app(Name, Apps) + end. + +%% Because we operate on a lock list, removing the app from the list +%% unlocks it. +unlock_higher_than(_, []) -> []; +unlock_higher_than(Level, [App | Apps]) -> + case rebar_app_info:dep_level(App) of + N when N > Level -> + unlock_higher_than(Level, Apps); + N when N =< Level -> + [App | unlock_higher_than(Level, Apps)] end. -spec format_error(any()) -> iolist(). diff --git a/src/rebar_state.erl b/src/rebar_state.erl index 29b7c3f8..70aba510 100644 --- a/src/rebar_state.erl +++ b/src/rebar_state.erl @@ -89,8 +89,10 @@ new(ParentState, Config, Dir) -> Opts = ParentState#state_t.opts, LocalOpts = case rebar_config:consult_file(filename:join(Dir, ?LOCK_FILE)) of [D] -> - LockedDeps = [X || X <- D, element(3, X) =:= 0], - dict:from_list([{{locks, default}, LockedDeps}, {{deps, default}, D} | Config]); + %% We want the top level deps only from the lock file. + %% This ensures deterministic overrides for configs. + Deps = [X || X <- D, element(3, X) =:= 0], + dict:from_list([{{locks, default}, D}, {{deps, default}, Deps} | Config]); _ -> D = proplists:get_value(deps, Config, []), dict:from_list([{{deps, default}, D} | Config]) @@ -133,6 +135,8 @@ current_profiles(#state_t{current_profiles=Profiles}) -> lock(#state_t{lock=Lock}) -> Lock. +lock(State=#state_t{}, Apps) when is_list(Apps) -> + State#state_t{lock=Apps}; lock(State=#state_t{lock=Lock}, App) -> State#state_t{lock=[App | Lock]}. diff --git a/test/rebar_deps_SUITE.erl b/test/rebar_deps_SUITE.erl index d67efe47..8407b559 100644 --- a/test/rebar_deps_SUITE.erl +++ b/test/rebar_deps_SUITE.erl @@ -1,4 +1,3 @@ -%%% TODO: check that warnings are appearing -module(rebar_deps_SUITE). -compile(export_all). -include_lib("common_test/include/ct.hrl"). diff --git a/test/rebar_test_utils.erl b/test/rebar_test_utils.erl index 6095d6d0..af409951 100644 --- a/test/rebar_test_utils.erl +++ b/test/rebar_test_utils.erl @@ -53,7 +53,17 @@ run_and_check(Config, RebarConfig, Command, Expect) -> ?assertEqual({error, Reason}, Res); {ok, Expected} -> {ok, _} = Res, - check_results(AppDir, Expected) + check_results(AppDir, Expected); + {unlocked, Expected} -> + ct:pal("Expected: ~p", [Expected]), + {ok, ResState} = Res, + Locks = rebar_state:lock(ResState), + ct:pal("Locks: ~p", [Locks]), + ?assertEqual(lists:sort(Expected), + lists:sort([rebar_app_info:name(Lock) + || Lock <- Locks])); + return -> + Res end. %% @doc Creates a dummy application including: diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl new file mode 100644 index 00000000..5915d0d1 --- /dev/null +++ b/test/rebar_upgrade_SUITE.erl @@ -0,0 +1,185 @@ +-module(rebar_upgrade_SUITE). +-include_lib("common_test/include/ct.hrl"). +-include_lib("eunit/include/eunit.hrl"). +-compile(export_all). + +all() -> [{group, git}].%, {group, pkg}]. + +groups() -> + [{all, [], [top, pair, triplet, tree]}, + {git, [], [{group, all}]}, + {pkg, [], [{group, all}]}]. + +init_per_suite(Config) -> + application:start(meck), + Config. + +end_per_suite(_Config) -> + application:stop(meck). + +init_per_group(git, Config) -> + [{deps_type, git} | Config]; +init_per_group(pkg, Config) -> + [{deps_type, pkg} | Config]; +init_per_group(_, Config) -> + Config. + +end_per_group(_, Config) -> + Config. + +init_per_testcase(Case, Config) -> + DepsType = ?config(deps_type, Config), + {Deps, Unlocks} = upgrades(Case), + Expanded = expand_deps(DepsType, Deps), + [{unlocks, normalize_unlocks(Unlocks)}, + {mock, fun() -> mock_deps(DepsType, Expanded, []) end} + | setup_project(Case, Config, Expanded)]. + +end_per_testcase(_, Config) -> + meck:unload(), + Config. + +setup_project(Case, Config0, Deps) -> + DepsType = ?config(deps_type, Config0), + Config = rebar_test_utils:init_rebar_state( + Config0, + atom_to_list(Case)++"_"++atom_to_list(DepsType)++"_" + ), + AppDir = ?config(apps, Config), + rebar_test_utils:create_app(AppDir, "Root", "0.0.0", [kernel, stdlib]), + TopDeps = top_level_deps(Deps), + RebarConf = rebar_test_utils:create_config(AppDir, [{deps, TopDeps}]), + [{rebarconfig, RebarConf} | Config]. + + +upgrades(top) -> + {[{"A", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% upgrade vs. locked after upgrade + [{"A", []}, + {"B", {error, transitive_dependency}}, + {"C", {error, transitive_dependency}}, + {"D", "1", {error, transitive_dependency}}, + {"D", "2", {error, unknown_dependency}}]}; +upgrades(pair) -> + {[{"A", [{"C", []}]}, + {"B", [{"D", []}]}], + [{"A", ["B"]}, + {"B", ["A"]}, + {"C", {error, transitive_dependency}}, + {"D", {error, transitive_dependency}}]}; +upgrades(triplet) -> + {[{"A", [{"D",[]}, + {"E",[]}]}, + {"B", [{"F",[]}, + {"G",[]}]}, + {"C", [{"H",[]}, + {"I",[]}]}], + [{"A", ["B","C"]}, + {"B", ["A","C"]}, + {"C", ["A","B"]}, + {"D", {error, transitive_dependency}}, + %% ... + {"I", {error, transitive_dependency}}]}; +upgrades(tree) -> + {[{"A", [{"D",[{"J",[]}]}, + {"E",[{"K",[]}]}]}, + {"B", [{"F",[]}, + {"G",[]}]}, + {"C", [{"H",[]}, + {"I",[]}]}], + [{"A", ["B","C"]}, + {"B", ["A","C"]}, + {"C", ["A","B"]}, + {"D", {error, transitive_dependency}}, + %% ... + {"K", {error, transitive_dependency}}]}. + +%% TODO: add a test that verifies that unlocking files and then +%% running the upgrade code is enough to properly upgrade things. + +top_level_deps([]) -> []; +top_level_deps([{{Name, Vsn, Ref}, _} | Deps]) -> + [{list_to_atom(Name), Vsn, Ref} | top_level_deps(Deps)]; +top_level_deps([{{pkg, Name, Vsn, _URL}, _} | Deps]) -> + [{list_to_atom(Name), Vsn} | top_level_deps(Deps)]. + +mock_deps(git, Deps, Upgrades) -> + mock_git_resource:mock([{deps, flat_deps(Deps)}, {upgrade, Upgrades}]); +mock_deps(pkg, Deps, Upgrades) -> + mock_pkg_resource:mock([{pkgdeps, flat_pkgdeps(Deps)}, {upgrade, Upgrades}]). + +flat_deps([]) -> []; +flat_deps([{{Name,_Vsn,Ref}, Deps} | Rest]) -> + [{{Name,vsn_from_ref(Ref)}, top_level_deps(Deps)}] + ++ + flat_deps(Deps) + ++ + flat_deps(Rest). + +vsn_from_ref({git, _, {_, Vsn}}) -> Vsn; +vsn_from_ref({git, _, Vsn}) -> Vsn. + +flat_pkgdeps([]) -> []; +flat_pkgdeps([{{pkg, Name, Vsn, _Url}, Deps} | Rest]) -> + [{{iolist_to_binary(Name),iolist_to_binary(Vsn)}, top_level_deps(Deps)}] + ++ + flat_pkgdeps(Deps) + ++ + flat_pkgdeps(Rest). + +expand_deps(_, []) -> []; +expand_deps(git, [{Name, Deps} | Rest]) -> + Dep = {Name, ".*", {git, "https://example.org/user/"++Name++".git", "master"}}, + [{Dep, expand_deps(git, Deps)} | expand_deps(git, Rest)]; +expand_deps(git, [{Name, Vsn, Deps} | Rest]) -> + Dep = {Name, Vsn, {git, "https://example.org/user/"++Name++".git", {tag, Vsn}}}, + [{Dep, expand_deps(git, Deps)} | expand_deps(git, Rest)]; +expand_deps(pkg, [{Name, Deps} | Rest]) -> + Dep = {pkg, Name, "0.0.0", "https://example.org/user/"++Name++".tar.gz"}, + [{Dep, expand_deps(pkg, Deps)} | expand_deps(pkg, Rest)]; +expand_deps(pkg, [{Name, Vsn, Deps} | Rest]) -> + Dep = {pkg, Name, Vsn, "https://example.org/user/"++Name++".tar.gz"}, + [{Dep, expand_deps(pkg, Deps)} | expand_deps(pkg, Rest)]. + +normalize_unlocks([]) -> []; +normalize_unlocks([{App, Locks} | Rest]) -> + [{iolist_to_binary(App), + normalize_unlocks_expect(Locks)} | normalize_unlocks(Rest)]; +normalize_unlocks([{App, Vsn, Locks} | Rest]) -> + [{iolist_to_binary(App), iolist_to_binary(Vsn), + normalize_unlocks_expect(Locks)} | normalize_unlocks(Rest)]. + +normalize_unlocks_expect({error, Reason}) -> + {error, Reason}; +normalize_unlocks_expect([]) -> + []; +normalize_unlocks_expect([{App,Vsn} | Rest]) -> + [{iolist_to_binary(App), iolist_to_binary(Vsn)} + | normalize_unlocks_expect(Rest)]; +normalize_unlocks_expect([App | Rest]) -> + [iolist_to_binary(App) | normalize_unlocks_expect(Rest)]. + +top(Config) -> run(Config). +pair(Config) -> run(Config). +triplet(Config) -> run(Config). +tree(Config) -> run(Config). + +run(Config) -> + apply(?config(mock, Config), []), + {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)), + %% Install dependencies before re-mocking for an upgrade + rebar_test_utils:run_and_check(Config, RebarConfig, ["lock"], {ok, []}), + Unlocks = ?config(unlocks, Config), + [begin + ct:pal("Unlocks: ~p -> ~p", [App, Unlocked]), + Expectation = case Unlocked of + Tuple when is_tuple(Tuple) -> Tuple; + _ -> {unlocked, Unlocked} + end, + rebar_test_utils:run_and_check( + Config, RebarConfig, ["upgrade", App], Expectation + ) + end || {App, Unlocked} <- Unlocks]. + From 9fb35fe6defb07dde99ba4442a66e3e6719c4d49 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Tue, 3 Feb 2015 15:03:23 +0000 Subject: [PATCH 2/8] Alternative attempt --- src/rebar_prv_install_deps.erl | 20 ++++++++------ src/rebar_prv_upgrade.erl | 50 +++++++++++++++++----------------- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl index 49aa65c7..094fae02 100644 --- a/src/rebar_prv_install_deps.erl +++ b/src/rebar_prv_install_deps.erl @@ -254,10 +254,9 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, {SeenAcc1, StateAcc1} = maybe_lock(Profile, AppInfo, SeenAcc, StateAcc, Level), {SrcDepsAcc1, PkgDepsAcc1, SrcAppsAcc1, StateAcc2, LocksAcc1} = case Update of - {true, UpdateName, UpdateLevel} -> + true -> + %{true, UpdateName, UpdateLevel} -> handle_update(AppInfo - ,UpdateName - ,UpdateLevel ,SrcDepsAcc ,PkgDepsAcc ,SrcAppsAcc @@ -285,14 +284,15 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, update_src_deps(Profile, Level+1, NewSrcDeps, NewPkgDeps, NewSrcApps, State1, Update, Seen1, NewLocks) end. -handle_update(AppInfo, UpdateName, UpdateLevel, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> +handle_update(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> Name = rebar_app_info:name(AppInfo), - {_, _, DepLevel} = lists:keyfind(Name, 1, Locks), - case UpdateLevel < DepLevel - orelse Name =:= UpdateName of - true -> + ct:pal("update ~p", [Name]), + case lists:keyfind(Name, 1, Locks) of + false -> + ct:pal("in lock"), case maybe_fetch(AppInfo, true, []) of true -> + ct:pal("fetch!"), handle_dep(AppInfo ,SrcDeps ,PkgDeps @@ -302,9 +302,11 @@ handle_update(AppInfo, UpdateName, UpdateLevel, SrcDeps, PkgDeps, SrcApps, Level ,Locks); false -> + ct:pal("nofetch"), {SrcDeps, PkgDeps, SrcApps, State, Locks} end; - false -> + _StillLocked -> + ct:pal("stillocked"), {SrcDeps, PkgDeps, SrcApps, State, Locks} end. diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index 4609f57c..207d36a2 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -12,7 +12,7 @@ -include("rebar.hrl"). -define(PROVIDER, upgrade). --define(DEPS, [lock]). +-define(DEPS, []). %% Also only upgrade top-level (0) deps. Transitive deps shouldn't be %% upgradable -- if the user wants this, they should declare it at the %% top level and then upgrade. @@ -41,38 +41,38 @@ init(State) -> do(State) -> {Args, _} = rebar_state:command_parsed_args(State), Name = ec_cnv:to_binary(proplists:get_value(package, Args)), - Locks = rebar_state:lock(State), - %% TODO: optimize by running the find + unlock in one sweep - case find_app(Name, Locks) of - {AppInfo, 0} -> - %% Unlock the app and all those with a lock level higher than - %% it has - NewLocks = unlock_higher_than(0, Locks -- [AppInfo]), - {ok, rebar_state:lock(State, NewLocks)}; - {_AppInfo, Level} when Level > 0 -> + Locks = rebar_state:get(State, {locks, default}, []), + case lists:keyfind(Name, 1, Locks) of + {_, _, 0} = Lock -> + Deps = rebar_state:get(State, deps), + case lists:keyfind(binary_to_atom(Name, utf8), 1, Deps) of + false -> + {error, unknown_dependency}; + Dep -> + Source = case Dep of + {_, Src} -> Src; + {_, _, Src} -> Src + end, + NewLocks = unlock_higher_than(0, Locks -- [Lock]), + State1 = rebar_state:set(State, {deps, default}, [{Name, Source, 0} | NewLocks]), + State2 = rebar_state:set(State1, {locks, default}, NewLocks), + rebar_prv_install_deps:do(State2) + end; + {_, _, Level} when Level > 0 -> {error, transitive_dependency}; false -> - {error, unknown_dependency} + ct:pal("deps: ~p", [{Name,Locks}]), + {error, unlocked_dependency} end. -find_app(_Name, []) -> false; -find_app(Name, [App|Apps]) -> - case rebar_app_info:name(App) of - Name -> {App, rebar_app_info:dep_level(App)}; - _ -> find_app(Name, Apps) - end. -%% Because we operate on a lock list, removing the app from the list -%% unlocks it. unlock_higher_than(_, []) -> []; -unlock_higher_than(Level, [App | Apps]) -> - case rebar_app_info:dep_level(App) of - N when N > Level -> - unlock_higher_than(Level, Apps); - N when N =< Level -> - [App | unlock_higher_than(Level, Apps)] +unlock_higher_than(Level, [App = {_,_,AppLevel} | Apps]) -> + if AppLevel > Level -> unlock_higher_than(Level, Apps); + AppLevel =< Level -> [App | unlock_higher_than(Level, Apps)] end. -spec format_error(any()) -> iolist(). format_error(Reason) -> io_lib:format("~p", [Reason]). + From 748a046133fa5b0b2570f4675c223326299d711a Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 4 Feb 2015 21:26:48 +0000 Subject: [PATCH 3/8] Partial work + Failing tests The problem with the current effort is handling of transitive dependency upgrades and possible values. --- src/rebar_prv_upgrade.erl | 2 +- test/mock_git_resource.erl | 5 +- test/rebar_test_utils.erl | 8 - test/rebar_upgrade_SUITE.erl | 328 +++++++++++++++++++++++++++-------- 4 files changed, 262 insertions(+), 81 deletions(-) diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index 207d36a2..0c1ed8ad 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -62,7 +62,7 @@ do(State) -> {error, transitive_dependency}; false -> ct:pal("deps: ~p", [{Name,Locks}]), - {error, unlocked_dependency} + {error, unknown_dependency} end. diff --git a/test/mock_git_resource.erl b/test/mock_git_resource.erl index 00f0a039..5d8288e2 100644 --- a/test/mock_git_resource.erl +++ b/test/mock_git_resource.erl @@ -54,11 +54,13 @@ mock_lock(_) -> %% @doc The config passed to the `mock/2' function can specify which apps %% should be updated on a per-name basis: `{update, ["App1", "App3"]}'. mock_update(Opts) -> - ToUpdate = proplists:get_value(update, Opts, []), + ToUpdate = proplists:get_value(upgrade, Opts, []), + ct:pal("TOUp: ~p", [ToUpdate]), meck:expect( ?MOD, needs_update, fun(_Dir, {git, Url, _Ref}) -> App = app(Url), + ct:pal("Needed update? ~p -> ~p", [App, lists:member(App, ToUpdate)]), lists:member(App, ToUpdate) end). @@ -106,6 +108,7 @@ mock_download(Opts) -> {git, Url, {_, Vsn}} = normalize_git(Git, Overrides, Default), App = app(Url), AppDeps = proplists:get_value({App,Vsn}, Deps, []), + ct:pal("creating app ~p", [{Dir, App, Vsn, AppDeps}]), rebar_test_utils:create_app( Dir, App, Vsn, [element(1,D) || D <- AppDeps] diff --git a/test/rebar_test_utils.erl b/test/rebar_test_utils.erl index af409951..827d1342 100644 --- a/test/rebar_test_utils.erl +++ b/test/rebar_test_utils.erl @@ -54,14 +54,6 @@ run_and_check(Config, RebarConfig, Command, Expect) -> {ok, Expected} -> {ok, _} = Res, check_results(AppDir, Expected); - {unlocked, Expected} -> - ct:pal("Expected: ~p", [Expected]), - {ok, ResState} = Res, - Locks = rebar_state:lock(ResState), - ct:pal("Locks: ~p", [Locks]), - ?assertEqual(lists:sort(Expected), - lists:sort([rebar_app_info:name(Lock) - || Lock <- Locks])); return -> Res end. diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index 5915d0d1..a7d4a06d 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -6,7 +6,10 @@ all() -> [{group, git}].%, {group, pkg}]. groups() -> - [{all, [], [top, pair, triplet, tree]}, + [{all, [], [top_a, top_b, top_c, top_d1, top_d2, top_e, + pair_a, pair_b, pair_c, + triplet_a, triplet_b, triplet_c, + tree_a, tree_b, tree_c]}, {git, [], [{group, all}]}, {pkg, [], [{group, all}]}]. @@ -29,10 +32,12 @@ end_per_group(_, Config) -> init_per_testcase(Case, Config) -> DepsType = ?config(deps_type, Config), - {Deps, Unlocks} = upgrades(Case), + {Deps, UpDeps, ToUp, Expectations} = upgrades(Case), Expanded = expand_deps(DepsType, Deps), - [{unlocks, normalize_unlocks(Unlocks)}, - {mock, fun() -> mock_deps(DepsType, Expanded, []) end} + UpExpanded = expand_deps(DepsType, UpDeps), + [{expected, normalize_unlocks(Expectations)}, + {mock, fun() -> mock_deps(DepsType, Expanded, []) end}, + {mock_update, fun() -> mock_deps(DepsType, UpExpanded, ToUp) end} | setup_project(Case, Config, Expanded)]. end_per_testcase(_, Config) -> @@ -52,49 +57,216 @@ setup_project(Case, Config0, Deps) -> [{rebarconfig, RebarConf} | Config]. -upgrades(top) -> - {[{"A", [{"B", [{"D", "1", []}]}, - {"C", [{"D", "2", []}]}]} - ], - %% upgrade vs. locked after upgrade - [{"A", []}, - {"B", {error, transitive_dependency}}, - {"C", {error, transitive_dependency}}, - {"D", "1", {error, transitive_dependency}}, - {"D", "2", {error, unknown_dependency}}]}; -upgrades(pair) -> - {[{"A", [{"C", []}]}, - {"B", [{"D", []}]}], - [{"A", ["B"]}, - {"B", ["A"]}, - {"C", {error, transitive_dependency}}, - {"D", {error, transitive_dependency}}]}; -upgrades(triplet) -> - {[{"A", [{"D",[]}, - {"E",[]}]}, - {"B", [{"F",[]}, - {"G",[]}]}, - {"C", [{"H",[]}, - {"I",[]}]}], - [{"A", ["B","C"]}, - {"B", ["A","C"]}, - {"C", ["A","B"]}, - {"D", {error, transitive_dependency}}, - %% ... - {"I", {error, transitive_dependency}}]}; -upgrades(tree) -> - {[{"A", [{"D",[{"J",[]}]}, - {"E",[{"K",[]}]}]}, - {"B", [{"F",[]}, - {"G",[]}]}, - {"C", [{"H",[]}, - {"I",[]}]}], - [{"A", ["B","C"]}, - {"B", ["A","C"]}, - {"C", ["A","B"]}, - {"D", {error, transitive_dependency}}, - %% ... - {"K", {error, transitive_dependency}}]}. +upgrades(top_a) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"A", [{"A","1"}, "B", "C", {"D","3"}]}}; +upgrades(top_b) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"B", {error, transitive_dependency}}}; +upgrades(top_c) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"C", {error, transitive_dependency}}}; +upgrades(top_d1) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"D", {error, transitive_dependency}}}; +upgrades(top_d2) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"D", {error, transitive_dependency}}}; +upgrades(top_e) -> + %% Original tree + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Updated tree + [{"A", "1", [{"B", [{"D", "3", []}]}, + {"C", [{"D", "2", []}]}]} + ], + %% Modified apps, gobally + ["A","B","D"], + %% upgrade vs. new tree + {"E", {error, unknown_dependency}}}; +upgrades(pair_a) -> + {[{"A", "1", [{"C", "1", []}]}, + {"B", "1", [{"D", "1", []}]} + ], + [{"A", "2", [{"C", "2", []}]}, + {"B", "2", [{"D", "2", []}]} + ], + ["A","B","C","D"], + {"A", [{"A","2"},{"C","2"},{"B","1"},{"D","1"}]}}; +upgrades(pair_b) -> + {[{"A", "1", [{"C", "1", []}]}, + {"B", "1", [{"D", "1", []}]} + ], + [{"A", "2", [{"C", "2", []}]}, + {"B", "2", [{"D", "2", []}]} + ], + ["A","B","C","D"], + {"B", [{"A","1"},{"C","1"},{"B","2"},{"D","2"}]}}; +upgrades(pair_c) -> + {[{"A", "1", [{"C", "1", []}]}, + {"B", "1", [{"D", "1", []}]} + ], + [{"A", "2", [{"C", "2", []}]}, + {"B", "2", [{"D", "2", []}]} + ], + ["A","B","C","D"], + {"C", {error, transitive_dependency}}}; +upgrades(triplet_a) -> + {[{"A", "1", [{"D",[]}, + {"E","3",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "0", [{"H","3",[]}, + {"I",[]}]}], + [{"A", "1", [{"D",[]}, + {"E","2",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "1", [{"H","4",[]}, + {"I",[]}]}], + ["A","C","E","H"], + {"A", [{"A","1"}, "D", {"E","2"}, + {"B","1"}, {"F","1"}, "G", + {"C","0"}, {"H","3"}, "I"]}}; +upgrades(triplet_b) -> + {[{"A", "1", [{"D",[]}, + {"E","3",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "0", [{"H","3",[]}, + {"I",[]}]}], + [{"A", "1", [{"D",[]}, + {"E","2",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "1", [{"H","4",[]}, + {"I",[]}]}], + ["A","C","E","H"], + {"B", [{"A","1"}, "D", {"E","3"}, + {"B","1"}, {"F","1"}, "G", + {"C","0"}, {"H","3"}, "I"]}}; +upgrades(triplet_c) -> + {[{"A", "1", [{"D",[]}, + {"E","3",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "0", [{"H","3",[]}, + {"I",[]}]}], + [{"A", "1", [{"D",[]}, + {"E","2",[]}]}, + {"B", "1", [{"F","1",[]}, + {"G",[]}]}, + {"C", "1", [{"H","4",[]}, + {"I",[]}]}], + ["A","C","E","H"], + {"C", [{"A","1"}, "D", {"E","3"}, + {"B","1"}, {"F","1"}, "G", + {"C","1"}, {"H","4"}, "I"]}}; +upgrades(tree_a) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}]} + ], + ["C"], + {"A", [{"A","1"}, "D", "J", "E", + {"B","1"}, "F", "G", + {"C","1"}, "H", {"I","2"}]}}; +upgrades(tree_b) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}]} + ], + ["C"], + {"B", [{"A","1"}, "D", "J", "E", + {"B","1"}, "F", "G", + {"C","1"}, "H", {"I","2"}]}}; +upgrades(tree_c) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}]} + ], + ["C"], + {"C", [{"A","1"}, "D", "J", "E", {"I","1"}, + {"B","1"}, "F", "G", + {"C","1"}, "H"]}}. %% TODO: add a test that verifies that unlocking files and then %% running the upgrade code is enough to properly upgrade things. @@ -106,8 +278,10 @@ top_level_deps([{{pkg, Name, Vsn, _URL}, _} | Deps]) -> [{list_to_atom(Name), Vsn} | top_level_deps(Deps)]. mock_deps(git, Deps, Upgrades) -> + catch mock_git_resource:unmock(), mock_git_resource:mock([{deps, flat_deps(Deps)}, {upgrade, Upgrades}]); mock_deps(pkg, Deps, Upgrades) -> + catch mock_pkg_resource:unmock(), mock_pkg_resource:mock([{pkgdeps, flat_pkgdeps(Deps)}, {upgrade, Upgrades}]). flat_deps([]) -> []; @@ -143,43 +317,55 @@ expand_deps(pkg, [{Name, Vsn, Deps} | Rest]) -> Dep = {pkg, Name, Vsn, "https://example.org/user/"++Name++".tar.gz"}, [{Dep, expand_deps(pkg, Deps)} | expand_deps(pkg, Rest)]. -normalize_unlocks([]) -> []; -normalize_unlocks([{App, Locks} | Rest]) -> - [{iolist_to_binary(App), - normalize_unlocks_expect(Locks)} | normalize_unlocks(Rest)]; -normalize_unlocks([{App, Vsn, Locks} | Rest]) -> - [{iolist_to_binary(App), iolist_to_binary(Vsn), - normalize_unlocks_expect(Locks)} | normalize_unlocks(Rest)]. +normalize_unlocks({App, Locks}) -> + {iolist_to_binary(App), + normalize_unlocks_expect(Locks)}; +normalize_unlocks({App, Vsn, Locks}) -> + {iolist_to_binary(App), iolist_to_binary(Vsn), + normalize_unlocks_expect(Locks)}. normalize_unlocks_expect({error, Reason}) -> {error, Reason}; normalize_unlocks_expect([]) -> []; normalize_unlocks_expect([{App,Vsn} | Rest]) -> - [{iolist_to_binary(App), iolist_to_binary(Vsn)} + [{dep, App, Vsn} | normalize_unlocks_expect(Rest)]; normalize_unlocks_expect([App | Rest]) -> - [iolist_to_binary(App) | normalize_unlocks_expect(Rest)]. + [{dep, App} | normalize_unlocks_expect(Rest)]. + +top_a(Config) -> run(Config). +top_b(Config) -> run(Config). +top_c(Config) -> run(Config). +top_d1(Config) -> run(Config). +top_d2(Config) -> run(Config). +top_e(Config) -> run(Config). + +pair_a(Config) -> run(Config). +pair_b(Config) -> run(Config). +pair_c(Config) -> run(Config). + +triplet_a(Config) -> run(Config). +triplet_b(Config) -> run(Config). +triplet_c(Config) -> run(Config). -top(Config) -> run(Config). -pair(Config) -> run(Config). -triplet(Config) -> run(Config). -tree(Config) -> run(Config). +tree_a(Config) -> run(Config). +tree_b(Config) -> run(Config). +tree_c(Config) -> run(Config). run(Config) -> apply(?config(mock, Config), []), {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)), %% Install dependencies before re-mocking for an upgrade rebar_test_utils:run_and_check(Config, RebarConfig, ["lock"], {ok, []}), - Unlocks = ?config(unlocks, Config), - [begin - ct:pal("Unlocks: ~p -> ~p", [App, Unlocked]), - Expectation = case Unlocked of - Tuple when is_tuple(Tuple) -> Tuple; - _ -> {unlocked, Unlocked} - end, - rebar_test_utils:run_and_check( - Config, RebarConfig, ["upgrade", App], Expectation - ) - end || {App, Unlocked} <- Unlocks]. + {App, Unlocks} = ?config(expected, Config), + ct:pal("Upgrades: ~p -> ~p", [App, Unlocks]), + Expectation = case Unlocks of + {error, Term} -> {error, Term}; + _ -> {ok, Unlocks} + end, + apply(?config(mock_update, Config), []), + rebar_test_utils:run_and_check( + Config, RebarConfig, ["upgrade", App], Expectation + ). From 92436d9960b9ce1ef8a8451c33e597248e1c9bfe Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sun, 8 Feb 2015 23:23:15 +0000 Subject: [PATCH 4/8] More progress on upgrades Only the most complex case is failing, where cross-dependencies would need to be refetched as an update clears an app of its dependencies and a different subtree should override it. --- src/rebar_prv_install_deps.erl | 28 +++++++++++++++++----------- src/rebar_prv_upgrade.erl | 4 ++-- test/rebar_upgrade_SUITE.erl | 13 +++++++++---- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl index 094fae02..125e8898 100644 --- a/src/rebar_prv_install_deps.erl +++ b/src/rebar_prv_install_deps.erl @@ -44,6 +44,8 @@ -define(PROVIDER, install_deps). -define(DEPS, [app_discovery]). +-define(APP_NAME_INDEX, 2). + -type src_dep() :: {atom(), {atom(), string(), string()}} | {atom(), string(), {atom(), string(), string()}}. -type pkg_dep() :: {atom(), binary()} | atom(). @@ -135,7 +137,8 @@ handle_deps(Profile, State, Deps) -> handle_deps(Profile, State, Deps, Update) when is_boolean(Update) -> handle_deps(Profile, State, Deps, Update, []); handle_deps(Profile, State, Deps, Locks) when is_list(Locks) -> - handle_deps(Profile, State, Deps, false, Locks). + Update = rebar_state:get(State, upgrade, false), + handle_deps(Profile, State, Deps, Update, Locks). -spec handle_deps(atom(), rebar_state:t(), list(), boolean() | {true, binary(), integer()}, list()) -> {ok, [rebar_app_info:t()], rebar_state:t()} | {error, string()}. @@ -277,7 +280,7 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, end end, {[], PkgDeps, SrcApps, State, Seen, Locks}, - lists:sort(SrcDeps)) of + sort_deps(SrcDeps)) of {[], NewPkgDeps, NewSrcApps, State1, Seen1, _NewLocks} -> {State1, NewSrcApps, NewPkgDeps, Seen1}; {NewSrcDeps, NewPkgDeps, NewSrcApps, State1, Seen1, NewLocks} -> @@ -286,13 +289,10 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, handle_update(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> Name = rebar_app_info:name(AppInfo), - ct:pal("update ~p", [Name]), case lists:keyfind(Name, 1, Locks) of false -> - ct:pal("in lock"), - case maybe_fetch(AppInfo, true, []) of + case maybe_fetch(AppInfo, true, sets:new()) of true -> - ct:pal("fetch!"), handle_dep(AppInfo ,SrcDeps ,PkgDeps @@ -302,11 +302,9 @@ handle_update(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> ,Locks); false -> - ct:pal("nofetch"), {SrcDeps, PkgDeps, SrcApps, State, Locks} end; _StillLocked -> - ct:pal("stillocked"), {SrcDeps, PkgDeps, SrcApps, State, Locks} end. @@ -365,9 +363,9 @@ maybe_fetch(AppInfo, Update, Seen) -> end end, - case not Exists orelse Update of + case not Exists of% orelse Update of true -> - ?INFO("Fetching ~s", [rebar_app_info:name(AppInfo)]), + ?INFO("Fetching ~s (~p)", [rebar_app_info:name(AppInfo), rebar_app_info:source(AppInfo)]), Source = rebar_app_info:source(AppInfo), case rebar_fetch:download_source(AppDir, Source) of {error, Reason} -> @@ -381,7 +379,7 @@ maybe_fetch(AppInfo, Update, Seen) -> false; false -> Source = rebar_app_info:source(AppInfo), - case rebar_fetch:needs_update(AppDir, Source) of + case Update andalso rebar_fetch:needs_update(AppDir, Source) of true -> ?INFO("Updating ~s", [rebar_app_info:name(AppInfo)]), case rebar_fetch:download_source(AppDir, Source) of @@ -462,6 +460,14 @@ new_dep(DepsDir, Name, Vsn, Source, State) -> rebar_state:overrides(S, ParentOverrides++Overrides)), rebar_app_info:source(Dep1, Source). +sort_deps(Deps) -> + %% We need a sort stable, based on the name. So that for multiple deps on + %% the same level with the same name, we keep the order the parents had. + %% `lists:keysort/2' is documented as stable in the stdlib. + %% The list of deps is revered when we get it. For the proper stable + %% result, re-reverse it. + lists:keysort(?APP_NAME_INDEX, lists:reverse(Deps)). + -spec parse_goal(binary(), binary()) -> pkg_dep(). parse_goal(Name, Constraint) -> case re:run(Constraint, "([^\\d]*)(\\d.*)", [{capture, [1,2], binary}]) of diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index 0c1ed8ad..e43841ce 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -56,12 +56,12 @@ do(State) -> NewLocks = unlock_higher_than(0, Locks -- [Lock]), State1 = rebar_state:set(State, {deps, default}, [{Name, Source, 0} | NewLocks]), State2 = rebar_state:set(State1, {locks, default}, NewLocks), - rebar_prv_install_deps:do(State2) + State3 = rebar_state:set(State2, upgrade, true), + rebar_prv_install_deps:do(State3) end; {_, _, Level} when Level > 0 -> {error, transitive_dependency}; false -> - ct:pal("deps: ~p", [{Name,Locks}]), {error, unknown_dependency} end. diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index a7d4a06d..0727fff8 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -38,13 +38,13 @@ init_per_testcase(Case, Config) -> [{expected, normalize_unlocks(Expectations)}, {mock, fun() -> mock_deps(DepsType, Expanded, []) end}, {mock_update, fun() -> mock_deps(DepsType, UpExpanded, ToUp) end} - | setup_project(Case, Config, Expanded)]. + | setup_project(Case, Config, Expanded, UpExpanded)]. end_per_testcase(_, Config) -> meck:unload(), Config. -setup_project(Case, Config0, Deps) -> +setup_project(Case, Config0, Deps, UpDeps) -> DepsType = ?config(deps_type, Config0), Config = rebar_test_utils:init_rebar_state( Config0, @@ -54,7 +54,8 @@ setup_project(Case, Config0, Deps) -> rebar_test_utils:create_app(AppDir, "Root", "0.0.0", [kernel, stdlib]), TopDeps = top_level_deps(Deps), RebarConf = rebar_test_utils:create_config(AppDir, [{deps, TopDeps}]), - [{rebarconfig, RebarConf} | Config]. + [{rebarconfig, RebarConf}, + {next_top_deps, top_level_deps(UpDeps)} | Config]. upgrades(top_a) -> @@ -279,6 +280,7 @@ top_level_deps([{{pkg, Name, Vsn, _URL}, _} | Deps]) -> mock_deps(git, Deps, Upgrades) -> catch mock_git_resource:unmock(), + ct:pal("mocked: ~p", [flat_deps(Deps)]), mock_git_resource:mock([{deps, flat_deps(Deps)}, {upgrade, Upgrades}]); mock_deps(pkg, Deps, Upgrades) -> catch mock_pkg_resource:unmock(), @@ -365,7 +367,10 @@ run(Config) -> _ -> {ok, Unlocks} end, apply(?config(mock_update, Config), []), + NewRebarConf = rebar_test_utils:create_config(?config(apps, Config), + [{deps, ?config(next_top_deps, Config)}]), + {ok, NewRebarConfig} = file:consult(NewRebarConf), rebar_test_utils:run_and_check( - Config, RebarConfig, ["upgrade", App], Expectation + Config, NewRebarConfig, ["upgrade", App], Expectation ). From f5acdfb9a5f600ae260a36a1fadd45576a1536fd Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Mon, 9 Feb 2015 16:05:08 +0000 Subject: [PATCH 5/8] Refactor install_deps to match 'upgrade' ideas - rename 'update' to 'upgrade' -- 'update' stands for package list updates, not dependencies upgrades - extracting some deeply nested code --- src/rebar_prv_install_deps.erl | 113 ++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 52 deletions(-) diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl index 125e8898..0e543244 100644 --- a/src/rebar_prv_install_deps.erl +++ b/src/rebar_prv_install_deps.erl @@ -134,17 +134,17 @@ handle_deps(Profile, State, Deps) -> -spec handle_deps(atom(), rebar_state:t(), list(), list() | boolean()) -> {ok, [rebar_app_info:t()], rebar_state:t()} | {error, string()}. -handle_deps(Profile, State, Deps, Update) when is_boolean(Update) -> - handle_deps(Profile, State, Deps, Update, []); +handle_deps(Profile, State, Deps, Upgrade) when is_boolean(Upgrade) -> + handle_deps(Profile, State, Deps, Upgrade, []); handle_deps(Profile, State, Deps, Locks) when is_list(Locks) -> - Update = rebar_state:get(State, upgrade, false), - handle_deps(Profile, State, Deps, Update, Locks). + Upgrade = rebar_state:get(State, upgrade, false), + handle_deps(Profile, State, Deps, Upgrade, Locks). -spec handle_deps(atom(), rebar_state:t(), list(), boolean() | {true, binary(), integer()}, list()) -> {ok, [rebar_app_info:t()], rebar_state:t()} | {error, string()}. handle_deps(_Profile, State, [], _, _) -> {ok, [], State}; -handle_deps(Profile, State, Deps, Update, Locks) -> +handle_deps(Profile, State, Deps, Upgrade, Locks) -> %% Read in package index and dep graph {Packages, Graph} = rebar_packages:get_packages(State), %% Split source deps from pkg deps, needed to keep backwards compatibility @@ -153,7 +153,7 @@ handle_deps(Profile, State, Deps, Update, Locks) -> %% Fetch transitive src deps {State1, SrcApps, PkgDeps1, Seen} = - update_src_deps(Profile, 0, SrcDeps, PkgDeps, [], State, Update, sets:new(), Locks), + update_src_deps(Profile, 0, SrcDeps, PkgDeps, [], State, Upgrade, sets:new(), Locks), {Solved, State2} = case PkgDeps1 of [] -> %% No pkg deps @@ -169,7 +169,7 @@ handle_deps(Profile, State, Deps, Update, Locks) -> [warn_skip_pkg(Pkg) || Pkg <- Discarded], Solution end, - update_pkg_deps(Profile, S, Packages, Update, Seen, State1) + update_pkg_deps(Profile, S, Packages, Upgrade, Seen, State1) end, AllDeps = lists:ukeymerge(2 @@ -184,7 +184,7 @@ handle_deps(Profile, State, Deps, Update, Locks) -> %% Internal functions %% =================================================================== -update_pkg_deps(Profile, Pkgs, Packages, Update, Seen, State) -> +update_pkg_deps(Profile, Pkgs, Packages, Upgrade, Seen, State) -> %% Create app_info record for each pkg dep DepsDir = rebar_dir:deps_dir(State), {Solved, _, State1} @@ -193,7 +193,7 @@ update_pkg_deps(Profile, Pkgs, Packages, Update, Seen, State) -> ,Packages ,Pkg), {SeenAcc1, StateAcc1} = maybe_lock(Profile, AppInfo, SeenAcc, StateAcc, 0), - case maybe_fetch(AppInfo, Update, SeenAcc1) of + case maybe_fetch(AppInfo, Upgrade, SeenAcc1) of true -> {[AppInfo | Acc], SeenAcc1, StateAcc1}; false -> @@ -239,7 +239,7 @@ package_to_app(DepsDir, Packages, {Name, Vsn}) -> end. -spec update_src_deps(atom(), non_neg_integer(), list(), list(), list(), rebar_state:t(), boolean(), sets:set(binary()), list()) -> {rebar_state:t(), list(), list(), sets:set(binary())}. -update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, Locks) -> +update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Upgrade, Seen, Locks) -> case lists:foldl(fun(AppInfo, {SrcDepsAcc, PkgDepsAcc, SrcAppsAcc, StateAcc, SeenAcc, LocksAcc}) -> %% If not seen, add to list of locks to write out Name = rebar_app_info:name(AppInfo), @@ -256,10 +256,10 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, false -> {SeenAcc1, StateAcc1} = maybe_lock(Profile, AppInfo, SeenAcc, StateAcc, Level), {SrcDepsAcc1, PkgDepsAcc1, SrcAppsAcc1, StateAcc2, LocksAcc1} = - case Update of + case Upgrade of true -> - %{true, UpdateName, UpdateLevel} -> - handle_update(AppInfo + %{true, UpgradeName, UpgradeLevel} -> + handle_upgrade(AppInfo ,SrcDepsAcc ,PkgDepsAcc ,SrcAppsAcc @@ -284,10 +284,10 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen, {[], NewPkgDeps, NewSrcApps, State1, Seen1, _NewLocks} -> {State1, NewSrcApps, NewPkgDeps, Seen1}; {NewSrcDeps, NewPkgDeps, NewSrcApps, State1, Seen1, NewLocks} -> - update_src_deps(Profile, Level+1, NewSrcDeps, NewPkgDeps, NewSrcApps, State1, Update, Seen1, NewLocks) + update_src_deps(Profile, Level+1, NewSrcDeps, NewPkgDeps, NewSrcApps, State1, Upgrade, Seen1, NewLocks) end. -handle_update(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> +handle_upgrade(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> Name = rebar_app_info:name(AppInfo), case lists:keyfind(Name, 1, Locks) of false -> @@ -334,7 +334,7 @@ handle_dep(State, DepsDir, AppInfo, Locks, Level) -> AppInfo1 = rebar_app_info:state(AppInfo, S3), Deps = rebar_state:get(S3, deps, []), - %% Update lock level to be the level the dep will have in this dep tree + %% Upgrade lock level to be the level the dep will have in this dep tree NewLocks = [{DepName, Source, LockLevel+Level} || {DepName, Source, LockLevel} <- rebar_state:get(S3, {locks, default}, [])], AppInfo2 = rebar_app_info:deps(AppInfo1, rebar_state:deps_names(Deps)), @@ -343,7 +343,7 @@ handle_dep(State, DepsDir, AppInfo, Locks, Level) -> -spec maybe_fetch(rebar_app_info:t(), boolean() | {true, binary(), integer()}, sets:set(binary())) -> boolean(). -maybe_fetch(AppInfo, Update, Seen) -> +maybe_fetch(AppInfo, Upgrade, Seen) -> AppDir = ec_cnv:to_list(rebar_app_info:dir(AppInfo)), Apps = rebar_app_discover:find_apps(["_checkouts"], all), case rebar_app_utils:find(rebar_app_info:name(AppInfo), Apps) of @@ -351,46 +351,15 @@ maybe_fetch(AppInfo, Update, Seen) -> %% Don't fetch dep if it exists in the _checkouts dir false; error -> - Exists = case rebar_app_utils:is_app_dir(filename:absname(AppDir)++"-*") of - {true, _} -> - true; - _ -> - case rebar_app_utils:is_app_dir(filename:absname(AppDir)) of - {true, _} -> - true; - _ -> - false - end - end, - - case not Exists of% orelse Update of + case not app_exists(AppDir) of true -> - ?INFO("Fetching ~s (~p)", [rebar_app_info:name(AppInfo), rebar_app_info:source(AppInfo)]), - Source = rebar_app_info:source(AppInfo), - case rebar_fetch:download_source(AppDir, Source) of - {error, Reason} -> - throw(Reason); - Result -> - Result - end; - _ -> + fetch_app(AppInfo, AppDir); + false -> case sets:is_element(rebar_app_info:name(AppInfo), Seen) of true -> false; false -> - Source = rebar_app_info:source(AppInfo), - case Update andalso rebar_fetch:needs_update(AppDir, Source) of - true -> - ?INFO("Updating ~s", [rebar_app_info:name(AppInfo)]), - case rebar_fetch:download_source(AppDir, Source) of - {error, Reason} -> - throw(Reason); - Result -> - Result - end; - false -> - false - end + maybe_upgrade(AppInfo, AppDir, Upgrade) end end end. @@ -460,6 +429,46 @@ new_dep(DepsDir, Name, Vsn, Source, State) -> rebar_state:overrides(S, ParentOverrides++Overrides)), rebar_app_info:source(Dep1, Source). +app_exists(AppDir) -> + case rebar_app_utils:is_app_dir(filename:absname(AppDir)++"-*") of + {true, _} -> + true; + _ -> + case rebar_app_utils:is_app_dir(filename:absname(AppDir)) of + {true, _} -> + true; + _ -> + false + end + end. + +fetch_app(AppInfo, AppDir) -> + ?INFO("Fetching ~s (~p)", [rebar_app_info:name(AppInfo), rebar_app_info:source(AppInfo)]), + Source = rebar_app_info:source(AppInfo), + case rebar_fetch:download_source(AppDir, Source) of + {error, Reason} -> + throw(Reason); + Result -> + Result + end. + +maybe_upgrade(_AppInfo, _AppDir, false) -> + false; +maybe_upgrade(AppInfo, AppDir, true) -> + Source = rebar_app_info:source(AppInfo), + case rebar_fetch:needs_update(AppDir, Source) of + true -> + ?INFO("Updating ~s", [rebar_app_info:name(AppInfo)]), + case rebar_fetch:download_source(AppDir, Source) of + {error, Reason} -> + throw(Reason); + Result -> + Result + end; + false -> + false + end. + sort_deps(Deps) -> %% We need a sort stable, based on the name. So that for multiple deps on %% the same level with the same name, we keep the order the parents had. From 62dc8fa9c745cca60f45ad301e05c8b70517626c Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 11 Feb 2015 14:00:43 +0000 Subject: [PATCH 6/8] Fix testcases, add multi-app upgrade support todo: - relock stuff - default to all apps needing upgrade - more tests? - pkgs? --- src/rebar_prv_install_deps.erl | 26 ++++++++++--- src/rebar_prv_upgrade.erl | 71 ++++++++++++++++++++++++++-------- test/mock_git_resource.erl | 3 +- test/rebar_upgrade_SUITE.erl | 69 ++++++++++++++++++++++++++++----- 4 files changed, 135 insertions(+), 34 deletions(-) diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl index 0e543244..dab4c24f 100644 --- a/src/rebar_prv_install_deps.erl +++ b/src/rebar_prv_install_deps.erl @@ -252,7 +252,22 @@ update_src_deps(Profile, Level, SrcDeps, PkgDeps, SrcApps, State, Upgrade, Seen, true -> ok end, - {SrcDepsAcc, PkgDepsAcc, SrcAppsAcc, StateAcc, SeenAcc, LocksAcc}; + %% scan for app children here if upgrading + case Upgrade of + false -> + {SrcDepsAcc, PkgDepsAcc, SrcAppsAcc, StateAcc, SeenAcc, LocksAcc}; + true -> + {SrcDepsAcc1, PkgDepsAcc1, SrcAppsAcc1, StateAcc2, LocksAcc1} = + handle_dep(AppInfo + ,SrcDepsAcc + ,PkgDepsAcc + ,SrcAppsAcc + ,Level + ,StateAcc + ,LocksAcc), + + {SrcDepsAcc1, PkgDepsAcc1, SrcAppsAcc1, StateAcc2, SeenAcc, LocksAcc1} + end; false -> {SeenAcc1, StateAcc1} = maybe_lock(Profile, AppInfo, SeenAcc, StateAcc, Level), {SrcDepsAcc1, PkgDepsAcc1, SrcAppsAcc1, StateAcc2, LocksAcc1} = @@ -302,10 +317,10 @@ handle_upgrade(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> ,Locks); false -> - {SrcDeps, PkgDeps, SrcApps, State, Locks} + {[AppInfo|SrcDeps], PkgDeps, SrcApps, State, Locks} end; _StillLocked -> - {SrcDeps, PkgDeps, SrcApps, State, Locks} + {[AppInfo|SrcDeps], PkgDeps, SrcApps, State, Locks} end. handle_dep(AppInfo, SrcDeps, PkgDeps, SrcApps, Level, State, Locks) -> @@ -452,8 +467,9 @@ fetch_app(AppInfo, AppDir) -> Result end. -maybe_upgrade(_AppInfo, _AppDir, false) -> - false; +maybe_upgrade(AppInfo, AppDir, false) -> + Source = rebar_app_info:source(AppInfo), + rebar_fetch:needs_update(AppDir, Source); maybe_upgrade(AppInfo, AppDir, true) -> Source = rebar_app_info:source(AppInfo), case rebar_fetch:needs_update(AppDir, Source) of diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index e43841ce..b9c64958 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -29,47 +29,84 @@ init(State) -> {module, ?MODULE}, {bare, false}, {deps, ?DEPS}, - {example, "rebar upgrade cowboy"}, + {example, "rebar upgrade cowboy[,ranch]"}, {short_desc, "Upgrade dependency."}, {desc, ""}, {opts, [ - {package, undefined, undefined, string, "Package to upgrade."} + {package, undefined, undefined, string, "Packages to upgrade."} ]}])), {ok, State1}. -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}. do(State) -> {Args, _} = rebar_state:command_parsed_args(State), - Name = ec_cnv:to_binary(proplists:get_value(package, Args)), + Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args))), + %% TODO: support many names. Only a subtree can be updated per app + %% mentioned. When no app is named, unlock *everything* Locks = rebar_state:get(State, {locks, default}, []), + Deps = rebar_state:get(State, deps), + case prepare_locks(Names, Deps, Locks, []) of + {error, Reason} -> + {error, Reason}; + {Locks0, Unlocks0} -> + Deps0 = top_level_deps(Deps, Locks), + State1 = rebar_state:set(State, {deps, default}, Deps0), + State2 = rebar_state:set(State1, {locks, default}, Locks0), + State3 = rebar_state:set(State2, upgrade, true), + Res = rebar_prv_install_deps:do(State3), + case Res of + {ok, S} -> + ct:pal("original locks ~p", [Locks]), + ct:pal("new locks ~p", [Locks0]), + ct:pal("old deps: ~p", [Deps]), + ct:pal("new deps: ~p", [Deps0]), + ct:pal("Unlocks: ~p", [Unlocks0]), + %% TODO: replace new locks onto the old locks list + rebar_prv_lock:do(S); + _ -> Res + end + end. + +parse_names(Bin) -> + lists:usort(re:split(Bin, <<" *, *">>, [trim])). + +prepare_locks([], _, Locks, Unlocks) -> + {Locks, Unlocks}; +prepare_locks([Name|Names], Deps, Locks, Unlocks) -> case lists:keyfind(Name, 1, Locks) of {_, _, 0} = Lock -> - Deps = rebar_state:get(State, deps), - case lists:keyfind(binary_to_atom(Name, utf8), 1, Deps) of + AtomName = binary_to_atom(Name, utf8), + case lists:keyfind(AtomName, 1, Deps) of false -> - {error, unknown_dependency}; + {error, {unknown_dependency, Name}}; Dep -> Source = case Dep of {_, Src} -> Src; {_, _, Src} -> Src end, - NewLocks = unlock_higher_than(0, Locks -- [Lock]), - State1 = rebar_state:set(State, {deps, default}, [{Name, Source, 0} | NewLocks]), - State2 = rebar_state:set(State1, {locks, default}, NewLocks), - State3 = rebar_state:set(State2, upgrade, true), - rebar_prv_install_deps:do(State3) + {NewLocks, NewUnlocks} = unlock_higher_than(0, Locks -- [Lock]), + prepare_locks(Names, + %deps_like_locks(Deps, [{Name,Source,0} | NewLocks]), + Deps, + NewLocks, + [{Name, Source, 0} | NewUnlocks ++ Unlocks]) end; {_, _, Level} when Level > 0 -> - {error, transitive_dependency}; + {error, {transitive_dependency,Name}}; false -> - {error, unknown_dependency} + {error, {unknown_dependency,Name}} end. +top_level_deps(Deps, Locks) -> + [Dep || Dep <- Deps, lists:keymember(0, 3, Locks)]. + +unlock_higher_than(Level, Locks) -> unlock_higher_than(Level, Locks, [], []). -unlock_higher_than(_, []) -> []; -unlock_higher_than(Level, [App = {_,_,AppLevel} | Apps]) -> - if AppLevel > Level -> unlock_higher_than(Level, Apps); - AppLevel =< Level -> [App | unlock_higher_than(Level, Apps)] +unlock_higher_than(_, [], Locks, Unlocks) -> + {Locks, Unlocks}; +unlock_higher_than(Level, [App = {_,_,AppLevel} | Apps], Locks, Unlocks) -> + if AppLevel > Level -> unlock_higher_than(Level, Apps, Locks, [App | Unlocks]); + AppLevel =< Level -> unlock_higher_than(Level, Apps, [App | Locks], Unlocks) end. -spec format_error(any()) -> iolist(). diff --git a/test/mock_git_resource.erl b/test/mock_git_resource.erl index 5d8288e2..2f7a72d4 100644 --- a/test/mock_git_resource.erl +++ b/test/mock_git_resource.erl @@ -60,7 +60,7 @@ mock_update(Opts) -> ?MOD, needs_update, fun(_Dir, {git, Url, _Ref}) -> App = app(Url), - ct:pal("Needed update? ~p -> ~p", [App, lists:member(App, ToUpdate)]), + ct:pal("Needed update? ~p (~p) -> ~p", [App, {Url,_Ref}, lists:member(App, ToUpdate)]), lists:member(App, ToUpdate) end). @@ -108,7 +108,6 @@ mock_download(Opts) -> {git, Url, {_, Vsn}} = normalize_git(Git, Overrides, Default), App = app(Url), AppDeps = proplists:get_value({App,Vsn}, Deps, []), - ct:pal("creating app ~p", [{Dir, App, Vsn, AppDeps}]), rebar_test_utils:create_app( Dir, App, Vsn, [element(1,D) || D <- AppDeps] diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index 0727fff8..64915272 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -7,9 +7,9 @@ all() -> [{group, git}].%, {group, pkg}]. groups() -> [{all, [], [top_a, top_b, top_c, top_d1, top_d2, top_e, - pair_a, pair_b, pair_c, + pair_a, pair_b, pair_ab, pair_c, triplet_a, triplet_b, triplet_c, - tree_a, tree_b, tree_c]}, + tree_a, tree_b, tree_c, tree_c2, tree_ac]}, {git, [], [{group, all}]}, {pkg, [], [{group, all}]}]. @@ -83,7 +83,7 @@ upgrades(top_b) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"B", {error, transitive_dependency}}}; + {"B", {error, {transitive_dependency, <<"B">>}}}}; upgrades(top_c) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -96,7 +96,7 @@ upgrades(top_c) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"C", {error, transitive_dependency}}}; + {"C", {error, {transitive_dependency, <<"C">>}}}}; upgrades(top_d1) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -109,7 +109,7 @@ upgrades(top_d1) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"D", {error, transitive_dependency}}}; + {"D", {error, {transitive_dependency, <<"D">>}}}}; upgrades(top_d2) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -122,7 +122,7 @@ upgrades(top_d2) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"D", {error, transitive_dependency}}}; + {"D", {error, {transitive_dependency, <<"D">>}}}}; upgrades(top_e) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -135,7 +135,7 @@ upgrades(top_e) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"E", {error, unknown_dependency}}}; + {"E", {error, {unknown_dependency, <<"E">>}}}}; upgrades(pair_a) -> {[{"A", "1", [{"C", "1", []}]}, {"B", "1", [{"D", "1", []}]} @@ -154,6 +154,15 @@ upgrades(pair_b) -> ], ["A","B","C","D"], {"B", [{"A","1"},{"C","1"},{"B","2"},{"D","2"}]}}; +upgrades(pair_ab) -> + {[{"A", "1", [{"C", "1", []}]}, + {"B", "1", [{"D", "1", []}]} + ], + [{"A", "2", [{"C", "2", []}]}, + {"B", "2", [{"D", "2", []}]} + ], + ["A","B","C","D"], + {"A,B", [{"A","2"},{"C","2"},{"B","2"},{"D","2"}]}}; upgrades(pair_c) -> {[{"A", "1", [{"C", "1", []}]}, {"B", "1", [{"D", "1", []}]} @@ -162,7 +171,7 @@ upgrades(pair_c) -> {"B", "2", [{"D", "2", []}]} ], ["A","B","C","D"], - {"C", {error, transitive_dependency}}}; + {"C", {error, {transitive_dependency, <<"C">>}}}}; upgrades(triplet_a) -> {[{"A", "1", [{"D",[]}, {"E","3",[]}]}, @@ -264,10 +273,47 @@ upgrades(tree_c) -> {"G",[]}]}, {"C", "1", [{"H",[]}]} ], - ["C"], + ["C","I"], {"C", [{"A","1"}, "D", "J", "E", {"I","1"}, {"B","1"}, "F", "G", - {"C","1"}, "H"]}}. + {"C","1"}, "H"]}}; +upgrades(tree_c2) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[{"K",[]}]}, + {"I","2",[]}]} + ], + ["C", "H"], + {"C", [{"A","1"}, "D", "J", "E", + {"B","1"}, "F", "G", + {"C","1"}, "H", {"I", "2"}, "K"]}}; +upgrades(tree_ac) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}]} + ], + ["C","I"], + {"C, A", [{"A","1"}, "D", "J", "E", {"I","1"}, + {"B","1"}, "F", "G", + {"C","1"}, "H"]}}. %% TODO: add a test that verifies that unlocking files and then %% running the upgrade code is enough to properly upgrade things. @@ -345,6 +391,7 @@ top_e(Config) -> run(Config). pair_a(Config) -> run(Config). pair_b(Config) -> run(Config). +pair_ab(Config) -> run(Config). pair_c(Config) -> run(Config). triplet_a(Config) -> run(Config). @@ -354,6 +401,8 @@ triplet_c(Config) -> run(Config). tree_a(Config) -> run(Config). tree_b(Config) -> run(Config). tree_c(Config) -> run(Config). +tree_c2(Config) -> run(Config). +tree_ac(Config) -> run(Config). run(Config) -> apply(?config(mock, Config), []), From 6d8567a7edbf206a0f595c22ecd17af0c46acb87 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 11 Feb 2015 20:06:49 +0000 Subject: [PATCH 7/8] Support multiple app upgrade & lock tests - Many apps is supported through and through - Not mentioning any app upgrades all apps - Locks are refreshed on disk and tested as such after an upgrade --- src/rebar_prv_upgrade.erl | 38 ++++++++++++++++----------------- test/rebar_test_utils.erl | 14 ++++++++++++ test/rebar_upgrade_SUITE.erl | 41 +++++++++++++++++++++++++++++++----- 3 files changed, 69 insertions(+), 24 deletions(-) diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index b9c64958..5ee2be65 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -40,35 +40,39 @@ init(State) -> -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}. do(State) -> {Args, _} = rebar_state:command_parsed_args(State), - Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args))), - %% TODO: support many names. Only a subtree can be updated per app - %% mentioned. When no app is named, unlock *everything* Locks = rebar_state:get(State, {locks, default}, []), Deps = rebar_state:get(State, deps), + Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args)), Locks), case prepare_locks(Names, Deps, Locks, []) of {error, Reason} -> {error, Reason}; - {Locks0, Unlocks0} -> + {Locks0, _Unlocks0} -> % unlocks may be useful for deletions Deps0 = top_level_deps(Deps, Locks), State1 = rebar_state:set(State, {deps, default}, Deps0), State2 = rebar_state:set(State1, {locks, default}, Locks0), State3 = rebar_state:set(State2, upgrade, true), Res = rebar_prv_install_deps:do(State3), case Res of - {ok, S} -> - ct:pal("original locks ~p", [Locks]), - ct:pal("new locks ~p", [Locks0]), - ct:pal("old deps: ~p", [Deps]), - ct:pal("new deps: ~p", [Deps0]), - ct:pal("Unlocks: ~p", [Unlocks0]), - %% TODO: replace new locks onto the old locks list - rebar_prv_lock:do(S); - _ -> Res + {ok, State4} -> + rebar_prv_lock:do(State4); + _ -> + Res end end. -parse_names(Bin) -> - lists:usort(re:split(Bin, <<" *, *">>, [trim])). +-spec format_error(any()) -> iolist(). +format_error(Reason) -> + io_lib:format("~p", [Reason]). + + +parse_names(Bin, Locks) -> + case lists:usort(re:split(Bin, <<" *, *">>, [trim])) of + %% Nothing submitted, use *all* apps + [<<"">>] -> [Name || {Name, _, 0} <- Locks]; + [] -> [Name || {Name, _, 0} <- Locks]; + %% Regular options + Other -> Other + end. prepare_locks([], _, Locks, Unlocks) -> {Locks, Unlocks}; @@ -109,7 +113,3 @@ unlock_higher_than(Level, [App = {_,_,AppLevel} | Apps], Locks, Unlocks) -> AppLevel =< Level -> unlock_higher_than(Level, Apps, [App | Locks], Unlocks) end. --spec format_error(any()) -> iolist(). -format_error(Reason) -> - io_lib:format("~p", [Reason]). - diff --git a/test/rebar_test_utils.erl b/test/rebar_test_utils.erl index 827d1342..96200a6e 100644 --- a/test/rebar_test_utils.erl +++ b/test/rebar_test_utils.erl @@ -103,6 +103,8 @@ create_random_vsn() -> check_results(AppDir, Expected) -> BuildDir = filename:join([AppDir, "_build", "lib"]), CheckoutsDir = filename:join([AppDir, "_checkouts"]), + LockFile = filename:join([AppDir, "rebar.lock"]), + Locks = lists:flatten(rebar_config:consult_file(LockFile)), Apps = rebar_app_discover:find_apps([AppDir]), InvalidApps = rebar_app_discover:find_apps([AppDir], invalid), ValidApps = rebar_app_discover:find_apps([AppDir], valid), @@ -153,6 +155,18 @@ check_results(AppDir, Expected) -> ?assertEqual(iolist_to_binary(Vsn), iolist_to_binary(rebar_app_info:original_vsn(App))) end + ; ({lock, Name}) -> + ct:pal("Name: ~p", [Name]), + ?assertNotEqual(false, lists:keyfind(iolist_to_binary(Name), 1, Locks)) + ; ({lock, Name, Vsn}) -> + ct:pal("Name: ~p, Vsn: ~p", [Name, Vsn]), + case lists:keyfind(iolist_to_binary(Name), 1, Locks) of + false -> + error({lock_not_found, Name}); + {_LockName, {_, _, {ref, LockVsn}}, _} -> + ?assertEqual(iolist_to_binary(Vsn), + iolist_to_binary(LockVsn)) + end end, Expected). write_src_file(Dir, Name) -> diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index 64915272..e0bb0118 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -7,9 +7,9 @@ all() -> [{group, git}].%, {group, pkg}]. groups() -> [{all, [], [top_a, top_b, top_c, top_d1, top_d2, top_e, - pair_a, pair_b, pair_ab, pair_c, + pair_a, pair_b, pair_ab, pair_c, pair_all, triplet_a, triplet_b, triplet_c, - tree_a, tree_b, tree_c, tree_c2, tree_ac]}, + tree_a, tree_b, tree_c, tree_c2, tree_ac, tree_all]}, {git, [], [{group, all}]}, {pkg, [], [{group, all}]}]. @@ -172,6 +172,15 @@ upgrades(pair_c) -> ], ["A","B","C","D"], {"C", {error, {transitive_dependency, <<"C">>}}}}; +upgrades(pair_all) -> + {[{"A", "1", [{"C", "1", []}]}, + {"B", "1", [{"D", "1", []}]} + ], + [{"A", "2", [{"C", "2", []}]}, + {"B", "2", [{"D", "2", []}]} + ], + ["A","B","C","D"], + {"", [{"A","2"},{"C","2"},{"B","2"},{"D","2"}]}}; upgrades(triplet_a) -> {[{"A", "1", [{"D",[]}, {"E","3",[]}]}, @@ -313,7 +322,25 @@ upgrades(tree_ac) -> ["C","I"], {"C, A", [{"A","1"}, "D", "J", "E", {"I","1"}, {"B","1"}, "F", "G", - {"C","1"}, "H"]}}. + {"C","1"}, "H"]}}; +upgrades(tree_all) -> + {[{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}, + {"I","2",[]}]} + ], + [{"A", "1", [{"D",[{"J",[]}]}, + {"E",[{"I","1",[]}]}]}, + {"B", "1", [{"F",[]}, + {"G",[]}]}, + {"C", "1", [{"H",[]}]} + ], + ["C","I"], + {"", [{"A","1"}, "D", "J", "E", {"I","1"}, + {"B","1"}, "F", "G", + {"C","1"}, "H"]}}. %% TODO: add a test that verifies that unlocking files and then %% running the upgrade code is enough to properly upgrade things. @@ -377,10 +404,12 @@ normalize_unlocks_expect({error, Reason}) -> normalize_unlocks_expect([]) -> []; normalize_unlocks_expect([{App,Vsn} | Rest]) -> - [{dep, App, Vsn} + [{dep, App, Vsn}, + {lock, App, Vsn} | normalize_unlocks_expect(Rest)]; normalize_unlocks_expect([App | Rest]) -> - [{dep, App} | normalize_unlocks_expect(Rest)]. + [{dep, App}, + {lock, App} | normalize_unlocks_expect(Rest)]. top_a(Config) -> run(Config). top_b(Config) -> run(Config). @@ -393,6 +422,7 @@ pair_a(Config) -> run(Config). pair_b(Config) -> run(Config). pair_ab(Config) -> run(Config). pair_c(Config) -> run(Config). +pair_all(Config) -> run(Config). triplet_a(Config) -> run(Config). triplet_b(Config) -> run(Config). @@ -403,6 +433,7 @@ tree_b(Config) -> run(Config). tree_c(Config) -> run(Config). tree_c2(Config) -> run(Config). tree_ac(Config) -> run(Config). +tree_all(Config) -> run(Config). run(Config) -> apply(?config(mock, Config), []), From 2e34e91dfb54218597f72faac3a39aedd57bcf97 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 11 Feb 2015 21:00:37 +0000 Subject: [PATCH 8/8] Warnings for Deletions and friendly errors Apps that are no longer used are not automatically deleted, but we tell users it can be done. This is safer while we're not sure of the correctness of these messages. Error messages are added for transient dependencies and dependencies not found. --- src/rebar_prv_upgrade.erl | 38 ++++++++++++++++++++++++++---------- test/mock_git_resource.erl | 2 +- test/rebar_upgrade_SUITE.erl | 37 ++++++++++++++++++++++++++--------- 3 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/rebar_prv_upgrade.erl b/src/rebar_prv_upgrade.erl index 5ee2be65..1cae5aad 100644 --- a/src/rebar_prv_upgrade.erl +++ b/src/rebar_prv_upgrade.erl @@ -29,11 +29,14 @@ init(State) -> {module, ?MODULE}, {bare, false}, {deps, ?DEPS}, - {example, "rebar upgrade cowboy[,ranch]"}, - {short_desc, "Upgrade dependency."}, - {desc, ""}, + {example, "rebar3 upgrade [cowboy[,ranch]]"}, + {short_desc, "Upgrade dependencies."}, + {desc, "Upgrade project dependecies. Mentioning no application " + "will upgrade all dependencies. To upgrade specific dependencies, " + "their names can be listed in the command."}, {opts, [ - {package, undefined, undefined, string, "Packages to upgrade."} + {package, undefined, undefined, string, + "List of packages to upgrade. If not specified, all dependencies are upgraded."} ]}])), {ok, State1}. @@ -42,11 +45,11 @@ do(State) -> {Args, _} = rebar_state:command_parsed_args(State), Locks = rebar_state:get(State, {locks, default}, []), Deps = rebar_state:get(State, deps), - Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args)), Locks), + Names = parse_names(ec_cnv:to_binary(proplists:get_value(package, Args, <<"">>)), Locks), case prepare_locks(Names, Deps, Locks, []) of {error, Reason} -> {error, Reason}; - {Locks0, _Unlocks0} -> % unlocks may be useful for deletions + {Locks0, _Unlocks0} -> Deps0 = top_level_deps(Deps, Locks), State1 = rebar_state:set(State, {deps, default}, Deps0), State2 = rebar_state:set(State1, {locks, default}, Locks0), @@ -54,6 +57,10 @@ do(State) -> Res = rebar_prv_install_deps:do(State3), case Res of {ok, State4} -> + info_useless( + [element(1,Lock) || Lock <- Locks], + [rebar_app_info:name(App) || App <- rebar_state:lock(State4)] + ), rebar_prv_lock:do(State4); _ -> Res @@ -61,6 +68,12 @@ do(State) -> end. -spec format_error(any()) -> iolist(). +format_error({unknown_dependency, Name}) -> + io_lib:format("Dependency ~ts not found", [Name]); +format_error({transitive_dependency, Name}) -> + io_lib:format("Dependency ~ts is transient and cannot be safely upgraded. " + "Promote it to your top-level rebar.config file to upgrade it.", + [Name]); format_error(Reason) -> io_lib:format("~p", [Reason]). @@ -82,7 +95,7 @@ prepare_locks([Name|Names], Deps, Locks, Unlocks) -> AtomName = binary_to_atom(Name, utf8), case lists:keyfind(AtomName, 1, Deps) of false -> - {error, {unknown_dependency, Name}}; + {error, {?MODULE, {unknown_dependency, Name}}}; Dep -> Source = case Dep of {_, Src} -> Src; @@ -90,15 +103,14 @@ prepare_locks([Name|Names], Deps, Locks, Unlocks) -> end, {NewLocks, NewUnlocks} = unlock_higher_than(0, Locks -- [Lock]), prepare_locks(Names, - %deps_like_locks(Deps, [{Name,Source,0} | NewLocks]), Deps, NewLocks, [{Name, Source, 0} | NewUnlocks ++ Unlocks]) end; {_, _, Level} when Level > 0 -> - {error, {transitive_dependency,Name}}; + {error, {?MODULE, {transitive_dependency,Name}}}; false -> - {error, {unknown_dependency,Name}} + {error, {?MODULE, {unknown_dependency,Name}}} end. top_level_deps(Deps, Locks) -> @@ -113,3 +125,9 @@ unlock_higher_than(Level, [App = {_,_,AppLevel} | Apps], Locks, Unlocks) -> AppLevel =< Level -> unlock_higher_than(Level, Apps, [App | Locks], Unlocks) end. +info_useless(Old, New) -> + [?INFO("App ~ts is no longer needed and can be deleted.", [Name]) + || Name <- Old, + not lists:member(Name, New)], + ok. + diff --git a/test/mock_git_resource.erl b/test/mock_git_resource.erl index 2f7a72d4..d2f02074 100644 --- a/test/mock_git_resource.erl +++ b/test/mock_git_resource.erl @@ -60,7 +60,7 @@ mock_update(Opts) -> ?MOD, needs_update, fun(_Dir, {git, Url, _Ref}) -> App = app(Url), - ct:pal("Needed update? ~p (~p) -> ~p", [App, {Url,_Ref}, lists:member(App, ToUpdate)]), +% ct:pal("Needed update? ~p (~p) -> ~p", [App, {Url,_Ref}, lists:member(App, ToUpdate)]), lists:member(App, ToUpdate) end). diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl index e0bb0118..39b9687c 100644 --- a/test/rebar_upgrade_SUITE.erl +++ b/test/rebar_upgrade_SUITE.erl @@ -9,7 +9,8 @@ groups() -> [{all, [], [top_a, top_b, top_c, top_d1, top_d2, top_e, pair_a, pair_b, pair_ab, pair_c, pair_all, triplet_a, triplet_b, triplet_c, - tree_a, tree_b, tree_c, tree_c2, tree_ac, tree_all]}, + tree_a, tree_b, tree_c, tree_c2, tree_ac, tree_all, + delete_d]}, {git, [], [{group, all}]}, {pkg, [], [{group, all}]}]. @@ -83,7 +84,7 @@ upgrades(top_b) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"B", {error, {transitive_dependency, <<"B">>}}}}; + {"B", {error, {rebar_prv_upgrade, {transitive_dependency, <<"B">>}}}}}; upgrades(top_c) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -96,7 +97,7 @@ upgrades(top_c) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"C", {error, {transitive_dependency, <<"C">>}}}}; + {"C", {error, {rebar_prv_upgrade, {transitive_dependency, <<"C">>}}}}}; upgrades(top_d1) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -109,7 +110,7 @@ upgrades(top_d1) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"D", {error, {transitive_dependency, <<"D">>}}}}; + {"D", {error, {rebar_prv_upgrade, {transitive_dependency, <<"D">>}}}}}; upgrades(top_d2) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -122,7 +123,7 @@ upgrades(top_d2) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"D", {error, {transitive_dependency, <<"D">>}}}}; + {"D", {error, {rebar_prv_upgrade, {transitive_dependency, <<"D">>}}}}}; upgrades(top_e) -> %% Original tree {[{"A", "1", [{"B", [{"D", "1", []}]}, @@ -135,7 +136,7 @@ upgrades(top_e) -> %% Modified apps, gobally ["A","B","D"], %% upgrade vs. new tree - {"E", {error, {unknown_dependency, <<"E">>}}}}; + {"E", {error, {rebar_prv_upgrade, {unknown_dependency, <<"E">>}}}}}; upgrades(pair_a) -> {[{"A", "1", [{"C", "1", []}]}, {"B", "1", [{"D", "1", []}]} @@ -171,7 +172,7 @@ upgrades(pair_c) -> {"B", "2", [{"D", "2", []}]} ], ["A","B","C","D"], - {"C", {error, {transitive_dependency, <<"C">>}}}}; + {"C", {error, {rebar_prv_upgrade, {transitive_dependency, <<"C">>}}}}}; upgrades(pair_all) -> {[{"A", "1", [{"C", "1", []}]}, {"B", "1", [{"D", "1", []}]} @@ -340,7 +341,17 @@ upgrades(tree_all) -> ["C","I"], {"", [{"A","1"}, "D", "J", "E", {"I","1"}, {"B","1"}, "F", "G", - {"C","1"}, "H"]}}. + {"C","1"}, "H"]}}; +upgrades(delete_d) -> + {[{"A", "1", [{"B", [{"D", "1", []}]}, + {"C", [{"D", "2", []}]}]} + ], + [{"A", "2", [{"B", []}, + {"C", []}]} + ], + ["A","B", "C"], + %% upgrade vs. new tree + {"", [{"A","2"}, "B", "C"]}}. %% TODO: add a test that verifies that unlocking files and then %% running the upgrade code is enough to properly upgrade things. @@ -353,7 +364,6 @@ top_level_deps([{{pkg, Name, Vsn, _URL}, _} | Deps]) -> mock_deps(git, Deps, Upgrades) -> catch mock_git_resource:unmock(), - ct:pal("mocked: ~p", [flat_deps(Deps)]), mock_git_resource:mock([{deps, flat_deps(Deps)}, {upgrade, Upgrades}]); mock_deps(pkg, Deps, Upgrades) -> catch mock_pkg_resource:unmock(), @@ -435,6 +445,15 @@ tree_c2(Config) -> run(Config). tree_ac(Config) -> run(Config). tree_all(Config) -> run(Config). +delete_d(Config) -> + meck:new(rebar_log, [no_link, passthrough]), + run(Config), + Infos = [{Str, Args} + || {_, {rebar_log, log, [info, Str, Args]}, _} <- meck:history(rebar_log)], + meck:unload(rebar_log), + ?assertNotEqual([], + [1 || {"App ~ts is no longer needed and can be deleted.", + [<<"D">>]} <- Infos]). run(Config) -> apply(?config(mock, Config), []), {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)),