From 58aa298581a8439e45d6f36bc15a6b0cca3b6539 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 12:16:56 -0800 Subject: [PATCH 1/5] Fix locking around Fill and Lookup Signed-off-by: Andrea Luzzardi --- dagger/compiler.go | 2 +- dagger/env.go | 12 ----------- dagger/spec.go | 23 ++++----------------- dagger/value.go | 51 ++++++++++++++++------------------------------ 4 files changed, 22 insertions(+), 66 deletions(-) diff --git a/dagger/compiler.go b/dagger/compiler.go index 9ca9d271..1f44e2e0 100644 --- a/dagger/compiler.go +++ b/dagger/compiler.go @@ -18,7 +18,7 @@ import ( // (we call it compiler to avoid confusion with dagger runtime) // Use this instead of cue.Runtime type Compiler struct { - sync.Mutex + sync.RWMutex cue.Runtime spec *Spec } diff --git a/dagger/env.go b/dagger/env.go index 0ab8e9f6..cdeea27b 100644 --- a/dagger/env.go +++ b/dagger/env.go @@ -3,7 +3,6 @@ package dagger import ( "context" "os" - "sync" "cuelang.org/go/cue" cueflow "cuelang.org/go/tools/flow" @@ -144,14 +143,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { return nil, err } - l := sync.Mutex{} - // Cueflow config flowCfg := &cueflow.Config{ UpdateFunc: func(c *cueflow.Controller, t *cueflow.Task) error { - l.Lock() - defer l.Unlock() - if t == nil { return nil } @@ -180,9 +174,6 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { } // Cueflow match func flowMatchFn := func(v cue.Value) (cueflow.Runner, error) { - l.Lock() - defer l.Unlock() - lg := lg. With(). Str("path", v.Path().String()). @@ -200,9 +191,6 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { return nil, err } return cueflow.RunnerFunc(func(t *cueflow.Task) error { - l.Lock() - defer l.Unlock() - return fn(ctx, c, t) }), nil } diff --git a/dagger/spec.go b/dagger/spec.go index 295d89b4..1b555860 100644 --- a/dagger/spec.go +++ b/dagger/spec.go @@ -1,7 +1,6 @@ package dagger import ( - "cuelang.org/go/cue" cueerrors "cuelang.org/go/cue/errors" "github.com/pkg/errors" ) @@ -12,28 +11,14 @@ type Spec struct { } // eg. Validate(op, "#Op") -func (s Spec) Validate(v *Value, defpath string) (err error) { - // Expand cue errors to get full details - // FIXME: there is probably a cleaner way to do this. - defer func() { - if err != nil { - err = errors.New(cueerrors.Details(err, nil)) - } - }() - +func (s Spec) Validate(v *Value, defpath string) error { // Lookup def by name, eg. "#Script" or "#Copy" // See dagger/spec.cue def := s.root.Get(defpath) - if err := def.Validate(); err != nil { - return err - } - merged := def.Unwrap().Fill(v.Value) - if err := merged.Err(); err != nil { - return err - } - if err := merged.Validate(cue.Final()); err != nil { - return err + if err := def.Fill(v.Value); err != nil { + return errors.New(cueerrors.Details(err, nil)) } + return nil } diff --git a/dagger/value.go b/dagger/value.go index d4ee3d62..bb0eea04 100644 --- a/dagger/value.go +++ b/dagger/value.go @@ -18,31 +18,18 @@ type Value struct { inst *cue.Instance } -func (v *Value) Lock() { - if v.cc == nil { - return - } - v.cc.Lock() -} - -func (v *Value) Unlock() { - if v.cc == nil { - return - } - v.cc.Unlock() -} - func (v *Value) Lookup(path ...string) *Value { - v.Lock() - defer v.Unlock() + v.cc.RLock() + defer v.cc.RUnlock() - return v.Wrap(v.Unwrap().LookupPath(cueStringsToCuePath(path...))) + return v.Wrap(v.Value.LookupPath(cueStringsToCuePath(path...))) } func (v *Value) LookupPath(p cue.Path) *Value { - v.Lock() - defer v.Unlock() - return v.Wrap(v.Unwrap().LookupPath(p)) + v.cc.RLock() + defer v.cc.RUnlock() + + return v.Wrap(v.Value.LookupPath(p)) } // FIXME: deprecated by Get() @@ -150,9 +137,13 @@ func (v *Value) Merge(x interface{}, path ...string) (*Value, error) { if xval.Compiler() != v.Compiler() { return nil, fmt.Errorf("can't merge values from different compilers") } - x = xval.Unwrap() + x = xval.Value } - result := v.Wrap(v.Unwrap().Fill(x, path...)) + + v.cc.Lock() + result := v.Wrap(v.Value.Fill(x, path...)) + v.cc.Unlock() + return result, result.Validate() } @@ -226,8 +217,6 @@ func (v *Value) IsConcreteR() bool { // Contrast with cue.Value.MarshalJSON which requires all values // to be concrete. func (v *Value) JSON() JSON { - v.Lock() - defer v.Unlock() var out JSON v.Walk( func(v cue.Value) bool { @@ -267,7 +256,7 @@ func (v *Value) Save(fs FS, filename string) (FS, error) { } func (v *Value) Validate(defs ...string) error { - if err := v.Unwrap().Validate(); err != nil { + if err := v.Value.Validate(); err != nil { return err } if len(defs) == 0 { @@ -289,13 +278,14 @@ func (v *Value) Validate(defs ...string) error { // This is the only method which changes the value in-place. // FIXME this co-exists awkwardly with the rest of Value. func (v *Value) Fill(x interface{}) error { + v.cc.Lock() + defer v.cc.Unlock() + v.Value = v.Value.Fill(x) return v.Validate() } func (v *Value) Source() ([]byte, error) { - v.Lock() - defer v.Unlock() return cueformat.Node(v.Eval().Syntax()) } @@ -313,9 +303,6 @@ func (v *Value) CueInst() *cue.Instance { } func (v *Value) Compiler() *Compiler { - // if v.cc == nil { - // return &Compiler{} - // } return v.cc } @@ -323,10 +310,6 @@ func (v *Value) Wrap(v2 cue.Value) *Value { return wrapValue(v2, v.inst, v.cc) } -func (v *Value) Unwrap() cue.Value { - return v.Value -} - func wrapValue(v cue.Value, inst *cue.Instance, cc *Compiler) *Value { return &Value{ Value: v, From 5e5ef6b843e8919ca4eed6a8fb0096eb820af9b5 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 12:55:42 -0800 Subject: [PATCH 2/5] value wrapper: better isolation of the underlying cue.Value Ported from #17 Signed-off-by: Andrea Luzzardi --- dagger/spec.go | 2 +- dagger/value.go | 437 ++++++++++++++++++++++++++---------------------- 2 files changed, 239 insertions(+), 200 deletions(-) diff --git a/dagger/spec.go b/dagger/spec.go index 1b555860..89f426c3 100644 --- a/dagger/spec.go +++ b/dagger/spec.go @@ -15,7 +15,7 @@ func (s Spec) Validate(v *Value, defpath string) error { // Lookup def by name, eg. "#Script" or "#Copy" // See dagger/spec.cue def := s.root.Get(defpath) - if err := def.Fill(v.Value); err != nil { + if err := def.Fill(v); err != nil { return errors.New(cueerrors.Details(err, nil)) } diff --git a/dagger/value.go b/dagger/value.go index bb0eea04..a7ef7501 100644 --- a/dagger/value.go +++ b/dagger/value.go @@ -9,38 +9,264 @@ import ( "github.com/moby/buildkit/client/llb" ) -// Polyfill for cue.Value. +// Value is a wrapper around cue.Value. // Use instead of cue.Value and cue.Instance type Value struct { - // FIXME: don't embed, cleaner API - cue.Value + val cue.Value cc *Compiler inst *cue.Instance } -func (v *Value) Lookup(path ...string) *Value { - v.cc.RLock() - defer v.cc.RUnlock() - - return v.Wrap(v.Value.LookupPath(cueStringsToCuePath(path...))) +func (v *Value) CueInst() *cue.Instance { + return v.inst } +func (v *Value) Compiler() *Compiler { + return v.cc +} + +func (v *Value) Wrap(v2 cue.Value) *Value { + return wrapValue(v2, v.inst, v.cc) +} + +func wrapValue(v cue.Value, inst *cue.Instance, cc *Compiler) *Value { + return &Value{ + val: v, + cc: cc, + inst: inst, + } +} + +// Fill is a concurrency safe wrapper around cue.Value.Fill() +// This is the only method which changes the value in-place. +func (v *Value) Fill(x interface{}) error { + v.cc.Lock() + defer v.cc.Unlock() + + // If calling Fill() with a Value, we want to use the underlying + // cue.Value to fill. + if val, ok := x.(*Value); ok { + v.val = v.val.Fill(val.val) + } else { + v.val = v.val.Fill(x) + } + return v.Validate() +} + +// LookupPath is a concurrency safe wrapper around cue.Value.LookupPath func (v *Value) LookupPath(p cue.Path) *Value { v.cc.RLock() defer v.cc.RUnlock() - return v.Wrap(v.Value.LookupPath(p)) + return v.Wrap(v.val.LookupPath(p)) } -// FIXME: deprecated by Get() -func (v *Value) LookupTarget(target string) *Value { - return v.LookupPath(cue.ParsePath(target)) +// Lookup is a helper function to lookup by path parts. +func (v *Value) Lookup(path ...string) *Value { + return v.LookupPath(cueStringsToCuePath(path...)) } +// Get is a helper function to lookup by path string func (v *Value) Get(target string) *Value { return v.LookupPath(cue.ParsePath(target)) } +// Proxy function to the underlying cue.Value +func (v *Value) Len() cue.Value { + return v.val.Len() +} + +// Proxy function to the underlying cue.Value +func (v *Value) List() (cue.Iterator, error) { + return v.val.List() +} + +// Proxy function to the underlying cue.Value +func (v *Value) Fields() (*cue.Iterator, error) { + return v.val.Fields() +} + +// Proxy function to the underlying cue.Value +func (v *Value) Struct() (*cue.Struct, error) { + return v.val.Struct() +} + +// Proxy function to the underlying cue.Value +func (v *Value) Exists() bool { + return v.val.Exists() +} + +// Proxy function to the underlying cue.Value +func (v *Value) String() (string, error) { + return v.val.String() +} + +// Proxy function to the underlying cue.Value +func (v *Value) Path() cue.Path { + return v.val.Path() +} + +// Proxy function to the underlying cue.Value +func (v *Value) Decode(x interface{}) error { + return v.val.Decode(x) +} + +func (v *Value) RangeList(fn func(int, *Value) error) error { + it, err := v.List() + if err != nil { + return err + } + i := 0 + for it.Next() { + if err := fn(i, v.Wrap(it.Value())); err != nil { + return err + } + i++ + } + return nil +} + +func (v *Value) RangeStruct(fn func(string, *Value) error) error { + it, err := v.Fields() + if err != nil { + return err + } + for it.Next() { + if err := fn(it.Label(), v.Wrap(it.Value())); err != nil { + return err + } + } + return nil +} + +// FIXME: receive string path? +func (v *Value) Merge(x interface{}, path ...string) (*Value, error) { + if xval, ok := x.(*Value); ok { + if xval.Compiler() != v.Compiler() { + return nil, fmt.Errorf("can't merge values from different compilers") + } + x = xval.val + } + + v.cc.Lock() + result := v.Wrap(v.val.Fill(x, path...)) + v.cc.Unlock() + + return result, result.Validate() +} + +func (v *Value) MergePath(x interface{}, p cue.Path) (*Value, error) { + // FIXME: array indexes and defs are not supported, + // they will be silently converted to regular fields. + // eg. `foo.#bar[0]` will become `foo["#bar"]["0"]` + return v.Merge(x, cuePathToStrings(p)...) +} + +func (v *Value) MergeTarget(x interface{}, target string) (*Value, error) { + return v.MergePath(x, cue.ParsePath(target)) +} + +// Recursive concreteness check. +// Return false if v is not concrete, or contains any +// non-concrete fields or items. +func (v *Value) IsConcreteR() bool { + // FIXME: use Value.Walk + if it, err := v.Fields(); err == nil { + for it.Next() { + w := v.Wrap(it.Value()) + if !w.IsConcreteR() { + return false + } + } + return true + } + if it, err := v.List(); err == nil { + for it.Next() { + w := v.Wrap(it.Value()) + if !w.IsConcreteR() { + return false + } + } + return true + } + dv, _ := v.val.Default() + return v.val.IsConcrete() || dv.IsConcrete() +} + +// Export concrete values to JSON. ignoring non-concrete values. +// Contrast with cue.Value.MarshalJSON which requires all values +// to be concrete. +func (v *Value) JSON() JSON { + var out JSON + v.val.Walk( + func(v cue.Value) bool { + b, err := v.MarshalJSON() + if err == nil { + newOut, err := out.Set(b, cuePathToStrings(v.Path())...) + if err == nil { + out = newOut + } + return false + } + return true + }, + nil, + ) + return out +} + +func (v *Value) SaveJSON(fs FS, filename string) FS { + return fs.Change(func(st llb.State) llb.State { + return st.File( + llb.Mkfile(filename, 0600, v.JSON()), + ) + }) +} + +func (v *Value) Save(fs FS, filename string) (FS, error) { + src, err := v.Source() + if err != nil { + return fs, err + } + return fs.Change(func(st llb.State) llb.State { + return st.File( + llb.Mkfile(filename, 0600, src), + ) + }), nil +} + +func (v *Value) Validate(defs ...string) error { + if err := v.val.Validate(); err != nil { + return err + } + if len(defs) == 0 { + return nil + } + spec, err := v.Compiler().Spec() + if err != nil { + return err + } + for _, def := range defs { + if err := spec.Validate(v, def); err != nil { + return err + } + } + return nil +} + +func (v *Value) Source() ([]byte, error) { + return cueformat.Node(v.val.Eval().Syntax()) +} + +func (v *Value) IsEmptyStruct() bool { + if st, err := v.Struct(); err == nil { + if st.Len() == 0 { + return true + } + } + return false +} + // Component returns the component value if v is a valid dagger component or an error otherwise. // If no '#dagger' annotation is present, os.ErrNotExist // is returned. @@ -130,190 +356,3 @@ func (v *Value) Spec() (*Spec, error) { root: v, }, nil } - -// FIXME: receive string path? -func (v *Value) Merge(x interface{}, path ...string) (*Value, error) { - if xval, ok := x.(*Value); ok { - if xval.Compiler() != v.Compiler() { - return nil, fmt.Errorf("can't merge values from different compilers") - } - x = xval.Value - } - - v.cc.Lock() - result := v.Wrap(v.Value.Fill(x, path...)) - v.cc.Unlock() - - return result, result.Validate() -} - -func (v *Value) MergePath(x interface{}, p cue.Path) (*Value, error) { - // FIXME: array indexes and defs are not supported, - // they will be silently converted to regular fields. - // eg. `foo.#bar[0]` will become `foo["#bar"]["0"]` - return v.Merge(x, cuePathToStrings(p)...) -} - -func (v *Value) MergeTarget(x interface{}, target string) (*Value, error) { - return v.MergePath(x, cue.ParsePath(target)) -} - -func (v *Value) RangeList(fn func(int, *Value) error) error { - it, err := v.List() - if err != nil { - return err - } - i := 0 - for it.Next() { - if err := fn(i, v.Wrap(it.Value())); err != nil { - return err - } - i++ - } - return nil -} - -func (v *Value) RangeStruct(fn func(string, *Value) error) error { - it, err := v.Fields() - if err != nil { - return err - } - for it.Next() { - if err := fn(it.Label(), v.Wrap(it.Value())); err != nil { - return err - } - } - return nil -} - -// Recursive concreteness check. -// Return false if v is not concrete, or contains any -// non-concrete fields or items. -func (v *Value) IsConcreteR() bool { - // FIXME: use Value.Walk - if it, err := v.Fields(); err == nil { - for it.Next() { - w := v.Wrap(it.Value()) - if !w.IsConcreteR() { - return false - } - } - return true - } - if it, err := v.List(); err == nil { - for it.Next() { - w := v.Wrap(it.Value()) - if !w.IsConcreteR() { - return false - } - } - return true - } - dv, _ := v.Default() - return v.IsConcrete() || dv.IsConcrete() -} - -// Export concrete values to JSON. ignoring non-concrete values. -// Contrast with cue.Value.MarshalJSON which requires all values -// to be concrete. -func (v *Value) JSON() JSON { - var out JSON - v.Walk( - func(v cue.Value) bool { - b, err := v.MarshalJSON() - if err == nil { - newOut, err := out.Set(b, cuePathToStrings(v.Path())...) - if err == nil { - out = newOut - } - return false - } - return true - }, - nil, - ) - return out -} - -func (v *Value) SaveJSON(fs FS, filename string) FS { - return fs.Change(func(st llb.State) llb.State { - return st.File( - llb.Mkfile(filename, 0600, v.JSON()), - ) - }) -} - -func (v *Value) Save(fs FS, filename string) (FS, error) { - src, err := v.Source() - if err != nil { - return fs, err - } - return fs.Change(func(st llb.State) llb.State { - return st.File( - llb.Mkfile(filename, 0600, src), - ) - }), nil -} - -func (v *Value) Validate(defs ...string) error { - if err := v.Value.Validate(); err != nil { - return err - } - if len(defs) == 0 { - return nil - } - spec, err := v.Compiler().Spec() - if err != nil { - return err - } - for _, def := range defs { - if err := spec.Validate(v, def); err != nil { - return err - } - } - return nil -} - -// Value implements Fillable. -// This is the only method which changes the value in-place. -// FIXME this co-exists awkwardly with the rest of Value. -func (v *Value) Fill(x interface{}) error { - v.cc.Lock() - defer v.cc.Unlock() - - v.Value = v.Value.Fill(x) - return v.Validate() -} - -func (v *Value) Source() ([]byte, error) { - return cueformat.Node(v.Eval().Syntax()) -} - -func (v *Value) IsEmptyStruct() bool { - if st, err := v.Struct(); err == nil { - if st.Len() == 0 { - return true - } - } - return false -} - -func (v *Value) CueInst() *cue.Instance { - return v.inst -} - -func (v *Value) Compiler() *Compiler { - return v.cc -} - -func (v *Value) Wrap(v2 cue.Value) *Value { - return wrapValue(v2, v.inst, v.cc) -} - -func wrapValue(v cue.Value, inst *cue.Instance, cc *Compiler) *Value { - return &Value{ - Value: v, - cc: cc, - inst: inst, - } -} From 644713e3f91fa4b93b5c3e4ac0e272a547a58be4 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 13:01:45 -0800 Subject: [PATCH 3/5] spec: simplify signature Signed-off-by: Andrea Luzzardi --- dagger/compiler.go | 13 ++++++------- dagger/value.go | 12 +++--------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/dagger/compiler.go b/dagger/compiler.go index 1f44e2e0..4aa30856 100644 --- a/dagger/compiler.go +++ b/dagger/compiler.go @@ -27,20 +27,19 @@ func (cc *Compiler) Cue() *cue.Runtime { return &(cc.Runtime) } -func (cc *Compiler) Spec() (*Spec, error) { +func (cc *Compiler) Spec() *Spec { if cc.spec != nil { - return cc.spec, nil + return cc.spec } v, err := cc.Compile("spec.cue", DaggerSpec) if err != nil { - return nil, err + panic(err) } - spec, err := v.Spec() + cc.spec, err = v.Spec() if err != nil { - return nil, err + panic(err) } - cc.spec = spec - return spec, nil + return cc.spec } // Compile an empty struct diff --git a/dagger/value.go b/dagger/value.go index a7ef7501..623ce007 100644 --- a/dagger/value.go +++ b/dagger/value.go @@ -242,10 +242,7 @@ func (v *Value) Validate(defs ...string) error { if len(defs) == 0 { return nil } - spec, err := v.Compiler().Spec() - if err != nil { - return err - } + spec := v.Compiler().Spec() for _, def := range defs { if err := spec.Validate(v, def); err != nil { return err @@ -324,11 +321,8 @@ func (v *Value) ScriptOrComponent() (interface{}, error) { func (v *Value) Op() (*Op, error) { // Merge #Op definition from spec to get default values - spec, err := v.Compiler().Spec() - if err != nil { - return nil, err - } - v, err = spec.Get("#Op").Merge(v) + spec := v.Compiler().Spec() + v, err := spec.Get("#Op").Merge(v) if err != nil { return nil, err } From d22326b512612feda8f14478753576e7a0a1ae32 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 13:03:28 -0800 Subject: [PATCH 4/5] export: log exported value Signed-off-by: Andrea Luzzardi --- dagger/op.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dagger/op.go b/dagger/op.go index 72ea0b02..7a376bc2 100644 --- a/dagger/op.go +++ b/dagger/op.go @@ -202,6 +202,12 @@ func (op *Op) Export(ctx context.Context, fs FS, out Fillable) (FS, error) { } switch format { case "string": + log. + Ctx(ctx). + Debug(). + Bytes("contents", contents). + Msg("exporting string") + if err := out.Fill(string(contents)); err != nil { return fs, err } @@ -210,6 +216,13 @@ func (op *Op) Export(ctx context.Context, fs FS, out Fillable) (FS, error) { if err := json.Unmarshal(contents, &o); err != nil { return fs, err } + + log. + Ctx(ctx). + Debug(). + Interface("contents", o). + Msg("exporting json") + if err := out.Fill(o); err != nil { return fs, err } From 425bbace9e74d7940e47862a6a0f06bca789d9fa Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 14:24:06 -0800 Subject: [PATCH 5/5] test: fix fetch container test Signed-off-by: Andrea Luzzardi --- examples/tests/fetch-container/exist/main.cue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/tests/fetch-container/exist/main.cue b/examples/tests/fetch-container/exist/main.cue index 641411c1..bfd36bee 100644 --- a/examples/tests/fetch-container/exist/main.cue +++ b/examples/tests/fetch-container/exist/main.cue @@ -31,7 +31,7 @@ busybox4: { #dagger: compute: [ { do: "fetch-container" - ref: "busyboxa@sha256:e2af53705b841ace3ab3a44998663d4251d33ee8a9acaf71b66df4ae01c3bbe7" + ref: "busybox@sha256:e2af53705b841ace3ab3a44998663d4251d33ee8a9acaf71b66df4ae01c3bbe7" }, ] }