Fix directory recursion in compiler

This patch contains two behaviour changes and reasserts other behaviours
that now line things up with user and documentation expectations:

1. The src directories remain recursive. We turned it on by accident at
   some point in the past and now people rely on it, so we're stuck with
   it. However a new test ensures that the feature can be turned off
   on-demand as documented on the website.

2. The test directories are no longer recursive by default. The fix is
   done by properly fixing how rebar3.erl does its feature injection by
   mandating the default value there.
   I'm somewhat nervous that this change could negatively impact some
   users and older compiler module versions, but if users stick to the
   rebar_dir interface, everything should keep working transparently.

3. The test directories' configuration is no longer silently dropped.
   Due to how rebar3.erl injected test state without looking for what
   the user may have specified, multiple extra_src_dirs entries existed
   at once and were run; one with the recursion set to true and one with
   whatever the user specified. If the user disabled recursion of the
   "test" extra_src_dir, then the injected value still ran it once...

4. The handling of extra files in the compiler module is fixed to
   actually use the rebar_dir interface properly, and reinjects
   non-default directory recursion settings into the swapped options
   for the shimmed extra apps. Not doing this annotation step resulted
   in the write for swapped opts to actually drop the configured
   recursion value and make everything recursive all the time.

A single new test actually validates all of that behaviour and seems to
work fine.

src/rebar3.erl

@@ -430,7 +430,18 @@ test_state(State) ->
     ProfileOpts = proplists:get_value(test, Profiles, []),
     ErlOpts = proplists:get_value(erl_opts, ProfileOpts, []),
     TestOpts = safe_define_test_macro(ErlOpts),
-    [{extra_src_dirs, ["test"]}, {erl_opts, TestOpts}].
+    %% Only define the test directory if it wasn't set by the user already,
+    %% otherwise our definition may clash with theirs
+    Extras = rebar_opts:get(Opts, extra_src_dirs, []),
+    ExtrasTest = proplists:get_value(extra_src_dirs, ProfileOpts, []),
+    IsDefined = lists:any(fun({"test", _}) -> true
+                          ;  ("test") -> true
+                          ;  (_) -> false
+                          end, Extras ++ ExtrasTest),
+    case IsDefined of
+        true -> [];
+        false -> [{extra_src_dirs, [{"test", [{recursive, false}]}]}]
+    end ++ [{erl_opts, TestOpts}].
 
 -spec safe_define_test_macro([any()]) -> [any()] | [{'d',atom()} | any()].
 safe_define_test_macro(Opts) ->

src/rebar_compiler.erl

@@ -354,10 +354,24 @@ clean_(CompilerMod, AppInfo, _Label) ->
     ok.
 
 annotate_extras(AppInfo) ->
-    ExtraDirs = rebar_dir:extra_src_dirs(rebar_app_info:opts(AppInfo), []),
-    OldSrcDirs = rebar_app_info:get(AppInfo, src_dirs, ["src"]),
+    AppOpts = rebar_app_info:opts(AppInfo),
+    ExtraDirs = rebar_dir:extra_src_dirs(AppOpts, []),
+    OldSrcDirs = rebar_dir:src_dirs(AppOpts, ["src"]),
+    %% Re-annotate the directories with non-default options if it is the
+    %% case; otherwise, later down the line, the options get dropped with
+    %% profiles. All of this must be done with the rebar_dir functionality
+    %% which properly tracks and handles the various legacy formats for
+    %% recursion setting (erl_opts vs. dir options and profiles)
+    ExtraDirsOpts = [case rebar_dir:recursive(AppOpts, Dir) of
+                         false -> {Dir, [{recursive, false}]};
+                         true -> Dir
+                     end || Dir <- ExtraDirs],
+    OldSrcDirsOpts = [case rebar_dir:recursive(AppOpts, Dir) of
+                          false -> {Dir, [{recursive, false}]};
+                          true -> Dir
+                      end || Dir <- OldSrcDirs],
     AppDir = rebar_app_info:dir(AppInfo),
-    lists:map(fun(Dir) ->
+    lists:map(fun({DirOpt, Dir}) ->
         EbinDir = filename:join(rebar_app_info:out_dir(AppInfo), Dir),
         %% need a unique name to prevent lookup issues that clobber entries
         AppName = unicode:characters_to_binary(
@@ -365,15 +379,15 @@ annotate_extras(AppInfo) ->
         ),
         AppInfo0 = rebar_app_info:name(AppInfo, AppName),
         AppInfo1 = rebar_app_info:ebin_dir(AppInfo0, EbinDir),
-        AppInfo2 = rebar_app_info:set(AppInfo1, src_dirs, [Dir]),
-        AppInfo3 = rebar_app_info:set(AppInfo2, extra_src_dirs, OldSrcDirs),
+        AppInfo2 = rebar_app_info:set(AppInfo1, src_dirs, [DirOpt]),
+        AppInfo3 = rebar_app_info:set(AppInfo2, extra_src_dirs, OldSrcDirsOpts),
         add_to_includes( % give access to .hrl in app's src/
             AppInfo3,
             [filename:join([AppDir, D]) || D <- OldSrcDirs]
         )
     end,
-    [ExtraDir || ExtraDir <- ExtraDirs,
-                 filelib:is_dir(filename:join(AppDir, ExtraDir))]
+    [T || T = {_DirOpt, ExtraDir} <- lists:zip(ExtraDirsOpts, ExtraDirs),
+          filelib:is_dir(filename:join(AppDir, ExtraDir))]
     ).
 
 find_source_files(BaseDir, SrcExt, SrcDirs, Opts) ->

test/rebar_compile_SUITE.erl

@@ -38,7 +38,7 @@ all() ->
      recompile_when_parse_transform_as_opt_changes,
      recompile_when_parse_transform_inline_changes,
      regex_filter_skip, regex_filter_regression,
-     recursive, no_recursive,
+     recursive, no_recursive, extra_recursion,
      always_recompile_when_erl_compiler_options_set,
      dont_recompile_when_erl_compiler_options_env_does_not_change,
      recompile_when_erl_compiler_options_env_changes,
@@ -2500,6 +2500,40 @@ no_recursive(Config) ->
     ?assert(false==lists:member("rec.beam",Files2)),
     ok.
 
+extra_recursion(Config) ->
+    AppDir = ?config(apps, Config),
+
+    Name = rebar_test_utils:create_random_name("app1_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
+    rebar_test_utils:write_src_file(filename:join([AppDir, "src", "src2"]), "rec.erl"),
+    rebar_test_utils:write_src_file(filename:join([AppDir, "test", "test2"]), "rectest.erl"),
+
+    %% Default for src directories: recursive
+    %% default for extra_src directories: non-recursive
+    RebarConfig1 = [],
+    rebar_test_utils:run_and_check(Config, RebarConfig1, ["as", "test", "compile"],
+                                   {ok, [{app, Name}]}),
+    EbinDir = filename:join([AppDir, "_build", "test", "lib", Name, "ebin"]),
+    {ok, Files1} = rebar_utils:list_dir(EbinDir),
+    ?assert(lists:member("rec.beam", Files1)),
+    file:delete(filename:join(EbinDir, "rec.beam")),
+
+    TestEbinDir = filename:join([AppDir, "_build", "test", "lib", Name, "test"]),
+    {ok, TestFiles1} = rebar_utils:list_dir(TestEbinDir),
+    ?assertNot(lists:member("rectest.beam", TestFiles1)),
+
+    RebarConfig2 = [{src_dirs,[{"src",[{recursive,false}]}]},
+                    {extra_src_dirs, [{"test", [{recursive, true}]}]}],
+    rebar_test_utils:run_and_check(Config, RebarConfig2, ["as", "test", "compile"],
+                                   {ok, [{app, Name}]}),
+    {ok, Files2} = rebar_utils:list_dir(EbinDir),
+    ?assertNot(lists:member("rec.beam",Files2)),
+
+    {ok, TestFiles2} = rebar_utils:list_dir(TestEbinDir),
+    ?assert(lists:member("rectest.beam", TestFiles2)),
+    ok.
+
 regex_filter_skip(Config) ->
     AppDir = ?config(apps, Config),
     Name = rebar_test_utils:create_random_name("regex_skip"),