-
Notifications
You must be signed in to change notification settings - Fork 519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix DAG resolving for recursive directories #2535
Conversation
0b5be07
to
e956803
Compare
This fixes erlang#2534 While fixing the bug reported above, as explained in erlang#2534 (comment) I found out that the DAG preparation took in include paths that were explicit, and did not resolve them properly (and therefore silently failed to track updates). On the other hand, the compiler worked fine, which highlighted a difference between options passed to EPP via the compiler, and those we pass internally when building the DAG. I fixed this, which in turn caused problems with test cases for recursive parse transforms; the reason being that modules were being resolved as source files under `_build/`, which turned out to be due to the EPP resolver we wrote that would go recursive in all directories to find files. So when the top-level directory for an app was passed in (which is needed for include files in legacy cases), it accidentally would find build modules before the rest. This was fixed by removing recursivity in EPP, which in turn broke the behaviour recursive lookup in the DAG; this required going back to the `rebar_compiler_erl` module's paths and sending in explicit lookup paths for includes (which are also in a set used for behaviours and parse transforms!) So here we are: 1. Take the recursive lookup out of EPP (which is used only for erl files anyway) 2. Move the path expansion around that to be done in the compiler behaviour's context function instead since it's required for include files as well (EPP won't cover these) 3. Ensure that path expansion in the context function also respects the rebar.config recursion options for the erlang compiler, which will prevent potential clashes around subdirectories with conflicting (or optionally-built) files 4. Adding a test for the new case This is one interesting case of tests partially saving our asses, once more. They're slow but they're good.
e956803
to
8133c24
Compare
Src <- rebar_dir:all_src_dirs(RebarOpts, ["src"], []) | ||
]) ++ | ||
%% top-level dir for legacy stuff | ||
[OutDir], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviews' sake: I got the list for these paths from line 242-246. Since we were passing them to the compiler, they had to be right already.
I confirm this fixes #2534. Thank you! |
found no breakages so far. Thanks! |
well there was one but we got it quick enough and cut a patch release: #2540 |
|
That list accumulator is just the pending queue of parallel compilation tasks. There's as many workers as there are either files or schedulers. All the tasks then get enqueued in the coordinator. I guess the only way we could fix this is to make a buffered synchronous pool that has a max queue depth (say 10x the worker count) after which, calls hang. The list of results are however still going to be queued up and accumulated. Is the problem really the accumulation of tasks on line 198, or do you think the accumulation of results on 237 is also agoing to be a problem after that? |
Yeah what I'm wondering here is whether there's even enough memory to carry the list of all directories and options in memory. I can imagine only tricky mechanisms to fix this:
Ultimately, it's going to be really hard to do any of this without an actual app to work on because without access to the build, I can't even profile or look at anything on my own. |
https://github.com/MYStill/rebar_indirs_test.git |
This fixes #2534
While fixing the bug reported above, as explained in #2534 (comment)
I found out that the DAG preparation took in include paths that were
explicit, and did not resolve them properly (and therefore silently
failed to track updates). On the other hand, the compiler worked fine,
which highlighted a difference between options passed to EPP via the
compiler, and those we pass internally when building the DAG.
I fixed this, which in turn caused problems with test cases for
recursive parse transforms; the reason being that modules were being
resolved as source files under
_build/
, which turned out to be due tothe EPP resolver we wrote that would go recursive in all directories to
find files. So when the top-level directory for an app was passed in
(which is needed for include files in legacy cases), it accidentally
would find build modules before the rest.
This was fixed by removing recursivity in EPP, which in turn broke the
behaviour recursive lookup in the DAG; this required going back to the
rebar_compiler_erl
module's paths and sending in explicit lookup pathsfor includes (which are also in a set used for behaviours and parse
transforms!)
So here we are:
files anyway)
behaviour's context function instead since it's required for include
files as well (EPP won't cover these)
rebar.config recursion options for the erlang compiler, which will
prevent potential clashes around subdirectories with conflicting (or
optionally-built) files
This is one interesting case of tests partially saving our asses, once
more. They're slow but they're good.
CC @max-au since this touches fun code y'all needed, but I expect no breakage.