From 0181ac61dad0022ffb85a4210067bb1faa0596ac Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 24 May 2016 12:03:05 -0400 Subject: [PATCH] DeepCopy should only generate types in k8s.io/kubernetes Also make generation more selective (require opt in) to prevent lots of unnecessary generated deep copies. --- cmd/libs/go2idl/args/args.go | 1 + .../deepcopy-gen/generators/deepcopy.go | 116 +++++++++++++++--- cmd/libs/go2idl/deepcopy-gen/main.go | 18 +++ pkg/api/resource/quantity.go | 1 + pkg/runtime/types.go | 3 + pkg/util/intstr/intstr.go | 1 + 6 files changed, 121 insertions(+), 19 deletions(-) diff --git a/cmd/libs/go2idl/args/args.go b/cmd/libs/go2idl/args/args.go index 9ca3497a496..85384975326 100644 --- a/cmd/libs/go2idl/args/args.go +++ b/cmd/libs/go2idl/args/args.go @@ -97,6 +97,7 @@ func (g *GeneratorArgs) NewBuilder() (*parser.Builder, error) { b.AddBuildTags("ignore_autogenerated") for _, d := range g.InputDirs { + d = strings.TrimLeft(d, "+-*") if g.Recursive { if err := b.AddDirRecursive(d); err != nil { return nil, fmt.Errorf("unable to add directory %q: %v", d, err) diff --git a/cmd/libs/go2idl/deepcopy-gen/generators/deepcopy.go b/cmd/libs/go2idl/deepcopy-gen/generators/deepcopy.go index d4cd5460e93..f6798d27194 100644 --- a/cmd/libs/go2idl/deepcopy-gen/generators/deepcopy.go +++ b/cmd/libs/go2idl/deepcopy-gen/generators/deepcopy.go @@ -19,6 +19,7 @@ package generators import ( "fmt" "io" + "path" "path/filepath" "strings" @@ -31,6 +32,15 @@ import ( "github.com/golang/glog" ) +// Constraints is a set of optional limitations on what deep copy will generate. +type Constraints struct { + // PackageConstraints is an optional set of package prefixes that constrain which types + // will have inline deep copy methods generated for. Any type outside of these packages + // (if specified) will not have a function generated and will result in a call to the + // cloner.DeepCopy method. + PackageConstraints []string +} + // TODO: This is created only to reduce number of changes in a single PR. // Remove it and use PublicNamer instead. func deepCopyNamer() *namer.NameStrategy { @@ -62,7 +72,32 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat glog.Fatalf("Failed loading boilerplate: %v", err) } - inputs := sets.NewString(arguments.InputDirs...) + initInputs := sets.NewString() + explicitInputs := sets.NewString() + inputs := sets.NewString() + for _, s := range arguments.InputDirs { + switch { + case strings.HasPrefix(s, "+"): + // packages with '+' prefix get functions generated for everything except gencopy=false, but + // no init function + s = strings.TrimPrefix(s, "+") + inputs.Insert(s) + case strings.HasPrefix(s, "-"): + // packages with '-' prefix only get functions generated for those with gencopy=true + s = strings.TrimPrefix(s, "-") + inputs.Insert(s) + explicitInputs.Insert(s) + default: + inputs.Insert(s) + initInputs.Insert(s) + } + } + + var restrictRange []string + if c, ok := arguments.CustomArgs.(Constraints); ok { + restrictRange = c.PackageConstraints + } + packages := generator.Packages{} header := append([]byte( `// +build !ignore_autogenerated @@ -76,11 +111,29 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat for _, p := range context.Universe { copyableType := false for _, t := range p.Types { - if copyableWithinPackage(t) && inputs.Has(t.Name.Package) { + if copyableWithinPackage(t, explicitInputs.Has(t.Name.Package)) && inputs.Has(t.Name.Package) { copyableType = true } } if copyableType { + // TODO: replace this with a more sophisticated algorithm that generates private copy methods + // (like auto_DeepCopy_...) for any type that is outside of the PackageConstraints. That would + // avoid having to make a reflection call. + canInlineTypeFn := func(c *generator.Context, t *types.Type) bool { + // types must be public structs or have a custom DeepCopy_ already defined + if !copyableWithinPackage(t, explicitInputs.Has(t.Name.Package)) && !publicCopyFunctionDefined(c, t) { + return false + } + + // only packages within the restricted range can be inlined + for _, s := range restrictRange { + if strings.HasPrefix(t.Name.Package, s) { + return true + } + } + return false + } + path := p.Path packages = append(packages, &generator.DefaultPackage{ @@ -90,7 +143,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { generators = []generator.Generator{} generators = append( - generators, NewGenDeepCopy("deep_copy_generated", path, inputs.Has(path))) + generators, NewGenDeepCopy("deep_copy_generated", path, initInputs.Has(path), explicitInputs.Has(path), canInlineTypeFn)) return generators }, FilterFunc: func(c *generator.Context, t *types.Type) bool { @@ -102,6 +155,9 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat return packages } +// CanInlineTypeFunc should return true if the provided type can be converted to a function call +type CanInlineTypeFunc func(*generator.Context, *types.Type) bool + const ( apiPackagePath = "k8s.io/kubernetes/pkg/api" conversionPackagePath = "k8s.io/kubernetes/pkg/conversion" @@ -110,23 +166,30 @@ const ( // genDeepCopy produces a file with autogenerated deep-copy functions. type genDeepCopy struct { generator.DefaultGen - targetPackage string - imports namer.ImportTracker - typesForInit []*types.Type - generateInitFunc bool + + targetPackage string + imports namer.ImportTracker + typesForInit []*types.Type + generateInitFunc bool + requireExplicitTag bool + canInlineTypeFn CanInlineTypeFunc + + context *generator.Context globalVariables map[string]interface{} } -func NewGenDeepCopy(sanitizedName, targetPackage string, generateInitFunc bool) generator.Generator { +func NewGenDeepCopy(sanitizedName, targetPackage string, generateInitFunc, requireExplicitTag bool, canInlineTypeFn CanInlineTypeFunc) generator.Generator { return &genDeepCopy{ DefaultGen: generator.DefaultGen{ OptionalName: sanitizedName, }, - targetPackage: targetPackage, - imports: generator.NewImportTracker(), - typesForInit: make([]*types.Type, 0), - generateInitFunc: generateInitFunc, + targetPackage: targetPackage, + imports: generator.NewImportTracker(), + typesForInit: make([]*types.Type, 0), + generateInitFunc: generateInitFunc, + requireExplicitTag: requireExplicitTag, + canInlineTypeFn: canInlineTypeFn, } } @@ -137,15 +200,29 @@ func (g *genDeepCopy) Namers(c *generator.Context) namer.NameSystems { func (g *genDeepCopy) Filter(c *generator.Context, t *types.Type) bool { // Filter out all types not copyable within the package. - copyable := copyableWithinPackage(t) + copyable := copyableWithinPackage(t, g.requireExplicitTag) if copyable { g.typesForInit = append(g.typesForInit, t) } return copyable } -func copyableWithinPackage(t *types.Type) bool { - if types.ExtractCommentTags("+", t.CommentLines)["gencopy"] == "false" { +// publicCopyFunctionDefined returns true if a DeepCopy function has already been defined in a given +// package, which allows more efficient deep copy implementations to be defined by the caller. +func publicCopyFunctionDefined(c *generator.Context, t *types.Type) bool { + p, ok := c.Universe[t.Name.Package] + if !ok { + return false + } + return p.Functions["DeepCopy_"+path.Base(t.Name.Package)+"_"+t.Name.Name] != nil +} + +func copyableWithinPackage(t *types.Type, explicitCopyRequired bool) bool { + tag := types.ExtractCommentTags("+", t.CommentLines)["gencopy"] + if tag == "false" { + return false + } + if explicitCopyRequired && tag != "true" { return false } // TODO: Consider generating functions for other kinds too. @@ -204,6 +281,7 @@ func (g *genDeepCopy) funcNameTmpl(t *types.Type) string { } func (g *genDeepCopy) Init(c *generator.Context, w io.Writer) error { + g.context = c cloner := c.Universe.Type(types.Name{Package: conversionPackagePath, Name: "Cloner"}) g.imports.AddType(cloner) g.globalVariables = map[string]interface{}{ @@ -282,7 +360,7 @@ func (g *genDeepCopy) doMap(t *types.Type, sw *generator.SnippetWriter) { if t.Elem.IsAssignable() { sw.Do("(*out)[key] = val\n", nil) } else { - if copyableWithinPackage(t.Elem) { + if g.canInlineTypeFn(g.context, t.Elem) { sw.Do("newVal := new($.|raw$)\n", t.Elem) funcName := g.funcNameTmpl(t.Elem) sw.Do(fmt.Sprintf("if err := %s(val, newVal, c); err != nil {\n", funcName), argsFromType(t.Elem)) @@ -313,7 +391,7 @@ func (g *genDeepCopy) doSlice(t *types.Type, sw *generator.SnippetWriter) { sw.Do("for i := range in {\n", nil) if t.Elem.IsAssignable() { sw.Do("(*out)[i] = in[i]\n", nil) - } else if copyableWithinPackage(t.Elem) { + } else if g.canInlineTypeFn(g.context, t.Elem) { funcName := g.funcNameTmpl(t.Elem) sw.Do(fmt.Sprintf("if err := %s(in[i], &(*out)[i], c); err != nil {\n", funcName), argsFromType(t.Elem)) sw.Do("return err\n", nil) @@ -346,7 +424,7 @@ func (g *genDeepCopy) doStruct(t *types.Type, sw *generator.SnippetWriter) { sw.Do("out.$.name$ = nil\n", args) sw.Do("}\n", nil) case types.Struct: - if copyableWithinPackage(m.Type) { + if g.canInlineTypeFn(g.context, m.Type) { funcName := g.funcNameTmpl(m.Type) sw.Do(fmt.Sprintf("if err := %s(in.$.name$, &out.$.name$, c); err != nil {\n", funcName), args) sw.Do("return err\n", nil) @@ -383,7 +461,7 @@ func (g *genDeepCopy) doPointer(t *types.Type, sw *generator.SnippetWriter) { sw.Do("*out = new($.Elem|raw$)\n", t) if t.Elem.Kind == types.Builtin { sw.Do("**out = *in", nil) - } else if copyableWithinPackage(t.Elem) { + } else if g.canInlineTypeFn(g.context, t.Elem) { funcName := g.funcNameTmpl(t.Elem) sw.Do(fmt.Sprintf("if err := %s(*in, *out, c); err != nil {\n", funcName), argsFromType(t.Elem)) sw.Do("return err\n", nil) diff --git a/cmd/libs/go2idl/deepcopy-gen/main.go b/cmd/libs/go2idl/deepcopy-gen/main.go index 4d39c8a1c13..b0cc9725dbf 100644 --- a/cmd/libs/go2idl/deepcopy-gen/main.go +++ b/cmd/libs/go2idl/deepcopy-gen/main.go @@ -31,6 +31,11 @@ import ( func main() { arguments := args.Default() + arguments.CustomArgs = generators.Constraints{ + // Types outside of this package will be inlined. + PackageConstraints: []string{"k8s.io/kubernetes/"}, + } + // Override defaults. These are Kubernetes specific input locations. arguments.InputDirs = []string{ "k8s.io/kubernetes/pkg/api", @@ -56,6 +61,19 @@ func main() { "k8s.io/kubernetes/pkg/apis/rbac/v1alpha1", "k8s.io/kubernetes/federation/apis/federation", "k8s.io/kubernetes/federation/apis/federation/v1alpha1", + + // generate all types, but do not register them + "+k8s.io/kubernetes/pkg/api/unversioned", + + "-k8s.io/kubernetes/pkg/api/meta", + "-k8s.io/kubernetes/pkg/api/meta/metatypes", + "-k8s.io/kubernetes/pkg/api/resource", + "-k8s.io/kubernetes/pkg/conversion", + "-k8s.io/kubernetes/pkg/labels", + "-k8s.io/kubernetes/pkg/runtime", + "-k8s.io/kubernetes/pkg/runtime/serializer", + "-k8s.io/kubernetes/pkg/util/intstr", + "-k8s.io/kubernetes/pkg/util/sets", } if err := arguments.Execute( diff --git a/pkg/api/resource/quantity.go b/pkg/api/resource/quantity.go index ddff16436d1..8784bb19a50 100644 --- a/pkg/api/resource/quantity.go +++ b/pkg/api/resource/quantity.go @@ -87,6 +87,7 @@ import ( // writing some sort of special handling code in the hopes that that will // cause implementors to also use a fixed point implementation. // +// +gencopy=false // +protobuf=true // +protobuf.embed=string // +protobuf.options.marshal=false diff --git a/pkg/runtime/types.go b/pkg/runtime/types.go index 33f7c3630fa..e646d2afaf9 100644 --- a/pkg/runtime/types.go +++ b/pkg/runtime/types.go @@ -40,6 +40,7 @@ import ( // TypeMeta is provided here for convenience. You may use it directly from this package or define // your own with the same fields. // +// +gencopy=true // +protobuf=true type TypeMeta struct { APIVersion string `json:"apiVersion,omitempty" yaml:"apiVersion,omitempty" protobuf:"bytes,1,opt,name=apiVersion"` @@ -92,6 +93,7 @@ const ( // in the Object. (TODO: In the case where the object is of an unknown type, a // runtime.Unknown object will be created and stored.) // +// +gencopy=true // +protobuf=true type RawExtension struct { // Raw is the underlying serialization of this object. @@ -109,6 +111,7 @@ type RawExtension struct { // TODO: Make this object have easy access to field based accessors and settors for // metadata and field mutatation. // +// +gencopy=true // +protobuf=true type Unknown struct { TypeMeta `json:",inline" protobuf:"bytes,1,opt,name=typeMeta"` diff --git a/pkg/util/intstr/intstr.go b/pkg/util/intstr/intstr.go index 3724a717fb1..53bc0fc7e62 100644 --- a/pkg/util/intstr/intstr.go +++ b/pkg/util/intstr/intstr.go @@ -32,6 +32,7 @@ import ( // accept a name or number. // TODO: Rename to Int32OrString // +// +gencopy=true // +protobuf=true // +protobuf.options.(gogoproto.goproto_stringer)=false type IntOrString struct {