From ef8a90df3bc1a3009381eb3fd10767f468fcefc2 Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Sun, 17 Jul 2022 15:05:39 +0800 Subject: [PATCH] feat: Refactor with Builder.Options (#615) * feat: Refactor with Builder.Options Signed-off-by: Ce Gao * fix: Fix bug Signed-off-by: Ce Gao --- pkg/app/build.go | 25 +++++++++--- pkg/app/up.go | 37 +++++++++++------- pkg/builder/build.go | 8 ++-- pkg/builder/builder.go | 67 ++++++++++++++++---------------- pkg/builder/builder_test.go | 18 +++++---- pkg/builder/testdata/build.MIDI | 11 ------ pkg/builder/testdata/config.MIDI | 0 7 files changed, 89 insertions(+), 77 deletions(-) delete mode 100644 pkg/builder/testdata/build.MIDI delete mode 100644 pkg/builder/testdata/config.MIDI diff --git a/pkg/app/build.go b/pkg/app/build.go index 0dcda5ed5..768926acc 100644 --- a/pkg/app/build.go +++ b/pkg/app/build.go @@ -83,11 +83,11 @@ func build(clicontext *cli.Context) error { return errors.Wrap(err, "failed to get absolute path of the build context") } - filename, funcname, err := builder.ParseFromStr(clicontext.String("from")) + fileName, funcName, err := builder.ParseFromStr(clicontext.String("from")) if err != nil { return err } - manifest, err := filepath.Abs(filepath.Join(buildContext, filename)) + manifest, err := filepath.Abs(filepath.Join(buildContext, fileName)) if err != nil { return errors.Wrap(err, "failed to get absolute path of the build file") } @@ -116,11 +116,26 @@ func build(clicontext *cli.Context) error { }) debug := clicontext.Bool("debug") output := clicontext.String("output") + + opt := builder.Options{ + ManifestFilePath: manifest, + ConfigFilePath: cfg, + BuildFuncName: funcName, + BuildContextDir: buildContext, + Tag: tag, + OutputOpts: output, + PubKeyPath: clicontext.Path("public-key"), + ProgressMode: "auto", + } + if debug { + opt.ProgressMode = "plain" + } + logger.WithFields(logrus.Fields{ - "output": output, + "builder-options": opt, }).Debug("starting build command") - builder, err := builder.New(clicontext.Context, cfg, - manifest, funcname, buildContext, tag, output, debug, clicontext.Path("public-key")) + + builder, err := builder.New(clicontext.Context, opt) if err != nil { return errors.Wrap(err, "failed to create the builder") } diff --git a/pkg/app/up.go b/pkg/app/up.go index 7ac761d1e..674eb4129 100644 --- a/pkg/app/up.go +++ b/pkg/app/up.go @@ -21,12 +21,10 @@ import ( "github.com/cockroachdb/errors" "github.com/sirupsen/logrus" - "github.com/spf13/viper" "github.com/urfave/cli/v2" "github.com/tensorchord/envd/pkg/builder" "github.com/tensorchord/envd/pkg/docker" - "github.com/tensorchord/envd/pkg/flag" "github.com/tensorchord/envd/pkg/home" "github.com/tensorchord/envd/pkg/lang/ir" "github.com/tensorchord/envd/pkg/ssh" @@ -111,12 +109,12 @@ func up(clicontext *cli.Context) error { if err != nil { return errors.Wrap(err, "failed to get absolute path of the build context") } - filename, funcname, err := builder.ParseFromStr(clicontext.String("from")) + fileName, funcName, err := builder.ParseFromStr(clicontext.String("from")) if err != nil { return err } - manifest, err := filepath.Abs(filepath.Join(buildContext, filename)) + manifest, err := filepath.Abs(filepath.Join(buildContext, fileName)) if err != nil { return errors.Wrap(err, "failed to get absolute path of the build file") } @@ -139,21 +137,30 @@ func up(clicontext *cli.Context) error { ctr := fileutil.Base(buildContext) detach := clicontext.Bool("detach") + debug := clicontext.Bool("debug") + output := "" + + opt := builder.Options{ + ManifestFilePath: manifest, + ConfigFilePath: config, + BuildFuncName: funcName, + BuildContextDir: buildContext, + Tag: tag, + OutputOpts: output, + PubKeyPath: clicontext.Path("public-key"), + ProgressMode: "auto", + } + if debug { + opt.ProgressMode = "plain" + } logger := logrus.WithFields(logrus.Fields{ - "build-context": buildContext, - "build-file": manifest, - "config": config, - "tag": tag, - "container-name": ctr, - "detach": detach, - flag.FlagBuildkitdImage: viper.GetString(flag.FlagBuildkitdImage), + "builder-options": opt, + "container-name": ctr, + "detach": detach, }) logger.Debug("starting up command") - debug := clicontext.Bool("debug") - output := "" - builder, err := builder.New(clicontext.Context, config, manifest, funcname, - buildContext, tag, output, debug, clicontext.Path("public-key")) + builder, err := builder.New(clicontext.Context, opt) if err != nil { return errors.Wrap(err, "failed to create the builder") } diff --git a/pkg/builder/build.go b/pkg/builder/build.go index 506cac80c..815e2eca9 100644 --- a/pkg/builder/build.go +++ b/pkg/builder/build.go @@ -24,11 +24,11 @@ import ( func (b generalBuilder) BuildFunc() func(ctx context.Context, c client.Client) (*client.Result, error) { return func(ctx context.Context, c client.Client) (*client.Result, error) { depsFiles := []string{ - b.pubKeyPath, - b.configFilePath, - b.manifestFilePath, + b.PubKeyPath, + b.ConfigFilePath, + b.ManifestFilePath, } - isUpdated, err := b.CheckDepsFileUpdate(ctx, b.tag, depsFiles) + isUpdated, err := b.CheckDepsFileUpdate(ctx, b.Tag, depsFiles) if err != nil { b.logger.Debugf("failed to check manifest update: %s", err) } diff --git a/pkg/builder/builder.go b/pkg/builder/builder.go index 359da0c29..c959af6c5 100644 --- a/pkg/builder/builder.go +++ b/pkg/builder/builder.go @@ -44,15 +44,27 @@ type Builder interface { NumGPUs() int } -type generalBuilder struct { - manifestFilePath string - configFilePath string - progressMode string - tag string - buildContextDir string - buildfuncname string - pubKeyPath string +type Options struct { + // ManifestFilePath is the path to the manifest file `build.envd`. + ManifestFilePath string + // ConfigFilePath is the path to the config file `config.envd`. + ConfigFilePath string + // ProgressMode is the output mode (auto, plain). + ProgressMode string + // Tag is the name of the image. + Tag string + // BuildContextDir is the directory of the build context. + BuildContextDir string + // BuildFuncName is the name of the build func. + BuildFuncName string + // PubKeyPath is the path to the ssh public key. + PubKeyPath string + // OutputOpts is the output options. + OutputOpts string +} +type generalBuilder struct { + Options entries []client.ExportEntry logger *logrus.Entry @@ -60,10 +72,8 @@ type generalBuilder struct { buildkitd.Client } -func New(ctx context.Context, configFilePath, manifestFilePath, funcname, - buildContextDir, tag string, output string, debug bool, - pubKeyPath string) (Builder, error) { - entries, err := parseOutput(output) +func New(ctx context.Context, opt Options) (Builder, error) { + entries, err := parseOutput(opt.OutputOpts) if err != nil { return nil, errors.Wrap(err, "failed to parse output") } @@ -80,22 +90,11 @@ func New(ctx context.Context, configFilePath, manifestFilePath, funcname, return nil, errors.New("only one output type is supported") } - var mode string = "auto" - if debug { - mode = "plain" - } - b := &generalBuilder{ - manifestFilePath: manifestFilePath, - buildfuncname: funcname, - configFilePath: configFilePath, - buildContextDir: buildContextDir, - entries: entries, - pubKeyPath: pubKeyPath, - progressMode: mode, - tag: tag, + Options: opt, + entries: entries, logger: logrus.WithFields(logrus.Fields{ - "tag": tag, + "tag": opt.Tag, }), } @@ -109,7 +108,7 @@ func New(ctx context.Context, configFilePath, manifestFilePath, funcname, } b.Client = cli - b.Interpreter = starlark.NewInterpreter(buildContextDir) + b.Interpreter = starlark.NewInterpreter(opt.BuildContextDir) return b, nil } @@ -154,7 +153,7 @@ func (b generalBuilder) CheckDepsFileUpdate(ctx context.Context, tag string, dep } func (b generalBuilder) Build(ctx context.Context) error { - pw, err := progresswriter.NewPrinter(ctx, os.Stdout, b.progressMode) + pw, err := progresswriter.NewPrinter(ctx, os.Stdout, b.ProgressMode) if err != nil { return errors.Wrap(err, "failed to create progress writer") } @@ -167,11 +166,11 @@ func (b generalBuilder) Build(ctx context.Context) error { func (b generalBuilder) interpret() error { // Evaluate config first. - if _, err := b.ExecFile(b.configFilePath, ""); err != nil { + if _, err := b.ExecFile(b.ConfigFilePath, ""); err != nil { return errors.Wrap(err, "failed to exec starlark file") } - if _, err := b.ExecFile(b.manifestFilePath, b.buildfuncname); err != nil { + if _, err := b.ExecFile(b.ManifestFilePath, b.BuildFuncName); err != nil { return errors.Wrap(err, "failed to exec starlark file") } return nil @@ -181,7 +180,7 @@ func (b generalBuilder) compile(ctx context.Context) (*llb.Definition, error) { if err := b.interpret(); err != nil { return nil, errors.Wrap(err, "failed to interpret") } - def, err := ir.Compile(ctx, fileutil.Base(b.buildContextDir), b.pubKeyPath) + def, err := ir.Compile(ctx, fileutil.Base(b.BuildContextDir), b.PubKeyPath) if err != nil { return nil, errors.Wrap(err, "failed to compile build.envd") } @@ -198,9 +197,9 @@ func (b generalBuilder) imageConfig(ctx context.Context) (string, error) { if err != nil { return "", errors.Wrap(err, "failed to get expose ports") } - labels[types.ImageLabelContext] = b.buildContextDir + labels[types.ImageLabelContext] = b.BuildContextDir - ep, err := ir.Entrypoint(b.buildContextDir) + ep, err := ir.Entrypoint(b.BuildContextDir) if err != nil { return "", errors.Wrap(err, "failed to get entrypoint") } @@ -235,7 +234,7 @@ func (b generalBuilder) build(ctx context.Context, pw progresswriter.Writer) err entry = client.ExportEntry{ Type: client.ExporterDocker, Attrs: map[string]string{ - "name": b.tag, + "name": b.Tag, // Ref https://github.com/r2d4/mockerfile/blob/140c6a912bbfdae220febe59ab535ef0acba0e1f/pkg/build/build.go#L65 "containerimage.config": imageConfig, }, diff --git a/pkg/builder/builder_test.go b/pkg/builder/builder_test.go index 43b4d7618..8f5d3e3a5 100644 --- a/pkg/builder/builder_test.go +++ b/pkg/builder/builder_test.go @@ -52,12 +52,14 @@ var _ = Describe("Builder", func() { ctrlStarlark := gomock.NewController(GinkgoT()) pub := sshconfig.GetPublicKey() b = &generalBuilder{ - manifestFilePath: manifestFilePath, - configFilePath: configFilePath, - progressMode: "plain", - tag: tag, - buildfuncname: "build", - pubKeyPath: pub, + Options: Options{ + ManifestFilePath: manifestFilePath, + ConfigFilePath: configFilePath, + ProgressMode: "plain", + Tag: tag, + BuildFuncName: "build", + PubKeyPath: pub, + }, logger: logrus.WithFields(logrus.Fields{ "tag": tag, }), @@ -82,7 +84,7 @@ var _ = Describe("Builder", func() { gomock.Any(), gomock.Eq("envd"), gomock.Any(), gomock.Any()). Return(nil, errors.New("build error")) - pw, err := progresswriter.NewPrinter(context.TODO(), os.Stdout, b.progressMode) + pw, err := progresswriter.NewPrinter(context.TODO(), os.Stdout, b.ProgressMode) Expect(err).NotTo(HaveOccurred()) close(pw.Status()) @@ -105,7 +107,7 @@ var _ = Describe("Builder", func() { gomock.Any(), gomock.Eq("envd"), gomock.Any(), gomock.Any()). Return(nil, nil) - pw, err := progresswriter.NewPrinter(context.TODO(), os.Stdout, b.progressMode) + pw, err := progresswriter.NewPrinter(context.TODO(), os.Stdout, b.ProgressMode) Expect(err).NotTo(HaveOccurred()) close(pw.Status()) diff --git a/pkg/builder/testdata/build.MIDI b/pkg/builder/testdata/build.MIDI deleted file mode 100644 index a8cbbbd9a..000000000 --- a/pkg/builder/testdata/build.MIDI +++ /dev/null @@ -1,11 +0,0 @@ -base(os="ubuntu20.04", language="python3") -pip_package(name = [ - "jupyter", -]) -pip_package(name = [ - "ormb", -]) -install_package(name = [ - "gcc" -]) -cuda(version="11.2", cudnn="8") diff --git a/pkg/builder/testdata/config.MIDI b/pkg/builder/testdata/config.MIDI deleted file mode 100644 index e69de29bb..000000000