Skip to content

Update runner examples to spxrunner #142

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

Draft
joeykchen wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Update runner examples to spxrunner #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 23 additions & 2 deletions modfile/gop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ xgo 1.1

project github.com/goplus/spx math
class .spx Sprite
runner github.com/goplus/spx/v2/cmd/spxrunner v2.0.1

require (
github.com/ajstarks/svgo v0.0.0-20210927141636-6d70534b1098
Copy link
Copy Markdown

@fennoai fennoai Bot Mar 30, 2026

Choose a reason for hiding this comment

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

goxmodSpx2 now includes a runner directive, but TestParse2 never asserts that the parsed Runner fields are correct. A test like:

if proj.Runner == nil || proj.Runner.Path != "github.com/goplus/spx/v2/cmd/spxrunner" || proj.Runner.Version != "v2.0.1" {
    t.Fatal("unexpected Runner:", proj.Runner)
}

would ensure the happy path is actually exercised and guard against silent regressions.

Expand All @@ -182,7 +183,7 @@ func TestGoModCompat2(t *testing.T) {
gopmod = goxmodSpx2
)
f, err := modfile.ParseLax("go.mod", []byte(gopmod), nil)
if err != nil || len(f.Syntax.Stmt) != 6 {
if err != nil || len(f.Syntax.Stmt) != 7 {
t.Fatal("modfile.ParseLax failed:", f, err)
}

Expand All @@ -191,7 +192,7 @@ func TestGoModCompat2(t *testing.T) {
t.Fatal("modfile.ParseLax xgo:", xgo)
}

require := f.Syntax.Stmt[5].(*modfile.LineBlock)
require := f.Syntax.Stmt[6].(*modfile.LineBlock)
if len(require.Token) != 1 || require.Token[0] != "require" {
t.Fatal("modfile.ParseLax require:", require)
}
Expand Down Expand Up @@ -345,6 +346,26 @@ import "\?" math
`)
doTestParseErr(t, `gop.mod:2: import must declare after a project definition`, `
import math
`)
doTestParseErr(t, `gop.mod:2: runner must declare after a project definition`, `
runner github.com/goplus/spx/v2/cmd/spxrunner
`)
doTestParseErr(t, `gop.mod:3: usage: runner runnerCmdPkgPath [version]`, `
project github.com/goplus/spx math
runner
`)
doTestParseErr(t, `gop.mod:3: invalid quoted string: invalid syntax`, `
project github.com/goplus/spx math
runner "\?"
`)
doTestParseErr(t, `gop.mod:3: invalid syntax`, `
project github.com/goplus/spx math
runner github.com/goplus/spx/v2/cmd/spxrunner "\?"
`)
doTestParseErr(t, `gop.mod:4: repeated runner statement`, `
project github.com/goplus/spx math
runner github.com/goplus/spx/v2/cmd/spxrunner
runner github.com/goplus/spx/v2/cmd/spxrunner
`)
doTestParseErr(t, `gop.mod:2: unknown directive: unknown`, `
unknown .spx
Expand Down
41 changes: 41 additions & 0 deletions modfile/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ type Compiler struct {
Version string
}

// A Runner is the runner statement that specifies a custom runner for the project.
// The runner directive must appear after a project statement and only one runner
// per project is allowed.
// Example: runner github.com/goplus/spx/v2/cmd/spxrunner
// Example with version: runner github.com/goplus/spx/v2/cmd/spxrunner v2.0.1
// The optional version must be written as the second argument, not as path@version.
Copy link
Copy Markdown

@fennoai fennoai Bot Mar 30, 2026

Choose a reason for hiding this comment

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

The comment warns against path@version syntax, but isPkgPath only checks for a non-empty string not starting with . or _. A path containing @ (e.g. github.com/foo/bar@v2) passes validation silently and is stored as-is in Runner.Path. Either enforce the restriction in parsePkgPath, or update the doc to clarify it's guidance only.

type Runner struct {
Path string // command package path of the runner
Version string // optional version of the runner command package
Syntax *Line
}

// A File is the parsed, interpreted form of a gox.mod file.
type File struct {
XGo *XGo
Expand Down Expand Up @@ -83,6 +95,7 @@ type Project struct {
Works []*Class // work class of classfile
PkgPaths []string // package paths of classfile and optional inline-imported packages.
Import []*Import // auto-imported packages
Runner *Runner // custom runner
Syntax *Line
}

Expand Down Expand Up @@ -319,6 +332,34 @@ usage: class [-embed -prefix=Prefix] *.workExt WorkClass [WorkPrototype]`, sw)
errorf("usage: import [name] pkgPath")
return
}
case "runner":
proj := f.proj()
if proj == nil {
errorf("runner must declare after a project definition")
return
}
if proj.Runner != nil {
errorf("repeated runner statement")
return
}
Copy link
Copy Markdown

@fennoai fennoai Bot Mar 30, 2026

Choose a reason for hiding this comment

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

There is no upper-bound guard (len(args) > 2). A directive like runner github.com/foo v1.0 extraToken silently drops extraToken. Other directives (e.g. xgo) reject extraneous arguments. Consider adding:

if len(args) > 2 {
    errorf("usage: runner runnerCmdPkgPath [version]")
    return
}

if len(args) < 1 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}
Comment on lines +345 to +348
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 30, 2026

Choose a reason for hiding this comment

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

medium

The current implementation doesn't check for the maximum number of arguments. A runner statement with more than two arguments (e.g., runner path version extra) will be partially processed, with the extra arguments being ignored. This can be misleading and is inconsistent with how other directives like import are handled.

It's better to enforce that runner has at most two arguments. You could also add a new test case for this scenario to ensure correctness.

Suggested change
if len(args) < 1 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}
if len(args) < 1 || len(args) > 2 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}

runnerPath, err := parsePkgPath(&args[0])
if err != nil {
wrapError(err)
return
}
runnerVer := ""
if len(args) > 1 {
runnerVer, err = parseString(&args[1])
if err != nil {
Copy link
Copy Markdown

@fennoai fennoai Bot Mar 30, 2026

Choose a reason for hiding this comment

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

parseString only unquotes the value — it performs no semver/module-version format validation. An arbitrary string like "foobar" or "2.0.1" (missing the v prefix) is silently accepted. Peer directives such as xgo validate their version against modfile.GoVersionRE. Consider calling semver.IsValid from golang.org/x/mod/semver here, or at minimum document that no format enforcement is applied.

wrapError(err)
return
}
}
proj.Runner = &Runner{Path: runnerPath, Version: runnerVer, Syntax: line}
default:
if strict {
errorf("unknown directive: %s", verb)
Expand Down