Merge pull request #6560 from emqx/improve-update-appup-50

chore(update_appup): Improve `update_appup.escript` (5.0)

Port of #6480 .

* Make the script regex-aware

This change makes the `update_appup.escript` check whether the new
version of an application (the _current_ one) is already contained in
entries in the _new_ .appup file for that application if such .appup
file contains regexes.

* Do not use `load_module` instructions if `restart_application` is present

Since the appup instruction `restart_application` already loads all
modules of a given application, there is no need to introduce those
instructions if a restart is already present.

* Do not force `.appup.src` render if contents are the same

To avoid losing comments and/or manual indentation in appup files that
are already up to date, we now check whether the contents have the
exact same terms as those we are about to write to an existint .appup
file.

* Insert `load_module`s after `application:stop`, if present

If there is already any `application:stop(Application)` call in the
appup instructions, we prefer to add `load_module` instructions after
it, so we can be sure that the load is replaced safely.

* Add expected versions check

For apps inside emqx umbrella, we try to bump only the patch part of
their version numbers, and use only 3-part version
numbers (`Major.Minor.Patch`).  With those assumptions, we may infer
all versions that need to be covered in a given upgrade, and check if
those are covered in regexes.

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,
@@ -249,9 +282,8 @@ parse_appup_diffs(Upgrade, OldUpgrade, Downgrade, OldDowngrade) ->
             {diffs, Diffs}
     end.
 
-%% TODO: handle regexes
-find_matching_version(Vsn, PresentChanges) ->
-    proplists:get_value(Vsn, PresentChanges).
+find_matching_version(VsnOrRegex, PresentChanges) ->
+    proplists:get_value(VsnOrRegex, PresentChanges).
 
 find_old_appup_actions(App, PrevVersion) ->
     {Upgrade0, Downgrade0} =
@@ -289,11 +321,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 +367,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 +441,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} ->
@@ -380,6 +487,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
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -399,16 +517,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
                  , {[], []}
@@ -438,6 +558,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
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -523,10 +652,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].