Repository: openllb/hlb
Pull request #79 (2020-04-11 16:47:34)

Refactor outputs and CLI outputs
- Generating dynamic CST is bad because it's hard to integrate with error reporting and debugging.
- Previously you weren't able to define multiple outputs per target on the CLI, now you can do duplicates like `hlb run -t foo,dockerPush=hinshun/foo:latest,dockerPush=hinshun/foo:stable`
- Refactored duplicate code


Patch:
diff --git a/checker/errors.go b/checker/errors.go
index ecbcdb9..7b3838f 100644
--- a/checker/errors.go
+++ b/checker/errors.go
@@ -85,11 +85,12 @@ func (e ErrWrongArgType) Error() string {
 }
 
 type ErrInvalidTarget struct {
-	Ident *parser.Ident
+	Node   parser.Node
+	Target string
 }
 
 func (e ErrInvalidTarget) Error() string {
-	return fmt.Sprintf("%s invalid compile target %s", FormatPos(e.Ident.Position()), e.Ident)
+	return fmt.Sprintf("%s invalid compile target %s", FormatPos(e.Node.Position()), e.Target)
 }
 
 type ErrCallUnexported struct {
diff --git a/cmd/hlb/command/run.go b/cmd/hlb/command/run.go
index 3628656..e68a847 100644
--- a/cmd/hlb/command/run.go
+++ b/cmd/hlb/command/run.go
@@ -112,28 +112,28 @@ func Run(ctx context.Context, cln *client.Client, rc io.ReadCloser, opts RunOpti
 		}
 	}
 
-	targets := []hlb.Target{}
+	var targets []codegen.Target
 	for _, target := range opts.Targets {
 		r := csv.NewReader(strings.NewReader(target))
 		fields, err := r.Read()
 		if err != nil {
 			return err
 		}
-		t := hlb.Target{
+		t := codegen.Target{
 			Name: fields[0],
 		}
 		for _, field := range fields[1:] {
 			switch {
 			case strings.HasPrefix(field, "dockerPush="):
-				t.DockerPushRef = strings.TrimPrefix(field, "dockerPush=")
+				t.Outputs = append(t.Outputs, codegen.Output{Type: codegen.OutputDockerPush, Ref: strings.TrimPrefix(field, "dockerPush=")})
 			case strings.HasPrefix(field, "dockerLoad="):
-				t.DockerLoadRef = strings.TrimPrefix(field, "dockerLoad=")
+				t.Outputs = append(t.Outputs, codegen.Output{Type: codegen.OutputDockerLoad, Ref: strings.TrimPrefix(field, "dockerLoad=")})
 			case strings.HasPrefix(field, "download="):
-				t.DownloadPath = strings.TrimPrefix(field, "download=")
+				t.Outputs = append(t.Outputs, codegen.Output{Type: codegen.OutputDownload, LocalPath: strings.TrimPrefix(field, "download=")})
 			case strings.HasPrefix(field, "downloadTarball="):
-				t.TarballPath = strings.TrimPrefix(field, "downloadTarball=")
+				t.Outputs = append(t.Outputs, codegen.Output{Type: codegen.OutputDownloadTarball, LocalPath: strings.TrimPrefix(field, "downloadTarball=")})
 			case strings.HasPrefix(field, "downloadOCITarball="):
-				t.OCITarballPath = strings.TrimPrefix(field, "downloadOCITarball=")
+				t.Outputs = append(t.Outputs, codegen.Output{Type: codegen.OutputDownloadOCITarball, LocalPath: strings.TrimPrefix(field, "downloadOCITarball=")})
 			default:
 				return fmt.Errorf("Unknown target option %q for target %q", field, t.Name)
 			}
diff --git a/codegen/codegen.go b/codegen/codegen.go
index 0a655c9..0bf5a72 100644
--- a/codegen/codegen.go
+++ b/codegen/codegen.go
@@ -12,7 +12,6 @@ import (
 
 	"github.com/docker/buildx/util/progress"
 	"github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/flags"
 	shellquote "github.com/kballard/go-shellquote"
 	"github.com/moby/buildkit/client"
 	"github.com/moby/buildkit/client/llb"
@@ -125,16 +124,16 @@ func (cg *CodeGen) SolveOptions(ctx context.Context, st llb.State) (opts []solve
 	return opts, nil
 }
 
-func (cg *CodeGen) Generate(ctx context.Context, mod *parser.Module, targets []*parser.CallStmt) (solver.Request, error) {
+func (cg *CodeGen) Generate(ctx context.Context, mod *parser.Module, targets []Target) (solver.Request, error) {
 	cg.request = solver.NewEmptyRequest()
 
 	for _, target := range targets {
 		// Reset codegen state for next target.
 		cg.reset()
 
-		obj := mod.Scope.Lookup(target.Func.Ident.Name)
+		obj := mod.Scope.Lookup(target.Name)
 		if obj == nil {
-			return cg.request, fmt.Errorf("unknown target %q", target.Func.Ident.Name)
+			return cg.request, fmt.Errorf("unknown target %q", target)
 		}
 
 		// Yield to the debugger before compiling anything.
@@ -143,31 +142,33 @@ func (cg *CodeGen) Generate(ctx context.Context, mod *parser.Module, targets []*
 			return cg.request, err
 		}
 
+		call := parser.NewCallStmt(target.Name, nil, nil, nil).Call
+
 		var st llb.State
 		switch obj.Kind {
 		case parser.DeclKind:
 			switch n := obj.Node.(type) {
 			case *parser.FuncDecl:
 				if n.Type.ObjType != parser.Filesystem {
-					return cg.request, checker.ErrInvalidTarget{Ident: target.Func.Ident}
+					return cg.request, checker.ErrInvalidTarget{Node: n, Target: target.Name}
 				}
 
-				st, err = cg.EmitFilesystemFuncDecl(ctx, mod.Scope, n, target, noopAliasCallback, nil)
+				st, err = cg.EmitFilesystemFuncDecl(ctx, mod.Scope, n, call, noopAliasCallback, nil)
 				if err != nil {
 					return cg.request, err
 				}
 			case *parser.AliasDecl:
 				if n.Func.Type.ObjType != parser.Filesystem {
-					return cg.request, checker.ErrInvalidTarget{Ident: target.Func.Ident}
+					return cg.request, checker.ErrInvalidTarget{Node: n, Target: target.Name}
 				}
 
-				st, err = cg.EmitFilesystemAliasDecl(ctx, mod.Scope, n, target, nil)
+				st, err = cg.EmitFilesystemAliasDecl(ctx, mod.Scope, n, call, nil)
 				if err != nil {
 					return cg.request, err
 				}
 			}
 		default:
-			return cg.request, checker.ErrInvalidTarget{Ident: target.Func.Ident}
+			return cg.request, checker.ErrInvalidTarget{Node: obj.Node, Target: target.Name}
 		}
 
 		def, err := st.Marshal(ctx, llb.LinuxAmd64)
@@ -186,6 +187,10 @@ func (cg *CodeGen) Generate(ctx context.Context, mod *parser.Module, targets []*
 		}
 
 		cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
+
+		for _, output := range target.Outputs {
+			cg.outputRequest(ctx, st, output)
+		}
 	}
 
 	return cg.request, nil
@@ -846,96 +851,18 @@ func (cg *CodeGen) EmitFilesystemChainStmt(ctx context.Context, scope *parser.Sc
 			return so, err
 		}
 
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithPushImage(ref))
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDockerPush, Ref: ref})
+			return st, err
 		}
 	case "dockerLoad":
 		ref, err := cg.EmitStringExpr(ctx, scope, call, args[0])
 		if err != nil {
 			return so, err
 		}
-		if cg.mw == nil {
-			return so, errors.WithStack(errors.Errorf("progress.MultiWriter must be provided for dockerLoad"))
-		}
-
-		if cg.dockerCli == nil {
-			cg.dockerCli, err = command.NewDockerCli()
-			if err != nil {
-				return so, err
-			}
-
-			err = cg.dockerCli.Initialize(flags.NewClientOptions())
-			if err != nil {
-				return so, err
-			}
-		}
-
-		r, w := io.Pipe()
-
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(w)))
-
-		done := make(chan struct{})
-		cg.solveOpts = append(cg.solveOpts, solver.WithWaiter(done))
-
-		go func() {
-			defer close(done)
-
-			resp, err := cg.dockerCli.Client().ImageLoad(ctx, r, true)
-			if err != nil {
-				r.CloseWithError(err)
-				return
-			}
-			defer resp.Body.Close()
-
-			pw := cg.mw.WithPrefix("", false)
-			progress.FromReader(pw, fmt.Sprintf("importing %s to docker", ref), resp.Body)
-		}()
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithDownloadDockerTarball(ref))
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDockerLoad, Ref: ref})
+			return st, err
 		}
 	case "download":
 		localPath, err := cg.EmitStringExpr(ctx, scope, call, args[0])
@@ -943,31 +870,9 @@ func (cg *CodeGen) EmitFilesystemChainStmt(ctx context.Context, scope *parser.Sc
 			return so, err
 		}
 
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-		s.Allow(filesync.NewFSSyncTargetDir(localPath))
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithDownload(localPath))
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDownload, LocalPath: localPath})
+			return st, err
 		}
 	case "downloadTarball":
 		localPath, err := cg.EmitStringExpr(ctx, scope, call, args[0])
@@ -975,36 +880,9 @@ func (cg *CodeGen) EmitFilesystemChainStmt(ctx context.Context, scope *parser.Sc
 			return so, err
 		}
 
-		f, err := os.Open(localPath)
-		if err != nil {
-			return so, err
-		}
-
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(f)))
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithDownloadTarball())
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDownloadTarball, LocalPath: localPath})
+			return st, err
 		}
 	case "downloadOCITarball":
 		localPath, err := cg.EmitStringExpr(ctx, scope, call, args[0])
@@ -1012,36 +890,9 @@ func (cg *CodeGen) EmitFilesystemChainStmt(ctx context.Context, scope *parser.Sc
 			return so, err
 		}
 
-		f, err := os.Open(localPath)
-		if err != nil {
-			return so, err
-		}
-
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(f)))
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithDownloadOCITarball())
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDownloadOCITarball, LocalPath: localPath})
+			return st, err
 		}
 	case "downloadDockerTarball":
 		localPath, err := cg.EmitStringExpr(ctx, scope, call, args[0])
@@ -1054,36 +905,9 @@ func (cg *CodeGen) EmitFilesystemChainStmt(ctx context.Context, scope *parser.Sc
 			return so, err
 		}
 
-		f, err := os.Open(localPath)
-		if err != nil {
-			return so, err
-		}
-
-		s, err := cg.newSession(ctx)
-		if err != nil {
-			return so, err
-		}
-		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(f)))
-
 		so = func(st llb.State) (llb.State, error) {
-			opts, err := cg.SolveOptions(ctx, st)
-			if err != nil {
-				return st, err
-			}
-
-			opts = append(opts, solver.WithDownloadDockerTarball(ref))
-			for _, iopt := range iopts {
-				opt := iopt.(solver.SolveOption)
-				opts = append(opts, opt)
-			}
-
-			def, err := st.Marshal(ctx, llb.LinuxAmd64)
-			if err != nil {
-				return st, err
-			}
-
-			cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
-			return st, nil
+			err := cg.outputRequest(ctx, st, Output{Type: OutputDownloadDockerTarball, LocalPath: localPath, Ref: ref})
+			return st, err
 		}
 	default:
 		// Must be a named reference.
diff --git a/codegen/decl.go b/codegen/decl.go
index 7321080..79f521a 100644
--- a/codegen/decl.go
+++ b/codegen/decl.go
@@ -50,7 +50,7 @@ func (cg *CodeGen) EmitFuncDecl(ctx context.Context, scope *parser.Scope, fun *p
 	case parser.Str:
 		return cg.EmitStringBlock(ctx, fun.Scope, fun.Body.NonEmptyStmts(), v)
 	default:
-		return v, checker.ErrInvalidTarget{Ident: fun.Name}
+		return v, checker.ErrInvalidTarget{Node: fun}
 	}
 }
 
diff --git a/codegen/output.go b/codegen/output.go
new file mode 100644
index 0000000..097bbcb
--- /dev/null
+++ b/codegen/output.go
@@ -0,0 +1,121 @@
+package codegen
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"os"
+
+	"github.com/docker/buildx/util/progress"
+	"github.com/docker/cli/cli/command"
+	"github.com/docker/cli/cli/flags"
+	"github.com/moby/buildkit/client/llb"
+	"github.com/moby/buildkit/session/filesync"
+	"github.com/openllb/hlb/solver"
+	"github.com/pkg/errors"
+)
+
+type Target struct {
+	Name    string
+	Outputs []Output
+}
+
+type Output struct {
+	Type      OutputType
+	LocalPath string
+	Ref       string
+}
+
+type OutputType int
+
+const (
+	OutputDockerPush OutputType = iota
+	OutputDockerLoad
+	OutputDownload
+	OutputDownloadTarball
+	OutputDownloadOCITarball
+	OutputDownloadDockerTarball
+)
+
+func (cg *CodeGen) outputRequest(ctx context.Context, st llb.State, output Output) error {
+	opts, err := cg.SolveOptions(ctx, st)
+	if err != nil {
+		return err
+	}
+
+	s, err := cg.newSession(ctx)
+	if err != nil {
+		return err
+	}
+
+	switch output.Type {
+	case OutputDockerPush:
+		opts = append(opts, solver.WithPushImage(output.Ref))
+	case OutputDockerLoad:
+		if cg.mw == nil {
+			return errors.WithStack(errors.Errorf("progress.MultiWriter must be provided for dockerLoad"))
+		}
+
+		if cg.dockerCli == nil {
+			cg.dockerCli, err = command.NewDockerCli()
+			if err != nil {
+				return err
+			}
+
+			err = cg.dockerCli.Initialize(flags.NewClientOptions())
+			if err != nil {
+				return err
+			}
+		}
+
+		r, w := io.Pipe()
+
+		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(w)))
+
+		done := make(chan struct{})
+		opts = append(opts, solver.WithWaiter(done))
+
+		go func() {
+			defer close(done)
+
+			resp, err := cg.dockerCli.Client().ImageLoad(ctx, r, true)
+			if err != nil {
+				r.CloseWithError(err)
+				return
+			}
+			defer resp.Body.Close()
+
+			pw := cg.mw.WithPrefix("", false)
+			progress.FromReader(pw, fmt.Sprintf("importing %s to docker", output.Ref), resp.Body)
+		}()
+
+		opts = append(opts, solver.WithDownloadDockerTarball(output.Ref))
+	case OutputDownload:
+		s.Allow(filesync.NewFSSyncTargetDir(output.LocalPath))
+		opts = append(opts, solver.WithDownload(output.LocalPath))
+	case OutputDownloadTarball, OutputDownloadOCITarball, OutputDownloadDockerTarball:
+		f, err := os.Open(output.LocalPath)
+		if err != nil {
+			return err
+		}
+
+		s.Allow(filesync.NewFSSyncTarget(outputFromWriter(f)))
+
+		switch output.Type {
+		case OutputDownloadTarball:
+			opts = append(opts, solver.WithDownloadTarball())
+		case OutputDownloadOCITarball:
+			opts = append(opts, solver.WithDownloadOCITarball())
+		case OutputDownloadDockerTarball:
+			opts = append(opts, solver.WithDownloadDockerTarball(output.Ref))
+		}
+	}
+
+	def, err := st.Marshal(ctx, llb.LinuxAmd64)
+	if err != nil {
+		return err
+	}
+
+	cg.request = cg.request.Peer(solver.NewRequest(s, def, opts...))
+	return nil
+}
diff --git a/hlb.go b/hlb.go
index 89275d2..bf2145b 100644
--- a/hlb.go
+++ b/hlb.go
@@ -10,11 +10,9 @@ import (
 	"github.com/alecthomas/participle/lexer"
 	isatty "github.com/mattn/go-isatty"
 	"github.com/moby/buildkit/client"
-	digest "github.com/opencontainers/go-digest"
 	"github.com/openllb/hlb/checker"
 	"github.com/openllb/hlb/codegen"
 	"github.com/openllb/hlb/module"
-	"github.com/openllb/hlb/parser"
 	"github.com/openllb/hlb/report"
 	"github.com/openllb/hlb/solver"
 )
@@ -27,16 +25,7 @@ func DefaultParseOpts() []ParseOption {
 	return opts
 }
 
-type Target struct {
-	Name           string
-	DockerPushRef  string
-	DockerLoadRef  string
-	DownloadPath   string
-	TarballPath    string
-	OCITarballPath string
-}
-
-func Compile(ctx context.Context, cln *client.Client, p solver.Progress, targets []Target, r io.Reader) (solver.Request, error) {
+func Compile(ctx context.Context, cln *client.Client, p solver.Progress, targets []codegen.Target, r io.Reader) (solver.Request, error) {
 	mod, ib, err := Parse(r, DefaultParseOpts()...)
 	if err != nil {
 		return nil, err
@@ -68,8 +57,6 @@ func Compile(ctx context.Context, cln *client.Client, p solver.Progress, targets
 	}
 
 	var names []string
-	var callTargets []*parser.CallStmt
-
 	for _, target := range targets {
 		obj := mod.Scope.Lookup(target.Name)
 		if obj == nil {
@@ -80,48 +67,6 @@ func Compile(ctx context.Context, cln *client.Client, p solver.Progress, targets
 			return nil, fmt.Errorf("target %q is not defined in %s", target.Name, name)
 		}
 		names = append(names, target.Name)
-
-		outputs := []*parser.Stmt{
-			parser.NewCallStmt(target.Name, nil, nil, nil),
-		}
-
-		if target.DockerPushRef != "" {
-			outputs = append(outputs, parser.NewCallStmt("dockerPush", []*parser.Expr{
-				parser.NewStringExpr(target.DockerPushRef),
-			}, nil, nil))
-		}
-		if target.DockerLoadRef != "" {
-			outputs = append(outputs, parser.NewCallStmt("dockerLoad", []*parser.Expr{
-				parser.NewStringExpr(target.DockerLoadRef),
-			}, nil, nil))
-		}
-		if target.DownloadPath != "" {
-			outputs = append(outputs, parser.NewCallStmt("download", []*parser.Expr{
-				parser.NewStringExpr(target.DownloadPath),
-			}, nil, nil))
-		}
-		if target.TarballPath != "" {
-			outputs = append(outputs, parser.NewCallStmt("downloadTarball", []*parser.Expr{
-				parser.NewStringExpr(target.TarballPath),
-			}, nil, nil))
-		}
-		if target.OCITarballPath != "" {
-			outputs = append(outputs, parser.NewCallStmt("downloadOCITarball", []*parser.Expr{
-				parser.NewStringExpr(target.OCITarballPath),
-			}, nil, nil))
-		}
-
-		targetOverride := target.Name
-		if len(outputs) > 1 {
-			// Generate a target override to plumb the outputs specified from the CLI.
-			targetOverride = digest.FromString(target.Name).String()
-			decl := parser.NewFuncDecl(parser.Filesystem, targetOverride, nil, outputs...)
-			checker.InitScope(mod, decl.Func)
-
-			mod.Decls = append(mod.Decls, decl)
-		}
-
-		callTargets = append(callTargets, parser.NewCallStmt(targetOverride, nil, nil, nil).Call)
 	}
 
 	var opts []codegen.CodeGenOption
@@ -143,7 +88,7 @@ func Compile(ctx context.Context, cln *client.Client, p solver.Progress, targets
 			return err
 		}
 
-		request, err = cg.Generate(ctx, mod, callTargets)
+		request, err = cg.Generate(ctx, mod, targets)
 		if err != nil {
 			return err
 		}


Test patch:
diff --git a/compile_test.go b/compile_test.go
index 6c16519..71c47a2 100644
--- a/compile_test.go
+++ b/compile_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/openllb/hlb/checker"
+	"github.com/openllb/hlb/codegen"
 	"github.com/openllb/hlb/solver"
 	"github.com/stretchr/testify/require"
 )
@@ -140,9 +141,9 @@ func TestCompile(t *testing.T) {
 
 			in := strings.NewReader(cleanup(tc.input))
 
-			var targets []Target
+			var targets []codegen.Target
 			for _, target := range tc.targets {
-				targets = append(targets, Target{Name: target})
+				targets = append(targets, codegen.Target{Name: target})
 			}
 
 			_, err = Compile(ctx, nil, p, targets, in)