Skip to content

Consistently use JSON when encoding output file maps #1777

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 1 commit into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions Sources/SwiftDriver/Driver/OutputFileMap.swift
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,15 @@ public struct OutputFileMap: Hashable, Codable {
/// Store the output file map at the given path.
public func store(
fileSystem: FileSystem,
file: AbsolutePath,
diagnosticEngine: DiagnosticsEngine
file: AbsolutePath
) throws {
let encoder = JSONEncoder()

#if os(Linux) || os(Android)
encoder.outputFormatting = [.prettyPrinted]
encoder.outputFormatting = [.prettyPrinted, .sortedKeys]
#else
if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) {
encoder.outputFormatting = [.prettyPrinted, .withoutEscapingSlashes]
encoder.outputFormatting = [.prettyPrinted, .sortedKeys, .withoutEscapingSlashes]
}
#endif

Expand Down
29 changes: 1 addition & 28 deletions Sources/SwiftDriver/Execution/ArgsResolver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ import protocol TSCBasic.FileSystem
import struct TSCBasic.AbsolutePath
import struct TSCBasic.SHA256

@_implementationOnly import Yams

/// How the resolver is to handle usage of response files
public enum ResponseFileHandling {
case forced
Expand Down Expand Up @@ -166,35 +164,10 @@ public final class ArgsResolver {
throws {
// FIXME: Need a way to support this for distributed build systems...
if let absPath = path.absolutePath {
// This uses Yams to escape and quote strings, but not to output the whole yaml file because
// it sometimes outputs mappings in explicit block format (https://yaml.org/spec/1.2/spec.html#id2798057)
// and the frontend (llvm) only seems to support implicit block format.
try fileSystem.writeFileContents(absPath) { out in
for (input, map) in outputFileMap.entries {
out.send("\(quoteAndEscape(path: VirtualPath.lookup(input))):")
if map.isEmpty {
out.send(" {}\n")
} else {
out.send("\n")
for (type, output) in map {
out.send(" \(type.name): \(quoteAndEscape(path: VirtualPath.lookup(output)))\n")
}
}
}
}
try outputFileMap.store(fileSystem: fileSystem, file: absPath)
}
}

private func quoteAndEscape(path: VirtualPath) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually have tests that exercise the quoting and escaping this is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a basic unit test for this since existing coverage wasn't great. Also ran this through source compatibility testing and the results look good there so far

let inputNode = Node.scalar(Node.Scalar(try! unsafeResolve(path: path),
Tag(.str), .doubleQuoted))
// Width parameter of -1 sets preferred line-width to unlimited so that no extraneous
// line-breaks will be inserted during serialization.
let string = try! Yams.serialize(node: inputNode, width: -1)
// Remove the newline from the end
return string.trimmingCharacters(in: .whitespacesAndNewlines)
}

private func createResponseFileIfNeeded(for job: Job, resolvedArguments: inout [String], useResponseFiles: ResponseFileHandling) throws -> Bool {
guard useResponseFiles != .disabled else {
return false
Expand Down
51 changes: 50 additions & 1 deletion Tests/SwiftDriverTests/SwiftDriverTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1473,7 +1473,7 @@ final class SwiftDriverTests: XCTestCase {
let sampleOutputFileMap = OutputFileMap(entries: pathyEntries)

try withTemporaryFile { file in
try sampleOutputFileMap.store(fileSystem: localFileSystem, file: file.path, diagnosticEngine: DiagnosticsEngine())
try sampleOutputFileMap.store(fileSystem: localFileSystem, file: file.path)
let contentsForDebugging = try localFileSystem.readFileContents(file.path).cString
_ = contentsForDebugging
let recoveredOutputFileMap = try OutputFileMap.load(fileSystem: localFileSystem, file: .absolute(file.path), diagnosticEngine: DiagnosticsEngine())
Expand Down Expand Up @@ -8290,6 +8290,55 @@ final class SwiftDriverTests: XCTestCase {
XCTAssertTrue(plannedJobs[0].commandLine.contains(.flag("-load-pass-plugin=/path/to/plugin")))
#endif
}

func testSupplementaryOutputFileMapUsage() throws {
// Ensure filenames are escaped properly when using a supplementary output file map
try withTemporaryDirectory { path in
try localFileSystem.changeCurrentWorkingDirectory(to: path)
let moduleCachePath = path.appending(component: "ModuleCache")
try localFileSystem.createDirectory(moduleCachePath)
let one = path.appending(component: "one.swift")
let two = path.appending(component: "needs to escape spaces.swift")
let three = path.appending(component: #"another"one.swift"#)
let four = path.appending(component: "4.swift")
try localFileSystem.writeFileContents(one, bytes:
"""
public struct A {}
"""
)
try localFileSystem.writeFileContents(two, bytes:
"""
struct B {}
"""
)
try localFileSystem.writeFileContents(three, bytes:
"""
struct C {}
"""
)
try localFileSystem.writeFileContents(four, bytes:
"""
struct D {}
"""
)

let sdkArgumentsForTesting = (try? Driver.sdkArgumentsForTesting()) ?? []
let invocationArguments = ["swiftc",
"-parse-as-library",
"-emit-library",
"-driver-filelist-threshold", "0",
"-module-cache-path", moduleCachePath.nativePathString(escaped: true),
"-working-directory", path.nativePathString(escaped: true),
one.nativePathString(escaped: true),
two.nativePathString(escaped: true),
three.nativePathString(escaped: true),
four.nativePathString(escaped: true)] + sdkArgumentsForTesting
var driver = try Driver(args: invocationArguments)
let jobs = try driver.planBuild()
try driver.run(jobs: jobs)
XCTAssertFalse(driver.diagnosticEngine.hasErrors)
}
}
}

func assertString(
Expand Down