Json Progress File #391

roulpriya · 2025-06-21T18:23:26Z

Fixes Issue #254

Added a JsonFileProgressReporter that writes progress updates and completion status as JSON lines to a file during toolchain installation.
Integrated with swiftly install, allowing progress to be tracked.
Implemented and tested progress reporting, including step updates, completion (success/failure), and file clearing.

cmcgee1024 · 2025-06-23T11:48:41Z

Sources/SwiftlyCore/FileManager+FilePath.swift

+        }
+
+        let fileHandle = try FileHandle(forWritingTo: URL(fileURLWithPath: path.string))
+        defer { fileHandle.closeFile() }


praise: This is exactly what I hope for with the defer capability in Swift. When I see something allocated, the release logic follows it, answering my question of "how does this get deallocated?"

cmcgee1024 · 2025-06-23T11:53:53Z

Sources/Swiftly/JsonFileProgressReporter.swift

+        do {
+            try jsonLine.append(to: self.filePath)
+        } catch {
+            print("Failed to write progress entry to \(self.filePath): \(error)")


issue: Printing directly to stdout here could do things like corrupt JSON output going there, depending on where we decide to use the progress reporter in the future. This should use the message printing infrastructure that you made in one of the previous PR's instead that will print it to stderr in that case.

cmcgee1024 · 2025-06-23T11:54:31Z

Sources/Swiftly/JsonFileProgressReporter.swift

+        do {
+            try FileManager.default.removeItem(atPath: self.filePath.string)
+        } catch {
+            print("Failed to clear progress file at \(self.filePath): \(error)")


issue: see above, printing directly to stdout can be problematic

cmcgee1024 · 2025-06-23T11:56:26Z

Tests/SwiftlyTests/InstallTests.swift

+        #expect(!lines.isEmpty, "Progress file should contain progress entries")
+
+        // Verify that at least one progress entry exists
+        let hasProgressEntry = lines.contains { line in


question: Why not decode the lines into a ProgressInfo value, so that values can be checked directly instead of inspecting the line strings?

changed all the tests to do this now

Tests/SwiftlyTests/JsonFileProgressReporterTests.swift

cmcgee1024 · 2025-06-23T12:01:23Z

Sources/SwiftlyCore/FileManager+FilePath.swift

@@ -190,6 +190,22 @@ extension String {
        try self.write(to: URL(fileURLWithPath: path.string), atomically: atomically, encoding: enc)
    }

+    public func append(to path: FilePath, encoding enc: String.Encoding = .utf8) throws {
+        if !FileManager.default.fileExists(atPath: path.string) {


issue: The progress file lifecycle (and type) is managed by the client of swiftly, so there should be no need to detect if it is exists here. Just go ahead and append to it, and fail if the append fails. This allows the client to allocate a "named pipe" so that they can get the progress streamed directly to a file descriptor that it monitors so that it can get immediate progress monitor updates.

You mean that the file must always exist?

Right, that's the contract between swiftly and its client. The client creates the file first and swiftly just appends to it when there's progress to report. It's a good idea to have that in the description of the command-line option.

This gives the client the flexibility to create the file as either a plain text file that it polls, or a pipe that streams directly to a reader thread. The inspiration for this is the way that VSCode currently gets updates from the Swift test runner.

I removed the existing check.

cmcgee1024 · 2025-06-23T12:05:36Z

Sources/SwiftlyCore/FileManager+FilePath.swift

+
+        let fileHandle = try FileHandle(forWritingTo: URL(fileURLWithPath: path.string))
+        defer { fileHandle.closeFile() }
+        fileHandle.seekToEndOfFile()


thought: I wonder if repeatedly opening, seeking to the end, and closing the file is a good idea in the context of a pipe (named pipe in Unix). swiftly will be a long running process here, so it could keep the file handle, and write to it when it has progress to report, close it when it is done. That approach might be much better in terms of performance, and perhaps less error prone.

Yes, I was not sure how files with multiple readers and writers worked. But just now I tested it and should be good to keep a file handle open.

You might find that you need to fflush() each line. Hopefully there's a Swift way of doing this instead of having to make the sys call.

cmcgee1024 · 2025-06-23T16:34:38Z

Documentation/SwiftlyDocs.docc/swiftly-cli-reference.md

+
+*A file path where progress information will be written in JSONL format*
+
+Progress information will be appended to this file as JSON objects, one per line.


suggestion: Let's spell out that the contract that the client must create this file first, and swiftly will append to it as it progresses.

Added:"The file must be writable, else an error will be thrown"

cmcgee1024 · 2025-06-23T16:37:28Z

Sources/Swiftly/Install.swift

-            )
+            let animation: ProgressAnimationProtocol =
+                progressFile != nil
+                    ? JsonFileProgressReporter(filePath: progressFile!)


It's generally bad practice in Swift to use the exclamation operator on optionals, except in certain cases.

suggestion: this could be rewritten like this:

let animation: ProgressAnimationProtocol = if let progressFile { JsonFileProgressReporter(filePath: progressFile) } else { PercentProgressAnimation(stream: stdoutStream, header: "Downloading \(version)") }

cmcgee1024 · 2025-06-24T13:58:56Z

Sources/Swiftly/JsonFileProgressReporter.swift

+            return
+        }
+
+        self.fileHandle.seekToEndOfFile()


suggestion: I think that seeking shouldn't be required here since we have a long-lived file handle, and the expectation is that there's only one writer.

Removed this line.

Sources/Swiftly/JsonFileProgressReporter.swift

Tests/SwiftlyTests/JsonFileProgressReporterTests.swift

cmcgee1024 · 2025-06-24T14:06:36Z

Sources/Swiftly/JsonFileProgressReporter.swift

+        self.writeProgress(ProgressInfo.complete(success: success))
+    }
+
+    func clear() {


question: Where's the requirement for a progress file clear action coming from? I feel that this isn't a necessary function, and is likely to fail on a pipe instead of a regular file.

It is part of the ProgressAnimationProtocol. Do you think we can keep it unimplemented then?

Yes, I think that a no-op makes sense here. The next progress entry will probably go backwards, and it will be up to a client to handle that case.

cmcgee1024 · 2025-06-24T15:52:37Z

Sources/Swiftly/JsonFileProgressReporter.swift

+
+    private func writeProgress(_ progress: ProgressInfo) {
+        let jsonData = try? self.encoder.encode(progress)
+        guard let jsonData = jsonData else {


thought: The progress animation protocol is limiting here because nothing is async. This message, while probably not very likely to happen, might not get presented in time because it's emitted in a separate task. If we did want to revisit this, I expect that we would make our own protocol that's async friendly, and some kind of a shim for the PercentProgressAnimation.

cmcgee1024 · 2025-06-24T15:58:42Z

Tests/SwiftlyTests/InstallTests.swift

+            group.addTask { try? await installTask.value }
+        }
+
+        try await Task.sleep(nanoseconds: 100_000_000) // 100ms


question: Is this sleep required? I think that awaiting the withTaskGroup above should wait until both child tasks are done.

I was unsure, hence I added sleep. I removed it.

Tests/SwiftlyTests/InstallTests.swift

Json Progress File

3d2574e

roulpriya requested review from shahmishal, cmcgee1024, justice-adams-apple and etcwilde as code owners June 21, 2025 18:23

cmcgee1024 reviewed Jun 23, 2025

View reviewed changes

Tests/SwiftlyTests/JsonFileProgressReporterTests.swift Outdated Show resolved Hide resolved

cmcgee1024 reviewed Jun 23, 2025

View reviewed changes

Improve JSON progress file handling and error reporting

6a07745

cmcgee1024 reviewed Jun 24, 2025

View reviewed changes

Sources/Swiftly/JsonFileProgressReporter.swift Outdated Show resolved Hide resolved

cmcgee1024 reviewed Jun 24, 2025

View reviewed changes

Tests/SwiftlyTests/JsonFileProgressReporterTests.swift Show resolved Hide resolved

cmcgee1024 reviewed Jun 24, 2025

View reviewed changes

roulpriya force-pushed the json-progress-file-install branch from f792e5f to 8d9fbcb Compare June 24, 2025 15:31

cmcgee1024 reviewed Jun 24, 2025

View reviewed changes

Tests/SwiftlyTests/InstallTests.swift Outdated Show resolved Hide resolved

roulpriya force-pushed the json-progress-file-install branch from 8d9fbcb to 9b7d3fe Compare June 24, 2025 16:02

Refactor progress file parsing and improve test validation

12a74b5

roulpriya force-pushed the json-progress-file-install branch from 9b7d3fe to 12a74b5 Compare June 24, 2025 16:05

cmcgee1024 merged commit bce44d7 into swiftlang:main Jun 24, 2025
24 checks passed


		A file path where progress information will be written in JSONL format

		Progress information will be appended to this file as JSON objects, one per line.

Json Progress File #391

Json Progress File #391

Uh oh!

Conversation

roulpriya commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roulpriya Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roulpriya Jun 24, 2025 •

edited

Loading