-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
Acquire a lock on the out dir in order to run tasks / commands #3599
Conversation
This results in things like mill-global-lock.movWe see the lock in action when the 3 sessions start (only one compiles the build while the other wait for the lock), and when they run the long-running command, with only one session running the command at a time. |
Note that the lock is only for write operations, not those that just read things. In my experience, issues happen mainly when workers intend to run tasks and write things at the same time. But a shared lock (allowing several workers to hold it while they read things) could be added too. |
(This includes #3530) |
For a first pass, let's place the lock around the entire As an escape hatch, we should add a flag Lastly, let's make the |
Isn't a lock around the whole of |
61cdc64
to
1400454
Compare
The last push makes the lock slightly broader compared to the previous state of this PR. The lock was around two things: Note that BSP is currently broken when the lock is enabled: the BSP server runs via the |
19ccd1f
to
aa2c64a
Compare
If I understand correctly, putting the lock around For BSP support, how about we disable the lock for BSP entirely, with the expectation that once #3530 lands we can move the BSP code path to use |
aa2c64a
to
5ad10ff
Compare
5ad10ff
to
c4df445
Compare
c987d9e
to
12ee4d7
Compare
Rebased on top of #3608, to show that this doesn't break the BSP server (and commands asking for an |
It should actually compile Scala code in most cases, AFAIK. In Metals at least. Compilation is needed to get the semanticdb files of each source, that Metals uses to provide code navigation in particular. |
@alexarchambault got it. For the current status of the PR, I assume you ended up disabling the BSP server as discussed? |
12ee4d7
to
4a4a041
Compare
There used to be a workaround so that the BSP server can use the lock too, but it breaks bin compat. I moved to hack to #3683 for later. |
e8ba056
to
12cdab9
Compare
12cdab9
to
b5d9c75
Compare
Later commits add more resources to the Using.resource(s) call
Mainly meant to be used in integration tests
d7c0a73
to
b41fe77
Compare
This looks good to me, @alexarchambault if you're done with I'll merge |
I'm done, yes |
First cut at #3519
There's no tests yet, but it works fine in manual tests.