Handle -continue-building-after-errors and add mechanism for stopping build on failure #371
Conversation
… build on failure
owenv
left a comment
owenv
left a comment
There was a problem hiding this comment.
This approach seems like it should work fine as far as I can tell, so these changes LGTM once one usage of _spi is temporarily dropped and we update SPM's executor to match the changed protocol requirement.
| delegate: JobExecutionDelegate, | ||
| numParallelJobs: Int, | ||
| continueBuildingAfterErrors: Bool, | ||
| forceResponseFiles: Bool, |
There was a problem hiding this comment.
This will require a corresponding update to SPM's executor
There was a problem hiding this comment.
That's why in my version, I put this bit in the workload parameter. Seemed like one could defend that the decision to run jobs after an error was a workload parameter.
| // FIXME: It's unfortunate we diagnose this twice, once for each job which uses the input. | ||
| verifier.expect(.error("input file '\(other.description)' was modified during the build")) |
There was a problem hiding this comment.
Is this still needed if -driver-continue-building... is set? Or can we 86 it?
There was a problem hiding this comment.
Nope not needed. It was getting emitted twice because the second job was running even though the first one failed.
There was a problem hiding this comment.
Oh @davidungar I missunderstood your question. Yes, if continue building after errors is set, then we would get 2 errors, but since it's not and the driver defaults to 1 job at a time, it only outputs once.
Is this not causing a test failure in #387 ?
There was a problem hiding this comment.
Thank you, @cltnschlosser . Apparently not causing a failure there. As soon as I've gotten my PRs in, I'll push forward on the legacy tests and see what pops up.
| @_spi(Testing) public let numParallelJobs: Int? | ||
|
|
||
| /// Whether jobs should continue to be executed after a failure. | ||
| @_spi(Testing) public let continueBuildingAfterErrors: Bool |
There was a problem hiding this comment.
We'll need to drop te @_spi here until the related build issues get sorted out
There was a problem hiding this comment.
It's being used a lot in this file. Not sure what the issue is, but it's still being used in a lot of places.
There was a problem hiding this comment.
I have a set of changes I have not pushed in a PR (yet) that comments out all the spi's so we can use the older compilers. Not sure what to do about this issue.
| override func inputsAvailable(_ engine: LLTaskBuildEngine) { | ||
| // Return early any of the input failed. | ||
| guard allInputsSucceeded else { | ||
| guard allInputsSucceeded, !context.isCancelled else { | ||
| return engine.taskIsComplete(DriverBuildValue.jobExecution(success: false)) | ||
| } | ||
|
|
||
| context.jobQueue.addOperation { | ||
| guard !self.context.isCancelled else { | ||
| return engine.taskIsComplete(DriverBuildValue.jobExecution(success: false)) | ||
| } | ||
| self.executeJob(engine) | ||
| } | ||
| } |
There was a problem hiding this comment.
If these changes work, this part of the PR is more elegant than mine.
|
@cltnschlosser What do you think? I like the workload integration in my PR, but I like the way your code stops the build better in your PR. What are your thoughts? |
|
Closing this in favor of #387 |
3 participants
Fixes Driver/continue-building-after-errors.swift
Long term we probably want to add more to llbuildSwift, so that we can cancel the build there instead of going through all the jobs. We could suspend the jobQueue so it doesn't start anymore. It also feels like llbuild should have a mechanism for throttling parallel builds, so maybe with that the queue could be replaced.