Skip to content

refactor(install): parse llard responses with cmdjsonl #159

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
luoliwoshang merged 3 commits into from
Jul 30, 2026
Merged

refactor(install): parse llard responses with cmdjsonl #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daef124
refactor(install): parse llard responses with cmdjsonl
MeteorsLiu Jul 24, 2026
493e640
test(install): cover cmdjsonl transport and size errors
MeteorsLiu Jul 29, 2026
974ae19
fix(install): preserve multiple llard errors
MeteorsLiu Jul 29, 2026
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
81 changes: 27 additions & 54 deletions cmd/llar/internal/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package internal
import (
"bufio"
"context"
"encoding/json"
"errors"
"fmt"
"io"
Expand All @@ -20,6 +19,7 @@ import (
buildcache "github.com/goplus/llar/internal/build/cache"
"github.com/goplus/llar/internal/metadata"
"github.com/goplus/llar/mod/module"
"github.com/qiniu/x/cmdjsonl"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -222,60 +222,33 @@ func requestInstallArtifacts(ctx context.Context, progress io.Writer, client *ht
}

var artifacts []installArtifactMessage
// TODO: Upgrade ixgo and replace this parser with github.com/qiniu/x/cmdjsonl.
//
// Dependency constraint:
// - ixgo v0.61.0 ships generated bindings for qiniu/x packages such as
// gsh, osx, stringutil, and xgo/ng.
// - Its stringutil binding references Builder, NewBuilder, and
// NewBuilderSize.
// - cmdjsonl first appears in qiniu/x v1.17.1, which removed those
// stringutil APIs, so upgrading qiniu/x alone breaks the build.
//
// Migration:
// - Upgrade ixgo to bindings compatible with qiniu/x v1.17.1 or newer.
// - Replace this temporary parser with cmdjsonl.Parser.
reader := bufio.NewReader(resp.Body)
for lineNo := 1; ; lineNo++ {
line, readErr := reader.ReadString('\n')
line = strings.TrimSuffix(strings.TrimSuffix(line, "\n"), "\r")
if line != "" {
command, data, ok := strings.Cut(line, " ")
if !ok {
return nil, fmt.Errorf("invalid llard response line %d", lineNo)
}
switch command {
case "info":
var message string
if err := json.Unmarshal([]byte(data), &message); err != nil {
return nil, fmt.Errorf("decode llard info line %d: %w", lineNo, err)
}
fmt.Fprintln(progress, message)
case "error":
var message string
if err := json.Unmarshal([]byte(data), &message); err != nil {
return nil, fmt.Errorf("decode llard error line %d: %w", lineNo, err)
}
return nil, fmt.Errorf("llard: %s", message)
case "artifact":
var artifact installArtifactMessage
if err := json.Unmarshal([]byte(data), &artifact); err != nil {
return nil, fmt.Errorf("decode llard artifact line %d: %w", lineNo, err)
}
if artifact.ID == "" || artifact.Type == "" || artifact.URL == "" {
return nil, fmt.Errorf("invalid llard artifact line %d", lineNo)
}
artifacts = append(artifacts, artifact)
default:
return nil, fmt.Errorf("unsupported llard response command %q", command)
}
}
if readErr != nil {
if errors.Is(readErr, io.EOF) {
break
}
return nil, readErr
var responseErrs []error
var parser cmdjsonl.Parser
if err := parser.HandleFunc("info", func(message string) {
fmt.Fprintln(progress, message)
}); err != nil {
return nil, err
}
if err := parser.HandleFunc("error", func(message string) {
responseErrs = append(responseErrs, fmt.Errorf("llard: %s", message))
}); err != nil {
return nil, err
}
if err := parser.HandleFunc("artifact", func(artifact installArtifactMessage) error {
if artifact.ID == "" || artifact.Type == "" || artifact.URL == "" {
return fmt.Errorf("invalid llard artifact")
}
artifacts = append(artifacts, artifact)
return nil
}); err != nil {
return nil, err
}
parseErr := parser.Parse(resp.Body, bufio.MaxScanTokenSize)
if len(responseErrs) > 0 {
return nil, errors.Join(responseErrs...)
}
if parseErr != nil {
return nil, parseErr
}
if len(artifacts) == 0 {
return nil, fmt.Errorf("llard returned no artifacts")
Expand Down
66 changes: 60 additions & 6 deletions cmd/llar/internal/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package internal

import (
"bufio"
"bytes"
"context"
"encoding/json"
Expand Down Expand Up @@ -559,6 +560,20 @@ func TestInstallReturnsLlardError(t *testing.T) {
}
}

func TestInstallReturnsMultipleLlardErrors(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/x-cmdjsonl")
writeInstallCommand(t, w, "error", "build failed")
writeInstallCommand(t, w, "error", "test failed")
}))
defer server.Close()

_, err := install(context.Background(), nil, server.URL, "test/root@v1.0.0", hostMatrix())
if err == nil || err.Error() != "llard: build failed\nllard: test failed" {
t.Fatalf("install() error = %v, want joined llard errors", err)
}
}

func TestRequestInstallArtifactsRejectsInvalidResponses(t *testing.T) {
tests := []struct {
name string
Expand All @@ -569,12 +584,13 @@ func TestRequestInstallArtifactsRejectsInvalidResponses(t *testing.T) {
}{
{name: "status", status: http.StatusServiceUnavailable, contentType: "application/x-cmdjsonl", want: "llard returned 503 Service Unavailable"},
{name: "content type", contentType: "text/plain", body: "artifact {}\n", want: `llard returned content type "text/plain", want application/x-cmdjsonl`},
{name: "line", contentType: "application/x-cmdjsonl", body: "invalid\n", want: "invalid llard response line 1"},
{name: "info JSON", contentType: "application/x-cmdjsonl", body: "info {\n", want: "decode llard info line 1"},
{name: "error JSON", contentType: "application/x-cmdjsonl", body: "error {\n", want: "decode llard error line 1"},
{name: "artifact JSON", contentType: "application/x-cmdjsonl", body: "artifact {\n", want: "decode llard artifact line 1"},
{name: "artifact fields", contentType: "application/x-cmdjsonl", body: "artifact {\"id\":\"test/root@v1?os=linux\"}\n", want: "invalid llard artifact line 1"},
{name: "command", contentType: "application/x-cmdjsonl", body: "done {}\n", want: `unsupported llard response command "done"`},
{name: "content type syntax", contentType: "application/x-cmdjsonl; invalid", body: "artifact {}\n", want: `llard returned content type "application/x-cmdjsonl; invalid", want application/x-cmdjsonl`},
{name: "line", contentType: "application/x-cmdjsonl", body: "invalid\n", want: "1: parse error when ParseCommand: no space found"},
{name: "info JSON", contentType: "application/x-cmdjsonl", body: "info {\n", want: "1: parse error when UnmarshalParam: unexpected end of JSON input"},
{name: "error JSON", contentType: "application/x-cmdjsonl", body: "error {\n", want: "1: parse error when UnmarshalParam: unexpected end of JSON input"},
{name: "artifact JSON", contentType: "application/x-cmdjsonl", body: "artifact {\n", want: "1: parse error when UnmarshalParam: unexpected end of JSON input"},
{name: "artifact fields", contentType: "application/x-cmdjsonl", body: "artifact {\"id\":\"test/root@v1?os=linux\"}\n", want: "1: parse error when CallHandler artifact: invalid llard artifact"},
{name: "command", contentType: "application/x-cmdjsonl", body: "done {}\n", want: "1: parse error when ParseCommand: unknown command 'done'"},

@fennoai fennoai Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These assertions pin the full cmdjsonl error strings, including the line-number prefix and internal phrasing (parse error when ParseCommand, UnmarshalParam, etc.). That wording is owned by the third-party qiniu/x package, so a future upgrade could silently break these exact-match assertions without any behavior change in llar. Consider asserting on a stable substring (e.g. no space found, unknown command 'done') where the prefix/line number isn't the point of the test.

{name: "no artifacts", contentType: "application/x-cmdjsonl", body: "info \"checking\"\n", want: "llard returned no artifacts"},
}
for _, tt := range tests {
Expand Down Expand Up @@ -604,6 +620,44 @@ func TestRequestInstallArtifactsRejectsInvalidResponses(t *testing.T) {
}
}

func TestRequestInstallArtifactsReturnsTransportError(t *testing.T) {
baseURL, err := url.Parse("https://llard.example")
if err != nil {
t.Fatal(err)
}
client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) {
return nil, errors.New("connection reset")
})}

_, err = requestInstallArtifacts(
context.Background(), nil, client, baseURL,
module.Version{Path: "test/root"}, url.Values{"os": {"linux"}},
)
if err == nil || !strings.Contains(err.Error(), "connection reset") {
t.Fatalf("requestInstallArtifacts() error = %v, want transport error", err)
}
}

func TestRequestInstallArtifactsRejectsOversizedResponse(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/x-cmdjsonl")
_, _ = io.WriteString(w, strings.Repeat("x", bufio.MaxScanTokenSize+1)+"\n")
}))
defer server.Close()

baseURL, err := url.Parse(server.URL)
if err != nil {
t.Fatal(err)
}
_, err = requestInstallArtifacts(
context.Background(), nil, server.Client(), baseURL,
module.Version{Path: "test/root"}, url.Values{"os": {"linux"}},
)
if err == nil || err.Error() != "1: parse error when ReadLine: line too long" {
t.Fatalf("requestInstallArtifacts() error = %v, want oversized line error", err)
}
}

func TestDownloadInstallArtifactResponses(t *testing.T) {
archive := makeInstallArtifact(t, ".zip", "include/root.h", "root", "/build/root", "-lroot", nil)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Expand Down
Loading