From c86aaca74e09a257ef43affcf0823502d0ab3897 Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Mon, 29 Aug 2022 14:28:21 -0700 Subject: [PATCH] Add ability to override/disable response file use in ArgsResolver This change deprecates 'ArgsResolver' API that takes a boolean 'forceResponseFiles' in favour of a new API that takes an 'ResponseFileHandling' enum. This enum allows the previous behaviors of '.forced' and '.heuristic' ('true' and 'false', respectively of 'forceResponseFiles'), and adds an option to override disable use of response files completely ('.disabled'). This is useful in the example of dependency scanning when it is done via a C interface to a compiler library: even for exceedingly long command-line argument lists, using response files is not necessary since we just pass in a pointer to an already-allocated memory buffer containing the argument list, directly. Resolves rdar://99296444 --- Sources/SwiftDriver/Driver/Driver.swift | 2 +- .../SwiftDriver/Execution/ArgsResolver.swift | 37 ++++++++++++++++--- .../ModuleDependencyScanning.swift | 10 ++--- .../MultiJobExecutor.swift | 2 +- .../SwiftDriverExecutor.swift | 6 ++- .../ExplicitModuleBuildTests.swift | 3 +- .../ParsableMessageTests.swift | 7 ++-- Tests/SwiftDriverTests/SwiftDriverTests.swift | 30 ++++++++++----- 8 files changed, 67 insertions(+), 30 deletions(-) diff --git a/Sources/SwiftDriver/Driver/Driver.swift b/Sources/SwiftDriver/Driver/Driver.swift index 27e6c810d..a2c52bb8f 100644 --- a/Sources/SwiftDriver/Driver/Driver.swift +++ b/Sources/SwiftDriver/Driver/Driver.swift @@ -1380,7 +1380,7 @@ extension Driver { // In verbose mode, print out the job if parsedOptions.contains(.v) { let arguments: [String] = try executor.resolver.resolveArgumentList(for: inPlaceJob, - forceResponseFiles: forceResponseFiles) + useResponseFiles: forceResponseFiles ? .forced : .heuristic) stdoutStream <<< arguments.map { $0.spm_shellEscaped() }.joined(separator: " ") <<< "\n" stdoutStream.flush() } diff --git a/Sources/SwiftDriver/Execution/ArgsResolver.swift b/Sources/SwiftDriver/Execution/ArgsResolver.swift index 6a48267ec..e08977954 100644 --- a/Sources/SwiftDriver/Execution/ArgsResolver.swift +++ b/Sources/SwiftDriver/Execution/ArgsResolver.swift @@ -14,6 +14,13 @@ import class Foundation.NSLock import TSCBasic @_implementationOnly import Yams +/// How the resolver is to handle usage of response files +public enum ResponseFileHandling { + case forced + case disabled + case heuristic +} + /// Resolver for a job's argument template. public final class ArgsResolver { /// The map of virtual path to the actual path. @@ -45,22 +52,36 @@ public final class ArgsResolver { } } - public func resolveArgumentList(for job: Job, forceResponseFiles: Bool, + public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic, quotePaths: Bool = false) throws -> [String] { - let (arguments, _) = try resolveArgumentList(for: job, forceResponseFiles: forceResponseFiles, + let (arguments, _) = try resolveArgumentList(for: job, useResponseFiles: useResponseFiles, quotePaths: quotePaths) return arguments } - public func resolveArgumentList(for job: Job, forceResponseFiles: Bool, + public func resolveArgumentList(for job: Job, useResponseFiles: ResponseFileHandling = .heuristic, quotePaths: Bool = false) throws -> ([String], usingResponseFile: Bool) { let tool = try resolve(.path(job.tool), quotePaths: quotePaths) var arguments = [tool] + (try job.commandLine.map { try resolve($0, quotePaths: quotePaths) }) let usingResponseFile = try createResponseFileIfNeeded(for: job, resolvedArguments: &arguments, - forceResponseFiles: forceResponseFiles) + useResponseFiles: useResponseFiles) return (arguments, usingResponseFile) } + @available(*, deprecated, message: "use resolveArgumentList(for:,useResponseFiles:,quotePaths:)") + public func resolveArgumentList(for job: Job, forceResponseFiles: Bool, + quotePaths: Bool = false) throws -> [String] { + let useResponseFiles: ResponseFileHandling = forceResponseFiles ? .forced : .heuristic + return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles, quotePaths: quotePaths) + } + + @available(*, deprecated, message: "use resolveArgumentList(for:,useResponseFiles:,quotePaths:)") + public func resolveArgumentList(for job: Job, forceResponseFiles: Bool, + quotePaths: Bool = false) throws -> ([String], usingResponseFile: Bool) { + let useResponseFiles: ResponseFileHandling = forceResponseFiles ? .forced : .heuristic + return try resolveArgumentList(for: job, useResponseFiles: useResponseFiles, quotePaths: quotePaths) + } + /// Resolve the given argument. public func resolve(_ arg: Job.ArgTemplate, quotePaths: Bool = false) throws -> String { @@ -167,11 +188,15 @@ public final class ArgsResolver { return string.trimmingCharacters(in: .whitespacesAndNewlines) } - private func createResponseFileIfNeeded(for job: Job, resolvedArguments: inout [String], forceResponseFiles: Bool) throws -> Bool { + private func createResponseFileIfNeeded(for job: Job, resolvedArguments: inout [String], useResponseFiles: ResponseFileHandling) throws -> Bool { func quote(_ string: String) -> String { return "\"\(String(string.flatMap { ["\\", "\""].contains($0) ? "\\\($0)" : "\($0)" }))\"" } - + guard useResponseFiles != .disabled else { + return false + } + + let forceResponseFiles = useResponseFiles == .forced if forceResponseFiles || (job.supportsResponseFiles && !commandLineFitsWithinSystemLimits(path: resolvedArguments[0], args: resolvedArguments)) { assert(!forceResponseFiles || job.supportsResponseFiles, diff --git a/Sources/SwiftDriver/ExplicitModuleBuilds/ModuleDependencyScanning.swift b/Sources/SwiftDriver/ExplicitModuleBuilds/ModuleDependencyScanning.swift index 450d98828..c10585f6d 100644 --- a/Sources/SwiftDriver/ExplicitModuleBuilds/ModuleDependencyScanning.swift +++ b/Sources/SwiftDriver/ExplicitModuleBuilds/ModuleDependencyScanning.swift @@ -125,7 +125,7 @@ public extension Driver { if isSwiftScanLibAvailable { let cwd = workingDirectory ?? fileSystem.currentWorkingDirectory! var command = try itemizedJobCommand(of: preScanJob, - forceResponseFiles: forceResponseFiles, + useResponseFiles: .disabled, using: executor.resolver) sanitizeCommandForLibScanInvocation(&command) imports = @@ -154,7 +154,7 @@ public extension Driver { if isSwiftScanLibAvailable { let cwd = workingDirectory ?? fileSystem.currentWorkingDirectory! var command = try itemizedJobCommand(of: scannerJob, - forceResponseFiles: forceResponseFiles, + useResponseFiles: .disabled, using: executor.resolver) sanitizeCommandForLibScanInvocation(&command) dependencyGraph = @@ -183,7 +183,7 @@ public extension Driver { if isSwiftScanLibAvailable { let cwd = workingDirectory ?? fileSystem.currentWorkingDirectory! var command = try itemizedJobCommand(of: batchScanningJob, - forceResponseFiles: forceResponseFiles, + useResponseFiles: .disabled, using: executor.resolver) sanitizeCommandForLibScanInvocation(&command) moduleVersionedGraphMap = @@ -328,10 +328,10 @@ public extension Driver { contents) } - fileprivate func itemizedJobCommand(of job: Job, forceResponseFiles: Bool, + fileprivate func itemizedJobCommand(of job: Job, useResponseFiles: ResponseFileHandling, using resolver: ArgsResolver) throws -> [String] { let (args, _) = try resolver.resolveArgumentList(for: job, - forceResponseFiles: forceResponseFiles) + useResponseFiles: useResponseFiles) return args } } diff --git a/Sources/SwiftDriverExecution/MultiJobExecutor.swift b/Sources/SwiftDriverExecution/MultiJobExecutor.swift index 4b8086b16..c11874758 100644 --- a/Sources/SwiftDriverExecution/MultiJobExecutor.swift +++ b/Sources/SwiftDriverExecution/MultiJobExecutor.swift @@ -579,7 +579,7 @@ class ExecuteJobRule: LLBuildRule { var pid = 0 do { let arguments: [String] = try resolver.resolveArgumentList(for: job, - forceResponseFiles: context.forceResponseFiles) + useResponseFiles: context.forceResponseFiles ? .forced : .heuristic) let process : ProcessProtocol diff --git a/Sources/SwiftDriverExecution/SwiftDriverExecutor.swift b/Sources/SwiftDriverExecution/SwiftDriverExecutor.swift index 14a262784..60f340c0d 100644 --- a/Sources/SwiftDriverExecution/SwiftDriverExecutor.swift +++ b/Sources/SwiftDriverExecution/SwiftDriverExecutor.swift @@ -35,8 +35,9 @@ public final class SwiftDriverExecutor: DriverExecutor { public func execute(job: Job, forceResponseFiles: Bool = false, recordedInputModificationDates: [TypedVirtualPath: Date] = [:]) throws -> ProcessResult { + let useResponseFiles : ResponseFileHandling = forceResponseFiles ? .forced : .heuristic let arguments: [String] = try resolver.resolveArgumentList(for: job, - forceResponseFiles: forceResponseFiles) + useResponseFiles: useResponseFiles) try job.verifyInputsNotModified(since: recordedInputModificationDates, fileSystem: fileSystem) @@ -86,7 +87,8 @@ public final class SwiftDriverExecutor: DriverExecutor { } public func description(of job: Job, forceResponseFiles: Bool) throws -> String { - let (args, usedResponseFile) = try resolver.resolveArgumentList(for: job, forceResponseFiles: forceResponseFiles) + let useResponseFiles : ResponseFileHandling = forceResponseFiles ? .forced : .heuristic + let (args, usedResponseFile) = try resolver.resolveArgumentList(for: job, useResponseFiles: useResponseFiles) var result = args.map { $0.spm_shellEscaped() }.joined(separator: " ") if usedResponseFile { diff --git a/Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift b/Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift index 6b85ac330..2a02cdcaf 100644 --- a/Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift +++ b/Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift @@ -1054,8 +1054,7 @@ final class ExplicitModuleBuildTests: XCTestCase { let resolver = try ArgsResolver(fileSystem: localFileSystem) let (args, _) = try resolver.resolveArgumentList(for: scannerJob, - forceResponseFiles: false, - quotePaths: true) + useResponseFiles: .disabled) XCTAssertTrue(args.count > 1) XCTAssertFalse(args[0].hasSuffix(".resp")) } diff --git a/Tests/SwiftDriverTests/ParsableMessageTests.swift b/Tests/SwiftDriverTests/ParsableMessageTests.swift index 31f3e3810..efeb20300 100644 --- a/Tests/SwiftDriverTests/ParsableMessageTests.swift +++ b/Tests/SwiftDriverTests/ParsableMessageTests.swift @@ -150,7 +150,7 @@ final class ParsableMessageTests: XCTestCase { "-working-directory", workdir.pathString]) let jobs = try driver.planBuild() let compileJob = jobs[0] - let args : [String] = try resolver.resolveArgumentList(for: compileJob, forceResponseFiles: false) + let args : [String] = try resolver.resolveArgumentList(for: compileJob, useResponseFiles: .disabled) let toolDelegate = ToolExecutionDelegate(mode: .parsableOutput, buildRecordInfo: nil, showJobLifecycle: false, @@ -237,7 +237,7 @@ final class ParsableMessageTests: XCTestCase { "-working-directory", "/WorkDir"]) let jobs = try driver.planBuild() compileJob = jobs[0] - args = try resolver.resolveArgumentList(for: compileJob!, forceResponseFiles: false) + args = try resolver.resolveArgumentList(for: compileJob!) toolDelegate = ToolExecutionDelegate(mode: .parsableOutput, buildRecordInfo: nil, showJobLifecycle: false, @@ -313,8 +313,7 @@ final class ParsableMessageTests: XCTestCase { "-working-directory", "/WorkDir"]) let jobs = try driver.planBuild() compileJob = jobs[0] - args = try resolver.resolveArgumentList(for: compileJob!, - forceResponseFiles: false) + args = try resolver.resolveArgumentList(for: compileJob!) toolDelegate = ToolExecutionDelegate(mode: .parsableOutput, buildRecordInfo: nil, showJobLifecycle: false, diff --git a/Tests/SwiftDriverTests/SwiftDriverTests.swift b/Tests/SwiftDriverTests/SwiftDriverTests.swift index 961ecaaa3..6e6eaa293 100644 --- a/Tests/SwiftDriverTests/SwiftDriverTests.swift +++ b/Tests/SwiftDriverTests/SwiftDriverTests.swift @@ -1485,7 +1485,7 @@ final class SwiftDriverTests: XCTestCase { XCTAssertTrue(jobs.count == 1 && jobs[0].kind == .interpret) let interpretJob = jobs[0] let resolver = try ArgsResolver(fileSystem: localFileSystem) - let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob, forceResponseFiles: false) + let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob) XCTAssertTrue(resolvedArgs.count == 2) XCTAssertEqual(resolvedArgs[1].first, "@") let responseFilePath = try AbsolutePath(validating: String(resolvedArgs[1].dropFirst())) @@ -1494,6 +1494,18 @@ final class SwiftDriverTests: XCTestCase { XCTAssertTrue(contents.contains("\"-D\"\n\"TEST_20000\"")) XCTAssertTrue(contents.contains("\"-D\"\n\"TEST_1\"")) } + + // Needs response file + disable override + do { + var driver = try Driver(args: ["swift"] + manyArgs + ["foo.swift"]) + let jobs = try driver.planBuild() + XCTAssertTrue(jobs.count == 1 && jobs[0].kind == .interpret) + let interpretJob = jobs[0] + let resolver = try ArgsResolver(fileSystem: localFileSystem) + let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob, useResponseFiles: .disabled) + XCTAssertFalse(resolvedArgs.contains { $0.hasPrefix("@") }) + } + // Forced response file do { var driver = try Driver(args: ["swift"] + ["foo.swift"]) @@ -1501,7 +1513,7 @@ final class SwiftDriverTests: XCTestCase { XCTAssertTrue(jobs.count == 1 && jobs[0].kind == .interpret) let interpretJob = jobs[0] let resolver = try ArgsResolver(fileSystem: localFileSystem) - let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob, forceResponseFiles: true) + let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob, useResponseFiles: .forced) XCTAssertTrue(resolvedArgs.count == 2) XCTAssertEqual(resolvedArgs[1].first, "@") let responseFilePath = try AbsolutePath(validating: String(resolvedArgs[1].dropFirst())) @@ -1516,8 +1528,8 @@ final class SwiftDriverTests: XCTestCase { XCTAssertTrue(jobs.count == 1 && jobs[0].kind == .interpret) let interpretJob = jobs[0] let resolver = try ArgsResolver(fileSystem: localFileSystem) - let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob, forceResponseFiles: false) - XCTAssertFalse(resolvedArgs.map { $0.hasPrefix("@") }.reduce(false){ $0 || $1 }) + let resolvedArgs: [String] = try resolver.resolveArgumentList(for: interpretJob) + XCTAssertFalse(resolvedArgs.contains { $0.hasPrefix("@") }) } } @@ -1539,7 +1551,7 @@ final class SwiftDriverTests: XCTestCase { let emitModuleJob = jobs.first(where: { $0.kind == .emitModule })! let emitModuleResolvedArgs: [String] = - try resolver.resolveArgumentList(for: emitModuleJob, forceResponseFiles: false) + try resolver.resolveArgumentList(for: emitModuleJob) XCTAssertEqual(emitModuleResolvedArgs.count, 2) XCTAssertEqual(emitModuleResolvedArgs[1].first, "@") @@ -1547,7 +1559,7 @@ final class SwiftDriverTests: XCTestCase { for compileJob in compileJobs { XCTAssertEqual(compileJobs.count, 2) let compileResolvedArgs: [String] = - try resolver.resolveArgumentList(for: compileJob, forceResponseFiles: false) + try resolver.resolveArgumentList(for: compileJob) XCTAssertEqual(compileResolvedArgs.count, 2) XCTAssertEqual(compileResolvedArgs[1].first, "@") } @@ -1565,7 +1577,7 @@ final class SwiftDriverTests: XCTestCase { let mergeModuleJob = jobs.first(where: { $0.kind == .mergeModule })! let mergeModuleResolvedArgs: [String] = - try resolver.resolveArgumentList(for: mergeModuleJob, forceResponseFiles: false) + try resolver.resolveArgumentList(for: mergeModuleJob) XCTAssertEqual(mergeModuleResolvedArgs.count, 2) XCTAssertEqual(mergeModuleResolvedArgs[1].first, "@") @@ -1573,7 +1585,7 @@ final class SwiftDriverTests: XCTestCase { for compileJob in compileJobs { XCTAssertEqual(compileJobs.count, 2) let compileResolvedArgs: [String] = - try resolver.resolveArgumentList(for: compileJob, forceResponseFiles: false) + try resolver.resolveArgumentList(for: compileJob) XCTAssertEqual(compileResolvedArgs.count, 2) XCTAssertEqual(compileResolvedArgs[1].first, "@") } @@ -1590,7 +1602,7 @@ final class SwiftDriverTests: XCTestCase { let generatePCMJob = jobs[0] let generatePCMResolvedArgs: [String] = - try resolver.resolveArgumentList(for: generatePCMJob, forceResponseFiles: false) + try resolver.resolveArgumentList(for: generatePCMJob) XCTAssertEqual(generatePCMResolvedArgs.count, 2) XCTAssertEqual(generatePCMResolvedArgs[1].first, "@") }