Fix --watch not recompiling sources modified during a compilation - #2850
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
16c820a to
b07130d
Compare
There was a problem hiding this comment.
Thanks for the submission!
Consider instead setting the modification time of the generated file to the time the compilation began. That way, checking the mtime of the source versus the destination will work even across separate runs of the executable, such as when using --update.
| }); | ||
|
|
||
| // Regression test for #2849. | ||
| test("when a dependency is modified during a compilation", () async { |
There was a problem hiding this comment.
I think it's worth having a parallel test for --update as well.
There was a problem hiding this comment.
I also thought about modifying the target's date, but I thought that this way it is a smaller change. Implementing the suggestion and adding the additional test.
Sources modified while a compilation is in progress are older than the output that compilation writes when it finishes, so --watch and --update wrongly consider them up to date and skip recompilation. Set the output's modification time to when compilation started so such sources compare as newer, including across restarts.
b07130d to
baf77a2
Compare
|
google/dart_cli_pkg#251 and #2859 should fix the CI errors here. |
|
Thanks for the contribution! |
Fixes #2849.
--watchdecides whether a stylesheet needs recompiling by comparing the modification time of the source and its dependencies against that of the CSS file it produced (compileStylesheet'sifModified). When a source is modified while a compilation is already in progress that comparison is wrong: the CSS file is written when the compilation finishes, so it is newer than the modification that arrived in the meantime. The source looks up to date, no recompilation happens, and the CSS on disk stays one generation behind, silently. It also survives a restart, since--updatemakes the same comparison.This threads the time at which the previous compilation started through to
compileStylesheetand uses it in place of the destination's modification time when it is earlier. A source that changed during a compilation is newer than that timestamp, so it is recompiled. The bound is always the earlier of the two, so this can only ever cause more compilations than before, never fewer, and only for sources whose modification time falls between the start of the last compilation and the output it wrote. Startup, one-shot compilations and--updateare unaffected, since they pass no timestamp.The regression test is in the existing
recompiles a watched filegroup. It needs a stylesheet that takes several seconds to compile, since the bug only exists while a compilation is in flight; the delay between the two writes is longer than the polling interval so that the two modifications aren't batched into a single recompilation under--poll. It fails onmainin both the--polland non---pollvariants and passes with this change; the rest oftest/cli/dart/watch_test.dartandupdate_test.dart(114 tests) pass with no retries.One related problem this doesn't address: modifications that land during the initial compilation are dropped outright rather than skipped, because
MultiDirWatcher'sStreamGrouphas no subscriber untilwatch()starts iterating it, which happens after that compilation finishes.I wanted to provide a minimal fix to the more common issue we face, and fixing that means attaching the subscription earlier, which changes startup behaviour, so it seemed better as a separate change. But if you want I can include that one as well.