[Incremental] stop if no -driver-continue-building-after-errors #387
Conversation
| @swift-ci please test |
| I’ll take a look at this tomorrow! Was a few weeks ago when I posted that PR, but I do remember not being completely satisfied with it. |
Thank you, @cltnschlosser . I'll look forward to your thoughts. |
| allJobs: [Job], | ||
| forceResponseFiles: Bool | ||
| ) throws { | ||
| let continueBuildingAfterErrors = parsedOptions.contains(.continueBuildingAfterErrors) |
There was a problem hiding this comment.
You'll see in my PR I made a function for computing this and copied over a comment from the c++ driver. ContineuBuildingAfterErrors is set to true when compiling in batch mode.
There was a problem hiding this comment.
Yes. Good idea! Since I'm not storing this value as a driver instance variable, the function doesn't have to be static, though the test is a little more complicated. The test will require my other PR to land to support -driver-use-frontend-path. But I do like the idea of the test.
There was a problem hiding this comment.
Yeah, I think I stored it specifically for test, but it's also not that important of a test.
There was a problem hiding this comment.
Thanks! I know at least one legacy will fail of the driver doesn't cancel when it needs to, but I like the idea of testing what your test does, so I put one in.
| public enum DriverExecutorWorkload { | ||
| case all([Job]) | ||
| case incremental(IncrementalCompilationState) | ||
| public struct DriverExecutorWorkload { |
There was a problem hiding this comment.
I do like this better, from a SPM compatibility perspective.
| private(set) var executeAllJobsTaskBuildEngine: LLTaskBuildEngine? = nil | ||
| | ||
| /// If a job fails, the driver needs to stop running jobs. | ||
| private(set) var shouldStopRunningJobs = false |
There was a problem hiding this comment.
naming: shouldStopRunningNewJobs, because it doesn't stop any actively running jobs, just scheduled ones.
There was a problem hiding this comment.
How about isBuildCancelled ? I like the specificity of including the object.
There was a problem hiding this comment.
That name seems fine :)
| if context.shouldStopRunningJobs { | ||
| engine.taskIsComplete(DriverBuildValue.jobExecution(success: false)) | ||
| return | ||
| } |
There was a problem hiding this comment.
You mentioned you liked my implementation better here. I'm fine with either. They should be logically equivalent, I was stopping before the call to executeJob and you're stopping near the beginning. I do think this should either be moved outside of executeJob or moved all the way to the top, because skips any job startup work that might occur. Environment variable merging in this case (Even though that's probably near 0 overhead anyway).
There was a problem hiding this comment.
Thank you. Yes, I've gone with your design, tightened up a bit.
| case (.terminated(let code), false) where code != EXIT_SUCCESS: | ||
| shouldStopRunningJobs = true | ||
| #if !os(Windows) | ||
| case (.signalled, _): |
There was a problem hiding this comment.
I'm guessing this is more correct than in my solution where I used result.success as the flag and never considered if the failure was due to a signal or not.
| @cltnschlosser Thank you for your thoughtful review. I'll work on incorporating your suggestions. |
Yes! I'll bring over that test. |
davidungar changed the title | @cltnschlosser If this looks good at this point, it would be productive to have your official approval. |
| guard allInputsSucceeded, !context.isBuildCancelled else { | ||
| return engine.taskIsComplete(DriverBuildValue.jobExecution(success: false)) | ||
| } | ||
| |
There was a problem hiding this comment.
Oh @davidungar Looks like you are missing https://github.com/apple/swift-driver/pull/371/files/a00c657b31853c25f2544f0cef6d836f517c2179#diff-b2d6d242100a7556a15175d3576106d4a413a2b7b5ffa3bbb52f74210b7408e3R454-R456 Right below this. It's needed to stop jobs which have been scheduled(inputs are available) but haven't started yet(due to throttling the number of concurrent jobs). This is why you don't see the test failure I was talking about in my recent comment on #371
There was a problem hiding this comment.
Ah, yes, I see. Given the issue, I think it means the other way is better, moving the test to the top of executeJob.
Thanks a lot for pointing it out!
There was a problem hiding this comment.
Yeah, I had it in both spots, inside the queue and before it, but inside only should be fine.
| @swift-ci please test |
Great point! And thanks for the approval. |
| Fixes rdar://71869814 |
| @swift-ci please test |
2 participants
Without being aware of @cltnschlosser 's similar PR ( https://github.com/apple/swift-driver/pull/371/files ), I wrote this code last week. I suspect the two PRs do the same thing. So, not being sure how to proceed, I thought I would put this up and we might review each other's work and come up with a nice synthesis, (hence the "DNM").
Rather than disturb the interface that goes across to swiftpm, I included the extra parameter in the workload structure.
BTW, some version of this functionality is needed for the legacy lit tests.
Have incorporated several great ideas from @cltnschlosser 's PR.