Merge pull request #424 from tomas-abrahamsson/gpb-recompilation-detection-2

Gpb recompilation detection (using base compiler)

inttest/proto_gpb/proto_gpb_rt.erl

@@ -29,6 +29,8 @@
          run/1]).
 
 -include_lib("eunit/include/eunit.hrl").
+-include_lib("kernel/include/file.hrl").
+-include_lib("deps/retest/include/retest.hrl").
 
 -define(MODULES,
         [foo,
@@ -42,6 +44,13 @@
          test4_gpb,
          test5_gpb]).
 
+-define(SOURCE_PROTO_FILES,
+        ["test.proto",
+         "a/test2.proto",
+         "a/b/test3.proto",
+         "c/test4.proto",
+         "c/d/test5.proto"]).
+
 files() ->
     [
      {copy, "../../rebar", "rebar"},
@@ -60,6 +69,23 @@ run(_Dir) ->
     %% generating the test_gpb.hrl file, and also that it generated
     %% the .hrl file was generated before foo was compiled.
     ok = check_beams_generated(),
+
+    ?DEBUG("Verifying recompilation~n", []),
+    TestErl = hd(generated_erl_files()),
+    TestProto = hd(source_proto_files()),
+    make_proto_newer_than_erl(TestProto, TestErl),
+    TestMTime1 = read_mtime(TestErl),
+    ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+    TestMTime2 = read_mtime(TestErl),
+    ?assert(TestMTime2 > TestMTime1),
+
+    ?DEBUG("Verifying recompilation with no changes~n", []),
+    TestMTime3 = read_mtime(TestErl),
+    ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+    TestMTime4 = read_mtime(TestErl),
+    ?assert(TestMTime3 =:= TestMTime4),
+
+    ?DEBUG("Verify cleanup~n", []),
     ?assertMatch({ok, _}, retest_sh:run("./rebar clean", [])),
     ok = check_files_deleted(),
     ok.
@@ -81,6 +107,12 @@ generated_erl_files() ->
 generated_hrl_files() ->
     add_dir("include", add_ext(?GENERATED_MODULES, ".hrl")).
 
+generated_beam_files() ->
+    add_dir("ebin", add_ext(?GENERATED_MODULES, ".beam")).
+
+source_proto_files() ->
+    add_dir("src", ?SOURCE_PROTO_FILES).
+
 file_does_not_exist(F) ->
     not filelib:is_regular(F).
 
@@ -90,6 +122,30 @@ add_ext(Modules, Ext) ->
 add_dir(Dir, Files) ->
     [filename:join(Dir, File) || File <- Files].
 
+read_mtime(File) ->
+    {ok, #file_info{mtime=MTime}} = file:read_file_info(File),
+    MTime.
+
+
+make_proto_newer_than_erl(Proto, Erl) ->
+    %% Do this by back-dating the erl file instead of touching the
+    %% proto file.  Do this instead of sleeping for a second to get a
+    %% reliable test.  Sleeping would have been needed sin ce the
+    %% #file_info{} (used by eg. filelib:last_modified) does not have
+    %% sub-second resolution (even though most file systems have).
+    {ok, #file_info{mtime=ProtoMTime}} = file:read_file_info(Proto),
+    {ok, ErlInfo} = file:read_file_info(Erl),
+    OlderMTime = update_seconds_to_datetime(ProtoMTime, -2),
+    OlderErlInfo = ErlInfo#file_info{mtime = OlderMTime},
+    ok = file:write_file_info(Erl, OlderErlInfo).
+
+update_seconds_to_datetime(DT, ToAdd) ->
+    calendar:gregorian_seconds_to_datetime(
+      calendar:datetime_to_gregorian_seconds(DT) + ToAdd).
+
+touch_file(File) ->
+    ?assertMatch({ok, _}, retest_sh:run("touch " ++ File, [])).
+
 check(Check, Files) ->
     lists:foreach(
       fun(F) ->

rebar.config

@@ -26,6 +26,7 @@
       - (\"neotoma\":\"file\"/\"2\")
       - (\"protobuffs_compile\":\"scan_file\"/\"2\")
       - (\"gpb_compile\":\"file\"/\"2\")
+      - (\"gpb_compile\":\"format_error\"/\"1\")
       - (\"diameter_codegen\":\"from_dict\"/\"4\")
       - (\"diameter_dict_util\":\"format_error\"/\"1\")
       - (\"diameter_dict_util\":\"parse\"/\"2\"))",

src/rebar_appups.erl

@@ -69,8 +69,8 @@
     {_Added, _Removed, Upgraded} = get_apps(Name, OldVerPath, NewVerPath),
 
     %% Get a list of any appup files that exist in the new release
-    NewAppUpFiles = rebar_utils:find_files(
+    NewAppUpFiles = rebar_utils:find_files_by_ext(
-                      filename:join([NewVerPath, "lib"]), "^[^._].*.appup$"),
+                      filename:join([NewVerPath, "lib"]), ".appup"),
 
     %% Convert the list of appup files into app names
     AppUpApps = [file_to_name(File) || File <- NewAppUpFiles],

src/rebar_base_compiler.erl

@@ -31,6 +31,7 @@
 -export([run/4,
          run/7,
          run/8,
+         run/5,
          ok_tuple/3,
          error_tuple/5]).
 
@@ -62,23 +63,31 @@ run(Config, FirstFiles, SourceDir, SourceExt, TargetDir, TargetExt,
 
 run(Config, FirstFiles, SourceDir, SourceExt, TargetDir, TargetExt,
     Compile3Fn, Opts) ->
-    %% Convert simple extension to proper regex
-    SourceExtRe = "^[^._].*\\" ++ SourceExt ++ [$$],
 
-    Recursive = proplists:get_value(recursive, Opts, true),
     %% Find all possible source files
-    FoundFiles = rebar_utils:find_files(SourceDir, SourceExtRe, Recursive),
+    Recursive = proplists:get_value(recursive, Opts, true),
+    FoundFiles = rebar_utils:find_files_by_ext(SourceDir, SourceExt, Recursive),
+
     %% Remove first files from found files
     RestFiles = [Source || Source <- FoundFiles,
                            not lists:member(Source, FirstFiles)],
 
+    FirstUnits = source_to_unit_each(FirstFiles,
+                                     SourceDir, SourceExt,
+                                     TargetDir, TargetExt),
+    RestUnits = source_to_unit_each(RestFiles,
+                                    SourceDir, SourceExt,
+                                    TargetDir, TargetExt),
+    run(Config, FirstUnits, RestUnits, Compile3Fn, Opts).
+
+%% FirstUnits and RestUnits are lists of tuples: {Source,Target}
+run(Config, FirstUnits, RestUnits, Compile3Fn, Opts) ->
+
     %% Check opts for flag indicating that compile should check lastmod
     CheckLastMod = proplists:get_bool(check_last_mod, Opts),
 
-    run(Config, FirstFiles, RestFiles,
+    run(Config, FirstUnits, RestUnits,
-        fun(S, C) ->
+        fun({S, Target}, C) ->
-                Target = target_file(S, SourceDir, SourceExt,
-                                     TargetDir, TargetExt),
                 simple_compile_wrapper(S, Target, Compile3Fn, C, CheckLastMod)
         end).
 
@@ -103,6 +112,10 @@ simple_compile_wrapper(Source, Target, Compile3Fn, Config, true) ->
             skipped
     end.
 
+source_to_unit_each(Files, SourceDir, SourceExt, TargetDir, TargetExt) ->
+    [{File, target_file(File, SourceDir, SourceExt, TargetDir, TargetExt)}
+     || File <- Files].
+
 target_file(SourceFile, SourceDir, SourceExt, TargetDir, TargetExt) ->
     BaseFile = remove_common_path(SourceFile, SourceDir),
     filename:join([TargetDir, filename:basename(BaseFile, SourceExt) ++ TargetExt]).
@@ -117,8 +130,8 @@ remove_common_path1(FilenameParts, _) ->
     filename:join(FilenameParts).
 
 
-compile(Source, Config, CompileFn) ->
+compile(Unit, Config, CompileFn) ->
-    case CompileFn(Source, Config) of
+    case CompileFn(Unit, Config) of
         ok ->
             ok;
         skipped ->
@@ -129,24 +142,29 @@ compile(Source, Config, CompileFn) ->
 
 compile_each([], _Config, _CompileFn) ->
     ok;
-compile_each([Source | Rest], Config, CompileFn) ->
+compile_each([Unit | Rest], Config, CompileFn) ->
-    case compile(Source, Config, CompileFn) of
+    case compile(Unit, Config, CompileFn) of
         ok ->
-            ?CONSOLE("Compiled ~s\n", [Source]);
+            ?CONSOLE("Compiled ~s\n", [unit_source(Unit)]);
         {ok, Warnings} ->
             report(Warnings),
-            ?CONSOLE("Compiled ~s\n", [Source]);
+            ?CONSOLE("Compiled ~s\n", [unit_source(Unit)]);
         skipped ->
-            ?INFO("Skipped ~s\n", [Source]);
+            ?INFO("Skipped ~s\n", [unit_source(Unit)]);
         Error ->
             ?CONSOLE("Compiling ~s failed:\n",
-                     [maybe_absname(Config, Source)]),
+                     [maybe_absname(Config, unit_source(Unit))]),
             maybe_report(Error),
             ?DEBUG("Compilation failed: ~p\n", [Error]),
             ?FAIL
     end,
     compile_each(Rest, Config, CompileFn).
 
+unit_source({Source, _Target}) ->
+    Source;
+unit_source(Source) ->
+    Source.
+
 compile_queue(_Config, [], []) ->
     ok;
 compile_queue(Config, Pids, Targets) ->
@@ -168,17 +186,17 @@ compile_queue(Config, Pids, Targets) ->
             ?DEBUG("Worker compilation failed: ~p\n", [Error]),
             ?FAIL;
 
-        {compiled, Source, Warnings} ->
+        {compiled, Unit, Warnings} ->
             report(Warnings),
-            ?CONSOLE("Compiled ~s\n", [Source]),
+            ?CONSOLE("Compiled ~s\n", [unit_source(Unit)]),
             compile_queue(Config, Pids, Targets);
 
-        {compiled, Source} ->
+        {compiled, Unit} ->
-            ?CONSOLE("Compiled ~s\n", [Source]),
+            ?CONSOLE("Compiled ~s\n", [unit_source(Unit)]),
             compile_queue(Config, Pids, Targets);
 
-        {skipped, Source} ->
+        {skipped, Unit} ->
-            ?INFO("Skipped ~s\n", [Source]),
+            ?INFO("Skipped ~s\n", [unit_source(Unit)]),
             compile_queue(Config, Pids, Targets);
 
         {'DOWN', Mref, _, Pid, normal} ->

src/rebar_erlc_compiler.erl

@@ -47,8 +47,6 @@
           info = {[], []} :: erlc_info()
         }).
 
--define(RE_PREFIX, "^[^._]").
-
 -ifdef(namespaced_types).
 %% digraph:graph() exists starting from Erlang 17.
 -type rebar_digraph() :: digraph:graph().
@@ -112,14 +110,14 @@ compile(Config, _AppFile) ->
 
 -spec clean(rebar_config:config(), file:filename()) -> 'ok'.
 clean(Config, _AppFile) ->
-    MibFiles = rebar_utils:find_files("mibs", ?RE_PREFIX".*\\.mib\$"),
+    MibFiles = rebar_utils:find_files_by_ext("mibs", ".mib"),
     MIBs = [filename:rootname(filename:basename(MIB)) || MIB <- MibFiles],
     rebar_file_utils:delete_each(
       [filename:join(["include",MIB++".hrl"]) || MIB <- MIBs]),
     lists:foreach(fun(F) -> ok = rebar_file_utils:rm_rf(F) end,
                   ["ebin/*.beam", "priv/mibs/*.bin"]),
 
-    YrlFiles = rebar_utils:find_files("src", ?RE_PREFIX".*\\.[x|y]rl\$"),
+    YrlFiles = rebar_utils:find_files_by_ext("src", ".[x|y]rl"),
     rebar_file_utils:delete_each(
       [ binary_to_list(iolist_to_binary(re:replace(F, "\\.[x|y]rl$", ".erl")))
         || F <- YrlFiles ]),
@@ -131,7 +129,7 @@ clean(Config, _AppFile) ->
     %% directory structure in ebin with .beam files within. As such, we want
     %% to scan whatever is left in the ebin/ directory for sub-dirs which
     %% satisfy our criteria.
-    BeamFiles = rebar_utils:find_files("ebin", ?RE_PREFIX".*\\.beam\$"),
+    BeamFiles = rebar_utils:find_files_by_ext("ebin", ".beam"),
     rebar_file_utils:delete_each(BeamFiles),
     lists:foreach(fun(Dir) -> delete_dir(Dir, dirs(Dir)) end, dirs("ebin")),
     ok.
@@ -142,7 +140,7 @@ clean(Config, _AppFile) ->
 
 test_compile(Config, Cmd, OutDir) ->
     %% Obtain all the test modules for inclusion in the compile stage.
-    TestErls = rebar_utils:find_files("test", ?RE_PREFIX".*\\.erl\$"),
+    TestErls = rebar_utils:find_files_by_ext("test", ".erl"),
 
     ErlOpts = rebar_utils:erl_opts(Config),
     {Config1, ErlOpts1} = test_compile_config_and_opts(Config, ErlOpts, Cmd),
@@ -153,8 +151,7 @@ test_compile(Config, Cmd, OutDir) ->
     SrcDirs = rebar_utils:src_dirs(proplists:append_values(src_dirs, ErlOpts1)),
     SrcErls = lists:foldl(
                 fun(Dir, Acc) ->
-                        Files = rebar_utils:find_files(
+                        Files = rebar_utils:find_files_by_ext(Dir, ".erl"),
-                                  Dir, ?RE_PREFIX".*\\.erl\$"),
                         lists:append(Acc, Files)
                 end, [], SrcDirs),
 
@@ -649,7 +646,7 @@ gather_src([], Srcs) ->
     Srcs;
 gather_src([Dir|Rest], Srcs) ->
     gather_src(
-      Rest, Srcs ++ rebar_utils:find_files(Dir, ?RE_PREFIX".*\\.erl\$")).
+      Rest, Srcs ++ rebar_utils:find_files_by_ext(Dir, ".erl")).
 
 -spec dirs(file:filename()) -> [file:filename()].
 dirs(Dir) ->

src/rebar_proto_compiler.erl

@@ -44,7 +44,7 @@
 %% ===================================================================
 
 compile(Config, AppFile) ->
-    case rebar_utils:find_files("src", "^[^._].*\\.proto$") of
+    case rebar_utils:find_files_by_ext("src", ".proto") of
         [] ->
             ok;
         Protos ->
@@ -56,7 +56,7 @@ compile(Config, AppFile) ->
 
 clean(Config, AppFile) ->
     %% Get a list of generated .beam and .hrl files and then delete them
-    Protos = rebar_utils:find_files("src", "^[^._].*\\.proto$"),
+    Protos = rebar_utils:find_files_by_ext("src", ".proto"),
     case Protos of
         [] ->
             ok;

src/rebar_proto_gpb_compiler.erl

@@ -47,9 +47,12 @@ proto_compile(Config, _AppFile, _ProtoFiles) ->
     %% since we have.proto files that need building
     case gpb_is_present() of
         true ->
+            GpbOpts = user_gpb_opts(Config),
+            Files = rebar_utils:find_files_by_ext("src", ".proto"),
+            Targets = [filename:join("src", target_filename(F, GpbOpts))
+                       || F <- Files],
             rebar_base_compiler:run(Config, [],
-                                    "src", ".proto",
+                                    lists:zip(Files, Targets),
-                                    "src", ".erl",
                                     fun compile_gpb/3,
                                     [{check_last_mod, true}]);
         false ->
@@ -57,14 +60,18 @@ proto_compile(Config, _AppFile, _ProtoFiles) ->
             ?FAIL
     end.
 
+target_filename(ProtoFileName, GpbOpts) ->
+    ModulePrefix = proplists:get_value(module_name_prefix, GpbOpts, ""),
+    ModuleSuffix = proplists:get_value(module_name_suffix, GpbOpts, ""),
+    Base = filename:basename(ProtoFileName, ".proto"),
+    ModulePrefix ++ Base ++ ModuleSuffix ++ ".erl".
+
 proto_clean(Config, _AppFile, ProtoFiles) ->
-    GpbOpts = gpb_opts(Config),
+    GpbOpts = user_gpb_opts(Config) ++ default_dest_opts(),
-    MPrefix = proplists:get_value(module_name_prefix, GpbOpts, ""),
-    MSuffix = proplists:get_value(module_name_suffix, GpbOpts, ""),
     rebar_file_utils:delete_each(
-      [beam_relpath(MPrefix, F, MSuffix) || F <- ProtoFiles]
+      [beam_file(F, GpbOpts) || F <- ProtoFiles]
-      ++ [erl_relpath(MPrefix, F, MSuffix) || F <- ProtoFiles]
+      ++ [erl_file(F, GpbOpts) || F <- ProtoFiles]
-      ++ [hrl_relpath(MPrefix, F, MSuffix) || F <- ProtoFiles]),
+      ++ [hrl_file(F, GpbOpts) || F <- ProtoFiles]),
     ok.
 
 %% ===================================================================
@@ -82,37 +89,55 @@ proto_info(help, compile) ->
 proto_info(help, clean) ->
     ?CONSOLE("", []).
 
-gpb_opts(Config) ->
-    rebar_config:get_local(Config, gpb_opts, []).
-
 gpb_is_present() ->
     code:which(gpb) =/= non_existing.
 
+user_gpb_opts(Config) ->
+    rebar_config:get_local(Config, gpb_opts, []).
+
+default_dest_opts() ->
+    [{o_erl, "src"}, {o_hrl, "include"}].
+
 compile_gpb(Source, _Target, Config) ->
     SourceFullPath = filename:absname(Source),
-    DefaultDestOpts = [{o_erl, "src"}, {o_hrl, "include"}],
+    GpbOpts = user_gpb_opts(Config) ++ default_dest_opts()
-    SelfIncludeOpt = [{i,filename:dirname(SourceFullPath)}],
+        ++ default_include_opts(SourceFullPath),
-    GpbOpts = gpb_opts(Config) ++ DefaultDestOpts ++ SelfIncludeOpt,
     ok = filelib:ensure_dir(filename:join("ebin", "dummy")),
     ok = filelib:ensure_dir(filename:join("include", "dummy")),
     case gpb_compile:file(SourceFullPath, GpbOpts) of
         ok ->
             ok;
-        {error, _Reason} ->
+        {error, Reason} ->
-            ?ERROR("Failed to compile ~s~n", [Source]),
+            ReasonStr = gpb_compile:format_error(Reason),
+            ?ERROR("Failed to compile ~s: ~s~n", [SourceFullPath, ReasonStr]),
             ?FAIL
     end.
 
-beam_relpath(Prefix, Proto, Suffix) ->
+default_include_opts(SourceFullPath) ->
-    proto_filename_to_relpath("ebin", Prefix, Proto, Suffix, ".beam").
+    [{i,filename:dirname(SourceFullPath)}].
 
-erl_relpath(Prefix, Proto, Suffix) ->
+beam_file(ProtoFile, GpbOpts) ->
-    proto_filename_to_relpath("src", Prefix, Proto, Suffix, ".erl").
+    proto_filename_to_path("ebin", ProtoFile, ".beam", GpbOpts).
 
-hrl_relpath(Prefix, Proto, Suffix) ->
+erl_file(ProtoFile, GpbOpts) ->
-    proto_filename_to_relpath("include", Prefix, Proto, Suffix, ".hrl").
+    ErlOutDir = get_erl_outdir(GpbOpts),
+    proto_filename_to_path(ErlOutDir, ProtoFile, ".erl", GpbOpts).
 
-proto_filename_to_relpath(Dir, Prefix, Proto, Suffix, NewExt) ->
+hrl_file(ProtoFile, GpbOpts) ->
-    BaseNoExt = filename:basename(Proto, ".proto"),
+    HrlOutDir = get_hrl_outdir(GpbOpts),
+    proto_filename_to_path(HrlOutDir, ProtoFile, ".hrl", GpbOpts).
+
+proto_filename_to_path(Dir, ProtoFile, NewExt, GpbOpts) ->
+    BaseNoExt = filename:basename(ProtoFile, ".proto"),
+    Prefix = proplists:get_value(module_name_prefix, GpbOpts, ""),
+    Suffix = proplists:get_value(module_name_suffix, GpbOpts, ""),
     filename:join([Dir, Prefix ++ BaseNoExt ++ Suffix ++ NewExt]).
 
+get_erl_outdir(Opts) ->
+    proplists:get_value(o_erl, Opts, get_outdir(Opts)).
+
+get_hrl_outdir(Opts) ->
+    proplists:get_value(o_hrl, Opts, get_outdir(Opts)).
+
+get_outdir(Opts) ->
+    proplists:get_value(o, Opts, ".").

src/rebar_qc.erl

@@ -211,7 +211,7 @@ qc_module(QC=eqc, [], M) -> QC:module(M);
 qc_module(QC=eqc, QCOpts, M) -> QC:module(QCOpts, M).
 
 find_prop_mods() ->
-    Beams = rebar_utils:find_files(?QC_DIR, "^[^._].*\\.beam\$"),
+    Beams = rebar_utils:find_files_by_ext(?QC_DIR, ".beam"),
     [M || M <- [rebar_utils:erl_to_mod(Beam) || Beam <- Beams], has_prop(M)].
 
 has_prop(Mod) ->

src/rebar_templater.erl

@@ -242,11 +242,10 @@ find_escript_templates(Files) ->
 
 find_disk_templates(Config) ->
     OtherTemplates = find_other_templates(Config),
-    HomeFiles = rebar_utils:find_files(filename:join([os:getenv("HOME"),
+    HomeTemplates = filename:join([os:getenv("HOME"), ".rebar", "templates"]),
-                                                      ".rebar", "templates"]),
+    HomeFiles = rebar_utils:find_files_by_ext(HomeTemplates, ".template"),
-                                       ?TEMPLATE_RE),
     Recursive = rebar_config:is_recursive(Config),
-    LocalFiles = rebar_utils:find_files(".", ?TEMPLATE_RE, Recursive),
+    LocalFiles = rebar_utils:find_files_by_ext(".", ".template", Recursive),
     [{file, F} || F <- OtherTemplates ++ HomeFiles ++ LocalFiles].
 
 find_other_templates(Config) ->
@@ -254,7 +253,7 @@ find_other_templates(Config) ->
         undefined ->
             [];
         TemplateDir ->
-            rebar_utils:find_files(TemplateDir, ?TEMPLATE_RE)
+            rebar_utils:find_files_by_ext(TemplateDir, ".template")
     end.
 
 select_template([], Template) ->

src/rebar_utils.erl

@@ -34,6 +34,8 @@
          sh_send/3,
          find_files/2,
          find_files/3,
+         find_files_by_ext/2,
+         find_files_by_ext/3,
          now_str/0,
          ensure_dir/1,
          beam_to_mod/2,
@@ -160,6 +162,28 @@ find_files(Dir, Regex, Recursive) ->
     filelib:fold_files(Dir, Regex, Recursive,
                        fun(F, Acc) -> [F | Acc] end, []).
 
+%% Find files by extension, for example ".erl", avoiding resource fork
+%% files in OS X.  Such files are named for example src/._xyz.erl
+%% Such files may also appear with network filesystems on OS X.
+%%
+%% The Ext is really a regexp, with any leading dot implicitly
+%% escaped, and anchored at the end of the string.
+%%
+find_files_by_ext(Dir, Ext) ->
+    find_files_by_ext(Dir, Ext, true).
+
+find_files_by_ext(Dir, Ext, Recursive) ->
+    %% Convert simple extension to proper regex
+    EscapeDot = case Ext of
+                    "." ++ _ ->
+                        "\\";
+                    _ ->
+                        %% allow for other suffixes, such as _pb.erl
+                        ""
+                end,
+    ExtRe = "^[^._].*" ++ EscapeDot ++ Ext ++ [$$],
+    find_files(Dir, ExtRe, Recursive).
+
 now_str() ->
     {{Year, Month, Day}, {Hour, Minute, Second}} = calendar:local_time(),
     lists:flatten(io_lib:format("~4b/~2..0b/~2..0b ~2..0b:~2..0b:~2..0b",