Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix #249 (erlc regression) #285

Merged
merged 2 commits into from Jun 15, 2014

Conversation

Projects
None yet
6 participants
Contributor

nevar commented May 27, 2014

Repository that don't declare *_first_files was inherited it from parent. And
than try to compile some files that don't exists.

Issue #249

Contributor

tuncer commented May 27, 2014

Thanks @nevar.

  • Are we sure that's the root cause? To be clear, rebar_erlc_compiler has been fetching erl_first_files via rebar_config:get_list/3 for as long as I can remember, and the last change there (b690842) didn't change that detail. Is the regression a side effect of recent changes (b690842, fd17693), or was it a hidden bug? I mean, strictly speaking, this changes the behavior and someone might be relying on inheritance of erl_first_files.
  • Can you please add a regression test in inttest/erlc?
  • Should we change all get_list(Config, ..._first_files, ...) calls (xrl, yrl, lfe)?
Contributor

nevar commented May 28, 2014

  • As @roland-karlsson-erlang-solutions-com say in #249: "rebar 2.2.0 work as expected", I start with that version.

    In 2.2.0 rebar don't include file from -include_lib in file dependencies. As result file has not dependent on file from $OTP_ROOT system app (like eunit.hrl). And when rebar check last modified time of file and dependencies files it get 0 (because this files don't exists in directory in project dependency). Check has false and rebar think that file don't need compile and skip it.

    In 2.3.0 rebar came new parse of file. With it rebar add included file from -include_lib to file dependencies. And now rebar check "is need compile" for file with same file dependencies from 2.2.0 + files from include_lib will not false. Because file from -inclide_lib exists it last modified time not 0 and rebar think it need compile file. But file don't exists in this project.

    After finding this, I thought that root evil is that project use erl_first_files from its parent project if don't set own.

    I hope my English is not so bad and you realize test case that I described.

  • Yes. I will add test for this case.

  • rebar_erlc_compiler already use get_local for xrl,yrl,mib. Maybe I miss some other places. It definitely need fix.

Contributor

tuncer commented May 28, 2014

rebar_erlc_compiler already use get_local for xrl,yrl,mib. Maybe I miss some other places. It definitely need fix.

Right, that leaves just rebar_lfe_compiler then.

Contributor

nevar commented May 28, 2014

  • fix tab -> space
  • make get_local in rebar_lfe_compiler
Contributor

nevar commented May 28, 2014

Add editor mode line header

Contributor

tuncer commented May 28, 2014

Add editor mode line header

Thanks, while at it, feel free to add it where missing to all files in inttest/erlc.

Contributor

tuncer commented May 28, 2014

After a debug session with @nevar, I think the root cause is that we don't have separate .rebar/erlcinfo files for each project. rebar processes (compiles) each project independently, so it's a bug I have introduced in b690842. In rebar 2.2.0, erl_first_files is also inherited, but as the file does not exist in the sub directory, rebar_erlc_compiler:needs_compile returns false. In rebar 2.3.0 we find the eunit.hrl dependency of first_erl.erl from the cached dependency graph and needs_compile returns true. @roland-karlsson-erlang-solutions-com, that explains the failure on the 2nd rebar compile run. To fix it, rebar_erlc_compiler:erlcinfo_file should use rebar_utils:get_cwd/0 instead of rebar_utils:base_dir/1. There's also the issue of erl_first_files entries not being used when initializing the dep digraph.

Contributor

tuncer commented May 29, 2014

A more descriptive commit message would be:

Fix #249 (erlc regression)

The combination of changes to rebar_erlc_compiler, and the fact
that erl_first_files is inherited, caused a regression. To fix
that, ensure every project uses its own .rebar/erlcinfo. While at
it, fix the issue that erl_first_files entries were not included
when initializing the dep digraph.

Reported-by: Louis-Philippe Gauthier
Reported-by: Roland Karlsson

@tuncer tuncer commented on an outdated diff May 29, 2014

inttest/erlc/rebar.config
@@ -1,6 +1,8 @@
%% -*- mode: erlang;erlang-indent-level: 4;indent-tabs-mode: nil -*-
%% ex: ts=4 sw=4 ft=erlang et
-{erl_first_files, ["first_xrl.erl", "first_yrl.erl"]}.
+{erl_first_files, ["first_xrl.erl", "first_yrl.erl", "src/first_erl.erl"]}.
@tuncer

tuncer May 29, 2014

Contributor

We should fix the first two entries by adding the src/ prefix.

@tuncer tuncer commented on an outdated diff May 29, 2014

src/rebar_erlc_compiler.erl
?DEBUG("erl_opts ~p~n", [ErlOpts]),
%% Support the src_dirs option allowing multiple directories to
%% contain erlang source. This might be used, for example, should
%% 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, ErlFirstFiles)],
+ AllErlFiles = gather_src(SrcDirs, []) ++ MoreSources,
+ {ErlFirstFiles, RestErls} = lists:partition(
+ fun(Source) ->
+ lists:member(Source, ErlFirstFilesConf) orelse
+ lists:member(filename:basename(Source), ErlFirstFilesConf) orelse
+ lists:member(filename:basename(Source, ".erl"), ErlFirstFilesConf)
+ end, AllErlFiles),
@tuncer

tuncer May 29, 2014

Contributor

This seems to extend what kinds of erl_first_files are supported. I don't have a strong opinion on this, but we should only fix the regression(s) and help string plus sample config file in this pull request. Feel free to open a followup pull request with the discarded erl_first_files changes.

@tuncer tuncer commented on an outdated diff May 29, 2014

inttest/erlc/erlc_rt.erl
@@ -53,6 +58,11 @@ run(_Dir) ->
ok = check_beams(true),
ok = check_debug_info(false),
?assertMatch(true, filelib:is_regular(MibResult)),
+
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
@tuncer

tuncer May 29, 2014

Contributor

We should probably add a reference to #249 and/or explain what compiling twice tests.

Contributor

nevar commented May 29, 2014

Fix rebar.config.sample and some other place with incorect define erl_first_files.
Remove extend usage of erl_first_files. Now only fix error.
Make better commit message.

Contributor

tuncer commented May 29, 2014

I think we should include a complete explanation for the regression in erlc_rt.erl.

If I had to write it, I would add the following:

    %% Regression test for https://github.com/rebar/rebar/issues/249
    %%
    %% Root cause: We didn't have per-project .rebar/erlcinfo but just one in
    %% <base_dir>/.rebar/erlcinfo.
    %%
    %% Solution: Ensure every project has its own .rebar/erlcinfo
    %%
    %% For the bug to happen, the following conditions must be met:
    %%
    %% 1. <base_dir>/rebar.config has erl_first_files
    %% 2. one of the 'first' files depends on another file (in this
    %%    case via -include_lib())
    %% 3. a sub project's rebar.config, if any, has no erl_first_files entry
    %%
    %% Now because erl_first_files is retrieved via rebar_config:get_list/3,
    %% base_dir/rebar.config's erl_first_files is inherited, and because we had
    %% a shared <base_dir>/.rebar/erlcinfo instead of one per project, the
    %% cached entry was reused. Next, while compiling the sub project
    %% rebar_erlc_compiler:needs_compile/3 gets a last modification time of
    %% zero for the 'first' file which does not exist inside the sub project.
    %% This, and the fact that it has at least one dependency, makes
    %% needs_compile/3 return 'true'. The root cause is that we didn't have per
    %% project .rebar/erlcinfo. For <base_dir>/.rebar/erlcinfo to be populated,
    %% base_dir has to be compiled at least once. Therefore, after the first
    %% compile any compile processing the sub project will fail because
    %% needs_compile/3 will always return true for the non-existent 'first'
    %% file.

@nevar nevar changed the title from Fix inheritance of *_first_files to Fix #249 (erlc regression) May 30, 2014

Contributor

nevar commented May 30, 2014

Fix commit message.
Make more complete explanation for the regression in erlc_rt.erl.
Fix some formating code.

Contributor

nevar commented May 30, 2014

Make more explanation of work with *_first_file.

Contributor

tuncer commented May 30, 2014

Thanks for including the extra comments.

Contributor

nevar commented May 30, 2014

Need dialyzer check before merged!

Contributor

tuncer commented May 30, 2014

@roland-karlsson-erlang-solutions-com @lpgauth can you verify the fix works for you?

Contributor

tuncer commented May 30, 2014

Need dialyzer check before merged!

It's clean with R16B03 Dialyzer, so if you get the same warnings with 17.x Dialyzer in rebar.git master, it's a different issue unrelated to this patch. Related: #263.

@nevar nevar Fix #249 (erlc regression)
The combination of changes to rebar_erlc_compiler, and the fact
that erl_first_files is inherited, caused a regression. To fix
that, ensure every project uses its own .rebar/erlcinfo. While at
it, fix the issue that erl_first_files entries were not included
when initializing the dep digraph.

Reported-by: Louis-Philippe Gauthier
Reported-by: Roland Karlsson

Thanks: Tuncer Ayaz
49c2564
Contributor

nevar commented May 30, 2014

Fix code formating.

Sorry for not replying. Missed it.

What fork and branch and commit am I supposed to test?

Contributor

nevar commented May 30, 2014

git clone https://github.com/nevar/rebar
cd rebar
git checkout fix_inheritance
make

It seems to work just fine!!!!

The erl files are not being compiled in the deps and the MIB file is not copied to deps/*/ebin any more.

Thank you!

Contributor

tuncer commented Jun 3, 2014

@jaredmorrow @ferd @tsloughter once this is reviewed and merged, we should make a new release.

Contributor

ferd commented Jun 13, 2014

I was reviewing this and I saw that in most of the code snippets, changes of the form:

-{erl_first_files, ["first_xrl.erl", "first_yrl.erl"]}.
+{erl_first_files, ["src/first_xrl.erl", "src/first_yrl.erl"]}.

were taking place.

Is this a change in what is supported? Is this backwards compatible?

Contributor

tuncer commented Jun 13, 2014

@ferd wrote:

I was reviewing this and I saw that in most of the code snippets, changes of the form:

-{erl_first_files, ["first_xrl.erl", "first_yrl.erl"]}.
+{erl_first_files, ["src/first_xrl.erl", "src/first_yrl.erl"]}.

were taking place.

Is this a change in what is supported? Is this backwards compatible?

The code snippet was wrong. For example, first_xrl.erl gets compiled due to being found in src/, but only src/first_xrl.erl allows it to be treated as a first file. The behavior is still the same as it was before.

Member

tsloughter commented Jun 15, 2014

Sounds good to me. Going to merge this. Doing a sweep to see if anything else is ready to merge and then we can cut a release.

@tsloughter tsloughter added a commit that referenced this pull request Jun 15, 2014

@tsloughter tsloughter Merge pull request #285 from nevar/fix_inheritance
Fix #249 (erlc regression)
8a0d8ad

@tsloughter tsloughter merged commit 8a0d8ad into rebar:master Jun 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Contributor

ferd commented Jun 15, 2014

Taking a note here to release rebar 2.3.2 on this, with all the merges from last week. Or might actually need to be 2.4.0 due to the eunit additions merged in last week?

Member

tsloughter commented Jun 15, 2014

Yea, I think 2.4.0 is good. Need to get together a list of changes and a build on R14.

@nevar nevar deleted the unknown repository branch Jun 16, 2014

Contributor

tuncer commented Jun 16, 2014

Further notes:

  • rebar/52: Someone should extend the release generation wiki page to mention slim releases (cc @shino).
  • rebar/289 was actually more than a typo fix. The typo made the function not work as intended.
  • Shouldn't we also document that rebar compile and rebar *-deps are the only commands which are applied recursively by default? In that context it might be worth it to mention {recursive_cmds, []} and -r/--recursive.
  • rebar/171 was backed out as it introduced a rebar eunit regression.
Contributor

ferd commented Jun 18, 2014

The release is out, I haven't had the time to extend documentation, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment