Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
28 changes: 20 additions & 8 deletions cmd/llar/internal/make.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

var makeVerbose bool
var makeOutput string
var makeMatrix string

// newRemoteStore creates the remote formula store. Overridable for testing.
var newRemoteStore = func() (repo.Store, error) {
Expand All @@ -48,6 +49,8 @@ var makeCmd = &cobra.Command{
func init() {
makeCmd.Flags().BoolVarP(&makeVerbose, "verbose", "v", false, "Enable verbose build output")
makeCmd.Flags().StringVarP(&makeOutput, "output", "o", "", "Output path (directory or .zip file)")
makeCmd.Flags().StringVar(&makeMatrix, "matrix", "", "Override matrix combination (internal)")
_ = makeCmd.Flags().MarkHidden("matrix")
rootCmd.AddCommand(makeCmd)
}

Expand All @@ -68,13 +71,16 @@ func runMake(cmd *cobra.Command, args []string) error {
makeOutput = abs
}

matrix := formula.Matrix{
Require: map[string][]string{
"os": {runtime.GOOS},
"arch": {runtime.GOARCH},
},
matrixStr := makeMatrix
if matrixStr == "" {
matrix := formula.Matrix{
Require: map[string][]string{
"os": {runtime.GOOS},
"arch": {runtime.GOARCH},
},
}
matrixStr = matrix.Combinations()[0]
Comment on lines +76 to +82

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This logic for creating a default matrix from the current runtime OS and architecture is duplicated in cmd/llar/internal/test.go. To improve maintainability and avoid code duplication, this logic should be extracted into a single, shared helper function. The defaultRuntimeMatrix() function in test.go could be moved to a shared location and reused here.

}
matrixStr := matrix.Combinations()[0]

// Set up remote formula store (always needed for deps)
remoteStore, err := newRemoteStore()
Expand All @@ -83,7 +89,7 @@ func runMake(cmd *cobra.Command, args []string) error {
}

if !isLocal {
return buildModule(ctx, remoteStore, pattern, version, matrixStr)
return buildModuleWithRunTest(ctx, remoteStore, pattern, version, matrixStr, false)
}

// Resolve local pattern
Expand All @@ -109,7 +115,7 @@ func runMake(cmd *cobra.Command, args []string) error {
if ver == "" {
ver = version // global @version from arg
}
if err := buildModule(ctx, store, m.Path, ver, matrixStr); err != nil {
if err := buildModuleWithRunTest(ctx, store, m.Path, ver, matrixStr, false); err != nil {
return err
}
}
Expand All @@ -118,6 +124,11 @@ func runMake(cmd *cobra.Command, args []string) error {

// buildModule loads and builds a single module.
func buildModule(ctx context.Context, store repo.Store, modPath, version, matrixStr string) error {
return buildModuleWithRunTest(ctx, store, modPath, version, matrixStr, false)
}

// buildModuleWithRunTest loads and builds a single module.
func buildModuleWithRunTest(ctx context.Context, store repo.Store, modPath, version, matrixStr string, runTest bool) error {
mods, err := modules.Load(ctx, module.Version{Path: modPath, Version: version}, modules.Options{
FormulaStore: store,
})
Expand Down Expand Up @@ -151,6 +162,7 @@ func buildModule(ctx context.Context, store repo.Store, modPath, version, matrix
buildOpts := build.Options{
Store: store,
MatrixStr: matrixStr,
RunTest: runTest,
}
if makeOutput != "" {
tmpDir, err := os.MkdirTemp("", "llar-make-*")
Expand Down
260 changes: 260 additions & 0 deletions cmd/llar/internal/test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
package internal

import (
"context"
"fmt"
"io"
"os"
"runtime"
"slices"
"strings"

"github.com/goplus/llar/formula"
"github.com/goplus/llar/internal/build"
"github.com/goplus/llar/internal/evaluator"
"github.com/goplus/llar/internal/formula/repo"
"github.com/goplus/llar/internal/modules"
"github.com/goplus/llar/internal/modules/modlocal"
"github.com/goplus/llar/internal/trace"
"github.com/goplus/llar/mod/module"
"github.com/spf13/cobra"
)

var testVerbose bool
var testAuto bool
var testTraceDump bool

var testCmd = &cobra.Command{
Use: "test [module@version]",
Short: "Build a module and run onTest",
Long: `Test builds a module and executes onTest callbacks. Use --auto to evaluate which matrix combinations must run tests.`,
Args: cobra.ExactArgs(1),
RunE: runTest,
}

func init() {
testCmd.Flags().BoolVarP(&testVerbose, "verbose", "v", false, "Enable verbose build/test output")
testCmd.Flags().BoolVar(&testAuto, "auto", false, "Automatically evaluate matrix combinations before running onTest")
testCmd.Flags().BoolVar(&testTraceDump, "trace-dump", false, "Print intercepted build trace for each auto probe")
rootCmd.AddCommand(testCmd)
}

func runTest(cmd *cobra.Command, args []string) error {
pattern, version, isLocal, err := parseModuleArg(args[0])
if err != nil {
return err
}
if testAuto && isLocal {
return fmt.Errorf("--auto does not support local patterns yet")
}
if testTraceDump && !testAuto {
return fmt.Errorf("--trace-dump requires --auto")
}

ctx := context.Background()
remoteStore, err := newRemoteStore()
if err != nil {
return err
}

if !isLocal {
return testModule(ctx, remoteStore, pattern, version)
}

cwd, err := os.Getwd()
if err != nil {
return fmt.Errorf("failed to get working directory: %w", err)
}
localMods, err := modlocal.Resolve(cwd, pattern)
if err != nil {
return err
}

locals := make(map[string]string, len(localMods))
for _, m := range localMods {
locals[m.Path] = m.Dir
}
store := repo.NewOverlayStore(remoteStore, locals)
for _, m := range localMods {
ver := m.Version
if ver == "" {
ver = version
}
if err := testModule(ctx, store, m.Path, ver); err != nil {
return err
}
}
return nil
}

func testModule(ctx context.Context, store repo.Store, modPath, version string) error {
combos := []string{defaultMatrixCombo()}
if testAuto {
matrix, err := loadModuleMatrix(ctx, store, modPath, version)
if err != nil {
return err
}
if matrix.CombinationCount() == 0 {
matrix = defaultRuntimeMatrix()
}
moduleArg := modPath
if version != "" {
moduleArg = modPath + "@" + version
}
var trusted bool
combos, trusted, err = evaluator.Watch(ctx, matrix, func(ctx context.Context, combo string) (evaluator.ProbeResult, error) {
return collectBuildTrace(ctx, store, moduleArg, modPath, version, combo)
})
if err != nil {
return fmt.Errorf("automatic matrix evaluation failed: %w", err)
}
if !trusted {
fmt.Fprintln(os.Stderr, "warning: automatic matrix evaluation is not trusted")
}
}

savedVerbose, savedOutput := makeVerbose, makeOutput
makeVerbose, makeOutput = testVerbose, ""
defer func() {
makeVerbose, makeOutput = savedVerbose, savedOutput
Comment on lines +121 to +124

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.

testModule reaches across to mutate make.go's package-level globals (makeVerbose, makeOutput) to reuse buildModuleWithRunTest. This tight coupling through shared mutable state is fragile — if the probe loop or build were ever parallelized, this would race. Consider passing these as explicit parameters to buildModuleWithRunTest instead.

}()
Comment on lines +121 to +125

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The testModule function modifies global variables (makeVerbose, makeOutput) defined in make.go. This creates a tight, implicit coupling between the test and make commands, which can make the code harder to reason about, debug, and maintain. It's better to refactor the shared functionality, like buildModuleWithRunTest, to accept all its dependencies as explicit parameters instead of relying on global state.


for _, combo := range combos {
if err := buildModuleWithRunTest(ctx, store, modPath, version, combo, true); err != nil {
return fmt.Errorf("test failed for %s@%s [%s]: %w", modPath, version, combo, err)
}
if testVerbose {
fmt.Printf("ok %s@%s [%s]\n", modPath, version, combo)
}
}
return nil
}

func loadModuleMatrix(ctx context.Context, store repo.Store, modPath, version string) (formula.Matrix, error) {
mods, err := modules.Load(ctx, module.Version{Path: modPath, Version: version}, modules.Options{FormulaStore: store})
if err != nil {
return formula.Matrix{}, fmt.Errorf("failed to load modules: %w", err)
}
for _, mod := range mods {
if mod.Path != modPath {
continue
}
if version != "" && mod.Version != version {
continue
}
return mod.Matrix, nil
}
return formula.Matrix{}, nil
}

func defaultRuntimeMatrix() formula.Matrix {
return formula.Matrix{
Require: map[string][]string{
"os": {runtime.GOOS},
"arch": {runtime.GOARCH},
},
}
}

func defaultMatrixCombo() string {
matrix := defaultRuntimeMatrix()
return matrix.Combinations()[0]
}

func collectBuildTrace(ctx context.Context, store repo.Store, moduleArg, modPath, version, combo string) (evaluator.ProbeResult, error) {
mods, err := modules.Load(ctx, module.Version{Path: modPath, Version: version}, modules.Options{
FormulaStore: store,
})
if err != nil {
return evaluator.ProbeResult{}, fmt.Errorf("failed to load modules for %s: %w", moduleArg, err)
}

for _, mod := range mods {
mod.SetStdout(io.Discard)
mod.SetStderr(io.Discard)
}

// Probe builds must stay quiet because formulas may write directly to process stdout/stderr.
savedStdout, savedStderr := os.Stdout, os.Stderr
devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
if err != nil {
return evaluator.ProbeResult{}, fmt.Errorf("failed to open devnull for %s: %w", moduleArg, err)
}
defer func() {
devNull.Close()
os.Stdout = savedStdout
os.Stderr = savedStderr
}()
os.Stdout = devNull
os.Stderr = devNull
Comment on lines +183 to +194

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.

Global os.Stdout/os.Stderr redirection to /dev/null is not concurrency-safe. This same pattern also exists in make.go:147-158. Any goroutine writing to stdout/stderr (including panic handlers) during this window will have output silently lost. Consider passing io.Writer through the call chain rather than mutating process globals.


builder, err := build.NewBuilder(build.Options{
Store: store,
MatrixStr: combo,
Trace: true,
})
if err != nil {
return evaluator.ProbeResult{}, fmt.Errorf("failed to create trace builder for %s: %w", moduleArg, err)
}

results, err := builder.Build(ctx, mods)
if err != nil {
return evaluator.ProbeResult{}, fmt.Errorf("failed to trace build for %s [%s]: %w", moduleArg, combo, err)
}
if len(results) == 0 {
return evaluator.ProbeResult{}, nil
}
result := results[len(results)-1]
records := result.Trace
if testTraceDump {
if _, err := io.WriteString(savedStderr, formatTraceDump(moduleArg, combo, records, result.InputDigests)); err != nil {
return evaluator.ProbeResult{}, fmt.Errorf("failed to write trace dump for %s [%s]: %w", moduleArg, combo, err)
}
}
return evaluator.ProbeResult{
Records: records,
Scope: result.TraceScope,
TraceDiagnostics: result.TraceDiagnostics,
InputDigests: result.InputDigests,
}, nil
}

func formatTraceDump(moduleArg, combo string, records []trace.Record, inputDigests map[string]string) string {
var b strings.Builder
fmt.Fprintf(&b, "TRACE %s [%s]\n", moduleArg, combo)
if len(inputDigests) > 0 {
b.WriteString("DIGESTS\n")
for _, path := range sortedDigestPaths(inputDigests) {
fmt.Fprintf(&b, " %s = %s\n", path, inputDigests[path])
}
}
if len(records) == 0 {
b.WriteString("(no records)\n")
return b.String()
}
for i, rec := range records {
fmt.Fprintf(&b, "%d. argv: %s\n", i+1, strings.Join(rec.Argv, " "))
if rec.Cwd != "" {
fmt.Fprintf(&b, " cwd: %s\n", rec.Cwd)
}
if len(rec.Inputs) > 0 {
fmt.Fprintf(&b, " inputs: %s\n", strings.Join(rec.Inputs, ", "))
}
if len(rec.Changes) > 0 {
fmt.Fprintf(&b, " changes: %s\n", strings.Join(rec.Changes, ", "))
}
}
return b.String()
}

func sortedDigestPaths(inputDigests map[string]string) []string {
if len(inputDigests) == 0 {
return nil
}
paths := make([]string, 0, len(inputDigests))
for path := range inputDigests {
paths = append(paths, path)
}
slices.Sort(paths)
return paths
}
43 changes: 43 additions & 0 deletions cmd/llar/internal/test_trace_dump_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package internal

import (
"strings"
"testing"

"github.com/goplus/llar/internal/trace"
)

func TestFormatTraceDump(t *testing.T) {
got := formatTraceDump("madler/zlib@v1.3.1", "linux-amd64", []trace.Record{
{
Argv: []string{"cmake", "--build", "build"},
Cwd: "/tmp/zlib",
Inputs: []string{"/tmp/zlib/CMakeLists.txt"},
Changes: []string{"/tmp/zlib/build/libz.a"},
},
}, map[string]string{
"/tmp/zlib/build/config.h": "aaaaaaaaaaaaaaaa",
})

checks := []string{
"TRACE madler/zlib@v1.3.1 [linux-amd64]",
"DIGESTS",
"/tmp/zlib/build/config.h = aaaaaaaaaaaaaaaa",
"1. argv: cmake --build build",
"cwd: /tmp/zlib",
"inputs: /tmp/zlib/CMakeLists.txt",
"changes: /tmp/zlib/build/libz.a",
}
for _, want := range checks {
if !strings.Contains(got, want) {
t.Fatalf("formatTraceDump() missing %q in %q", want, got)
}
}
}

func TestFormatTraceDumpEmpty(t *testing.T) {
got := formatTraceDump("mod", "combo", nil, nil)
if !strings.Contains(got, "(no records)") {
t.Fatalf("formatTraceDump() = %q, want empty marker", got)
}
}
Loading
Loading