plugin: handle chunks of output correctly (#77)

Motivation
----------

`containertool` produces its normal output in single writes of less than
PIPE_BUF bytes, which should be delivered atomically through the pipe to
the plugin:

> Reading or writing pipe data is atomic if the size of data written is
not greater than PIPE_BUF. This means that the data transfer seems to be
an instantaneous unit, in that nothing else in the system can observe a
state in which it is partially complete. Atomic I/O may not begin right
away (it may need to wait for buffer space or for data), but once it
does begin it finishes immediately.

https://www.gnu.org/software/libc/manual/html_node/Pipe-Atomicity.html

Progress messages written in this way are displayed correctly.
Unfortunately error output from `swift-argument-parser` is delivered in
smaller chunks which are often broken across several lines by the
plugin:

```
% swift package --allow-network-connections all  build-container-image       
Building for debugging...
[1/1] Write swift-version-2380AA06D3543E1B.txt
Build of product 'containertool' complete! (1.74s)
Building for debugging...
[0/3] Write swift-version-2380AA06D3543E1B.txt
Build of product 'hello-world' complete! (2.04s)
[ContainerImageBuilder] itory <repository>'
Help:  --repository <
[ContainerImageBuilder] repository>  Repository path
[ContainerImageBuilder] Usage: containertoo
[ContainerImageBuilder] l [<optio
[ContainerImageBuilder] ns>] --re
[ContainerImageBuilder] posito
[ContainerImageBuilder] ry <rep
error: Missing expected argument '--repos
[ContainerImageBuilder] ository
[ContainerImageBuilder] > <ex
[ContainerImageBuilder] ecutable>
  Se
[ContainerImageBuilder] e 'containertool --help' for more inf
[ContainerImageBuilder] ormation
[ContainerImageBuilder] .
```

At one point the plugin reassembled output from the pipe and re-split it
on newlines, but the code triggered strict concurrency warnings with the
Swift 6 language mode so it was removed in favour of `PIPE_BUF` write
atomicity. This commit restores the pipe output reassembly.

Modifications
-------------

* Plugin reassembles output from `containertool` and splits it on
newlines
* All output after an error is now printed using `Diagnostics.error`.
Otherwise error and progress output would be interleaved.

Result
------

Error output from `containertool` will be presented correctly by the
plugin.


Test Plan
---------

Existing tests continue to pass.
A new integration test checks that error output is not broken into
multiple lines.

Author: euanh

.github/workflows/integration_tests.yml

@@ -34,3 +34,25 @@ jobs:
                   REGISTRY_PORT: 5000
               run: |
                   swift test
+
+    plugin-streaming-output-test:
+        name: Plugin streaming output test
+        runs-on: ubuntu-latest
+        services:
+            registry:
+                image: registry:2
+                ports:
+                    - 5000:5000
+        steps:
+            - name: Checkout repository
+              uses: actions/checkout@v4
+              with:
+                  persist-credentials: false
+
+            - name: Mark the workspace as safe
+              # https://github.com/actions/checkout/issues/766
+              run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
+
+            - name: Check plugin streaming output is reassembled and printed properly
+              run: |
+                  scripts/test-plugin-output-streaming.sh

Plugins/ContainerImageBuilder/main.swift

@@ -99,16 +99,48 @@ extension PluginError: CustomStringConvertible {
 
         await withThrowingTaskGroup(of: Void.self) { group in
             group.addTask {
+                enum LoggingState {
+                    // Normal output is logged at 'progress' level.
+                    case progress
+
+                    // If an error is detected, all output from that point onwards is logged at 'error' level, which is always printed.
+                    // Errors are reported even without the --verbose flag and cause the build to return a nonzero exit code.
+                    case error
+
+                    func log(_ msg: String) {
+                        let trimmed = msg.trimmingCharacters(in: .newlines)
+                        switch self {
+                        case .progress: Diagnostics.progress(trimmed)
+                        case .error: Diagnostics.error(trimmed)
+                        }
+                    }
+                }
+
+                var buf = ""
+                var logger = LoggingState.progress
+
                 for try await line in err.lines {
-                    let errorLabel = "Error: "  // SwiftArgumentParser adds this prefix to all errors which bubble up
-                    let trimmed = line.trimmingCharacters(in: .whitespacesAndNewlines)
+                    buf.append(line)
 
-                    if trimmed.starts(with: errorLabel) {
-                        // Errors are reported even without the --verbose flag and cause the build to fail.
-                        Diagnostics.error(String(trimmed.dropFirst(errorLabel.count)))
-                    } else {
-                        Diagnostics.progress(trimmed)
+                    guard let (before, after) = buf.splitOn(first: "\n") else {
+                        continue
+                    }
+
+                    var msg = before
+                    buf = String(after)
+
+                    let errorLabel = "Error: "  // SwiftArgumentParser adds this prefix to all errors which bubble up
+                    if msg.starts(with: errorLabel) {
+                        logger = .error
+                        msg.trimPrefix(errorLabel)
                     }
+
+                    logger.log(String(msg))
+                }
+
+                // Print any leftover output in the buffer, in case the child exited without sending a final newline.
+                if !buf.isEmpty {
+                    logger.log(buf)
                 }
             }
 
@@ -118,3 +150,13 @@ extension PluginError: CustomStringConvertible {
         }
     }
 }
+
+extension Collection where Element: Equatable {
+    func splitOn(first element: Element) -> (before: SubSequence, after: SubSequence)? {
+        guard let idx = self.firstIndex(of: element) else {
+            return nil
+        }
+
+        return (self[..<idx], self[idx...].dropFirst())
+    }
+}

scripts/test-plugin-output-streaming.sh

@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
+
+# Test that error output streamed from containertool is printed correctly by the plugin. 
+
+set -exo pipefail
+
+log() { printf -- "** %s\n" "$*" >&2; }
+error() { printf -- "** ERROR: %s\n" "$*" >&2; }
+fatal() { error "$@"; exit 1; }
+
+# Work in a temporary directory, deleted after the test finishes
+PKGPATH=$(mktemp -d)
+cleanup() {
+  log "Deleting temporary package $PKGPATH"
+  rm -rf "$PKGPATH"
+}
+trap cleanup EXIT
+
+swift package --package-path "$PKGPATH" init --type executable --name hello
+cat >> "$PKGPATH/Package.swift" <<EOF
+package.dependencies += [
+    .package(path: "$PWD"),
+]
+EOF
+
+# Run the plugin, forgetting a mandatory argument.   Verify that the output is not corrupted.
+# The `swift package` command will return a nonzero exit code.   This is expected, so disable pipefail.
+set +o pipefail
+swift package --package-path "$PKGPATH" --allow-network-connections all build-container-image 2>&1 | tee "$PKGPATH/output"
+set -o pipefail
+
+grep -F -x -e "error: Missing expected argument '--repository <repository>'" \
+           -e "error: Help:  --repository <repository>  Repository path" \
+           -e "error: Usage: containertool [<options>] --repository <repository> <executable>" \
+           -e "error:   See 'containertool --help' for more information." "$PKGPATH/output"
+
+echo Plugin error output: PASSED
+