Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
32 changes: 21 additions & 11 deletions cmd/llar/internal/make.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,7 @@ func runMake(cmd *cobra.Command, args []string) error {
makeOutput = abs
}

matrix := formula.Matrix{
Require: map[string][]string{
"os": {runtime.GOOS},
"arch": {runtime.GOARCH},
},
}
matrixStr := matrix.Combinations()[0]
matrixStr := hostMatrixCombo()

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

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

// Resolve local pattern
Expand All @@ -109,15 +103,30 @@ 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 := buildModule(ctx, store, m.Path, ver, matrixStr, false); err != nil {
return err
}
}
return nil
}

// buildModule loads and builds a single module.
func buildModule(ctx context.Context, store repo.Store, modPath, version, matrixStr string) error {
// hostMatrixCombo returns the matrix combination for the current host
// (os+arch). It is used by both `llar make` and `llar test` to select
// the default build variant when the user does not specify one.
func hostMatrixCombo() string {
matrix := formula.Matrix{
Require: map[string][]string{
"os": {runtime.GOOS},
"arch": {runtime.GOARCH},
},
}
return matrix.Combinations()[0]
}

// buildModule loads and builds a single module. When runTest is true, the
// builder also runs each module's onTest hook after onBuild succeeds and
// bypasses the build cache.
Comment thread
MeteorsLiu marked this conversation as resolved.
Outdated
func buildModule(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 +160,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
2 changes: 1 addition & 1 deletion cmd/llar/internal/make_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ func TestBuildModule_SilentSuccess(t *testing.T) {
r, w, _ := os.Pipe()
os.Stdout = w

err := buildModule(context.Background(), store, "test/liba", "1.0.0", matrixStr)
err := buildModule(context.Background(), store, "test/liba", "1.0.0", matrixStr, false)

w.Close()
os.Stdout = old
Expand Down
84 changes: 84 additions & 0 deletions cmd/llar/internal/test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package internal

import (
"context"
"fmt"
"os"

"github.com/goplus/llar/internal/formula/repo"
"github.com/goplus/llar/internal/modules/modlocal"
"github.com/spf13/cobra"
)

var testVerbose bool

var testCmd = &cobra.Command{
Use: "test [module@version]",
Short: "Build a module and run its onTest hook",
Long: `Test builds a module the same way as 'llar make', then executes
the module's onTest callback on the freshly-built artifacts.

The build cache is bypassed for test runs so onTest always executes against
a fresh build. Test runs do not update the cache either, so normal builds
remain cacheable.`,
Comment thread
MeteorsLiu marked this conversation as resolved.
Outdated
Args: cobra.ExactArgs(1),
RunE: runTest,
}

func init() {
testCmd.Flags().BoolVarP(&testVerbose, "verbose", "v", false, "Enable verbose build/test output")
rootCmd.AddCommand(testCmd)
}

func runTest(cmd *cobra.Command, args []string) error {
pattern, version, isLocal, err := parseModuleArg(args[0])
if err != nil {
return err
}

ctx := context.Background()
Comment thread
MeteorsLiu marked this conversation as resolved.

// Reuse the verbose-redirection logic in buildModule by toggling the
// shared makeVerbose flag for the duration of the test run.
savedVerbose := makeVerbose
makeVerbose = testVerbose
Comment thread
MeteorsLiu marked this conversation as resolved.
defer func() { makeVerbose = savedVerbose }()
Comment thread
fennoai[bot] marked this conversation as resolved.
Comment on lines +44 to +46

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.

Code quality: shared mutable state for verbose flag

The save/mutate/restore of makeVerbose works because CLI commands are single-threaded, but it makes buildModule depend on a global side-channel. If parallelism is ever introduced (the TODO at build.go:342 mentions parallel builds), this becomes a data race.

Consider passing verbose as an explicit parameter to buildModule (or bundling it into an options struct alongside runTest), which would eliminate this ceremony entirely.


matrixStr := hostMatrixCombo()

remoteStore, err := newRemoteStore()
if err != nil {
return err
}

if !isLocal {

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.

Nit: code duplication with runMake

Lines 55-83 duplicate the local module resolution logic from runMake (make.go:79-110) almost verbatim — the only difference is the final true/false argument to buildModule. Consider extracting a shared helper like resolveAndBuild(ctx, remoteStore, pattern, version, matrixStr, runTest) to keep the two commands in sync as the local resolution logic evolves.

return buildModule(ctx, remoteStore, pattern, version, matrixStr, true)
}

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

localMods, err := modlocal.Resolve(cwd, pattern)
Comment thread
MeteorsLiu marked this conversation as resolved.
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 := buildModule(ctx, store, m.Path, ver, matrixStr, true); err != nil {
return err
}
}
return nil
}
Comment thread
fennoai[bot] marked this conversation as resolved.
8 changes: 8 additions & 0 deletions formula/classfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type ModuleF struct {

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

modPath string
modFromVer string
Expand Down Expand Up @@ -198,6 +199,13 @@ func (p *ModuleF) OnBuild(f func(ctx *Context, proj *Project, out *BuildResult))
p.fOnBuild = f
}

// OnTest event is used to run post-build verification for a project.
// It fires after OnBuild has completed successfully, reusing the same build
Comment on lines +221 to +222

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.

Docs: misleading for cache-hit path

This says "It fires after OnBuild has completed successfully" but on a cache hit, OnBuild never runs — OnTest fires against cached artifacts directly. Since this is the formula-author-facing API doc, consider rewording to: "It fires after build artifacts are available (either freshly built or reused from cache)".

// context so tests can locate built artifacts via ctx.OutputDir.
Comment thread
fennoai[bot] marked this conversation as resolved.
func (p *ModuleF) OnTest(f func(ctx *Context, proj *Project, out *BuildResult)) {
p.fOnTest = f
}

// -----------------------------------------------------------------------------

// Gopt_ModuleF_Main is main entry of this classfile.
Expand Down
52 changes: 36 additions & 16 deletions internal/build/build.go

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这里 OnTest 复用了带有可写 installDir 的 build context,test 有可能就有可能对产物文件产生修改,这里看了一下 brew 的 测试,会有一个临时目录去作为测试运行的环境,对于构建出产物就像普通用户一样是读取消费,这样边界是否可能会更清晰

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

onTest这里并不是写错了,而是有意为之,这主要是因为onTest需要使用构建产物才能完成编译

一个onTest E2E例子如下:

	tc := cmake.new(testSrc, testBuild, testBuild+"/_out")
	tc.buildType "Release"
	tc.define "CMAKE_POLICY_VERSION_MINIMUM", "3.5"
	tc.use installDir

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

或者说我们现在对测试逻辑就是预期对installDir可写的嘛

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

其实是因为我们没办法控制它不可写,我们不是操作系统没有控制它仅可写的权限

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got it

Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ import (
type Builder struct {
store repo.Store
matrix string
runTest bool
workspaceDir string
newRepo func(repoPath string) (vcs.Repo, error) // defaults to vcs.NewRepo
newRepo func(repoPath string) (vcs.Repo, error) // defaults to vcs.NewRepo
}

type Result struct {
Expand All @@ -32,6 +33,7 @@ type Result struct {
type Options struct {
Store repo.Store
MatrixStr string
RunTest bool
WorkspaceDir string
}

Expand Down Expand Up @@ -61,6 +63,7 @@ func NewBuilder(opts Options) (*Builder, error) {
return &Builder{
store: opts.Store,
matrix: opts.MatrixStr,
runTest: opts.RunTest,
workspaceDir: workspaceDir,
newRepo: vcs.NewRepo,
}, nil
Expand Down Expand Up @@ -187,12 +190,16 @@ func (b *Builder) Build(ctx context.Context, targets []*modules.Module) ([]Resul
}
defer unlock()

// Check cache
cache, err := b.loadCache(mod.Path)
if err == nil {
if entry, ok := cache.get(mod.Version, b.matrix); ok {
dir, _ := b.installDir(mod.Path, mod.Version)
return Result{Metadata: entry.Metadata, OutputDir: dir}, nil
// When runTest is requested, bypass the build cache so onTest
// cannot be skipped by a cached build hit.
var cache *buildCache
if !b.runTest {
cache, err = b.loadCache(mod.Path)
Comment thread
fennoai[bot] marked this conversation as resolved.
Outdated
if err == nil {
if entry, ok := cache.get(mod.Version, b.matrix); ok {
dir, _ := b.installDir(mod.Path, mod.Version)
return Result{Metadata: entry.Metadata, OutputDir: dir}, nil
}
}
}
Comment thread
MeteorsLiu marked this conversation as resolved.
Outdated

Expand Down Expand Up @@ -246,16 +253,29 @@ func (b *Builder) Build(ctx context.Context, targets []*modules.Module) ([]Resul
return Result{}, errors.Join(out.Errs()...)
}

// Save to cache
if cache == nil {
cache = &buildCache{}
// Run onTest inline right after onBuild succeeds, reusing the
// same build context so tests see the just-built artifacts.
if b.runTest && mod.OnTest != nil {
var testOut classfile.BuildResult
mod.OnTest(buildContext, project, &testOut)
if len(testOut.Errs()) > 0 {
return Result{}, fmt.Errorf("onTest failed for %s@%s: %w", mod.Path, mod.Version, errors.Join(testOut.Errs()...))
}
Comment thread
fennoai[bot] marked this conversation as resolved.
Outdated
}
Comment thread
MeteorsLiu marked this conversation as resolved.
Outdated
cache.set(mod.Version, b.matrix, &buildEntry{
Metadata: out.Metadata(),
BuildTime: time.Now(),
})
if err := b.saveCache(mod.Path, cache); err != nil {
return Result{}, err

// Save to cache (skipped when runTest to keep the cache stable
// for normal, non-test builds).
if !b.runTest {
if cache == nil {
cache = &buildCache{}
}
cache.set(mod.Version, b.matrix, &buildEntry{
Metadata: out.Metadata(),
BuildTime: time.Now(),
})
if err := b.saveCache(mod.Path, cache); err != nil {
return Result{}, err
}
}
Comment thread
MeteorsLiu marked this conversation as resolved.
Outdated

return Result{Metadata: out.Metadata(), OutputDir: installDir}, nil
Expand Down
Loading
Loading