Skip to content
This repository has been archived by the owner on May 12, 2018. It is now read-only.

rebar doesn't respect the order of erl_first_files given in the rebar.conf file #445

Merged
merged 2 commits into from
Feb 2, 2015

Conversation

norton
Copy link
Contributor

@norton norton commented Jan 26, 2015

The first commit is a fix for this issue. The second commit is a test for this issue. The second commit will fail if cherry-picked to the current master branch.

@norton
Copy link
Contributor Author

norton commented Jan 26, 2015

This merge request is for issue #429.

@ferd
Copy link
Contributor

ferd commented Jan 26, 2015

So just so I'm clear, was the issue that lists:partition does not keep the expected order of files?

@norton
Copy link
Contributor Author

norton commented Jan 26, 2015

No. The root issue is that AllErlFiles is not sorted to respect the order of ErlFirstFiles.

On Jan 26, 2015, at 09:18, Fred Hebert notifications@github.com wrote:

So just so I'm clear, was the issue that lists:partition does not keep the expected order of files?


Reply to this email directly or view it on GitHub.

@ghost
Copy link

ghost commented Jan 26, 2015

The root issue is that AllErlFiles is not sorted to respect the order of ErlFirstFiles.

So, should we sort in gather_src/2, then?

@norton
Copy link
Contributor Author

norton commented Jan 26, 2015

I don't think so. The fix made by Slava is fine by me.

On Jan 26, 2015, at 16:09, Tuncer Ayaz notifications@github.com wrote:

The root issue is that AllErlFiles is not sorted to respect the order of ErlFirstFiles.

So, should we sort in gather_src/2, then?


Reply to this email directly or view it on GitHub.

@nevar
Copy link
Contributor

nevar commented Jan 27, 2015

After we make lists:partition we loose order in ErlFirstFilesConf. We use order from file system and that is where bug came. We loose dependency order from user defined order.

@ferd
Copy link
Contributor

ferd commented Jan 27, 2015

Ok, that makes sense to me. @Tuncer anything else you can think of before I merge this in later today?

@ghost
Copy link

ghost commented Jan 27, 2015

We can merge it as a fix, even though I don't like the extra list traversals too much.

@norton
Copy link
Contributor Author

norton commented Jan 31, 2015

Just checking ... Is anything else required for this pull request?

@nevar
Copy link
Contributor

nevar commented Jan 31, 2015

@norton If you complete remove erl_first_files from config does rebar compile file in right order?

My main issue with this that erl_first_files use as is and don't order by rebar inner algorithm. So we need remove don't exists file and hope that user use correct order for this file.

I think that better ignore erl_first_files and use rebar algorithm for order files. But I not sure that all use case didn't use erl_first_files for some interesting behavior or rebar correct parse and analyze all posible erl file.

We can deprecate erl_first_files and lay on rebar order algorithm and improve it. Or keep erl_first_files and have more and more code for that options.

So my main question: Why we need erl_first_files if rebar can order erl file by himself?

@norton
Copy link
Contributor Author

norton commented Jan 31, 2015

@nevar fixing the behavior of erl_first_files and deprecating el_first_files are different goals. My goal for this issue is simply to fix the erl_first_files regression reported in #429 and not introduce any new harm.

@ghost
Copy link

ghost commented Jan 31, 2015

I've been thinking of ways to avoid the extra scanning, and didn't come up with a good solution. In order to fix the issue, we should get this in and try to optimize it in another patch.

@ghost
Copy link

ghost commented Jan 31, 2015

To deprecate or remove erl_first_files, we'd have to be sure there are no valid cases where erl_first_files is used for something not covered by dependency resolution.

@norton
Copy link
Contributor Author

norton commented Jan 31, 2015

+1

On Jan 31, 2015, at 12:44, Tuncer Ayaz notifications@github.com wrote:

To deprecate or remove erl_first_files, we'd have to be sure there are no valid cases where erl_first_files is used for something not covered by dependency resolution.


Reply to this email directly or view it on GitHub.

@norton
Copy link
Contributor Author

norton commented Jan 31, 2015

+1

On Jan 31, 2015, at 12:40, Tuncer Ayaz notifications@github.com wrote:

I've been thinking of ways to avoid the extra scanning, and didn't come up with a good solution. In order to fix the issue, we should get this in and try to optimize it in another patch.


Reply to this email directly or view it on GitHub.

ferd added a commit that referenced this pull request Feb 2, 2015
rebar doesn't respect the order of erl_first_files given in the rebar.conf file
@ferd ferd merged commit c051530 into rebar:master Feb 2, 2015
@norton norton deleted the norton-erlc-order branch February 7, 2015 03:45
cmkarlsson added a commit to cmkarlsson/rebar3 that referenced this pull request Feb 13, 2015
Match erl_first_files properly against all files needing
compilation and make sure the order of erl_first_files as specified in
rebar.config is preserved (rebar/rebar#445)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants