Add a check for empty kernel files #2387
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Towards #2362 We don't know where the zero byte files are being introduced to the system - if they are coming from the kernel worker try to catch them earlier by checking for when we copy a file with no content. This won't solve the issue but if errors surface through this they should be more clear. - Add a `requireContent` argument to `copyOutput`. In the future we might consider making the default either with or without an output - we don't know of any use cases where we want to allow copying a zero byte file. - Use the new argument from the kernel builder to check earlier whether a kernel file is empty.
jakemac53
reviewed
Jul 30, 2019
| @@ -1,3 +1,7 @@ | |||
| ## 2.4.1 | |||
There was a problem hiding this comment.
nit: leave -dev until we are actually ready to publish (dev dependencies removed) - ideally in the pubspec too
There was a problem hiding this comment.
Will keep that in mind for next time - I plan to publish right away so I won't bother waiting for another round of travis this time.
jakemac53
approved these changes
Jul 30, 2019
dart-bot
pushed a commit
to dart-lang/sdk
that referenced
this pull request
Oct 31, 2019
Problem: We have been having a lot of reports of failures when initializing the incremental compiler (see #38102). These appear to be the result of empty kernel files but we have added other logic to check for that when kernel files are written, which has had zero reports of throwing from users (dart-lang/build#2387). We also use the same mechanism for temp directory management as we did with analyzer, but only see this problem with the switch to kernel, and specifically the incremental compiler. Solution: Add retry logic and see if that fixes the problem. Any time there is a retry log a warning so that we don't just silently do retries all the time. This is a general defensive mechanism to cover a broad spectrum of potential failures with the incremental compiler in the wild. I don't intend to remove it as it isn't harmful, and the warning logs should be enough to encourage issues to be filed if it is happening often. I have no direct reason to believe this will actually solve the particular linked problem other than that we only see this issue when the incremental compiler is enabled. Bug: #39122, #38102 Change-Id: Iaabb4497d6da69684692c1d7b9c030c59ebc6072 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/123001 Commit-Queue: Jake Macdonald <jakemac@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Auto-Submit: Jake Macdonald <jakemac@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards #2362
We don't know where the zero byte files are being introduced to the
system - if they are coming from the kernel worker try to catch them
earlier by checking for when we copy a file with no content. This won't
solve the issue but if errors surface through this they should be more
clear.
requireContentargument tocopyOutput. In the future wemight consider making the default either with or without an output -
we don't know of any use cases where we want to allow copying a zero
byte file.
a kernel file is empty.