Skip to content

change prototype of onBuild, onTest #161

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

Open
xushiwei wants to merge 2 commits into
base: main
Choose a base branch
from
Open

change prototype of onBuild, onTest #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ba9687
change prototype of onBuild, onTest
xushiwei Jul 26, 2026
17ac0a7
autotools: use panic instead of return err
xushiwei Jul 26, 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
8 changes: 4 additions & 4 deletions cmd/llar/internal/make_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,21 +716,21 @@ func TestMakeLocal_VerboseWritesBuildOutputToStderr(t *testing.T) {
t.Cleanup(func() { makeVerbose = savedVerbose })

stdout, stderr, restore := captureProcessStreams(t)
var out formula.BuildResult
buildCtx := formula.NewContext(nil, "", "", "", nil)
err = execbroker.Do(execbroker.Scope{
Stdout: os.Stderr,
Stderr: os.Stderr,
}, func() error {
mods[0].OnBuild(nil, nil, &out)
mods[0].OnBuild(buildCtx)
return nil
})
restore()
if err != nil {
t.Fatal(err)
}

if out.Metadata() != "-lA" {
t.Fatalf("metadata = %q, want %q", out.Metadata(), "-lA")
if buildCtx.Out.Metadata() != "-lA" {
t.Fatalf("metadata = %q, want %q", buildCtx.Out.Metadata(), "-lA")
}
if got := strings.TrimSpace(stdout.String()); got != "" {
t.Fatalf("stdout = %q, want no build output", got)
Expand Down
44 changes: 7 additions & 37 deletions formula/classfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ type ModuleF struct {
gsh.App

fOnRequire func(proj *Project, deps *ModuleDeps)
fOnBuild func(ctx *Context, proj *Project, out *BuildResult)
fOnTest func(ctx *Context, proj *Project, out *TestResult)
fOnBuild func(ctx *Context)
fOnTest func(ctx *Context)
fFilter func() bool

modPath string
Expand Down Expand Up @@ -214,58 +214,28 @@ func (p *ModuleF) OnRequire(f func(proj *Project, deps *ModuleDeps)) {

// BuildResult represents the result of building a project.
type BuildResult struct {
errs []error
metadata string // build output metadata, for C/C++ it's the result of pkg-config.
}

// AddErr records a build error.
func (b *BuildResult) AddErr(err error) {
b.errs = append(b.errs, err)
}

// Errs returns all errors collected during build.
func (b *BuildResult) Errs() []error {
return b.errs
meta string // build output metadata, for C/C++ it's the result of pkg-config.
}

// Metadata returns the build output metadata.
func (b *BuildResult) Metadata() string {
return b.metadata
return b.meta
}

// SetMetadata sets the build output metadata.
func (b *BuildResult) SetMetadata(metadata string) {
b.metadata = metadata
b.meta = metadata
}

// OnBuild event is used to instruct the Formula to compile a project.
func (p *ModuleF) OnBuild(f func(ctx *Context, proj *Project, out *BuildResult)) {
func (p *ModuleF) OnBuild(f func(ctx *Context)) {
p.fOnBuild = f
}

// TestResult represents the outcome of a formula's onTest hook.
// Unlike BuildResult it has no metadata field: a test's job is to verify
// the build, not to emit additional pkg-config-style flags. Future
// extensions (pass/fail counts, skip markers, captured logs) should be
// added here without polluting BuildResult.
type TestResult struct {
errs []error
}

// AddErr records a test failure.
func (t *TestResult) AddErr(err error) {
t.errs = append(t.errs, err)
}

// Errs returns all errors collected during the test hook.
func (t *TestResult) Errs() []error {
return t.errs
}

// OnTest event is used to run post-build verification for a project.
// It fires after OnBuild has completed successfully, reusing the same build
// context so tests can locate built artifacts via ctx.OutputDir.
func (p *ModuleF) OnTest(f func(ctx *Context, proj *Project, out *TestResult)) {
func (p *ModuleF) OnTest(f func(ctx *Context)) {
p.fOnTest = f
}

Expand Down
4 changes: 2 additions & 2 deletions formula/classfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ func (f *testFormula) MainEntry() {
f.Defaults(map[string]string{"debug": "OFF"})
f.Filter(func() bool { return true })
f.OnRequire(func(*Project, *ModuleDeps) {})
f.OnBuild(func(*Context, *Project, *BuildResult) {})
f.OnTest(func(*Context, *Project, *TestResult) {})
f.OnBuild(func(*Context) {})
f.OnTest(func(*Context) {})
}

// TestGopt_ModuleF_Main exercises the classfile entry point together with
Expand Down
29 changes: 6 additions & 23 deletions formula/demo/zlib/zlib_llar.gox
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,13 @@ id "madler/zlib"

fromVer "1.0.0"

onBuild (ctx, proj, out) => {
installDir, err := ctx.outputDir()
if err != nil {
out.addErr err
return
}
onBuild ctx => {
installDir := ctx.outputDir

a := autotools.new(ctx.SourceDir, ctx.SourceDir+"/_build", installDir)
a.configure "--static"
a.build
a.install

err = a.configure("--static")
if err != nil {
out.addErr err
return
}
err = a.build()
if err != nil {
out.addErr err
return
}
err = a.install()
if err != nil {
out.addErr err
return
}

out.setMetadata "-lz"
ctx.setMetadata "-lz"
}
26 changes: 20 additions & 6 deletions formula/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"io/fs"

"github.com/goplus/llar/mod/module"
"github.com/qiniu/x/errors"
)

// -----------------------------------------------------------------------------
Expand All @@ -25,7 +26,10 @@ func (p *Project) ReadFile(path string) ([]byte, error) {

// Context represents the build context.
type Context struct {
Proj *Project
SourceDir string
Out BuildResult
Errs errors.List

buildResults map[module.Version]BuildResult

Expand All @@ -35,26 +39,36 @@ type Context struct {
getOutputDir func(matrixStr string, mod module.Version) (string, error)
}

// NewContext creates a Context with build-internal fields.
func NewContext(sourceDir, installDir, matrixStr string, getOutputDir func(string, module.Version) (string, error)) *Context {
func NewContext(proj *Project, sourceDir, installDir, matrixStr string, getOutputDir func(string, module.Version) (string, error)) *Context {

@fennoai fennoai Bot Jul 26, 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.

The exported NewContext lost its doc comment (// NewContext creates a Context with build-internal fields.) in this change and now gains a new proj parameter. Please restore a doc comment (mentioning the new proj) to keep the exported API documented.

return &Context{
Proj: proj,
SourceDir: sourceDir,
installDir: installDir,
matrixStr: matrixStr,
getOutputDir: getOutputDir,
}
}

// SetMetadata sets the build output metadata.
func (c *Context) SetMetadata(metadata string) {
c.Out.SetMetadata(metadata)
}

// OutputDir__0 returns the current module's output (install) directory.
// In DSL: ctx.outputDir()
func (c *Context) OutputDir__0() (string, error) {
return c.installDir, nil
func (c *Context) OutputDir__0() string {
return c.installDir
}

// OutputDir__1 returns the output (install) directory for the given dependency.
// In DSL: ctx.outputDir(dep)
func (c *Context) OutputDir__1(mod module.Version) (string, error) {
return c.getOutputDir(c.matrixStr, mod)
func (c *Context) OutputDir__1(mod module.Version) string {
dir, err := c.getOutputDir(c.matrixStr, mod)
if err != nil {
c.Errs.Add(err)
panic(err)

@fennoai fennoai Bot Jul 26, 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.

Two concerns with panic(err) here:

  1. Unrecovered panic can crash the process. OnBuild/OnTest run inside execbroker.Do(...) in internal/build/build.go, and there is no recover() anywhere in the repo. ctx.outputDir(dep) can legitimately fail (e.g. an undeclared dependency), so this panics on a routine, input-triggerable error. On the HTTP build path Builder.Build runs inside a singleflight.DoChan goroutine (internal/build/http/http.go:107) — a panic there is not recovered by net/http and takes down the whole server. Prefer recording into c.Errs and returning "" (the existing len(buildContext.Errs) > 0 check then surfaces it), or add a recover boundary around the hook invocation.
  2. Dead Errs.Add. Because the panic is never recovered, the c.Errs.Add(err) on the line above is never read. Either surface via Errs and return, or panic without the redundant Add.

Also: the doc comment (// OutputDir__1 returns ... // In DSL: ctx.outputDir(dep)) no longer mentions that this now panics on failure and drops the error return — worth documenting for formula authors.

}
return dir
}

// BuildResult returns the stored build result for the module, if any.
Expand Down
44 changes: 21 additions & 23 deletions formula/project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,8 @@ func TestModuleDeps_Require(t *testing.T) {
}
}

func TestBuildResult_ErrsAndMetadata(t *testing.T) {
func TestBuildResult_Metadata(t *testing.T) {
result := &BuildResult{}
errA := errors.New("first")
errB := errors.New("second")

result.AddErr(errA)
result.AddErr(errB)

if got := result.Errs(); len(got) != 2 || got[0] != errA || got[1] != errB {
t.Fatalf("BuildResult.Errs() = %#v, want [%v %v]", got, errA, errB)
}

if result.Metadata() != "" {
t.Fatalf("BuildResult.Metadata() = %q, want empty string", result.Metadata())
}
Expand Down Expand Up @@ -84,7 +74,7 @@ func TestContext_DoesNotExposeCurrentMatrix(t *testing.T) {
func TestNewContext(t *testing.T) {
getOutputDir := func(_ string, _ module.Version) (string, error) { return "", nil }

ctx := NewContext("/src", "/install", "amd64-linux", getOutputDir)
ctx := NewContext(nil, "/src", "/install", "amd64-linux", getOutputDir)

if ctx.SourceDir != "/src" {
t.Errorf("SourceDir = %q, want %q", ctx.SourceDir, "/src")
Expand Down Expand Up @@ -114,25 +104,19 @@ func TestContext_OutputDir(t *testing.T) {
return "/out/" + m.Path, nil
}

ctx := NewContext("/src", "/install", "amd64-linux", getOutputDir)
ctx := NewContext(nil, "/src", "/install", "amd64-linux", getOutputDir)

t.Run("OutputDir__0 returns own installDir", func(t *testing.T) {
got, err := ctx.OutputDir__0()
if err != nil {
t.Fatalf("OutputDir__0() error = %v", err)
}
got := ctx.OutputDir__0()
if got != "/install" {
t.Errorf("OutputDir__0() = %q, want %q", got, "/install")
t.Errorf("OutputDir() = %q, want %q", got, "/install")
}
})

t.Run("OutputDir__1 dispatches through getOutputDir", func(t *testing.T) {
got, err := ctx.OutputDir__1(dep)
if err != nil {
t.Fatalf("OutputDir__1() error = %v", err)
}
got := ctx.OutputDir__1(dep)
if got != "/out/owner/dep" {
t.Errorf("OutputDir__1() = %q, want %q", got, "/out/owner/dep")
t.Errorf("OutputDir() = %q, want %q", got, "/out/owner/dep")
}
if gotMatrix != "amd64-linux" {
t.Errorf("getOutputDir matrix = %q, want %q", gotMatrix, "amd64-linux")
Expand All @@ -143,6 +127,20 @@ func TestContext_OutputDir(t *testing.T) {
})
}

func TestContext_ErrOutputDir(t *testing.T) {
getOutputDir := func(matrixStr string, m module.Version) (string, error) {
return "", errors.New("failed to get output dir")
}

ctx := NewContext(nil, "/src", "/install", "amd64-linux", getOutputDir)
defer func() {
if r := recover(); r == nil {
t.Errorf("OutputDir did not panic on error")
}
}()
ctx.OutputDir__1(module.Version{Path: "owner/dep", Version: "1.0.0"})
}

func TestContext_BuildResult(t *testing.T) {
ctx := &Context{}
mod := module.Version{Path: "owner/repo", Version: "1.0.0"}
Expand Down
21 changes: 9 additions & 12 deletions internal/build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,15 +282,14 @@ func (b *Builder) Build(ctx context.Context, targets []*modules.Module) ([]Resul
getOutputDir := func(_ string, m module.Version) (string, error) {
return b.installDir(m.Path, m.Version)
}
buildContext := classfile.NewContext(tmpSourceDir, installDir, b.matrix, getOutputDir)
project := &classfile.Project{Deps: deps, SourceFS: mod.FS.(fs.ReadFileFS)}
buildContext := classfile.NewContext(project, tmpSourceDir, installDir, b.matrix, getOutputDir)

// Inject results of already-built dependencies
for modVer, result := range builtResults {
buildContext.AddBuildResult(modVer, result)
}

project := &classfile.Project{Deps: deps, SourceFS: mod.FS.(fs.ReadFileFS)}

var metadata string
if err := execbroker.Do(execbroker.Scope{
Dir: tmpSourceDir,
Expand All @@ -302,22 +301,20 @@ func (b *Builder) Build(ctx context.Context, targets []*modules.Module) ([]Resul
if cacheHit {
metadata = entry.Metadata
} else {
var out classfile.BuildResult
mod.OnBuild(buildContext, project, &out)
if len(out.Errs()) > 0 {
return errors.Join(out.Errs()...)
mod.OnBuild(buildContext)
if len(buildContext.Errs) > 0 {
return errors.Join(buildContext.Errs...)
}
metadata = out.Metadata()
metadata = buildContext.Out.Metadata()
}

// Run OnTest (root only) against the just-built or cached
// artifacts, reusing the same build context so tests see a
// consistent environment either way.
if testThisMod {
var testOut classfile.TestResult
mod.OnTest(buildContext, project, &testOut)
if len(testOut.Errs()) > 0 {
return fmt.Errorf("onTest failed for %s@%s: %w", mod.Path, mod.Version, errors.Join(testOut.Errs()...))
mod.OnTest(buildContext)
if len(buildContext.Errs) > 0 {
return fmt.Errorf("onTest failed for %s@%s: %w", mod.Path, mod.Version, buildContext.Errs)

@fennoai fennoai Bot Jul 26, 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.

buildContext.Errs is qiniu/x/errors.List (type List []error), which does not implement Unwrap(). Wrapping it with %w produces a chain that errors.Is cannot descend past, so errors.Is(err, cause) no longer finds the injected error.

The unchanged assertion in TestBuild_RunTest_EnabledSurfacesOnTestError (build_test.go:956) — if !errors.Is(err, wantErr) — will fail with this change. The previous code used errors.Join(testOut.Errs()...), which yields an Unwrap() []error chain that errors.Is can walk.

Suggest wrapping buildContext.Errs.ToError() (returns the single error directly when len == 1) or keeping errors.Join(buildContext.Errs...) so the cause stays inspectable.

}
}
return nil
Expand Down
17 changes: 8 additions & 9 deletions internal/build/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,9 +910,9 @@ func TestBuild_RunTest_DisabledSkipsOnTest(t *testing.T) {
// Inject an OnTest callback that would fail the build if invoked.
var called bool
for _, m := range mods {
m.OnTest = func(ctx *classfile.Context, proj *classfile.Project, out *classfile.TestResult) {
m.OnTest = func(ctx *classfile.Context) {
called = true
out.AddErr(errors.New("onTest should not have been invoked"))
ctx.Errs.Add(errors.New("onTest should not have been invoked"))
}
}

Expand Down Expand Up @@ -941,8 +941,8 @@ func TestBuild_RunTest_EnabledSurfacesOnTestError(t *testing.T) {

wantErr := errors.New("boom from onTest")
for _, m := range mods {
m.OnTest = func(ctx *classfile.Context, proj *classfile.Project, out *classfile.TestResult) {
out.AddErr(wantErr)
m.OnTest = func(ctx *classfile.Context) {
ctx.Errs.Add(wantErr)
}
}

Expand Down Expand Up @@ -988,7 +988,7 @@ func TestBuild_RunTest_ReusesCacheWhenHit(t *testing.T) {

var testCalled bool
for _, m := range mods {
m.OnTest = func(ctx *classfile.Context, proj *classfile.Project, out *classfile.TestResult) {
m.OnTest = func(ctx *classfile.Context) {
testCalled = true
}
}
Expand Down Expand Up @@ -1041,7 +1041,7 @@ func TestBuild_RunTest_SavesCacheOnMiss(t *testing.T) {

var testCalled bool
for _, m := range mods {
m.OnTest = func(ctx *classfile.Context, proj *classfile.Project, out *classfile.TestResult) {
m.OnTest = func(ctx *classfile.Context) {
testCalled = true
}
}
Expand Down Expand Up @@ -1097,13 +1097,12 @@ func TestBuild_RunTest_DepOnTestNotInvoked(t *testing.T) {
m := m
switch m.Path {
case "test/depresult":
m.OnTest = func(_ *classfile.Context, _ *classfile.Project, _ *classfile.TestResult) {
m.OnTest = func(_ *classfile.Context) {
rootCalled = true
}
case "test/liba":
m.OnTest = func(_ *classfile.Context, _ *classfile.Project, out *classfile.TestResult) {
m.OnTest = func(_ *classfile.Context) {
depCalled = true
out.AddErr(errors.New("dep OnTest should not have been invoked"))
}
}
}
Expand Down
Loading
Loading