avoid pre-compile time errors in expand_include_lib_path #485

ghost · 2015-04-25T19:01:08Z

No description provided.

ghost · 2015-04-25T20:29:55Z

Thanks. Do you mind extending the commit message with a previously broken include_lib directive?

ghost · 2015-04-25T20:53:41Z

No problem. Done.

ghost · 2015-04-25T21:10:54Z

This probably warrants a debug log msg.

ghost · 2015-04-25T21:19:30Z

ok

carlosedp · 2015-04-27T13:37:02Z

Tested the changes and the pull request works here. Thanks @Tuncer.

ghost · 2015-05-04T12:50:01Z

src/rebar_erlc_compiler.erl

+    [Lib | Parts] = filename:split(filename:dirname(File)),
+    SubDir = case Parts of
+                 [] -> % prevent function clause error
+                     [];


Would it make more sense to print the debug msg here instead?

Probably yes. I have pushed an updated version.

ghost · 2015-05-05T08:49:10Z

@kejv should we have tests to go with this?

ghost · 2015-05-05T18:03:02Z

Are we able to tell whether compilation failed because of rebar internal error or compile time error (I am thinking of retest_sh:run/2)?

ghost · 2015-05-05T19:42:17Z

On Tue, May 5, 2015 at 8:03 PM, kejv wrote:

Are we able to tell whether compilation failed because of rebar internal error or compile time error (I am thinking of retest_sh:run/2)?

Scanning the error output from run/2 would be the usual way.

ghost · 2015-05-06T16:08:49Z

Compare the rebar errors produced by 1) erlc, 2) rebar itself:

{stopped,{1,
          ["5396","==> erlc_dep_graph_rt (compile)",
           "src/lisp.erl:11: can't find include lib \"include/non/existent.hrl\"",
           "Compiling src/lisp.erl failed:",
           "ERROR: compile failed while processing /home/david/git/rebar/rt.work/20150506_175922/erlc_dep_graph_rt: rebar_abort"]}}

{stopped,{1,
          ["5550","==> erlc_dep_graph_rt (compile)",
           "ERROR: compile failed while processing /home/david/git/rebar/rt.work/20150506_175958/erlc_dep_graph_rt: {'EXIT',",
           "    {function_clause,",
           "        [{filename,join,[[]],[{file,\"filename.erl\"},{line,392}]},",
           "         {rebar_erlc_compiler,expand_include_lib_path,1,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,696}]},",
           "         {rebar_erlc_compiler,process_attr,3,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,646}]},",
           "         {rebar_erlc_compiler,parse_attrs,2,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,614}]},",
           "         {rebar_erlc_compiler,modify_erlcinfo,4,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,444}]},",
           "         {rebar_erlc_compiler,'-update_erlcinfo_fun/2-fun-0-',4,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,399}]},",
           "         {lists,foldl,3,[{file,\"lists.erl\"},{line,1248}]},",
           "         {rebar_erlc_compiler,init_erlcinfo,2,",
           "             [{file,\"src/rebar_erlc_compiler.erl\"},{line,389}]}]}}"]}}

I don't think there's any sane way how to test the difference (and I don't consider scanning the strings in the list with regex as sane). retest_sh:run/2 should probably provide some more detailed error codes.

ghost · 2015-05-06T18:56:01Z

Doesn't that mean we should (1) print a more informative error in erlc_compiler, and (2) can't we scan for the ?DEBUG message?

ghost · 2015-05-06T20:19:36Z

(1) No, since we don't print any error message at all in erlc_compiler. Quite the contrary actually, we want to try to avoid any errors.
(2) No, since ?DEBUG message is present only if compiling on debug level and I don't think we should rely on this fact.

Moreover, I really start thinking that printing the debug message is not such a good idea. Because it will print for e.g. -include_lib("erl_compile.hrl"). but not for -include_lib("stdlib/includ/erl_compile.hrl"). (note the typo). I think we should not pretend that we are trying to catch invalid include_lib file at this place. User will get compilation error later anyway. Only thing done here is trying to prevent any rebar internal errors.

ghost · 2015-05-06T20:32:27Z

I agree, but as we're treating the included header as a dependency, shouldn't we at least log a DEBUG msg on ENOFILE?

ghost · 2015-05-06T20:42:32Z

Note that at this stage we don't know yet whether the header is a dependency for rebar logic (erlcinfo). This will be decided only in expand_file_names/2 based on content of Dirs variable. Generally I think there's no reason to duplicate the work of erlc. If the header file doesn't exist erlc will throw an error anyway.

ghost · 2015-05-06T21:15:24Z

But don't we already do the check before updating the digraph?

ghost · 2015-05-06T22:45:32Z

No. We collect all included headers (in process_attr) and then filter out those which are not in the include path (in expand_file_names) and this will typically filter out all include_lib files.

Generally I think there's no reason to duplicate the work of erlc. If the header file doesn't exist erlc will throw an error anyway.

Still this is the strongest reason why rebar should not try to check existence of files, yet even printing some related debug messages.

ghost · 2015-05-07T18:17:52Z

Okay. Let's add the previously crashing scenario as a regression test and merge this.

Previously when user specified broken include_lib, such as -include_lib("my_app.hrl"), the code throwed too early exception. We fix this simply by avoiding any possible internal errors, possibly letting even nonsensical paths pass. These will be checked and filtered later in expand_file_names/2.

ghost · 2015-05-07T18:35:11Z

Until retest_sh:run/2 starts producing more detailed error codes I am afraid we will have to live without regression test...

ghost · 2015-05-07T19:26:04Z

Having fixed the rebar-internal error, why can't we (1) expect run/2 failure AND (2) scan for can't find include lib...?

ghost · 2015-05-07T20:37:37Z

You mean something like this?

{stopped,{_,Lines}} = Result,
?assertMatch([ _ | _ ], lists:filter( fun(L) ->
        re:run(L, "can't find include lib \"include/non/existent.hrl\" ) /= nomatch
end, Lines)

This seems quite hacky to me. And moreover it is also fragile. What if someone decides that he really wants that debugging message (which I didn't) and formulates it in exactly the same way as erlc? Then if he breaks the code the test will still pass. Or what if erlc for some reason changes the error message?

Frankly, I would rather have no tests at all than tests with potential of false negatives or positives.

ghost · 2015-05-07T21:16:01Z

Something like that or inttest/logging.

If someone adds that debug msg, the test would have to be adapted. Also, looking for a less specific error msg and having a minimal module that's unable to provoke any other error should make the scan resilient against changes in compile:file.

However, the missing test is not important enough to hold up the patch. So, I'm okay without it.

ferd · 2015-05-08T13:00:16Z

Uh yeah, so that +7-3 patch seems good then. Merging.

avoid pre-compile time errors in expand_include_lib_path

ghost mentioned this pull request Apr 25, 2015

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

Merged

ghost reviewed May 4, 2015
View reviewed changes

ferd added a commit that referenced this pull request May 8, 2015

Merge pull request #485 from kejv/robust-expand_include_lib_path

dec5bed

avoid pre-compile time errors in expand_include_lib_path

ferd merged commit dec5bed into rebar:master May 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid pre-compile time errors in expand_include_lib_path #485

avoid pre-compile time errors in expand_include_lib_path #485

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

carlosedp commented Apr 27, 2015

ghost May 4, 2015

ghost May 4, 2015

ghost commented May 5, 2015

ghost commented May 5, 2015

ghost commented May 5, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ferd commented May 8, 2015

avoid pre-compile time errors in expand_include_lib_path #485

avoid pre-compile time errors in expand_include_lib_path #485

Conversation

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

ghost commented Apr 25, 2015

carlosedp commented Apr 27, 2015

ghost May 4, 2015

Choose a reason for hiding this comment

ghost May 4, 2015

Choose a reason for hiding this comment

ghost commented May 5, 2015

ghost commented May 5, 2015

ghost commented May 5, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 6, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ghost commented May 7, 2015

ferd commented May 8, 2015