Merge pull request #6480 from emqx/improve-update-appup-43

chore(update_appup): Improve `update_appup.escript`

scripts/update_appup.escript

@@ -189,8 +189,11 @@ find_appup_actions(CurrApps, PrevApps) ->
     maps:fold(
       fun(App, CurrAppIdx, Acc) ->
               case PrevApps of
-                  #{App := PrevAppIdx} -> find_appup_actions(App, CurrAppIdx, PrevAppIdx) ++ Acc;
-                  _                    -> Acc %% New app, nothing to upgrade here.
+                  #{App := PrevAppIdx} ->
+                      find_appup_actions(App, CurrAppIdx, PrevAppIdx) ++ Acc;
+                  _ ->
+                      %% New app, nothing to upgrade here.
+                      Acc
               end
       end,
       [],
@@ -199,8 +202,12 @@ find_appup_actions(CurrApps, PrevApps) ->
 find_appup_actions(_App, AppIdx, AppIdx) ->
     %% No changes to the app, ignore:
     [];
-find_appup_actions(App, CurrAppIdx, PrevAppIdx = #app{version = PrevVersion}) ->
-    {OldUpgrade, OldDowngrade} = find_old_appup_actions(App, PrevVersion),
+find_appup_actions(App,
+                   CurrAppIdx = #app{version = CurrVersion},
+                   PrevAppIdx = #app{version = PrevVersion}) ->
+    {OldUpgrade0, OldDowngrade0} = find_old_appup_actions(App, PrevVersion),
+    OldUpgrade = ensure_all_patch_versions(App, CurrVersion, OldUpgrade0),
+    OldDowngrade = ensure_all_patch_versions(App, CurrVersion, OldDowngrade0),
     Upgrade = merge_update_actions(App, diff_app(App, CurrAppIdx, PrevAppIdx), OldUpgrade),
     Downgrade = merge_update_actions(App, diff_app(App, PrevAppIdx, CurrAppIdx), OldDowngrade),
     if OldUpgrade =:= Upgrade andalso OldDowngrade =:= Downgrade ->
@@ -210,14 +217,40 @@ find_appup_actions(App, CurrAppIdx, PrevAppIdx = #app{version = PrevVersion}) ->
             [{App, {Upgrade, Downgrade, OldUpgrade, OldDowngrade}}]
     end.
 
+%% To avoid missing one patch version when upgrading, we try to
+%% optimistically generate the list of expected versions that should
+%% be covered by the upgrade.
+ensure_all_patch_versions(App, CurrVsn, OldActions) ->
+    case is_app_external(App) of
+        true ->
+            %% we do not attempt to predict the version list for
+            %% external dependencies, as those may not follow our
+            %% conventions.
+            OldActions;
+        false ->
+            do_ensure_all_patch_versions(App, CurrVsn, OldActions)
+    end.
+
+do_ensure_all_patch_versions(App, CurrVsn, OldActions) ->
+    case enumerate_past_versions(CurrVsn) of
+        {ok, ExpectedVsns} ->
+            CoveredVsns = [V || {V, _} <- OldActions, V =/= <<".*">>],
+            ExpectedVsnStrs = [vsn_number_to_string(V) || V <- ExpectedVsns],
+            MissingActions = [{V, []} || V <- ExpectedVsnStrs, not contains_version(V, CoveredVsns)],
+            MissingActions ++ OldActions;
+        {error, bad_version} ->
+            log("WARN: Could not infer expected versions to upgrade from for ~p~n", [App]),
+            OldActions
+    end.
+
 %% For external dependencies, show only the changes that are missing
 %% in their current appup.
 diff_appup_instructions(ComputedChanges, PresentChanges) ->
     lists:foldr(
-      fun({Vsn, ComputedActions}, Acc) ->
-              case find_matching_version(Vsn, PresentChanges) of
+      fun({VsnOrRegex, ComputedActions}, Acc) ->
+              case find_matching_version(VsnOrRegex, PresentChanges) of
                   undefined ->
-                      [{Vsn, ComputedActions} | Acc];
+                      [{VsnOrRegex, ComputedActions} | Acc];
                   PresentActions ->
                       DiffActions = ComputedActions -- PresentActions,
                       case DiffActions of
@@ -225,7 +258,7 @@ diff_appup_instructions(ComputedChanges, PresentChanges) ->
                               %% no diff
                               Acc;
                           _ ->
-                              [{Vsn, DiffActions} | Acc]
+                              [{VsnOrRegex, DiffActions} | Acc]
                       end
               end
       end,
@@ -250,8 +283,11 @@ parse_appup_diffs(Upgrade, OldUpgrade, Downgrade, OldDowngrade) ->
     end.
 
 %% TODO: handle regexes
-find_matching_version(Vsn, PresentChanges) ->
-    proplists:get_value(Vsn, PresentChanges).
+%% Since the first argument may be a regex itself, we would need to
+%% check if it is "contained" within other regexes inside list of
+%% versions in the second argument.
+find_matching_version(VsnOrRegex, PresentChanges) ->
+    proplists:get_value(VsnOrRegex, PresentChanges).
 
 find_old_appup_actions(App, PrevVersion) ->
     {Upgrade0, Downgrade0} =
@@ -289,11 +325,38 @@ do_merge_update_actions(App, {New0, Changed0, Deleted0}, OldActions) ->
     New = New0 -- AlreadyHandled,
     Changed = Changed0 -- AlreadyHandled,
     Deleted = Deleted0 -- AlreadyHandled,
-    [{load_module, M, brutal_purge, soft_purge, []} || M <- Changed ++ New] ++
-        OldActions ++
+    Reloads = [{load_module, M, brutal_purge, soft_purge, []}
+               || not contains_restart_application(App, OldActions),
+                  M <- Changed ++ New],
+    {OldActionsWithStop, OldActionsAfterStop} =
+        find_application_stop_instruction(App, OldActions),
+    OldActionsWithStop ++
+        Reloads ++
+        OldActionsAfterStop ++
         [{delete_module, M} || M <- Deleted] ++
         AppSpecific.
 
+%% If an entry restarts an application, there's no need to use
+%% `load_module' instructions.
+contains_restart_application(Application, Actions) ->
+    lists:member({restart_application, Application}, Actions).
+
+%% If there is an `application:stop(Application)' call in the
+%% instructions, we insert `load_module' instructions after it.
+find_application_stop_instruction(Application, Actions) ->
+    {Before, After0} =
+        lists:splitwith(
+          fun({apply, {application, stop, [App]}}) when App =:= Application ->
+                  false;
+             (_) ->
+                  true
+          end, Actions),
+    case After0 of
+        [StopInst | After] ->
+            {Before ++ [StopInst], After};
+        [] ->
+            {[], Before}
+    end.
 
 %% @doc Process the existing actions to exclude modules that are
 %% already handled
@@ -308,14 +371,57 @@ process_old_action(_) ->
     [].
 
 ensure_version(Version, OldInstructions) ->
-    OldVersions = [ensure_string(element(1, I)) || I <- OldInstructions],
-    case lists:member(Version, OldVersions) of
+    OldVersions = [element(1, I) || I <- OldInstructions],
+    case contains_version(Version, OldVersions) of
         false ->
-            [{Version, []}|OldInstructions];
-        _ ->
+            [{Version, []} | OldInstructions];
+        true ->
             OldInstructions
     end.
 
+contains_version(Needle, Haystack) when is_list(Needle) ->
+    lists:any(
+      fun(Regex) when is_binary(Regex) ->
+              case re:run(Needle, Regex) of
+                  {match, _} ->
+                      true;
+                  nomatch ->
+                      false
+              end;
+         (Vsn) ->
+              Vsn =:= Needle
+      end,
+      Haystack).
+
+%% As a best effort approach, we assume that we only bump patch
+%% version numbers between release upgrades for our dependencies and
+%% that we deal only with 3-part version schemas
+%% (`Major.Minor.Patch').  Using those assumptions, we enumerate the
+%% past versions that should be covered by regexes in .appup file
+%% instructions.
+enumerate_past_versions(Vsn) when is_list(Vsn) ->
+    case parse_version_number(Vsn) of
+        {ok, ParsedVsn} ->
+            {ok, enumerate_past_versions(ParsedVsn)};
+        Error ->
+            Error
+    end;
+enumerate_past_versions({Major, Minor, Patch}) ->
+    [{Major, Minor, P} || P <- lists:seq(Patch - 1, 0, -1)].
+
+parse_version_number(Vsn) when is_list(Vsn) ->
+    Nums = string:split(Vsn, ".", all),
+    Results = lists:map(fun string:to_integer/1, Nums),
+    case Results of
+        [{Major, []}, {Minor, []}, {Patch, []}] ->
+            {ok, {Major, Minor, Patch}};
+        _ ->
+            {error, bad_version}
+    end.
+
+vsn_number_to_string({Major, Minor, Patch}) ->
+    io_lib:format("~b.~b.~b", [Major, Minor, Patch]).
+
 read_appup(File) ->
     %% NOTE: appup file is a script, it may contain variables or functions.
     case file:script(File, [{'VSN', "VSN"}]) of
@@ -339,7 +445,12 @@ update_appups(Changes) ->
 do_update_appup(App, Upgrade, Downgrade, OldUpgrade, OldDowngrade) ->
     case locate(src, App, ".appup.src") of
         {ok, AppupFile} ->
-            render_appfile(AppupFile, Upgrade, Downgrade);
+            case contains_contents(AppupFile, Upgrade, Downgrade) of
+                true ->
+                    ok;
+                false ->
+                    render_appfile(AppupFile, Upgrade, Downgrade)
+            end;
         undefined ->
             case create_stub(App) of
                 {ok, AppupFile} ->
@@ -367,7 +478,8 @@ render_appfile(File, Upgrade, Downgrade) ->
     ok = file:write_file(File, IOList).
 
 create_stub(App) ->
-    case locate(src, App, Ext = ".app.src") of
+    Ext = ".app.src",
+    case locate(src, App, Ext) of
         {ok, AppSrc} ->
             DirName = filename:dirname(AppSrc),
             AppupFile = filename:basename(AppSrc, Ext) ++ ".appup.src",
@@ -379,6 +491,17 @@ create_stub(App) ->
             false
     end.
 
+%% we check whether the destination file already has the contents we
+%% want to write to avoid writing and losing indentation and comments.
+contains_contents(File, Upgrade, Downgrade) ->
+    %% the file may contain the VSN variable, so it's a script
+    case file:script(File, [{'VSN', 'VSN'}]) of
+        {ok, {_, Upgrade, Downgrade}} ->
+            true;
+        _ ->
+            false
+    end.
+
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %% application and release indexing
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -398,16 +521,18 @@ index_app(AppFile) ->
               , modules       = Modules
               }}.
 
-diff_app(App, #app{version = NewVersion, modules = NewModules}, #app{version = OldVersion, modules = OldModules}) ->
+diff_app(App,
+         #app{version = NewVersion, modules = NewModules},
+         #app{version = OldVersion, modules = OldModules}) ->
     {New, Changed} =
         maps:fold( fun(Mod, MD5, {New, Changed}) ->
                            case OldModules of
                                #{Mod := OldMD5} when MD5 =:= OldMD5 ->
                                    {New, Changed};
                                #{Mod := _} ->
-                                   {New, [Mod|Changed]};
+                                   {New, [Mod | Changed]};
                                _ ->
-                                   {[Mod|New], Changed}
+                                   {[Mod | New], Changed}
                            end
                    end
                  , {[], []}
@@ -437,6 +562,15 @@ hashsums(EbinDir) ->
                      filelib:wildcard("*.beam", EbinDir)
                     )).
 
+is_app_external(App) ->
+    Ext = ".app.src",
+    case locate(src, App, Ext) of
+        {ok, _} ->
+            false;
+        undefined ->
+            true
+    end.
+
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %% Global state
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -522,10 +656,5 @@ log(Msg) ->
 log(Msg, Args) ->
     io:format(standard_error, Msg, Args).
 
-ensure_string(Str) when is_binary(Str) ->
-    binary_to_list(Str);
-ensure_string(Str) when is_list(Str) ->
-    Str.
-
 otp_standard_apps() ->
     [ssl, mnesia, kernel, asn1, stdlib].