erlc: clean-up, enhance, and regression fix fd17693

* update files
* fix Dialyzer warning
* unconditionally enable info fil
* clean-up inconsistencies
* use term_to_binary compression
* use try...catch instead of case...catch...of
* do not write build info file if the graph is unmodified
* store info file as <base_dir>/.rebarinfo
* properly support list of compile directives
* fix regressions:
 - Fix a bug in handling of files to compile first.
 - If a file that is depended upon itself depends on other files, make sure
   those are compiled first. While at it, rename variables for correctness.
   Reported-by: David Robakowski
 - Make sure that FirstFiles has no dupes and preserves the proper order.
 - headers referenced via -include_lib() were not properly resolved to absolute
   filenames
 - .erl files found in sub dirs of src_dirs were not properly resolved to
   absolute filenames

1 files changed, 200 insertions, 69 deletions

diff --git a/src/rebar_erlc_compiler.erl b/src/rebar_erlc_compiler.erl
index 5a19966..a1740b0 100644
--- a/src/rebar_erlc_compiler.erl
+++ b/src/rebar_erlc_compiler.erl
@@ -36,7 +36,16 @@
 -include("rebar.hrl").
 -include_lib("stdlib/include/erl_compile.hrl").
 
--define(REBAR_BUILD_INFO, ".rebar.build.info").
+-define(ERLCINFO_VSN, 1).
+-define(ERLCINFO_FILE, "erlcinfo").
+-type erlc_info_v() :: {digraph:vertex(), term()} | 'false'.
+-type erlc_info_e() :: {digraph:vertex(), digraph:vertex()}.
+-type erlc_info() :: {list(erlc_info_v()), list(erlc_info_e())}.
+-record(erlcinfo,
+        {
+          vsn = ?ERLCINFO_VSN :: pos_integer(),
+          info = {[], []} :: erlc_info()
+        }).
 
 %% ===================================================================
 %% Public API
@@ -92,7 +101,7 @@ compile(Config, _AppFile) ->
     doterl_compile(Config, "ebin").
 
 -spec clean(rebar_config:config(), file:filename()) -> 'ok'.
-clean(_Config, _AppFile) ->
+clean(Config, _AppFile) ->
     MibFiles = rebar_utils:find_files("mibs", "^.*\\.mib\$"),
     MIBs = [filename:rootname(filename:basename(MIB)) || MIB <- MibFiles],
     rebar_file_utils:delete_each(
@@ -106,7 +115,7 @@ clean(_Config, _AppFile) ->
         || F <- YrlFiles ]),
 
     %% Delete the build graph, if any
-    rebar_file_utils:rm_rf(filename:join("ebin", ?REBAR_BUILD_INFO)),
+    rebar_file_utils:rm_rf(erlcinfo_file(Config)),
 
     %% Erlang compilation is recursive, so it's possible that we have a nested
     %% directory structure in ebin with .beam files within. As such, we want
@@ -265,7 +274,7 @@ doterl_compile(Config, OutDir) ->
     doterl_compile(Config, OutDir, []).
 
 doterl_compile(Config, OutDir, MoreSources) ->
-    FirstErls = rebar_config:get_list(Config, erl_first_files, []),
+    ErlFirstFiles = rebar_config:get_list(Config, erl_first_files, []),
     ErlOpts = rebar_utils:erl_opts(Config),
     ?DEBUG("erl_opts ~p~n", [ErlOpts]),
     %% Support the src_dirs option allowing multiple directories to
@@ -273,20 +282,22 @@ doterl_compile(Config, OutDir, MoreSources) ->
     %% eunit tests be separated from the core application source.
     SrcDirs = rebar_utils:src_dirs(proplists:append_values(src_dirs, ErlOpts)),
     RestErls  = [Source || Source <- gather_src(SrcDirs, []) ++ MoreSources,
-                           not lists:member(Source, FirstErls)],
+                           not lists:member(Source, ErlFirstFiles)],
     %% Make sure that ebin/ exists and is on the path
     ok = filelib:ensure_dir(filename:join("ebin", "dummy.beam")),
     CurrPath = code:get_path(),
     true = code:add_path(filename:absname("ebin")),
     OutDir1 = proplists:get_value(outdir, ErlOpts, OutDir),
-    G = init_graph(Config, RestErls),
-    %% Split RestErls so that parse_transforms and behaviours are instead added
-    %% to erl_first_files, parse transforms first.
+    G = init_erlcinfo(Config, RestErls),
+    %% Split RestErls so that files which are depended on are treated
+    %% like erl_first_files.
     {OtherFirstErls, OtherErls} =
         lists:partition(
           fun(F) ->
-                  case [Erl || Erl <- get_children(G, F),
-                               filename:extension(Erl) == ".erl"] of
+                  Children = get_children(G, F),
+                  log_files(?FMT("Files dependent on ~s", [F]), Children),
+
+                  case erls(Children) of
                       [] ->
                           %% There are no files dependent on this file.
                           false;
@@ -297,15 +308,60 @@ doterl_compile(Config, OutDir, MoreSources) ->
                           true
                   end
           end, RestErls),
-    NewFirstErls = FirstErls ++ OtherFirstErls,
+    %% Dependencies of OtherFirstErls that must be compiled first.
+    OtherFirstErlsDeps = lists:flatmap(
+                           fun(Erl) -> erls(get_parents(G, Erl)) end,
+                           OtherFirstErls),
+    %% NOTE: In case the way we retrieve OtherFirstErlsDeps or merge
+    %% it with OtherFirstErls does not result in the correct compile
+    %% priorities, or the method in use proves to be too slow for
+    %% certain projects, consider using a more elaborate method (maybe
+    %% digraph_utils) or alternatively getting and compiling the .erl
+    %% parents of an individual Source in internal_erl_compile. By not
+    %% handling this in internal_erl_compile, we also avoid extra
+    %% needs_compile/2 calls.
+    FirstErls = ErlFirstFiles ++ uo_merge(OtherFirstErlsDeps, OtherFirstErls),
+    ?DEBUG("Files to compile first: ~p~n", [FirstErls]),
     rebar_base_compiler:run(
-      Config, NewFirstErls, OtherErls,
+      Config, FirstErls, OtherErls,
       fun(S, C) ->
               internal_erl_compile(C, S, OutDir1, ErlOpts, G)
       end),
     true = code:set_path(CurrPath),
     ok.
 
+%%
+%% Return all .erl files from a list of files
+%%
+erls(Files) ->
+    [Erl || Erl <- Files, filename:extension(Erl) =:= ".erl"].
+
+%%
+%% Return a list without duplicates while preserving order
+%%
+ulist(L) ->
+    ulist(L, []).
+
+ulist([H|T], Acc) ->
+    case lists:member(H, T) of
+        true ->
+            ulist(T, Acc);
+        false ->
+            ulist(T, [H|Acc])
+    end;
+ulist([], Acc) ->
+    lists:reverse(Acc).
+
+%%
+%% Merge two lists without duplicates while preserving order
+%%
+uo_merge(L1, L2) ->
+    lists:foldl(fun(E, Acc) -> u_add_element(E, Acc) end, ulist(L1), L2).
+
+u_add_element(Elem, [Elem|_]=Set) -> Set;
+u_add_element(Elem, [E1|Set])     -> [E1|u_add_element(Elem, Set)];
+u_add_element(Elem, [])           -> [Elem].
+
 -spec include_path(file:filename(),
                    rebar_config:config()) -> [file:filename(), ...].
 include_path(Source, Config) ->
@@ -315,22 +371,40 @@ include_path(Source, Config) ->
 
 -spec needs_compile(file:filename(), file:filename(),
                     [string()]) -> boolean().
-needs_compile(Source, Target, Hrls) ->
+needs_compile(Source, Target, Parents) ->
     TargetLastMod = filelib:last_modified(Target),
     lists:any(fun(I) -> TargetLastMod < filelib:last_modified(I) end,
-              [Source] ++ Hrls).
+              [Source] ++ Parents).
 
-init_graph(Config, Erls) ->
-    KeepGraph = rebar_config:get_list(Config, keep_build_info, false),
-    G = restore_graph("ebin", KeepGraph),
-    lists:foreach(
-      fun(Erl) ->
-              update_graph(G, Erl, include_path(Erl, Config))
-      end, Erls),
-    store_graph(G, "ebin", KeepGraph),
+check_erlcinfo(_Config, #erlcinfo{vsn=?ERLCINFO_VSN}) ->
+    ok;
+check_erlcinfo(Config, #erlcinfo{vsn=Vsn}) ->
+    ?ABORT("~s file version is incompatible. expected: ~b got: ~b~n",
+           [erlcinfo_file(Config), ?ERLCINFO_VSN, Vsn]);
+check_erlcinfo(Config, _) ->
+    ?ABORT("~s file is invalid. Please delete before next run.~n",
+           [erlcinfo_file(Config)]).
+
+erlcinfo_file(Config) ->
+    filename:join([rebar_utils:base_dir(Config), ".rebar", ?ERLCINFO_FILE]).
+
+init_erlcinfo(Config, Erls) ->
+    G = restore_erlcinfo(Config),
+    %% Get a unique list of dirs based on the source files' locations.
+    %% This is used for finding files in sub dirs of the configured
+    %% src_dirs. For example, src/sub_dir/foo.erl.
+    Dirs = sets:to_list(lists:foldl(
+                          fun(Erl, Acc) ->
+                                  Dir = filename:dirname(Erl),
+                                  sets:add_element(Dir, Acc)
+                          end, sets:new(), Erls)),
+    Updates = [update_erlcinfo(G, Erl, include_path(Erl, Config) ++ Dirs)
+               || Erl <- Erls],
+    Modified = lists:member(modified, Updates),
+    ok = store_erlcinfo(G, Config, Modified),
     G.
 
-update_graph(G, Source, IncludePath) ->
+update_erlcinfo(G, Source, Dirs) ->
     case digraph:vertex(G, Source) of
         {_, LastUpdated} ->
             LastModified = filelib:last_modified(Source),
@@ -338,44 +412,42 @@ update_graph(G, Source, IncludePath) ->
                     %% The file doesn't exist anymore,
                     %% erase it from the graph.
                     %% All the edges will be erased automatically.
-                    digraph:del_vertex(G, Source);
+                    digraph:del_vertex(G, Source),
+                    modified;
                LastUpdated < LastModified ->
-                    modify_graph(G, Source, IncludePath);
+                    modify_erlcinfo(G, Source, Dirs);
+                    modified;
                true ->
-                    ok
+                    unmodified
             end;
         false ->
-            modify_graph(G, Source, IncludePath)
+            modify_erlcinfo(G, Source, Dirs),
+            modified
     end.
 
-modify_graph(G, Source, IncludePath) ->
-    case file:open(Source, [read]) of
-        {ok, Fd} ->
-            Incls = parse_attrs(Fd, []),
-            AbsIncls = expand_file_names(Incls, IncludePath),
-            catch file:close(Fd),
-            LastUpdated = {date(), time()},
-            digraph:add_vertex(G, Source, LastUpdated),
-            lists:foreach(
-              fun(Incl) ->
-                      update_graph(G, Incl, IncludePath),
-                      digraph:add_edge(G, Source, Incl)
-              end, AbsIncls);
-        _Err ->
-            ok
-    end.
+modify_erlcinfo(G, Source, Dirs) ->
+    {ok, Fd} = file:open(Source, [read]),
+    Incls = parse_attrs(Fd, []),
+    AbsIncls = expand_file_names(Incls, Dirs),
+    ok = file:close(Fd),
+    LastUpdated = {date(), time()},
+    digraph:add_vertex(G, Source, LastUpdated),
+    lists:foreach(
+      fun(Incl) ->
+              update_erlcinfo(G, Incl, Dirs),
+              digraph:add_edge(G, Source, Incl)
+      end, AbsIncls).
 
-restore_graph(_OutDir, _KeepGraph = false) ->
-    digraph:new();
-restore_graph(OutDir, _KeepGraph = true) ->
-    File = filename:join(OutDir, ?REBAR_BUILD_INFO),
+restore_erlcinfo(Config) ->
+    File = erlcinfo_file(Config),
     G = digraph:new(),
     case file:read_file(File) of
         {ok, Data} ->
-            case catch binary_to_term(Data) of
-                {'EXIT', _} ->
-                    ok;
-                {Vs, Es} ->
+            try binary_to_term(Data) of
+                Erlcinfo ->
+                    ok = check_erlcinfo(Config, Erlcinfo),
+                    #erlcinfo{info=ErlcInfo} = Erlcinfo,
+                    {Vs, Es} = ErlcInfo,
                     lists:foreach(
                       fun({V, LastUpdated}) ->
                               digraph:add_vertex(G, V, LastUpdated)
@@ -384,15 +456,21 @@ restore_graph(OutDir, _KeepGraph = true) ->
                       fun({V1, V2}) ->
                               digraph:add_edge(G, V1, V2)
                       end, Es)
+            catch
+                error:badarg ->
+                    ?ERROR(
+                       "Failed (binary_to_term) to restore rebar info file."
+                       " Discard file.~n", []),
+                    ok
             end;
         _Err ->
             ok
     end,
     G.
 
-store_graph(_G, _OutDir, _KeepGraph = false) ->
+store_erlcinfo(_G, _Config, _Modified = false) ->
     ok;
-store_graph(G, OutDir, _KeepGraph = true) ->
+store_erlcinfo(G, Config, _Modified) ->
     Vs = lists:map(
            fun(V) ->
                    digraph:vertex(G, V)
@@ -405,14 +483,23 @@ store_graph(G, OutDir, _KeepGraph = true) ->
                              {V1, V2}
                      end, digraph:out_edges(G, V))
            end, Vs),
-    File = filename:join(OutDir, ?REBAR_BUILD_INFO),
-    file:write_file(File, term_to_binary({Vs, Es})).
-
+    File = erlcinfo_file(Config),
+    ok = filelib:ensure_dir(File),
+    Data = term_to_binary(#erlcinfo{info={Vs, Es}}, [{compressed, 9}]),
+    file:write_file(File, Data).
+
+%% NOTE: If, for example, one of the entries in Files, refers to
+%% gen_server.erl, that entry will be dropped. It is dropped because
+%% such an entry usually refers to the beam file, and we don't pass a
+%% list of OTP src dirs for finding gen_server.erl's full path. Also,
+%% if gen_server.erl was modified, it's not rebar's task to compile a
+%% new version of the beam file. Therefore, it's reasonable to drop
+%% such entries. Also see process_attr(behaviour, Form, Includes).
 -spec expand_file_names([file:filename()],
                         [file:filename()]) -> [file:filename()].
-expand_file_names(Files, IncludePath) ->
-    %% We check if Files exist by itself or
-    %% within the directories listed in IncludePath.
+expand_file_names(Files, Dirs) ->
+    %% We check if Files exist by itself or within the directories
+    %% listed in Dirs.
     %% Return the list of files matched.
     lists:flatmap(
       fun(Incl) ->
@@ -421,15 +508,15 @@ expand_file_names(Files, IncludePath) ->
                       [Incl];
                   false ->
                       lists:flatmap(
-                        fun(Path) ->
-                                FullPath = filename:join(Path, Incl),
+                        fun(Dir) ->
+                                FullPath = filename:join(Dir, Incl),
                                 case filelib:is_regular(FullPath) of
                                     true ->
                                         [FullPath];
                                     false ->
                                         []
                                 end
-                        end, IncludePath)
+                        end, Dirs)
               end
       end, Files).
 
@@ -446,18 +533,19 @@ get_children(G, Source) ->
 -spec internal_erl_compile(rebar_config:config(), file:filename(),
                            file:filename(), list(),
                            digraph()) -> 'ok' | 'skipped'.
-internal_erl_compile(Config, Source, Outdir, ErlOpts, G) ->
+internal_erl_compile(Config, Source, OutDir, ErlOpts, G) ->
     %% Determine the target name and includes list by inspecting the source file
     Module = filename:basename(Source, ".erl"),
-    Hrls = get_parents(G, Source),
+    Parents = get_parents(G, Source),
+    log_files(?FMT("~s depends on", [Source]), Parents),
 
     %% Construct the target filename
-    Target = filename:join([Outdir | string:tokens(Module, ".")]) ++ ".beam",
+    Target = filename:join([OutDir | string:tokens(Module, ".")]) ++ ".beam",
     ok = filelib:ensure_dir(Target),
 
     %% If the file needs compilation, based on last mod date of includes or
     %% the target
-    case needs_compile(Source, Target, Hrls) of
+    case needs_compile(Source, Target, Parents) of
         true ->
             Opts = [{outdir, filename:dirname(Target)}] ++
                 ErlOpts ++ [{i, "include"}, return],
@@ -566,6 +654,8 @@ process_attr(Form, Includes) ->
         AttrName = erl_syntax:atom_value(erl_syntax:attribute_name(Form)),
         process_attr(AttrName, Form, Includes)
     catch _:_ ->
+            %% TODO: We should probably try to be more specific here
+            %% and not suppress all errors.
             Includes
     end.
 
@@ -585,7 +675,8 @@ process_attr(include, Form, Includes) ->
     [File|Includes];
 process_attr(include_lib, Form, Includes) ->
     [FileNode] = erl_syntax:attribute_arguments(Form),
-    File = erl_syntax:string_value(FileNode),
+    RawFile = erl_syntax:string_value(FileNode),
+    File = maybe_expand_include_lib_path(RawFile),
     [File|Includes];
 process_attr(behaviour, Form, Includes) ->
     [FileNode] = erl_syntax:attribute_arguments(Form),
@@ -599,10 +690,40 @@ process_attr(compile, Form, Includes) ->
         {core_transform, Mod} ->
             [atom_to_list(Mod) ++ ".erl"|Includes];
         L when is_list(L) ->
-            {_, Mod} = lists:keyfind(parse_transform, 1, L),
-            [atom_to_list(Mod) ++ ".erl"|Includes]
+            lists:foldl(
+              fun({parse_transform, M}, Acc) ->
+                      [atom_to_list(M) ++ ".erl"|Acc];
+                 ({core_transform, M}, Acc) ->
+                      [atom_to_list(M) ++ ".erl"|Acc];
+                 (_, Acc) ->
+                      Acc
+              end, Includes, L)
     end.
 
+%% Given the filename from an include_lib attribute, if the path
+%% exists, return unmodified, or else get the absolute ERL_LIBS
+%% path.
+maybe_expand_include_lib_path(File) ->
+    case filelib:is_regular(File) of
+        true ->
+            File;
+        false ->
+            expand_include_lib_path(File)
+    end.
+
+%% Given a path like "stdlib/include/erl_compile.hrl", return
+%% "OTP_INSTALL_DIR/lib/erlang/lib/stdlib-x.y.z/include/erl_compile.hrl".
+%% Usually a simple [Lib, SubDir, File1] = filename:split(File) should
+%% work, but to not crash when an unusual include_lib path is used,
+%% utilize more elaborate logic.
+expand_include_lib_path(File) ->
+    File1 = filename:basename(File),
+    Split = filename:split(filename:dirname(File)),
+    Lib = hd(Split),
+    SubDir = filename:join(tl(Split)),
+    Dir = code:lib_dir(list_to_atom(Lib), list_to_atom(SubDir)),
+    filename:join(Dir, File1).
+
 %%
 %% Ensure all files in a list are present and abort if one is missing
 %%
@@ -615,3 +736,13 @@ check_file(File) ->
         false -> ?ABORT("File ~p is missing, aborting\n", [File]);
         true -> File
     end.
+
+%% Print prefix followed by list of files. If the list is empty, print
+%% on the same line, otherwise use a separate line.
+log_files(Prefix, Files) ->
+    case Files of
+        [] ->
+            ?DEBUG("~s: ~p~n", [Prefix, Files]);
+        _ ->
+            ?DEBUG("~s:~n~p~n", [Prefix, Files])
+    end.