diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2a1d856..19303cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,35 @@ jobs: swift run -c release monitorctl list swift run -c release monitorctl read + # The front doors, against the real binaries. CommandLineTests covers the + # parsing, but the bug in #48 was that `--help` reached the start path — + # a daemon booting instead of printing is only visible from outside the + # process. Every invocation here must return promptly and write nothing. + - name: Smoke test CLI help and version + run: | + set -euo pipefail + for binary in monitorctl monitord; do + swift run -c release "$binary" --help + swift run -c release "$binary" --version + done + # An unrecognised flag must be refused, not ignored. `!` because a + # non-zero exit is the pass condition. + ! swift run -c release monitord --nonsense + ! swift run -c release monitorctl list --nonsense + + # monitord --help must not leave a CSV behind: writing one is exactly the + # symptom #48 reported. + - name: monitord --help writes nothing + run: | + set -euo pipefail + dir="$(mktemp -d)" + swift run -c release monitord --dir "$dir" --help + if [ -n "$(ls -A "$dir")" ]; then + echo "monitord --help wrote to $dir:" + ls -la "$dir" + exit 1 + fi + lint: name: Format check if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository diff --git a/.gitignore b/.gitignore index 18536d1..da45548 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,11 @@ # Swift Package Manager .build/ .swiftpm/ -Package.resolved +# Package.resolved is deliberately NOT ignored. It was, correctly, while the +# package resolved nothing; now that it pins swift-argument-parser, committing +# it is what makes a build reproducible — a version range resolves to whatever +# is newest on the day, and a release built from a different revision than the +# one that was tested is not the release that was tested. # Xcode *.xcuserstate diff --git a/AGENTS.md b/AGENTS.md index 01bf8ae..b49348f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,8 +15,10 @@ oversight. Persistence and a background sampler come later — see ## Tech Stack -- Swift 6 (`swift-tools-version: 6.0`), SwiftPM, macOS 14+. No third-party - dependencies. +- Swift 6 (`swift-tools-version: 6.0`), SwiftPM, macOS 14+. One third-party + dependency, and it is Apple's: `swift-argument-parser`, used by `monitorctl` + and `monitord` for their flags. Nothing in the app, the UI or the sources + depends on it. - SwiftUI, Swift Charts, `Canvas` for the gauges. - System APIs: mach (`host_processor_info`, `host_statistics64`), IOKit (`IOBlockStorageDriver`, `IOAccelerator`), `getifaddrs`, `sysctl`, @@ -30,8 +32,10 @@ oversight. Persistence and a background sampler come later — see ## Environment & Dependencies -- A Mac running macOS 14 or later with a Swift 6 toolchain. Nothing else — the - package resolves no dependencies, so there is no install step. +- A Mac running macOS 14 or later with a Swift 6 toolchain, and a network on + the first build so SwiftPM can fetch `swift-argument-parser`. There is no + other install step. `Package.resolved` is committed, so that fetch is pinned + to one revision rather than to whatever the range resolves to today. - `swiftformat` must be on `PATH` for the lint gate. CI installs it with `brew install swiftformat` when it is missing. - `MonitorSourcesTests` read the real machine, so they need a real Mac. They @@ -434,6 +438,20 @@ are no component-level AGENTS.md files. two choices determine the axis, the formatting and whether it needs rate differentiation, and getting them wrong produces a chart that is quietly wrong rather than obviously broken. +- **The CLIs declare their flags; they do not parse them.** `monitorctl` and + `monitord` are `ParsableCommand`s, so `--help` is rendered from the `@Option` + and `@Flag` declarations and an unrecognised flag is refused by the same + table. Add a flag by adding a property — there is no usage string to update, + which is the point. Both binaries used to hand-roll a `firstIndex(of:)` scan + beside a usage literal that nothing reached: `monitord --help` started the + daemon, and `--intrval 0.1` was silently ignored, so the CSV recorded one + sampling rate while its operator believed another. Two rules worth keeping: + a flag's **choices come from the type** (`LogRetention.allValueStrings`, the + source registry's `allIDs`), never from a list written out in prose; and a + value that parses but cannot work — a zero interval, a count below one — is + rejected in `validate()`, because type conversion does not catch it. + `CommandLineTests` covers both binaries' front doors, which no other suite + touches: the daemon itself was never broken. ## Guardrails diff --git a/Package.resolved b/Package.resolved new file mode 100644 index 0000000..0fcfef8 --- /dev/null +++ b/Package.resolved @@ -0,0 +1,15 @@ +{ + "originHash" : "ac4ed41d40d625a9898c5bbf4f276ea9fc1be0316d979a4e269100e05122b6e1", + "pins" : [ + { + "identity" : "swift-argument-parser", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-argument-parser", + "state" : { + "revision" : "6a52f3251125d74daf04fcbd5e6f08a75d074382", + "version" : "1.8.2" + } + } + ], + "version" : 3 +} diff --git a/Package.swift b/Package.swift index 2f823d5..047dd44 100644 --- a/Package.swift +++ b/Package.swift @@ -40,6 +40,13 @@ let package = Package( .executable(name: "monitorctl", targets: ["monitorctl"]), .executable(name: "monitord", targets: ["monitord"]), ], + dependencies: [ + // The only third-party dependency, and it is Apple's. Both CLIs used to + // hand-roll their parsing, which is how `monitord --help` came to start + // the daemon instead of printing usage (#48). Help generated from the + // flag declarations cannot drift from the flags. + .package(url: "https://github.com/apple/swift-argument-parser", from: "1.5.0"), + ], targets: [ .target(name: "MonitorCore", plugins: ["StampCommit"]), // A prebuild plugin, so the commit in the title bar cannot go stale the @@ -56,14 +63,33 @@ let package = Package( // the point, not an oversight. .target(name: "MonitorUI", dependencies: ["MonitorCore", "MonitorSources"]), .executableTarget(name: "monitor", dependencies: ["MonitorUI"]), - .executableTarget(name: "monitorctl", dependencies: ["MonitorCore", "MonitorSources"]), - .executableTarget(name: "monitord", dependencies: ["MonitorLog", "MonitorSources"]), + .executableTarget( + name: "monitorctl", + dependencies: [ + "MonitorCore", "MonitorSources", + .product(name: "ArgumentParser", package: "swift-argument-parser"), + ]), + .executableTarget( + name: "monitord", + dependencies: [ + "MonitorLog", "MonitorSources", + .product(name: "ArgumentParser", package: "swift-argument-parser"), + ]), .testTarget(name: "MonitorCoreTests", dependencies: ["MonitorCore"]), .testTarget( name: "MonitorSourcesTests", dependencies: ["MonitorSources", "MonitorCore"]), .testTarget(name: "MonitorStoreTests", dependencies: ["MonitorStore", "MonitorCore"]), .testTarget(name: "MonitorLogTests", dependencies: ["MonitorLog", "MonitorCore"]), + // The two CLIs' argument parsing. The bug that motivated it (#48) was + // invisible to every other suite: both binaries built, ran and sampled + // correctly, and only their front doors were wrong. + .testTarget( + name: "CommandLineTests", + dependencies: [ + "monitorctl", "monitord", "MonitorCore", + .product(name: "ArgumentParser", package: "swift-argument-parser"), + ]), // AppModel decides what the panel draws and which sources are read on // a given tick. Both are arithmetic, and both are wrong in ways that // look like a rendering glitch, so they are worth testing directly. diff --git a/Sources/MonitorCore/Version.swift b/Sources/MonitorCore/Version.swift index c2adfb3..2400637 100644 --- a/Sources/MonitorCore/Version.swift +++ b/Sources/MonitorCore/Version.swift @@ -12,3 +12,14 @@ public enum MonitorVersion { /// one name is the sort of thing nobody notices until a screenshot. public static let name = "Monitor" } + +public extension MonitorVersion { + /// What `--version` prints: the release version and the commit it was built + /// from, on one line. + /// + /// Both, because they answer different questions. The version says which + /// release this is; the commit says whether it is the change just made, and + /// carries `-dirty` when it is not any commit at all. A CSV is more useful + /// when its reader can say exactly which build wrote it. + static var detailed: String { "\(string) (\(BuildStamp.commit))" } +} diff --git a/Sources/MonitorSources/SourceRegistry.swift b/Sources/MonitorSources/SourceRegistry.swift index 5372514..a4b1219 100644 --- a/Sources/MonitorSources/SourceRegistry.swift +++ b/Sources/MonitorSources/SourceRegistry.swift @@ -24,7 +24,15 @@ public enum SourceRegistry { return makeAll().filter { wanted.contains($0.id) } } - public static var allIDs: [String] { makeAll().map(\.id) } + /// Every source's id. + /// + /// A `let`, not a computed property: it used to call `makeAll()` on every + /// access, and `makeAll()` builds real readers — `SMCSource` opens an IOKit + /// connection. Cheap when the app asks once at launch, and not cheap at all + /// once `monitorctl` put this list in a `--help` string that ArgumentParser + /// rebuilds on every parse. The ids never change within a process, so build + /// them once and let the readers go. + public static let allIDs: [String] = makeAll().map(\.id) /// Descriptors for every metric the app can produce, whether or not this /// machine can currently read it. The UI lays out from this. diff --git a/Sources/monitorctl/Monitorctl.swift b/Sources/monitorctl/Monitorctl.swift new file mode 100644 index 0000000..8222672 --- /dev/null +++ b/Sources/monitorctl/Monitorctl.swift @@ -0,0 +1,205 @@ +import ArgumentParser +import Foundation +import MonitorCore +import MonitorSources + +// A headless harness for the sampling code. +// +// Sampling is the part most likely to be wrong, and the GUI is the worst place +// to find out. Every source can be read, listed and watched from here without +// launching a window, which makes a broken reader a one-line command rather +// than a debugging session. +// +// The parsing is declared rather than hand-rolled, for the reason set out in +// `monitord`'s Monitord.swift: the usage text was a literal that only a leading +// `-` reached, and an unknown flag was silently ignored (#48). `monitorctl` +// rejected an unknown *command* but not an unknown *flag*, which is the half +// that matters — a mistyped `--intrval` changes what is measured and says +// nothing. + +@main +struct Monitorctl: AsyncParsableCommand { + static let configuration = CommandConfiguration( + commandName: "monitorctl", + abstract: "Read the system metrics that the Monitor app charts.", + discussion: """ + Counter-derived metrics (disk, network, paging) need two readings to \ + produce a rate, so `read` prints nothing for them and `watch` prints \ + nothing on its first line. That is correct behaviour, not a failure: \ + there is no rate yet. + + Nothing here writes to disk unless you ask it to. There is no such flag \ + yet. + """, + version: MonitorVersion.detailed, + subcommands: [List.self, Read.self, Watch.self] + ) +} + +/// `--source`, shared by all three subcommands. +/// +/// One declaration, so the known-source list in the help and the list the +/// validation checks against are the same array. They used to be a sentence in +/// a string literal and a `make(ids:)` call that returned an empty array. +struct SourceSelection: ParsableArguments { + /// Built once. This string is interpolated into the `@Option` below, which + /// ArgumentParser evaluates every time it initialises the type — often, and + /// once per parameterised test. + static let known = "Known: \(SourceRegistry.allIDs.joined(separator: ", "))" + + @Option( + name: .customLong("source"), + parsing: .singleValue, + help: ArgumentHelp( + "Limit to one source; repeatable. Default: all.", + discussion: SourceSelection.known, + valueName: "id" + ) + ) + var ids: [String] = [] + + func validate() throws { + let known = Set(SourceRegistry.allIDs) + let unknown = ids.filter { !known.contains($0) } + guard unknown.isEmpty else { + throw ValidationError( + "no such source: \(unknown.joined(separator: ", "))." + + " Known: \(SourceRegistry.allIDs.joined(separator: ", "))." + ) + } + } + + func resolve() -> [any MetricSource] { + ids.isEmpty ? SourceRegistry.makeAll() : SourceRegistry.make(ids: ids) + } +} + +/// `--interval` and `--json`, shared by `read` and `watch`. +struct SamplingOptions: ParsableArguments { + @Option(help: ArgumentHelp("Sampling interval in seconds.", valueName: "sec")) + var interval: Double = 1.0 + + @Flag(help: "Emit one JSON object per sample instead of a table.") + var json: Bool = false + + func validate() throws { + guard interval > 0 else { + throw ValidationError("--interval must be greater than zero, not \(interval).") + } + } +} + +extension Monitorctl { + struct List: ParsableCommand { + static let configuration = CommandConfiguration( + abstract: "List every source and the metrics it declares." + ) + + @OptionGroup var selection: SourceSelection + + func run() { + for source in selection.resolve() { + print("\(source.id)") + for descriptor in source.descriptors { + print( + " \(descriptor.id.rawValue.padding(toLength: 28, withPad: " ", startingAt: 0))" + + " \(descriptor.group) / \(descriptor.name)" + + " [\(descriptor.unit.rawValue), \(descriptor.kind.rawValue)]" + ) + } + } + } + } + + struct Read: AsyncParsableCommand { + static let configuration = CommandConfiguration( + abstract: "Read once and print the values." + ) + + @OptionGroup var selection: SourceSelection + @OptionGroup var sampling: SamplingOptions + + func run() async { + let sources = selection.resolve() + let printer = SamplePrinter(sources: sources, json: sampling.json) + let sampler = Sampler(sources: sources, sinks: [], interval: sampling.interval) + // Two ticks, so counter-derived rates have a previous reading to + // work from. Otherwise `read` would report nothing for disk and + // network and look broken. + _ = await sampler.tick(at: Date().timeIntervalSince1970) + try? await Task.sleep(for: .seconds(min(sampling.interval, 1.0))) + await printer.emit(sampler.tick(at: Date().timeIntervalSince1970)) + } + } + + struct Watch: AsyncParsableCommand { + static let configuration = CommandConfiguration( + abstract: "Read repeatedly until interrupted." + ) + + @OptionGroup var selection: SourceSelection + @OptionGroup var sampling: SamplingOptions + + @Option(help: ArgumentHelp("Stop after n samples.", valueName: "n")) + var count: Int? + + func validate() throws { + if let count, count < 1 { + throw ValidationError("--count must be at least 1, not \(count).") + } + } + + func run() async { + let sources = selection.resolve() + let printer = SamplePrinter(sources: sources, json: sampling.json) + let sampler = Sampler(sources: sources, sinks: [], interval: sampling.interval) + var taken = 0 + while count.map({ taken < $0 }) ?? true { + let batch = await sampler.tick(at: Date().timeIntervalSince1970) + if !batch.samples.isEmpty { + if !sampling.json { + let time = Date(timeIntervalSince1970: batch.timestamp) + print("— \(time.formatted(date: .omitted, time: .standard))") + } + printer.emit(batch) + taken += 1 + } + try? await Task.sleep(for: .seconds(sampling.interval)) + } + } + } +} + +/// Prints a batch as a table or as one JSON object per line. +struct SamplePrinter { + let descriptors: [MetricID: MetricDescriptor] + let json: Bool + + init(sources: [any MetricSource], json: Bool) { + descriptors = Dictionary( + sources.flatMap(\.descriptors).map { ($0.id, $0) }, uniquingKeysWith: { first, _ in + first + } + ) + self.json = json + } + + func emit(_ batch: SampleBatch) { + guard !batch.samples.isEmpty else { return } + if json { + var object: [String: Any] = ["timestamp": batch.timestamp] + for sample in batch.samples { object[sample.metric.rawValue] = sample.value } + if let data = try? JSONSerialization.data(withJSONObject: object), + let line = String(data: data, encoding: .utf8) + { + print(line) + } + return + } + for sample in batch.samples.sorted(by: { $0.metric.rawValue < $1.metric.rawValue }) { + let unit = descriptors[sample.metric]?.unit ?? .count + let name = sample.metric.rawValue.padding(toLength: 28, withPad: " ", startingAt: 0) + print(" \(name) \(Format.value(sample.value, unit: unit))") + } + } +} diff --git a/Sources/monitorctl/main.swift b/Sources/monitorctl/main.swift deleted file mode 100644 index 61340a0..0000000 --- a/Sources/monitorctl/main.swift +++ /dev/null @@ -1,138 +0,0 @@ -import Foundation -import MonitorCore -import MonitorSources - -// A headless harness for the sampling code. -// -// Sampling is the part most likely to be wrong, and the GUI is the worst place -// to find out. Every source can be read, listed and watched from here without -// launching a window, which makes a broken reader a one-line command rather -// than a debugging session. Hand-rolled argument parsing: this has a handful of -// flags and does not need a dependency. - -let usage = """ -monitorctl — read the system metrics that the Monitor app charts. - -USAGE - monitorctl list list every source and the metrics it declares - monitorctl read [options] read once and print the values - monitorctl watch [options] read repeatedly until interrupted - -OPTIONS - --source limit to one source; repeatable. Default: all. - Known: \(SourceRegistry.allIDs.joined(separator: ", ")) - --interval sampling interval for `watch` (default 1.0) - --count stop after n samples (default: run until interrupted) - --json emit one JSON object per sample instead of a table - -NOTES - Counter-derived metrics (disk, network, paging) need two readings to produce - a rate, so `read` prints nothing for them and `watch` prints nothing on its - first line. That is correct behaviour, not a failure: there is no rate yet. - - Nothing here writes to disk unless you ask it to. There is no such flag yet. -""" - -func value(for flag: String, in arguments: [String]) -> String? { - guard let index = arguments.firstIndex(of: flag), index + 1 < arguments.count else { - return nil - } - return arguments[index + 1] -} - -func values(for flag: String, in arguments: [String]) -> [String] { - var result: [String] = [] - for (index, argument) in arguments.enumerated() - where argument == flag && index + 1 < arguments.count - { - result.append(arguments[index + 1]) - } - return result -} - -let arguments = Array(CommandLine.arguments.dropFirst()) -guard let command = arguments.first, !command.hasPrefix("-") else { - print(usage) - exit(arguments.isEmpty ? 1 : 0) -} - -let requested = values(for: "--source", in: arguments) -let sources = requested.isEmpty ? SourceRegistry.makeAll() : SourceRegistry.make(ids: requested) -guard !sources.isEmpty else { - FileHandle.standardError.write( - Data("no such source: \(requested.joined(separator: ", "))\n".utf8) - ) - exit(1) -} - -let asJSON = arguments.contains("--json") -let interval = value(for: "--interval", in: arguments).flatMap(Double.init) ?? 1.0 -let limit = value(for: "--count", in: arguments).flatMap(Int.init) - -let descriptors = Dictionary( - sources.flatMap(\.descriptors).map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first } -) - -func emit(_ batch: SampleBatch) { - guard !batch.samples.isEmpty else { return } - if asJSON { - var object: [String: Any] = ["timestamp": batch.timestamp] - for sample in batch.samples { object[sample.metric.rawValue] = sample.value } - if let data = try? JSONSerialization.data(withJSONObject: object), - let line = String(data: data, encoding: .utf8) - { - print(line) - } - return - } - for sample in batch.samples.sorted(by: { $0.metric.rawValue < $1.metric.rawValue }) { - let unit = descriptors[sample.metric]?.unit ?? .count - let name = sample.metric.rawValue.padding(toLength: 28, withPad: " ", startingAt: 0) - print(" \(name) \(Format.value(sample.value, unit: unit))") - } -} - -switch command { -case "list": - for source in sources { - print("\(source.id)") - for descriptor in source.descriptors { - print( - " \(descriptor.id.rawValue.padding(toLength: 28, withPad: " ", startingAt: 0))" - + " \(descriptor.group) / \(descriptor.name)" - + " [\(descriptor.unit.rawValue), \(descriptor.kind.rawValue)]" - ) - } - } - -case "read": - let sampler = Sampler(sources: sources, sinks: [], interval: interval) - // Two ticks, so counter-derived rates have a previous reading to work - // from. Otherwise `read` would report nothing for disk and network and - // look broken. - let now = Date().timeIntervalSince1970 - _ = await sampler.tick(at: now) - try? await Task.sleep(for: .seconds(min(interval, 1.0))) - await emit(sampler.tick(at: Date().timeIntervalSince1970)) - -case "watch": - let sampler = Sampler(sources: sources, sinks: [], interval: interval) - var taken = 0 - while limit.map({ taken < $0 }) ?? true { - let batch = await sampler.tick(at: Date().timeIntervalSince1970) - if !batch.samples.isEmpty { - if !asJSON { - print( - "— \(Date(timeIntervalSince1970: batch.timestamp).formatted(date: .omitted, time: .standard))" - ) - } - emit(batch) - taken += 1 - } - try? await Task.sleep(for: .seconds(interval)) - } - -default: - print(usage) - exit(1) -} diff --git a/Sources/monitord/Monitord.swift b/Sources/monitord/Monitord.swift new file mode 100644 index 0000000..82b1229 --- /dev/null +++ b/Sources/monitord/Monitord.swift @@ -0,0 +1,120 @@ +import ArgumentParser +import Foundation +import MonitorCore +import MonitorLog +import MonitorSources + +/// A headless daemon that logs every metric to rotating CSV files. +/// +/// The app's ring buffer is a ten-minute live view and dies with the window. +/// This is the long-running counterpart: it samples on the same clock and writes +/// human-readable CSV that other processes can read to correlate performance with +/// temperature or throttling. Run it as a launchd LaunchAgent to log for days. +/// +/// The flags are declared, not parsed. The usage text used to be a string +/// literal beside a hand-rolled `firstIndex(of:)` scan, and nothing reached it: +/// `--help` fell through to the start path and booted the daemon, and an +/// unrecognised flag was silently ignored, so `--intrval 0.1` logged at the +/// wrong rate and said nothing (#48). ArgumentParser renders the help from the +/// declarations below, so a flag added here appears in `--help` because there is +/// no second place to add it to. +struct MonitordCommand: ParsableCommand { + static let configuration = CommandConfiguration( + commandName: "monitord", + abstract: "Log system metrics to rotating CSV files.", + discussion: """ + Files are named sensors.._