-
Notifications
You must be signed in to change notification settings - Fork 202
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
Remove betasty directory on successful compilation backgroundTasks #2410
base: main
Are you sure you want to change the base?
Conversation
Looks like this indeed does help in case of the repository I sent your way, but the Scala compiler is failing with cryptic stuff like:
any idea why? |
This is still pretty unfinished, however I am almost done with the working version. After just this change the incremental compiler interactions can be incorrect, so I also implemented a thing where we properly restart the compilation after the first unsuccessful one (this was also incorrect before, but it was not really noticable, due to us still having betasty we could use from the previous compilation - I'll try to explain this specific issue and the fix later, after I push the code here). Also one race condition I noticed caused a crash from one of the reports we got this week, so that also should be fixed. With that said none of these fixes fix the behavior in the compiler unfortunately. The main issue there seems to be with the java source files, which I am pretty sure we do not handle yet in best effort compilation :(. When I tested the best effort compilation in the compiler I ended up running into a compiler crash and errors related to missing java symbols - now I can't be 100% sure whether the crash was also caused by the java files, but the compiler is not always able to recover gracefully from the missing symbols, so that might be it... I am sure there is some roundabout way to work with those even without updating the compiler, however it might take more time (I'll work on it next week). |
Wouldn't that be caused by the problems with compilation? So if the Scala source do not compile and produce classfiles, the Java sources which use them will not be able to compile either. Especially that the compilation mode in the compiler is "mixed". I would say the Java compilation issues are not relevant to the actual problem. |
Also, obviously it should always recover after fixing the issues, no idea why it does not do that in the compiler codebase, so I'll probably start investigating with this. EDIT: I wrote this before I saw the comment above, but maybe it answers the doubt there? The java stuff should not matter for successful compilations, it only produces additional fake errors/other issues when the compilation is failing. |
(I pushed it but I still want to recheck it/ clean it up, so I am leaving this as a draft for now) |
In the meantime let me quickly explain why we do those recompilations here now:
Lets say the first Utils compilation would give us: |
5936fa2
to
9b1b8f6
Compare
6a92110
to
615db68
Compare
case t: Throwable => | ||
t.printStackTrace() | ||
val sw = new StringWriter() | ||
t.printStackTrace(new PrintWriter(sw)) | ||
logger.info(sw.toString()) | ||
val backgroundTasks = | ||
toBackgroundTasks(backgroundTasksForFailedCompilation.toList) | ||
Result.Failed(Nil, Some(t), elapsed, backgroundTasks, None) | ||
val failedProblems = findFailedProblems(reporter, None) | ||
Result.Failed(failedProblems, Some(t), elapsed, backgroundTasks, None) | ||
} |
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.
Changes here are not strictly related to best effort, but not returning failedProblems in the case of compiler crash caused those annoying leftover errors in the problem tab in the compiler codebase (I can't see why it wouldn't behave the same way even for crashes unrelated to best effort). I also figured that it might be useful to also log the stacktraces of those crashes, although maybe a different log level would be better (debug?).
About the compiler crash itself (as seen while using best effort in the compiler codebase), the fix for that will have to be in the compiler (I already have a preliminary fix working there) |
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.
Thanks for working on this!
@@ -39,6 +39,11 @@ import bloop.util.BestEffortUtils.BestEffortProducts | |||
import monix.execution.CancelableFuture | |||
import monix.reactive.MulticastStrategy | |||
import monix.reactive.Observable | |||
import sbt.internal.inc.BloopComponentCompiler |
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.
Any chance to run organize imports?
@@ -555,9 +578,11 @@ object Compiler { | |||
) | |||
} | |||
.flatMap(clientClassesObserver.nextAnalysis) | |||
Task | |||
|
|||
deleteBestEffortDir() *> Task |
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.
Isn't this already added below? Or do you need to run it first? In that case we can probably remove it from below. Ideally though, we would remove it together for it to be more efficient.
Also, is there a non symbolic way of defining it? Would that not be just a flatMap?
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.
Yes, I need to tun this before updateClientState
(I missed the repeat below, thank you for the catch!), but I'll try rearranging the tasks so that the other ones can be run concurrently
@@ -710,9 +738,13 @@ object Compiler { | |||
Result.Failed(failedProblems, None, elapsed, backgroundTasks, None) | |||
case t: Throwable => | |||
t.printStackTrace() | |||
val sw = new StringWriter() | |||
t.printStackTrace(new PrintWriter(sw)) | |||
logger.info(sw.toString()) |
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.
logger.info(sw.toString()) | |
logger.error(sw.toString()) |
?
// Delete everything outside of betasty and semanticdb | ||
val deletedCompileProducts = | ||
BloopClassFileManager.supportedCompileProducts.filter(_ != ".betasty") :+ ".class" | ||
Files | ||
.walk(clientClassesDir.underlying) | ||
.filter(path => Files.isRegularFile(path)) | ||
.filter(path => if (Files.exists(path)) Files.isRegularFile(path) else false) |
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.
.filter(path => if (Files.exists(path)) Files.isRegularFile(path) else false) | |
.filter(path => Files.exists(path) && Files.isRegularFile(path)) |
Task | ||
.gatherUnordered(List(firstTask, secondTask)) | ||
.map(_ => ()) | ||
.forEach(path => if (Files.exists(path)) Files.delete(path)) |
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.
.forEach(path => if (Files.exists(path)) Files.delete(path)) | |
.forEach(path => Files.delete(path) |
Isn't this checked above? We could probably join the 2 filters and a foreach now
.list(compileOut.internalReadOnlyClassesDir) | ||
.length == 0) | ||
) { | ||
if (compileOut.analysisOut.exists) BloopPaths.delete(compileOut.analysisOut) |
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.
deleting analysis should probably be enough as that will result in a full recompilation. I think we do that for clean
Also what is the exact case we want to remove everything?
) | ||
val recompile = | ||
if ( | ||
!previousWasBestEffort && !(compileOut.internalReadOnlyClassesDir.exists && BloopPaths |
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.
Maybe you could extract the part after &&
and name it to explain what is actually being checked or why?
) if recompile => | ||
// we restart the compilation, starting from scratch (without any previous artifacts) | ||
inputs.reporter.reset() | ||
val foundSrcs = getAllSourceInputs(project) |
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.
This should not be needed here. We already calculate sources in other places. Maybe if we just remove analysis it will not be needed?
Fix to scalameta/metals#6628
Usually it was fine to always put betasty files at the end of the classpath, but in the reproduction for some reason despite the code being correct, those files were still being used, which resulted in an incoherent state in bloop (we obtained only betasty files on what was being considered non-best effort compilation). This lead to trying to apply incremental compilation there, producing singular betasty files, and losing information about symbols from other files, producing errors.
The fix is very simple, but so far I have been unable to properly add any tests for it, as the
backgroundTasks
do not seem to be run in the tests suites.