-
Notifications
You must be signed in to change notification settings - Fork 521
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
Refactor env path handling and fix some bugs related to it #1907
Conversation
62eb63d
to
9e06e18
Compare
Cleaned up commits in a more logical unit. Probably ready to merge after reviews and confirmation it does not break things (and possibly fixes them!) for @tothlac and @eproxus since you both are probably our biggest plugin users. I tried it on my own rebar3 proper and found no problems. Probably worth taking some time to do profiling as well. |
A quick check on run times for the test suite reveals that this patch set is almost 40% slower than what we had before. There's obviously a need to rework the stuff taking place in this PR so that it is faster, or gets similar paths set at a lower runtime cost. |
Move path management out of rebar_utils manual code path function handling (which we leave there for backwards compat), and centralize them to allow easier coordination of paths between plugins and deps. On top of path handling, do a check of loaded modules to only purge and reload those that actually need it done in order to prevent all kinds of weird interaction and accidental purge kills. It also allows the possible cohabitation of both at once, with a "in case of conflict pick X" as a policy Changing path handling in providers also highlighted a bunch of bugs in some tests and appears to fix some in other providers, specifically around plugins.
Also handle some formatting
Some finishing touch to that code
- Only set paths that need to be put as a priority - Clean up paths before leaving API mode The first point accounted for some performance cost, but the latter one explains the 40% overhead in test runs: since rebar3 calls rebar3 a lot with a bunch of fake apps, and that the new mechanism for path handling by default does not _remove_ paths, it just _orders_ them, we would end up in a situation where as the tests ran, more and more fake paths would get added to the VM. By the time the run was over, all path handling would take longer since more paths needed filtering every time. By resetting paths at the end of an API run, we prevent a given 'project' from polluting another one's runtime and performance once the API successfully returns.
b2e4bcb
to
dada4e3
Compare
I found the source of the performance regression and fixed it:
The first point accounted for some performance cost, but nothing major. By the time the run was over, all path handling would take longer since This likely makes rebar3 a bit more usable as an API, and such path leak |
Works fine with the GRiSP plug-in:
Also runs fine with |
Got the approval from @tothlac over Slack as well. Merging! |
This updates and replaces how most of the path handling in rebar3 is being done. Instead of adding paths here and there mostly at random, this attempts to take a more systematic approach of tracking currently-used modules and replacing them explicitly, while caring for plugins or deps to be in proper order.
Implements the fixes proposed at #1899
Also fixes a path bug in the compiler.