From 3ff6661a589186ebae16db2bbe12ea290c2a0ced Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 17 Dec 2021 14:48:41 -0300 Subject: [PATCH 1/8] chore(update_appup): take regexes into account when comparing vsns 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. NOTE: this does not cover the case in which we calculate the difference between _old_ and _new_ appup entries, and those consist of regexes. In such case, we would need to check if one regex is "contained" in the other, which is not currently supported by this patch. --- scripts/update_appup.escript | 56 +++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 7674a941c..df2f2b164 100755 --- a/scripts/update_appup.escript +++ b/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, [], @@ -214,10 +217,10 @@ find_appup_actions(App, CurrAppIdx, PrevAppIdx = #app{version = PrevVersion}) -> %% 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 +228,7 @@ diff_appup_instructions(ComputedChanges, PresentChanges) -> %% no diff Acc; _ -> - [{Vsn, DiffActions} | Acc] + [{VsnOrRegex, DiffActions} | Acc] end end end, @@ -250,8 +253,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} = @@ -279,11 +285,11 @@ merge_update_actions(App, Changes, Vsns) -> lists:map(fun(Ret = {<<".*">>, _}) -> Ret; ({Vsn, Actions}) -> - {Vsn, do_merge_update_actions(App, Changes, Actions)} + {Vsn, do_merge_update_actions(App, Vsn, Changes, Actions)} end, Vsns). -do_merge_update_actions(App, {New0, Changed0, Deleted0}, OldActions) -> +do_merge_update_actions(App, Vsn, {New0, Changed0, Deleted0}, OldActions) -> AppSpecific = app_specific_actions(App) -- OldActions, AlreadyHandled = lists:flatten(lists:map(fun process_old_action/1, OldActions)), New = New0 -- AlreadyHandled, @@ -308,14 +314,30 @@ 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]; _ -> 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; + (Needle) -> + true; + (_) -> + false + end, + Haystack). + read_appup(File) -> %% NOTE: appup file is a script, it may contain variables or functions. case file:script(File, [{'VSN', "VSN"}]) of @@ -398,16 +420,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 , {[], []} From e1e72c144a696dde51b475b3e7978ddb324ccbc7 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 17 Dec 2021 15:50:47 -0300 Subject: [PATCH 2/8] chore(update_appup): do not use load_module if restart_application 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. --- scripts/update_appup.escript | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index df2f2b164..65be8239b 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -295,11 +295,18 @@ do_merge_update_actions(App, Vsn, {New0, Changed0, Deleted0}, OldActions) -> New = New0 -- AlreadyHandled, Changed = Changed0 -- AlreadyHandled, Deleted = Deleted0 -- AlreadyHandled, - [{load_module, M, brutal_purge, soft_purge, []} || M <- Changed ++ New] ++ + Reloads = [{load_module, M, brutal_purge, soft_purge, []} + || not contains_restart_application(App, OldActions), + M <- Changed ++ New], + Reloads ++ OldActions ++ [{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). %% @doc Process the existing actions to exclude modules that are %% already handled From 42ca5ab5a974f4fa99195a6585c75834f158cb3f Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 17 Dec 2021 16:01:48 -0300 Subject: [PATCH 3/8] chore(update_appup): do not force appup 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. --- scripts/update_appup.escript | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 65be8239b..530f15072 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -368,7 +368,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} -> @@ -408,6 +413,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 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% From fef8a18bfb8c3a09395d91fdab97985ed6cd97e6 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 17 Dec 2021 16:31:04 -0300 Subject: [PATCH 4/8] chore(update_appup): insert `load_module`s after `application:stop` 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. --- scripts/update_appup.escript | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 530f15072..efd98b046 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -298,8 +298,11 @@ do_merge_update_actions(App, Vsn, {New0, Changed0, Deleted0}, OldActions) -> Reloads = [{load_module, M, brutal_purge, soft_purge, []} || not contains_restart_application(App, OldActions), M <- Changed ++ New], - Reloads ++ - OldActions ++ + {OldActionsWithStop, OldActionsAfterStop} = + find_application_stop_instruction(App, OldActions), + OldActionsWithStop ++ + Reloads ++ + OldActionsAfterStop ++ [{delete_module, M} || M <- Deleted] ++ AppSpecific. @@ -308,6 +311,23 @@ do_merge_update_actions(App, Vsn, {New0, Changed0, Deleted0}, OldActions) -> 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, [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 process_old_action({purge, Modules}) -> From af3a1326d13c972da79c7a1f67e2cac832599b41 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 20 Dec 2021 13:07:28 -0300 Subject: [PATCH 5/8] chore(update_appup): bugfix: variable not pinned in lambda --- scripts/update_appup.escript | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index efd98b046..050a180bb 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -316,7 +316,7 @@ contains_restart_application(Application, Actions) -> find_application_stop_instruction(Application, Actions) -> {Before, After0} = lists:splitwith( - fun({apply, {application, stop, [Application]}}) -> + fun({apply, {application, stop, [App]}}) when App =:= Application -> false; (_) -> true @@ -345,7 +345,7 @@ ensure_version(Version, OldInstructions) -> case contains_version(Version, OldVersions) of false -> [{Version, []} | OldInstructions]; - _ -> + true -> OldInstructions end. @@ -358,10 +358,8 @@ contains_version(Needle, Haystack) when is_list(Needle) -> nomatch -> false end; - (Needle) -> - true; - (_) -> - false + (Vsn) -> + Vsn =:= Needle end, Haystack). From ce4a193cbb8009b417f3f9fbbec983da475f671d Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 20 Dec 2021 13:08:01 -0300 Subject: [PATCH 6/8] chore(update_appup): rm unused fn --- scripts/update_appup.escript | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 050a180bb..c57272085 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -587,10 +587,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]. From b2396438a0efcadfc652e0604e114b6ccc54fb7a Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 20 Dec 2021 13:38:08 -0300 Subject: [PATCH 7/8] chore(update_appup): 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 | 75 ++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index c57272085..6dfbc4c1b 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -202,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 -> @@ -213,6 +217,32 @@ 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) -> @@ -363,6 +393,35 @@ contains_version(Needle, Haystack) when is_list(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 @@ -419,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", @@ -502,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 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% From 62ff6a8b30ff90d8a33f9c890c5cdcadeed4bb93 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 20 Dec 2021 13:41:14 -0300 Subject: [PATCH 8/8] chore(update_appup): rm unused value --- scripts/update_appup.escript | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 6dfbc4c1b..965de1efe 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -315,11 +315,11 @@ merge_update_actions(App, Changes, Vsns) -> lists:map(fun(Ret = {<<".*">>, _}) -> Ret; ({Vsn, Actions}) -> - {Vsn, do_merge_update_actions(App, Vsn, Changes, Actions)} + {Vsn, do_merge_update_actions(App, Changes, Actions)} end, Vsns). -do_merge_update_actions(App, Vsn, {New0, Changed0, Deleted0}, OldActions) -> +do_merge_update_actions(App, {New0, Changed0, Deleted0}, OldActions) -> AppSpecific = app_specific_actions(App) -- OldActions, AlreadyHandled = lists:flatten(lists:map(fun process_old_action/1, OldActions)), New = New0 -- AlreadyHandled,