From c4dce2d1267a8f67ed6797a937815a66c8a1148a Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Mon, 19 Feb 2024 17:28:48 -0500 Subject: [PATCH 1/3] Fix clean hooks when override are presents The issue appears to be that because the 'clean' operation might be run while dependencies have not yet been compiled, we applied a partial app detection mechanism with `rebar_app_disover:find_apps(..., ..., all, ...)`, which worked to parse "invalid" (unbuilt) apps, but also did not apply overrides. Instead, we trust the `install_deps` provider dependency by reusing the apps as they were fully parsed _if_ they were valid, and falling back to the `rebar_app_discover:find_apps/4` call only to cover the unreadable ones. This, it turns out, has the side effect of properly applying hooks when apps are fully parsed, and fixes https://github.com/erlang/rebar3/issues/2862 Note that we can only clean paths safely if the discovery steps for the apps is done with the right profile and options, which may also impact configurations and hooks. So rather than duplicating that, we invoke the 'as' provider. This also opens the door on choosing a different provider (such as 'app_discover' only) down the road if the -a option isn't given. --- apps/rebar/src/rebar_prv_clean.erl | 19 ++++++++++++++++--- apps/rebar/test/rebar_as_SUITE.erl | 3 ++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/apps/rebar/src/rebar_prv_clean.erl b/apps/rebar/src/rebar_prv_clean.erl index 71a8066c3..cff7636f9 100644 --- a/apps/rebar/src/rebar_prv_clean.erl +++ b/apps/rebar/src/rebar_prv_clean.erl @@ -12,7 +12,7 @@ -include("rebar.hrl"). -define(PROVIDER, clean). --define(DEPS, [app_discovery, install_deps]). +-define(DEPS, []). %% =================================================================== %% Public API @@ -37,7 +37,13 @@ do(State) -> Providers = rebar_state:providers(State), {All, Profiles, Specific} = handle_args(State), - State1 = rebar_state:apply_profiles(State, [list_to_atom(X) || X <- Profiles]), + %% Invoke provider deps as the desired profile(s) so the discovery of + %% apps respects profile options. + State0 = rebar_state:command_args( + State, + lists:join(",", ["default"|Profiles]) ++ ["install_deps"] + ), + {ok, State1} = rebar_prv_as:do(State0), Cwd = rebar_dir:get_cwd(), rebar_hooks:run_all_hooks(Cwd, pre, ?PROVIDER, Providers, State1), @@ -45,7 +51,14 @@ do(State) -> if All; Specific =/= [] -> DepsDir = rebar_dir:deps_dir(State1), DepsDirs = filelib:wildcard(filename:join(DepsDir, "*")), - AllApps = rebar_app_discover:find_apps(DepsDirs, all, State), + ProjectApps = rebar_state:project_apps(State1), + Deps = rebar_state:all_deps(State1), + KnownAppNames = [rebar_app_info:name(App) || App <- ProjectApps++Deps], + ParsedApps = rebar_app_discover:find_apps(DepsDirs, all, State1), + AllApps = ProjectApps ++ Deps ++ + [App || App <- ParsedApps, + not lists:member(rebar_app_info:name(App), + KnownAppNames)], Filter = case All of true -> fun(_) -> true end; false -> fun(AppInfo) -> filter_name(AppInfo, Specific) end diff --git a/apps/rebar/test/rebar_as_SUITE.erl b/apps/rebar/test/rebar_as_SUITE.erl index 78ea8aec9..fce1efcd9 100644 --- a/apps/rebar/test/rebar_as_SUITE.erl +++ b/apps/rebar/test/rebar_as_SUITE.erl @@ -198,4 +198,5 @@ clean_as_profile(Config) -> rebar_test_utils:run_and_check(Config, [], ["clean", "-a", "-p", "foo"], - {ok, [{app, Name, invalid}]}). + {ok, [{app, Name, invalid}]}), + ok. From c55885deb2e7949abc27f5b82927941a3e9b2a68 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 21 Feb 2024 08:33:09 -0500 Subject: [PATCH 2/3] Only call install_deps if 'clean' impacts deps The selection is a bit coarse right now (it's possible a specific app is not a dep but a top-level app), but we regardless keep basic cleaning as a simple discovery step to avoid installing deps there. --- apps/rebar/src/rebar_prv_clean.erl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/rebar/src/rebar_prv_clean.erl b/apps/rebar/src/rebar_prv_clean.erl index cff7636f9..010d67cad 100644 --- a/apps/rebar/src/rebar_prv_clean.erl +++ b/apps/rebar/src/rebar_prv_clean.erl @@ -39,9 +39,12 @@ do(State) -> %% Invoke provider deps as the desired profile(s) so the discovery of %% apps respects profile options. + Task = if All; Specific =/= [] -> "install_deps"; + true -> "app_discovery" + end, State0 = rebar_state:command_args( State, - lists:join(",", ["default"|Profiles]) ++ ["install_deps"] + lists:join(",", ["default"|Profiles]) ++ [Task] ), {ok, State1} = rebar_prv_as:do(State0), From 862229bda66683162e8bf14f18b51f972de7c1ad Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Wed, 21 Feb 2024 13:18:07 -0500 Subject: [PATCH 3/3] fix tests for new leaner clean runs --- apps/rebar/test/rebar_hooks_SUITE.erl | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/rebar/test/rebar_hooks_SUITE.erl b/apps/rebar/test/rebar_hooks_SUITE.erl index bdef11482..b000125c5 100644 --- a/apps/rebar/test/rebar_hooks_SUITE.erl +++ b/apps/rebar/test/rebar_hooks_SUITE.erl @@ -166,10 +166,17 @@ deps_clean_hook_namespace(Config) -> ]} ]} ], + %% Only detect dependencies when asked to parse them. + %% Avoids scanning and fetching them only to clean them. rebar_test_utils:run_and_check( Config, RebarConfig, ["clean"], + {ok, [{dep_not_exist, "some_dep"}]} + ), + rebar_test_utils:run_and_check( + Config, RebarConfig, ["clean", "-a"], {ok, [{dep, "some_dep"}]} - ). + ), + ok. %% Checks that a hook that is defined on an app (not a top level hook of a project with subapps) is run eunit_app_hooks(Config) ->