Merge pull request #2031 from ferd/fix-transient-upgrade-locks

Fix handling of transient locks during upgrade

2 files changed, 71 insertions, 4 deletions

diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl
index 068c4c8..0187b4f 100644
--- a/src/rebar_prv_install_deps.erl
+++ b/src/rebar_prv_install_deps.erl
@@ -206,10 +206,23 @@ maybe_lock(Profile, AppInfo, Seen, State, Level) ->
                 default ->
                     case sets:is_element(Name, Seen) of
                         false ->
+                            %% Check whether the currently existing lock is
+                            %% deeper than the current one (which can happen
+                            %% during an upgrade). If the current app is
+                            %% shallower than the existing lock, replace the
+                            %% existing lock. This prevents weird transient
+                            %% lock-tree states (which would self-heal on a
+                            %% later run) after a `rebar3 upgrade <app>'
+                            %% command when a deep dep switches lineages for
+                            %% another newer parent.
                             Locks = rebar_state:lock(State),
-                            case lists:any(fun(App) -> rebar_app_info:name(App) =:= Name end, Locks) of
-                                true ->
+                            case find_app_and_level_by_name(Locks, Name) of
+                                {ok, _App, LockLvl} when LockLvl =< Level ->
                                     {sets:add_element(Name, Seen), State};
+                                {ok, App, _LockLvl} ->
+                                    LockedApp = rebar_app_info:dep_level(AppInfo, Level),
+                                    {sets:add_element(Name, Seen),
+                                     rebar_state:lock(State, [LockedApp | Locks -- [App]])};
                                 false ->
                                     {sets:add_element(Name, Seen),
                                      rebar_state:lock(State, rebar_app_info:dep_level(AppInfo, Level))}
@@ -426,3 +439,12 @@ not_needs_compile(App) ->
     not(rebar_app_info:is_checkout(App))
         andalso rebar_app_info:valid(App)
           andalso rebar_app_info:has_all_artifacts(App) =:= true.
+
+find_app_and_level_by_name([], _) ->
+    false;
+find_app_and_level_by_name([App|Apps], Name) ->
+    case rebar_app_info:name(App) of
+        Name -> {ok, App, rebar_app_info:dep_level(App)};
+        _ -> find_app_and_level_by_name(Apps, Name)
+    end.
+
diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl
index c55456c..54ff18c 100644
--- a/test/rebar_upgrade_SUITE.erl
+++ b/test/rebar_upgrade_SUITE.erl
@@ -12,7 +12,7 @@ groups() ->
                 tree_a, tree_b, tree_c, tree_c2, tree_cj, tree_ac, tree_all,
                 delete_d, promote, stable_lock, fwd_lock,
                 compile_upgrade_parity, umbrella_config,
-                profiles, profiles_exclusion]},
+                profiles, profiles_exclusion, tree_migration]},
      {git, [], [{group, all}]},
      {pkg, [], [{group, all}]}].
 
@@ -512,7 +512,15 @@ upgrades(profiles_exclusion) ->
      ["A","B","C","E","F","H"],
      {"A", [{"A","1.0.0"}, "D", {"E","3.0.0"},
             {"B","2.0.0"}, {"F","2.0.0"}, "G",
-            {"C","1.0.0"}, {"H","4.0.0"}, "I"]}}.
+            {"C","1.0.0"}, {"H","4.0.0"}, "I"]}};
+upgrades(tree_migration) ->
+    {[{"B", "1.0.0", []},
+      {"C", "1.0.0", [{"D","1.0.0",[{"E", "1.0.0", []}]}]}],
+     [{"B", "2.0.0", [{"E","1.0.0",[]}]},
+      {"C", "1.0.0", [{"D","1.0.0",[]}]}],
+     ["B"],
+     {"B", [{"A","1.0.0"}, "D", {"E","1.0.0"},
+            {"B","2.0.0"}]}}.
 
 %% TODO: add a test that verifies that unlocking files and then
 %% running the upgrade code is enough to properly upgrade things.
@@ -702,6 +710,43 @@ profiles(Config) ->
 
 profiles_exclusion(Config) -> profiles(Config).
 
+tree_migration(Config) ->
+    apply(?config(mock, 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),
+
+    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(ConfigPath),
+                                                  [{deps, ?config(next_top_deps, Config)}]),
+    {ok, NewRebarConfig} = file:consult(NewRebarConf),
+    {ok, NewState} = rebar_test_utils:run_and_check(
+        Config, NewRebarConfig, ["upgrade", App], return
+    ),
+    meck:unload(rebar_prv_upgrade),
+    %% Check that the internal state properly has E with a lock-level
+    %% of 1.
+    Locks = rebar_state:lock(NewState),
+    [Locked] = [X || X <- Locks, rebar_app_info:name(X) =:= <<"E">>],
+    ?assertEqual(1, rebar_app_info:dep_level(Locked)),
+    %% Check that the lockfile on disk agrees
+    AppDir = ?config(apps, Config),
+    Lockfile = filename:join([AppDir, "rebar.lock"]),
+    case file:consult(Lockfile) of
+        {ok, [{_Vsn, Prop}|_]} -> % packages
+            ?assertMatch({<<"E">>, _, 1}, lists:keyfind(<<"E">>, 1, Prop));
+        {ok, [Prop]} -> % git source
+            ?assertMatch({<<"E">>, _, 1}, lists:keyfind(<<"E">>, 1, Prop))
+    end,
+    ok.
+
+
 run(Config) ->
     apply(?config(mock, Config), []),
     ConfigPath = ?config(rebarconfig, Config),