-
Notifications
You must be signed in to change notification settings - Fork 204
[Incremental] Only warn if cannot write prior module dependencies, and print more error info #586
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
[Incremental] Only warn if cannot write prior module dependencies, and print more error info #586
Conversation
This works iff the build record is updated to reflect that we failed. Otherwise we will startup again with bogus priors. |
Yes, I was imagining the case where there was a path error or permission error and there were no priors written. If this were a first-time compile--there had been no priors read--then this failure could be non-fatal. |
Would like to get your thoughts on this version. The idea is to move the build record if the graph write fails. TIA |
@swift-ci please test |
Looks fine for a temporary workaround. Please add a reference to a radar and a note somewhere about what this is working around. |
|
@swift-ci please test |
We got CI blue again. Could you scale this patch back to just printing the error info? |
Not sure I follow. Are you proposing to keep the behavior of having the whole compile fail if writing the priors fails? If so, could you explain the superiority of failing the compile, as opposed to letting it succeed but warning about the loss of incrementality? |
@swift-ci please test |
254e109
to
70bb830
Compare
@swift-ci please test |
70bb830
to
83c0682
Compare
83c0682
to
911febb
Compare
@swift-ci please test |
@swift-ci please test macos platform |
Okay, now I really don't think we need this. Mishal was able to fix this with a sandbox configuration change, and I was able to improve the error message in #594 |
Right. Do we need this change right now? Nope. But, would our code base be better with this change or without it? Here is the argument against it, that I can see: It adds some code and complexity to the driver. Here are the arguments for it:
If you still want to veto this PR, you'd have to: a) be in favor of the inconsistency between build-record errors and priors errors, and b) want to potentially fail a long compile when it is not strictly necessary. If you could choose between two kinds of bug reports, a warning disabling incrementality, vs. a completely failed compile, wouldn't you choose the former? Thinking about it in these broader terms, is there any chance you might agree to land this PR? |
@swift-ci please test linux platform |
@@ -496,23 +496,26 @@ extension IncrementalCompilationState { | |||
// MARK: - Serialization | |||
|
|||
extension IncrementalCompilationState { | |||
@_spi(Testing) public func writeDependencyGraph() { | |||
@_spi(Testing) public func writeDependencyGraph() -> Bool { |
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.
Let’s throw something.
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.
Okey-dokey.
@_spi(Testing) public func write( | ||
to path: VirtualPath, | ||
on fileSystem: FileSystem, | ||
compilerVersion: String | ||
) { | ||
) -> Bool { |
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.
Same here
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.
Sure!
@@ -1055,6 +1052,23 @@ extension Driver { | |||
recordedInputModificationDates: recordedInputModificationDates) | |||
} | |||
|
|||
private func writeIncrementalBuildInformation(_ jobs: [Job]) { | |||
// In case the write fails, don't crash the build. | |||
// A mitigation to rdar://76359678. |
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.
Now it’s a correctness fix, no? We need to ensure that when the write fails, subsequent runs of the driver do not trust the information on disk.
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.
Just nits
@swift-ci please test |
Thanks! |
A mitigation to rdar://76359678.
If the write fails, import incrementality is lost, but it is not a fatal error.
@CodaFi , what do you think?