Fix response file support for batch jobs. #24008
Conversation
lib/Driver/ToolChain.cpp
Outdated
| @@ -62,6 +62,27 @@ ToolChain::JobContext::getTemporaryFilePath(const llvm::Twine &name, | |||
| return C.getArgs().MakeArgString(buffer.str()); | |||
| } | |||
|
|
|||
| ToolChain::ResponseFileInfo ToolChain::getResponseFileInfoIfNeeded( | |||
There was a problem hiding this comment.
Why not return an Optional? Wherever ResponseFileInfo is used and the pointers might be null, you could use the optional type. That would ensure that the next maintainer doesn't forget the null checks.
There was a problem hiding this comment.
Good call. I originally did that, but it didn't seem worth it because I didn't also change the Job constructor signature to use it, so the call sites actually became more complicated. After passing the optional throughout Job/BatchJob as well, it came out much cleaner.
include/swift/Driver/ToolChain.h
Outdated
| struct ResponseFileInfo { | ||
| /// The path to the response file that a job should use, or null if no | ||
| /// response file is needed. | ||
| const char *responseFilePath; |
There was a problem hiding this comment.
I'm not sure about this, but would std:string work better for these?
There was a problem hiding this comment.
(Addressed in the other comment about std::string.)
allevato
force-pushed
the
batch-mode-response-files
branch
from
ac19ccc to
6d72230
Compare
|
Is there anything else to be done here? Could someone kick off CI please? |
|
@davidungar Friendly ping |
|
Getting the CI, and I'll check in with David today. @swift-ci Please test |
|
@allevato Looks fine! Sorry I missed your ping earlier. @jrose-apple Thanks for the ping! |
|
Please cherry-pick this to the 5.1 branch as well. (I'll help merge it through.) |
…files Fix response file support for batch jobs.
…4829) Fix response file support for batch jobs.
This fixes an issue where batch jobs don't recompute whether they need response files due to command line length limits, because
BatchJobs are added directly to the task queue and don't go through the same code path inconstructJobthat checks the length (the comment inconstructBatchJobwas incorrect).I've verified that the test added in this PR fails when this patch is not present (the batch frontend job doesn't use a response file) and succeeds when it is.