Skip to content

Json Progress File #391

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.

Sign up for GitHub

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 3 commits into from
Jun 24, 2025
Merged

Json Progress File #391

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d2574e
Json Progress File
roulpriya Jun 21, 2025
6a07745
Improve JSON progress file handling and error reporting
roulpriya Jun 23, 2025
12a74b5
Refactor progress file parsing and improve test validation
roulpriya Jun 24, 2025
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
10 changes: 9 additions & 1 deletion Documentation/SwiftlyDocs.docc/swiftly-cli-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ swiftly [--version] [--help]
Install a new toolchain.

```
swiftly install [<version>] [--use] [--verify|no-verify] [--post-install-file=<post-install-file>] [--assume-yes] [--verbose] [--version] [--help]
swiftly install [<version>] [--use] [--verify|no-verify] [--post-install-file=<post-install-file>] [--progress-file=<progress-file>] [--assume-yes] [--verbose] [--version] [--help]
```

**version:**
Expand Down Expand Up @@ -80,6 +80,14 @@ If the toolchain that is installed has extra post installation steps, they will
written to this file as commands that can be run after the installation.


**--progress-file=\<progress-file\>:**

*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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Each progress entry contains timestamp, progress percentage, and a descriptive message.


**--assume-yes:**

*Disable confirmation prompts by assuming 'yes'*
Expand Down
61 changes: 43 additions & 18 deletions Sources/Swiftly/Install.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,20 @@ struct Install: SwiftlyCommand {
))
var postInstallFile: FilePath?

@Option(
help: ArgumentHelp(
"A file path where progress information will be written in JSONL format",
discussion: """
Progress information will be appended to this file as JSON objects, one per line.
Each progress entry contains timestamp, progress percentage, and a descriptive message.
"""
))
var progressFile: FilePath?

@OptionGroup var root: GlobalOptions

private enum CodingKeys: String, CodingKey {
case version, use, verify, postInstallFile, root
case version, use, verify, postInstallFile, root, progressFile
}

mutating func run() async throws {
Expand All @@ -93,7 +103,9 @@ struct Install: SwiftlyCommand {
try await validateLinked(ctx)

var config = try await Config.load(ctx)
let toolchainVersion = try await Self.determineToolchainVersion(ctx, version: self.version, config: &config)
let toolchainVersion = try await Self.determineToolchainVersion(
ctx, version: self.version, config: &config
)

let (postInstallScript, pathChanged) = try await Self.execute(
ctx,
Expand All @@ -102,7 +114,8 @@ struct Install: SwiftlyCommand {
useInstalledToolchain: self.use,
verifySignature: self.verify,
verbose: self.root.verbose,
assumeYes: self.root.assumeYes
assumeYes: self.root.assumeYes,
progressFile: self.progressFile
)

let shell =
Expand Down Expand Up @@ -192,8 +205,9 @@ struct Install: SwiftlyCommand {
await ctx.message("Setting up toolchain proxies...")
}

let proxiesToCreate = Set(toolchainBinDirContents).subtracting(swiftlyBinDirContents).union(
overwrite)
let proxiesToCreate = Set(toolchainBinDirContents).subtracting(swiftlyBinDirContents)
.union(
overwrite)

for p in proxiesToCreate {
let proxy = Swiftly.currentPlatform.swiftlyBinDir(ctx) / p
Expand Down Expand Up @@ -248,7 +262,8 @@ struct Install: SwiftlyCommand {
useInstalledToolchain: Bool,
verifySignature: Bool,
verbose: Bool,
assumeYes: Bool
assumeYes: Bool,
progressFile: FilePath? = nil
) async throws -> (postInstall: String?, pathChanged: Bool) {
guard !config.installedToolchains.contains(version) else {
await ctx.message("\(version) is already installed.")
Expand All @@ -258,10 +273,11 @@ struct Install: SwiftlyCommand {
// Ensure the system is set up correctly before downloading it. Problems that prevent installation
// will throw, while problems that prevent use of the toolchain will be written out as a post install
// script for the user to run afterwards.
let postInstallScript = try await Swiftly.currentPlatform.verifySystemPrerequisitesForInstall(
ctx, platformName: config.platform.name, version: version,
requireSignatureValidation: verifySignature
)
let postInstallScript = try await Swiftly.currentPlatform
.verifySystemPrerequisitesForInstall(
ctx, platformName: config.platform.name, version: version,
requireSignatureValidation: verifySignature
)

await ctx.message("Installing \(version)")

Expand Down Expand Up @@ -296,10 +312,13 @@ struct Install: SwiftlyCommand {
}
}

let animation = PercentProgressAnimation(
stream: stdoutStream,
header: "Downloading \(version)"
)
let animation: ProgressAnimationProtocol =
progressFile != nil
? JsonFileProgressReporter(filePath: progressFile!)
Copy link
Member

Choose a reason for hiding this comment

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

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)")
}

: PercentProgressAnimation(
stream: stdoutStream,
header: "Downloading \(version)"
)

var lastUpdate = Date()

Expand All @@ -315,7 +334,9 @@ struct Install: SwiftlyCommand {
reportProgress: { progress in
let now = Date()

guard lastUpdate.distance(to: now) > 0.25 || progress.receivedBytes == progress.totalBytes
guard
lastUpdate.distance(to: now) > 0.25
|| progress.receivedBytes == progress.totalBytes
else {
return
}
Expand All @@ -334,7 +355,8 @@ struct Install: SwiftlyCommand {
}
)
} catch let notFound as DownloadNotFoundError {
throw SwiftlyError(message: "\(version) does not exist at URL \(notFound.url), exiting")
throw SwiftlyError(
message: "\(version) does not exist at URL \(notFound.url), exiting")
} catch {
animation.complete(success: false)
throw error
Expand Down Expand Up @@ -401,7 +423,9 @@ struct Install: SwiftlyCommand {
}

/// Utilize the swift.org API along with the provided selector to select a toolchain for install.
public static func resolve(_ ctx: SwiftlyCoreContext, config: Config, selector: ToolchainSelector)
public static func resolve(
_ ctx: SwiftlyCoreContext, config: Config, selector: ToolchainSelector
)
async throws -> ToolchainVersion
{
switch selector {
Expand All @@ -426,7 +450,8 @@ struct Install: SwiftlyCommand {
}

if let patch {
return .stable(ToolchainVersion.StableRelease(major: major, minor: minor, patch: patch))
return .stable(
ToolchainVersion.StableRelease(major: major, minor: minor, patch: patch))
}

await ctx.message("Fetching the latest stable Swift \(major).\(minor) release...")
Expand Down
58 changes: 58 additions & 0 deletions Sources/Swiftly/JsonFileProgressReporter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import Foundation
import SwiftlyCore
import SystemPackage
import TSCUtility

enum ProgressInfo: Codable {
case step(timestamp: Date, percent: Int, text: String)
case complete(success: Bool)
}

struct JsonFileProgressReporter: ProgressAnimationProtocol {
let filePath: FilePath
private let encoder: JSONEncoder

init(filePath: FilePath, encoder: JSONEncoder = JSONEncoder()) {
self.filePath = filePath
self.encoder = encoder
}

private func writeProgress(_ progress: ProgressInfo) {
let jsonData = try? self.encoder.encode(progress)
guard let jsonData = jsonData, let jsonString = String(data: jsonData, encoding: .utf8)
else {
print("Failed to encode progress entry to JSON")
return
}

let jsonLine = jsonString + "\n"

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

Choose a reason for hiding this comment

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

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.

}
}

func update(step: Int, total: Int, text: String) {
assert(step <= total)
self.writeProgress(
ProgressInfo.step(
timestamp: Date(),
percent: Int(Double(step) / Double(total) * 100),
text: text
))
}

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

func clear() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

}
}
}
16 changes: 16 additions & 0 deletions Sources/SwiftlyCore/FileManager+FilePath.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean that the file must always exist?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 removed the existing check.

try self.write(to: path, atomically: true, encoding: enc)
return
}

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

Choose a reason for hiding this comment

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

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?"

fileHandle.seekToEndOfFile()
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if let data = self.data(using: enc) {
fileHandle.write(data)
} else {
throw SwiftlyError(message: "Failed to convert string to data with encoding \(enc)")
}
}

public init(contentsOf path: FilePath, encoding enc: String.Encoding = .utf8) throws {
try self.init(contentsOf: URL(fileURLWithPath: path.string), encoding: enc)
}
Expand Down
34 changes: 34 additions & 0 deletions Tests/SwiftlyTests/InstallTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Foundation
@testable import Swiftly
@testable import SwiftlyCore
import SystemPackage
import Testing

@Suite struct InstallTests {
Expand Down Expand Up @@ -262,4 +263,37 @@ import Testing
try await SwiftlyTests.installMockedToolchain(selector: ToolchainVersion.newStable.name, args: ["--use"])
try await SwiftlyTests.validateInUse(expected: .newStable)
}

/// Verify that progress information is written to the progress file when specified.
@Test(.testHomeMockedToolchain()) func installProgressFile() async throws {
let progressFile = fs.mktemp(ext: ".json")

try await SwiftlyTests.runCommand(Install.self, [
"install", "5.7.0",
"--post-install-file=\(fs.mktemp())",
"--progress-file=\(progressFile.string)",
])

#expect(try await fs.exists(atPath: progressFile))

let progressContent = try String(contentsOfFile: progressFile.string)
let lines = progressContent.components(separatedBy: .newlines).filter { !$0.isEmpty }

#expect(!lines.isEmpty, "Progress file should contain progress entries")

// Verify that at least one progress entry exists
let hasProgressEntry = lines.contains { line in
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed all the tests to do this now

line.contains("\"step\"") && line.contains("\"percent\"") && line.contains("\"timestamp\"")
}
#expect(hasProgressEntry, "Progress file should contain step progress entries")

// Verify that a completion entry exists
let hasCompletionEntry = lines.contains { line in
line.contains("\"complete\"") && line.contains("\"success\"")
}
#expect(hasCompletionEntry, "Progress file should contain completion entry")

// Clean up
try FileManager.default.removeItem(atPath: progressFile.string)
}
}
Loading