Change formatter to attempt to acquire a build lock #391
Conversation
mattbaker
force-pushed
the
formatter-lock-acquire
branch
from
c654bbf
to
47c2bd5
Compare
| defp can_format?(file_uri, project_dir) do | ||
| file_path = file_uri |> SourceFile.path_from_uri() |> Path.absname() | ||
| def try_format(func) do | ||
| if Mix.Project.umbrella?() do |
There was a problem hiding this comment.
Why not lock non-umbrella also?
There was a problem hiding this comment.
Good question. From what I can understand the bug is only happening with umbrella apps. It sounds like the current working directory changes from app to app as compilation occurs. If the cwd is in a different application than the file being formatted there's an error trying to get the formatter.exs.
So I wanted to avoid acquiring a lock if we don't have to, my thinking is the more we do that in the code base the higher the odds of contention when it's time for a new build.
What do you think?
There was a problem hiding this comment.
The error logs indicate that it changes also when compiling deps in regular apps (see #252 (comment), #381 (comment))
There was a problem hiding this comment.
Ah I see! I missed the deps part, thanks for pointing it out. Sounds like we'll need to do this for all projects then as you suggest.
|
I talked with @benwilson512 about projects with long recompilation* times, where this would just end up failing for different reasons (it'd time out). I'm not so sure about this approach anymore. As I see it:
If that's true, then:
I think the real answer is to invest in diving deep into Mix.Tasks.Format to see if we can achieve the same thing it does with respect to getting proper formatting options without considering the current working directory. If we can, we'll bypass all this nonsense altogether. I'm going to close for now. We can always return to this as an option but I'm hoping we won't need it. * When I say "recompilation time" I'm thinking of the time it takes, on average, to recompile the project after changing a single file. |
Potential approach to fixing #252. I hate to bring a global lock into the picture again, but I think the strange race conditions with
File.cwdare out of our hands since Elixir itself is using the current working directory to find the.formatter.exsconfigs.This PR attempts to acquire a build lock as a means of delaying formatting until compilation completes. It makes three retries and Erlang will use a graduated delay between them of 250ms, 500ms, and 1000ms.
I hammered changes as fast as I could (at a rate that was truly unrealistic) and on my project/machine I found this improves things immensely. I only tested on an umbrella project of decent size.
From what I can see this makes things better, even if they're not perfect. It'd be good to see how this behaves for others.
I am by no means tied to this solution, I just wanted to get something out there. I won't be hurt if you close it :)