properly support top level app erl_opts from REBAR_CONFIG os var (#1889)

When REBAR_CONFIG was set it would not effect the top level app's
configuration because app_discover was rereading the top level
rebar.config which ignored REBAR_CONFIG. Instead this patch has
it use the existing configuration from REBAR_CONFIG.

rebar.config

@@ -34,7 +34,8 @@
             {platform_define, "^(19|2)", rand_only},
             {platform_define, "^2", unicode_str},
             {platform_define, "^(R|1|20)", fun_stacktrace},
-            warnings_as_errors]}.
+            warnings_as_errors
+           ]}.
 
 %% Use OTP 18+ when dialyzing rebar3
 {dialyzer, [

src/rebar_app_discover.erl

@@ -7,7 +7,7 @@
          find_unbuilt_apps/1,
          find_apps/1,
          find_apps/2,
-         find_apps/3,
+         find_apps/4,
          find_app/2,
          find_app/3]).
 
@@ -23,7 +23,7 @@ do(State, LibDirs) ->
     Dirs = [filename:join(BaseDir, LibDir) || LibDir <- LibDirs],
     RebarOpts = rebar_state:opts(State),
     SrcDirs = rebar_dir:src_dirs(RebarOpts, ["src"]),
-    Apps = find_apps(Dirs, SrcDirs, all),
+    Apps = find_apps(Dirs, SrcDirs, all, State),
     ProjectDeps = rebar_state:deps_names(State),
     DepsDir = rebar_dir:deps_dir(State),
     CurrentProfiles = rebar_state:current_profiles(State),
@@ -52,7 +52,7 @@ do(State, LibDirs) ->
                         Name = rebar_app_info:name(AppInfo),
                         case enable(State, AppInfo) of
                             true ->
-                                {AppInfo1, StateAcc1} = merge_deps(AppInfo, StateAcc),
+                                {AppInfo1, StateAcc1} = merge_opts(AppInfo, StateAcc),
                                 OutDir = filename:join(DepsDir, Name),
                                 AppInfo2 = rebar_app_info:out_dir(AppInfo1, OutDir),
                                 ProjectDeps1 = lists:delete(Name, ProjectDeps),
@@ -87,33 +87,34 @@ format_error({module_list, File}) ->
 format_error({missing_module, Module}) ->
     io_lib:format("Module defined in app file missing: ~p~n", [Module]).
 
-%% @doc handles the merging and application of profiles and overrides
-%% for a given application, within its own context.
--spec merge_deps(rebar_app_info:t(), rebar_state:t()) ->
+%% @doc merges configuration of a project app and the top level state
+%% some configuration like erl_opts must be merged into a subapp's opts
+%% while plugins and hooks need to be kept defined to only either the
+%% top level state or an individual application.
+-spec merge_opts(rebar_app_info:t(), rebar_state:t()) ->
     {rebar_app_info:t(), rebar_state:t()}.
-merge_deps(AppInfo, State) ->
+merge_opts(AppInfo, State) ->
     %% These steps make sure that hooks and artifacts are run in the context of
     %% the application they are defined at. If an umbrella structure is used and
     %% they are defined at the top level they will instead run in the context of
     %% the State and at the top level, not as part of an application.
     CurrentProfiles = rebar_state:current_profiles(State),
-    Default = reset_hooks(rebar_state:default(State), CurrentProfiles),
-    {AppInfo0, State1} = project_app_config(AppInfo, Default, State),
+    {AppInfo1, State1} = maybe_reset_hooks_plugins(AppInfo, State),
 
-    Name = rebar_app_info:name(AppInfo0),
+    Name = rebar_app_info:name(AppInfo1),
 
     %% We reset the opts here to default so no profiles are applied multiple times
-    AppInfo1 = rebar_app_info:apply_overrides(rebar_state:get(State1, overrides, []), AppInfo0),
-    AppInfo2 = rebar_app_info:apply_profiles(AppInfo1, CurrentProfiles),
+    AppInfo2 = rebar_app_info:apply_overrides(rebar_state:get(State1, overrides, []), AppInfo1),
+    AppInfo3 = rebar_app_info:apply_profiles(AppInfo2, CurrentProfiles),
 
     %% Will throw an exception if checks fail
-    rebar_app_info:verify_otp_vsn(AppInfo2),
+    rebar_app_info:verify_otp_vsn(AppInfo3),
 
     State2 = lists:foldl(fun(Profile, StateAcc) ->
-                                 handle_profile(Profile, Name, AppInfo2, StateAcc)
+                                 handle_profile(Profile, Name, AppInfo3, StateAcc)
                          end, State1, lists:reverse(CurrentProfiles)),
 
-    {AppInfo2, State2}.
+    {AppInfo3, State2}.
 
 %% @doc Applies a given profile for an app, ensuring the deps
 %% match the context it will require.
@@ -151,25 +152,15 @@ parse_profile_deps(Profile, Name, Deps, Opts, State) ->
                               ,Locks
                               ,1).
 
-%% @doc Find the app-level config and return the state updated
-%% with the relevant app-level data.
--spec project_app_config(rebar_app_info:t(), rebar_dict(), rebar_state:t()) ->
-    {Config, rebar_state:t()} when
-      Config :: [any()].
-project_app_config(AppInfo, Default, State) ->
-    C = rebar_config:consult(rebar_app_info:dir(AppInfo)),
-    AppInfo1 = rebar_app_info:update_opts(AppInfo, Default, C),
-    {AppInfo2, State1} = maybe_reset_hooks_plugins(AppInfo1, State),
-    {AppInfo2, State1}.
-
+%% reset the State hooks if there is a top level application
 -spec maybe_reset_hooks_plugins(AppInfo, State) ->  {AppInfo, State} when
       AppInfo :: rebar_app_info:t(),
       State :: rebar_state:t().
 maybe_reset_hooks_plugins(AppInfo, State) ->
     Dir = rebar_app_info:dir(AppInfo),
+    CurrentProfiles = rebar_state:current_profiles(State),
     case ec_file:real_dir_path(rebar_dir:root_dir(State)) of
         Dir ->
-            CurrentProfiles = rebar_state:current_profiles(State),
             Opts = reset_hooks(rebar_state:opts(State), CurrentProfiles),
             State1 = rebar_state:opts(State, Opts),
 
@@ -179,7 +170,11 @@ maybe_reset_hooks_plugins(AppInfo, State) ->
 
             {AppInfo1, State1};
         _ ->
-            {AppInfo, State}
+            %% if not in the top root directory then we need to merge in the
+            %% default state opts to this subapp's opts
+            Default = reset_hooks(rebar_state:default(State), CurrentProfiles),
+            AppInfo1 = rebar_app_info:update_opts(AppInfo, Default),
+            {AppInfo1, State}
     end.
 
 
@@ -211,8 +206,8 @@ reset_hooks(Opts, CurrentProfiles) ->
 -spec all_app_dirs([file:name()]) -> [{file:name(), [file:name()]}].
 all_app_dirs(LibDirs) ->
     lists:flatmap(fun(LibDir) ->
-                        {_, SrcDirs} = find_config_src(LibDir, ["src"]),
-                        app_dirs(LibDir, SrcDirs)
+                          {_, SrcDirs} = find_config_src(LibDir, ["src"]),
+                          app_dirs(LibDir, SrcDirs)
                   end, LibDirs).
 
 %% @private find the directories for all apps based on their source dirs
@@ -270,11 +265,11 @@ find_apps(LibDirs, Validate) ->
 %% @doc for each directory passed, with the configured source directories,
 %% find all apps according to the validity rule passed in.
 %% Returns all the related app info records.
--spec find_apps([file:filename_all()], [file:filename_all()], valid | invalid | all) -> [rebar_app_info:t()].
-find_apps(LibDirs, SrcDirs, Validate) ->
+-spec find_apps([file:filename_all()], [file:filename_all()], valid | invalid | all, rebar_state:t()) -> [rebar_app_info:t()].
+find_apps(LibDirs, SrcDirs, Validate, State) ->
     rebar_utils:filtermap(
       fun({AppDir, AppSrcDirs}) ->
-            find_app(rebar_app_info:new(), AppDir, AppSrcDirs, Validate)
+            find_app(rebar_app_info:new(), AppDir, AppSrcDirs, Validate, State)
       end,
       all_app_dirs(LibDirs, SrcDirs)
     ).
@@ -305,8 +300,19 @@ find_app(AppInfo, AppDir, Validate) ->
 %% the directories where source files can be located. Returns the related
 %% app info record.
 -spec find_app(rebar_app_info:t(), file:filename_all(),
-               [file:filename_all()], valid | invalid | all) ->
+               [file:filename_all()], valid | invalid | all, rebar_state:t()) ->
     {true, rebar_app_info:t()} | false.
+find_app(AppInfo, AppDir, SrcDirs, Validate, State) ->
+    AppInfo1 = case ec_file:real_dir_path(rebar_dir:root_dir(State)) of
+                   AppDir ->
+                       Opts = rebar_state:opts(State),
+                       rebar_app_info:default(rebar_app_info:opts(AppInfo, Opts), Opts);
+                   _ ->
+                       Config = rebar_config:consult(AppDir),
+                       rebar_app_info:update_opts(AppInfo, rebar_app_info:opts(AppInfo), Config)
+               end,
+    find_app_(AppInfo1, AppDir, SrcDirs, Validate).
+
 find_app(AppInfo, AppDir, SrcDirs, Validate) ->
     Config = rebar_config:consult(AppDir),
     AppInfo1 = rebar_app_info:update_opts(AppInfo, rebar_app_info:opts(AppInfo), Config),

src/rebar_app_info.erl

@@ -7,6 +7,7 @@
          new/4,
          new/5,
          update_opts/3,
+         update_opts/2,
          update_opts_deps/2,
          discover/1,
          name/1,
@@ -158,7 +159,7 @@ update_opts(AppInfo, Opts, Config) ->
                        %% don't set anything here.
                        [];
                    _ ->
-                       deps_from_config(dir(AppInfo), Config)
+                       deps_from_config(dir(AppInfo), proplists:get_value(deps, Config, []))
                end,
 
     Plugins = proplists:get_value(plugins, Config, []),
@@ -171,6 +172,13 @@ update_opts(AppInfo, Opts, Config) ->
     AppInfo#app_info_t{opts=NewOpts,
                        default=NewOpts}.
 
+%% @doc update current app info opts by merging in a new dict of opts
+-spec update_opts(t(), rebar_dict()) -> t().
+update_opts(AppInfo=#app_info_t{opts=LocalOpts}, Opts) ->
+    NewOpts = rebar_opts:merge_opts(LocalOpts, Opts),
+    AppInfo#app_info_t{opts=NewOpts,
+                       default=NewOpts}.
+
 %% @doc update the opts based on new deps, usually from an app's hex registry metadata
 -spec update_opts_deps(t(), [any()]) -> t().
 update_opts_deps(AppInfo=#app_info_t{opts=Opts}, Deps) ->
@@ -183,10 +191,10 @@ update_opts_deps(AppInfo=#app_info_t{opts=Opts}, Deps) ->
 
 %% @private extract the deps for an app in `Dir' based on its config file data
 -spec deps_from_config(file:filename(), [any()]) -> [{tuple(), any()}, ...].
-deps_from_config(Dir, Config) ->
+deps_from_config(Dir, ConfigDeps) ->
     case rebar_config:consult_lock_file(filename:join(Dir, ?LOCK_FILE)) of
         [] ->
-            [{{deps, default}, proplists:get_value(deps, Config, [])}];
+            [{{deps, default}, ConfigDeps}];
         D ->
             %% We want the top level deps only from the lock file.
             %% This ensures deterministic overrides for configs.

src/rebar_erlc_compiler.erl

@@ -99,6 +99,7 @@ compile(AppInfo, CompileOpts) when element(1, AppInfo) == app_info_t ->
                {recursive, dir_recursive(RebarOpts, "src", CompileOpts)}],
     MibsOpts = [check_last_mod,
                 {recursive, dir_recursive(RebarOpts, "mibs", CompileOpts)}],
+
     rebar_base_compiler:run(RebarOpts,
                             check_files([filename:join(Dir, File)
                                          || File <- rebar_opts:get(RebarOpts, xrl_first_files, [])]),

src/rebar_prv_plugins.erl

@@ -36,7 +36,7 @@ do(State) ->
     GlobalPlugins = rebar_state:get(GlobalConfig, plugins, []),
     GlobalSrcDirs = rebar_state:get(GlobalConfig, src_dirs, ["src"]),
     GlobalPluginsDir = filename:join([rebar_dir:global_cache_dir(rebar_state:opts(State)), "plugins", "*"]),
-    GlobalApps = rebar_app_discover:find_apps([GlobalPluginsDir], GlobalSrcDirs, all),
+    GlobalApps = rebar_app_discover:find_apps([GlobalPluginsDir], GlobalSrcDirs, all, State),
     display_plugins("Global plugins", GlobalApps, GlobalPlugins),
 
     RebarOpts = rebar_state:opts(State),
@@ -44,7 +44,7 @@ do(State) ->
     Plugins = rebar_state:get(State, plugins, []),
     PluginsDirs = filelib:wildcard(filename:join(rebar_dir:plugins_dir(State), "*")),
     CheckoutsDirs = filelib:wildcard(filename:join(rebar_dir:checkouts_dir(State), "*")),
-    Apps = rebar_app_discover:find_apps(CheckoutsDirs++PluginsDirs, SrcDirs, all),
+    Apps = rebar_app_discover:find_apps(CheckoutsDirs++PluginsDirs, SrcDirs, all, State),
     display_plugins("Local plugins", Apps, Plugins),
     {ok, State}.
 

test/rebar_compile_SUITE.erl

@@ -38,7 +38,8 @@ all() ->
      recursive, no_recursive,
      always_recompile_when_erl_compiler_options_set,
      dont_recompile_when_erl_compiler_options_env_does_not_change,
-     recompile_when_erl_compiler_options_env_changes].
+     recompile_when_erl_compiler_options_env_changes,
+     rebar_config_os_var].
 
 groups() ->
     [{basic_app, [], [build_basic_app, paths_basic_app, clean_basic_app]},
@@ -774,7 +775,7 @@ recompile_when_opts_change(Config) ->
 
     rebar_test_utils:create_config(AppDir, [{erl_opts, [{d, some_define}]}]),
 
-    rebar_test_utils:run_and_check(Config, [], ["compile"], {ok, [{app, Name}]}),
+    rebar_test_utils:run_and_check(Config, [{erl_opts, [{d, some_define}]}], ["compile"], {ok, [{app, Name}]}),
 
     {ok, NewFiles} = rebar_utils:list_dir(EbinDir),
     NewModTime = [filelib:last_modified(filename:join([EbinDir, F]))
@@ -1944,6 +1945,29 @@ recompile_when_erl_compiler_options_env_changes(Config) ->
         _     -> os:putenv("ERL_COMPILER_OPTIONS", ExistingEnv)
     end.
 
+rebar_config_os_var(Config) ->
+    AppDir = ?config(apps, Config),
+
+    Name = rebar_test_utils:create_random_name("rebar_config_os_var_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
+
+    rebar_test_utils:create_config(AppDir, [{erl_opts, []}]),
+
+    AltConfig = filename:join(AppDir, "test.rebar.config"),
+    file:write_file(AltConfig, "{erl_opts, [compressed]}."),
+    true = os:putenv("REBAR_CONFIG", AltConfig),
+
+    rebar_test_utils:run_and_check(Config, ["compile"], {ok, [{app, Name}]}),
+
+    Path = filename:join([AppDir, "_build", "default", "lib", Name, "ebin"]),
+    code:add_patha(Path),
+
+    Mod = list_to_atom("not_a_real_src_" ++ Name),
+
+    true = lists:member(compressed, proplists:get_value(options, Mod:module_info(compile), [])),
+    ok.
+
 %% this test sets the env var, compiles, records the file last modified
 %% timestamp, recompiles and compares the file last modified timestamp to
 %% ensure it has changed. this test should run on 18.x

test/rebar_hooks_SUITE.erl

@@ -1,20 +1,6 @@
 -module(rebar_hooks_SUITE).
 
--export([suite/0,
-         init_per_suite/1,
-         end_per_suite/1,
-         init_per_testcase/2,
-         end_per_testcase/2,
-         all/0,
-         build_and_clean_app/1,
-         escriptize_artifacts/1,
-         run_hooks_once/1,
-         run_hooks_once_profiles/1,
-         run_hooks_for_plugins/1,
-         eunit_app_hooks/1,
-         deps_hook_namespace/1,
-         bare_compile_hooks_default_ns/1,
-         deps_clean_hook_namespace/1]).
+-compile(export_all).
 
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
@@ -38,7 +24,8 @@ end_per_testcase(_, _Config) ->
 all() ->
     [build_and_clean_app, run_hooks_once, run_hooks_once_profiles,
      escriptize_artifacts, run_hooks_for_plugins, deps_hook_namespace,
-     bare_compile_hooks_default_ns, deps_clean_hook_namespace, eunit_app_hooks].
+     bare_compile_hooks_default_ns, deps_clean_hook_namespace, eunit_app_hooks,
+     sub_app_hooks, root_hooks].
 
 %% Test post provider hook cleans compiled project app, leaving it invalid
 build_and_clean_app(Config) ->
@@ -97,7 +84,7 @@ run_hooks_once(Config) ->
 
     Name = rebar_test_utils:create_random_name("app1_"),
     Vsn = rebar_test_utils:create_random_vsn(),
-    RebarConfig = [{pre_hooks, [{compile, "mkdir blah"}]}],
+    RebarConfig = [{pre_hooks, [{compile, "mkdir  $REBAR_ROOT_DIR/blah"}]}],
     rebar_test_utils:create_config(AppDir, RebarConfig),
     rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
     rebar_test_utils:run_and_check(Config, RebarConfig, ["compile"], {ok, [{app, Name, valid}]}).
@@ -109,7 +96,7 @@ run_hooks_once_profiles(Config) ->
 
     Name = rebar_test_utils:create_random_name("app1_"),
     Vsn = rebar_test_utils:create_random_vsn(),
-    RebarConfig = [{profiles, [{hooks, [{pre_hooks, [{compile, "mkdir blah"}]}]}]}],
+    RebarConfig = [{profiles, [{hooks, [{pre_hooks, [{compile, "mkdir $REBAR_ROOT_DIR/blah"}]}]}]}],
     rebar_test_utils:create_config(AppDir, RebarConfig),
     rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
     rebar_test_utils:run_and_check(Config, RebarConfig, ["as", "hooks", "compile"], {ok, [{app, Name, valid}]}).
@@ -215,3 +202,45 @@ run_hooks_for_plugins(Config) ->
     rebar_test_utils:run_and_check(Config, RConf, ["compile"], {ok, [{app, Name, valid},
                                                                      {plugin, PluginName},
                                                                      {file, filename:join([AppDir, "_build", "default", "plugins", PluginName, "randomfile"])}]}).
+
+%% test that a subapp of a project keeps its hooks
+sub_app_hooks(Config) ->
+    AppDir = ?config(apps, Config),
+
+    Name = rebar_test_utils:create_random_name("sub_app1_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+
+    SubAppsDir = filename:join([AppDir, "apps", Name]),
+
+    rebar_test_utils:create_app(SubAppsDir, Name, Vsn, [kernel, stdlib]),
+    rebar_test_utils:create_config(SubAppsDir, [{provider_hooks, [{post, [{compile, clean}]}]}]),
+
+    RConfFile = rebar_test_utils:create_config(AppDir, []),
+    {ok, RConf} = file:consult(RConfFile),
+
+    %% Build with deps.
+    rebar_test_utils:run_and_check(
+      Config, RConf, ["compile"],
+      {ok, [{app, Name, invalid}]}
+     ).
+
+%% test that hooks at the top level don't run in the subapps
+root_hooks(Config) ->
+    AppDir = ?config(apps, Config),
+
+    Name = rebar_test_utils:create_random_name("sub_app1_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+
+    SubAppsDir = filename:join([AppDir, "apps", Name]),
+
+    rebar_test_utils:create_app(SubAppsDir, Name, Vsn, [kernel, stdlib]),
+    rebar_test_utils:create_config(SubAppsDir, [{provider_hooks, [{post, [{compile, clean}]}]}]),
+
+    RConfFile = rebar_test_utils:create_config(AppDir, [{pre_hooks, [{compile, "mkdir $REBAR_ROOT_DIR/blah"}]}]),
+    {ok, RConf} = file:consult(RConfFile),
+
+    %% Build with deps.
+    rebar_test_utils:run_and_check(
+      Config, RConf, ["compile"],
+      {ok, [{app, Name, invalid}]}
+     ).

test/rebar_test_utils.erl

@@ -1,7 +1,7 @@
 -module(rebar_test_utils).
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
--export([init_rebar_state/1, init_rebar_state/2, run_and_check/4, check_results/3]).
+-export([init_rebar_state/1, init_rebar_state/2, run_and_check/3, run_and_check/4, check_results/3]).
 -export([expand_deps/2, flat_deps/1, top_level_deps/1]).
 -export([create_app/4, create_plugin/4, create_eunit_app/4, create_empty_app/4,
          create_config/2, create_config/3, package_app/4]).
@@ -82,6 +82,33 @@ run_and_check(Config, RebarConfig, Command, Expect) ->
         rebar_abort when Expect =:= rebar_abort -> rebar_abort
     end.
 
+run_and_check(Config, Command, Expect) ->
+    %% Assumes init_rebar_state has run first
+    AppDir = ?config(apps, Config),
+    {ok, Cwd} = file:get_cwd(),
+    try
+        ok = file:set_cwd(AppDir),
+        Res = rebar3:run(Command),
+        case Expect of
+            {error, Reason} ->
+                ?assertEqual({error, Reason}, Res);
+            {ok, Expected} ->
+                {ok, _} = Res,
+                check_results(AppDir, Expected, "*"),
+                Res;
+            {ok, Expected, ProfileRun} ->
+                {ok, _} = Res,
+                check_results(AppDir, Expected, ProfileRun),
+                Res;
+            return ->
+                Res
+        end
+    catch
+        rebar_abort when Expect =:= rebar_abort -> rebar_abort
+    after
+        ok = file:set_cwd(Cwd)
+    end.
+
 %% @doc Creates a dummy application including:
 %% - src/<file>.erl
 %% - src/<file>.app.src