Skip to content

[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

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

davidungar
Copy link
Contributor

@davidungar davidungar commented Apr 8, 2021

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?

@davidungar davidungar requested a review from CodaFi April 8, 2021 04:59
@CodaFi
Copy link
Contributor

CodaFi commented Apr 8, 2021

This works iff the build record is updated to reflect that we failed. Otherwise we will startup again with bogus priors.

@davidungar
Copy link
Contributor Author

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.

@davidungar
Copy link
Contributor Author

This works iff the build record is updated to reflect that we failed. Otherwise we will startup again with bogus priors.

Would like to get your thoughts on this version. The idea is to move the build record if the graph write fails. TIA

@davidungar
Copy link
Contributor Author

@swift-ci please test

@CodaFi
Copy link
Contributor

CodaFi commented Apr 8, 2021

Looks fine for a temporary workaround. Please add a reference to a radar and a note somewhere about what this is working around.

@davidungar
Copy link
Contributor Author

Looks fine for a temporary workaround. Please add a reference to a radar and a note somewhere about what this is working around.
There's a reference to a radar here, in the PR, so I'm guessing you mean a reference to the radar in the code, as well as a note. Will do.

@davidungar
Copy link
Contributor Author

@swift-ci please test

@CodaFi
Copy link
Contributor

CodaFi commented Apr 11, 2021

We got CI blue again. Could you scale this patch back to just printing the error info?

@davidungar
Copy link
Contributor Author

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?

@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar davidungar force-pushed the only-warn-on-prior-write-error branch from 254e109 to 70bb830 Compare April 12, 2021 17:45
@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar davidungar force-pushed the only-warn-on-prior-write-error branch from 70bb830 to 83c0682 Compare April 12, 2021 22:19
@davidungar davidungar force-pushed the only-warn-on-prior-write-error branch from 83c0682 to 911febb Compare April 12, 2021 22:25
@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar
Copy link
Contributor Author

@swift-ci please test macos platform

@CodaFi
Copy link
Contributor

CodaFi commented Apr 13, 2021

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

@davidungar
Copy link
Contributor Author

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.
Congrats on your work to track down this error!

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:

  1. It brings the priors error handling into consistency with the build-record-writing error handling. In other words, when the build-record write fails, the driver does not fail the whole compile, it just emits a warning. (See the writeBuildRecord function.)

  2. If, in the future, something causes the write to fail, wouldn't it be better to allow the compilation to succeed? The compilation might take a very long time, as we've heard from some users. Do you really want to have them waste that time on an unnecessary failure?

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?

@davidungar
Copy link
Contributor Author

@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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s throw something.

Copy link
Contributor Author

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

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.
Copy link
Contributor

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.

Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits

@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar
Copy link
Contributor Author

Just nits

Thanks!

@davidungar davidungar merged commit 8a0dcbf into swiftlang:main Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants