From 96c0284e9110a2a0ca165f437cc03501e07d9a90 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 23 Jun 2016 21:08:11 +0900 Subject: [PATCH 1/9] go2idl: clarify comments wrt get-or-create This is just comment clarity. --- cmd/libs/go2idl/types/types.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/cmd/libs/go2idl/types/types.go b/cmd/libs/go2idl/types/types.go index b6e9391d71..2853a246b8 100644 --- a/cmd/libs/go2idl/types/types.go +++ b/cmd/libs/go2idl/types/types.go @@ -118,7 +118,9 @@ func (p *Package) Has(name string) bool { return has } -// Get (or add) the given type +// Type gets the given Type in this Package. If the Type is not already +// defined, this will add it and return the new Type value. The caller is +// expected to finish initialization. func (p *Package) Type(typeName string) *Type { if t, ok := p.Types[typeName]; ok { return t @@ -135,9 +137,10 @@ func (p *Package) Type(typeName string) *Type { return t } -// Get (or add) the given function. If a function is added, it's the caller's -// responsibility to finish construction of the function by setting Underlying -// to the correct type. +// Function gets the given function Type in this Package. If the function is +// not already defined, this will add it. If a function is added, it's the +// caller's responsibility to finish construction of the function by setting +// Underlying to the correct type. func (p *Package) Function(funcName string) *Type { if t, ok := p.Functions[funcName]; ok { return t @@ -148,7 +151,8 @@ func (p *Package) Function(funcName string) *Type { return t } -// Get (or add) the given varaible. If a variable is added, it's the caller's +// Variable gets the given varaible Type in this Package. If the variable is +// not already defined, this will add it. If a variable is added, it's the caller's // responsibility to finish construction of the variable by setting Underlying // to the correct type. func (p *Package) Variable(varName string) *Type { @@ -169,19 +173,20 @@ func (p *Package) HasImport(packageName string) bool { } // Universe is a map of all packages. The key is the package name, but you -// should use Get() or Package() instead of direct access. +// should use Package(), Type(), Function(), or Variable() instead of direct +// access. type Universe map[string]*Package -// Get returns the canonical type for the given fully-qualified name. Builtin +// Type returns the canonical type for the given fully-qualified name. Builtin // types will always be found, even if they haven't been explicitly added to -// the map. If a non-existing type is requested, u will create (a marker for) +// the map. If a non-existing type is requested, this will create (a marker for) // it. func (u Universe) Type(n Name) *Type { return u.Package(n.Package).Type(n.Name) } // Function returns the canonical function for the given fully-qualified name. -// If a non-existing function is requested, u will create (a marker for) it. +// If a non-existing function is requested, this will create (a marker for) it. // If a marker is created, it's the caller's responsibility to finish // construction of the function by setting Underlying to the correct type. func (u Universe) Function(n Name) *Type { @@ -189,7 +194,7 @@ func (u Universe) Function(n Name) *Type { } // Variable returns the canonical variable for the given fully-qualified name. -// If a non-existing variable is requested, u will create (a marker for) it. +// If a non-existing variable is requested, this will create (a marker for) it. // If a marker is created, it's the caller's responsibility to finish // construction of the variable by setting Underlying to the correct type. func (u Universe) Variable(n Name) *Type { @@ -205,7 +210,10 @@ func (u Universe) AddImports(packagePath string, importPaths ...string) { } } -// Get (create if needed) the package. +// Package returns the Package for the given path. +// If a non-existing package is requested, this will create (a marker for) it. +// If a marker is created, it's the caller's responsibility to finish +// construction of the package. func (u Universe) Package(packagePath string) *Package { if p, ok := u[packagePath]; ok { return p From ab16ccc1582862ad750add5ecfa085daaa52d0b1 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 24 Jun 2016 21:07:17 +0900 Subject: [PATCH 2/9] go2idl: don't mutate a map being iterated This was causing us to process packages we didn't really want, which was only visible when debugging was enabled. --- cmd/libs/go2idl/parser/parse.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cmd/libs/go2idl/parser/parse.go b/cmd/libs/go2idl/parser/parse.go index 6496dc04c8..331bfc92f4 100644 --- a/cmd/libs/go2idl/parser/parse.go +++ b/cmd/libs/go2idl/parser/parse.go @@ -337,7 +337,13 @@ func (b *Builder) typeCheckPackage(id string) (*tc.Package, error) { func (b *Builder) makePackages() error { b.pkgs = map[string]*tc.Package{} + + // Take a snapshot to iterate, since this will recursively mutate b.parsed. + keys := []string{} for id := range b.parsed { + keys = append(keys, id) + } + for _, id := range keys { // We have to check here even though we made a new one above, // because typeCheckPackage follows the import graph, which may // cause a package to be filled before we get to it in this From ed59210f047265e6b4f7df977d020b35df148403 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 25 Jun 2016 01:50:29 +0900 Subject: [PATCH 3/9] Renames for clarity in conversion-gen This is to clarify the intent of the code for new readers (me). --- .../conversion-gen/generators/conversion.go | 100 +++++++++--------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index 8cf6c1b556..a2a21749b1 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -88,23 +88,23 @@ func getInternalTypeFor(context *generator.Context, t *types.Type) (*types.Type, return context.Universe.Package(internalPackage).Type(t.Name.Name), true } -type conversionType struct { +type conversionPair struct { inType *types.Type outType *types.Type } // All of the types in conversions map are of type "DeclarationOf" with // the underlying type being "Func". -type conversions map[conversionType]*types.Type +type conversionFuncMap map[conversionPair]*types.Type -// Returns all already existing conversion functions that we are able to find. -func existingConversionFunctions(context *generator.Context) conversions { +// Returns all manually-defined conversion functions that we are able to find. +func getManualConversionFunctions(context *generator.Context) conversionFuncMap { scopeName := types.Name{Package: conversionPackagePath, Name: "Scope"} errorName := types.Name{Package: "", Name: "error"} buffer := &bytes.Buffer{} sw := generator.NewSnippetWriter(buffer, context, "$", "$") - preexisting := make(conversions) + manualMap := make(conversionFuncMap) for _, p := range context.Universe { for _, f := range p.Functions { if f.Underlying == nil || f.Underlying.Kind != types.Func { @@ -137,28 +137,28 @@ func existingConversionFunctions(context *generator.Context) conversions { args := argsFromType(inType.Elem, outType.Elem) sw.Do("Convert_$.inType|public$_To_$.outType|public$", args) if f.Name.Name == buffer.String() { - key := conversionType{inType.Elem, outType.Elem} - if v, ok := preexisting[key]; ok && v != nil { + key := conversionPair{inType.Elem, outType.Elem} + if v, ok := manualMap[key]; ok && v != nil { panic(fmt.Sprintf("duplicate static conversion defined: %#v", key)) } - preexisting[key] = f + manualMap[key] = f } buffer.Reset() } } - return preexisting + return manualMap } // All of the types in conversions map are of type "DeclarationOf" with // the underlying type being "Func". -type defaulters map[*types.Type]*types.Type +type defaulterFuncMap map[*types.Type]*types.Type -// Returns all already existing defaulting functions that we are able to find. -func existingDefaultingFunctions(context *generator.Context) defaulters { +// Returns all manually-defined defaulting functions that we are able to find. +func getManualDefaultingFunctions(context *generator.Context) defaulterFuncMap { buffer := &bytes.Buffer{} sw := generator.NewSnippetWriter(buffer, context, "$", "$") - preexisting := make(defaulters) + manualMap := make(defaulterFuncMap) for _, p := range context.Universe { for _, f := range p.Functions { if f.Underlying == nil || f.Underlying.Kind != types.Func { @@ -191,15 +191,15 @@ func existingDefaultingFunctions(context *generator.Context) defaulters { sw.Do("$.inType|defaultfn$", args) if f.Name.Name == buffer.String() { key := inType.Elem - if v, ok := preexisting[key]; ok && v != nil { + if v, ok := manualMap[key]; ok && v != nil { panic(fmt.Sprintf("duplicate static defaulter defined: %#v", key)) } - preexisting[key] = f + manualMap[key] = f } buffer.Reset() } } - return preexisting + return manualMap } func Packages(context *generator.Context, arguments *args.GeneratorArgs) generator.Packages { @@ -218,8 +218,8 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat `)...) // Compute all pre-existing conversion functions. - preexisting := existingConversionFunctions(context) - preexistingDefaults := existingDefaultingFunctions(context) + manualConversions := getManualConversionFunctions(context) + manualDefaulters := getManualDefaultingFunctions(context) // We are generating conversions only for packages that are explicitly // passed as InputDir, and only for those that have a corresponding type @@ -254,7 +254,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat } // If we can generate conversion in any direction, we should // generate this package. - if isConvertible(t, internalType, preexisting) || isConvertible(internalType, t, preexisting) { + if isConvertible(t, internalType, manualConversions) || isConvertible(internalType, t, manualConversions) { convertibleType = true } } @@ -268,7 +268,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { generators = []generator.Generator{} generators = append( - generators, NewGenConversion("conversion_generated", path, preexisting, preexistingDefaults)) + generators, NewGenConversion("conversion_generated", path, manualConversions, manualDefaulters)) return generators }, FilterFunc: func(c *generator.Context, t *types.Type) bool { @@ -292,21 +292,21 @@ func findMember(t *types.Type, name string) (types.Member, bool) { return types.Member{}, false } -func isConvertible(in, out *types.Type, preexisting conversions) bool { +func isConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool { // If there is pre-existing conversion function, return true immediately. - if _, ok := preexisting[conversionType{in, out}]; ok { + if _, ok := manualConversions[conversionPair{in, out}]; ok { return true } - return isDirectlyConvertible(in, out, preexisting) + return isDirectlyConvertible(in, out, manualConversions) } -func isDirectlyConvertible(in, out *types.Type, preexisting conversions) bool { +func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool { // If one of the types is Alias, resolve it. if in.Kind == types.Alias { - return isConvertible(in.Underlying, out, preexisting) + return isConvertible(in.Underlying, out, manualConversions) } if out.Kind == types.Alias { - return isConvertible(in, out.Underlying, preexisting) + return isConvertible(in, out.Underlying, manualConversions) } if in.Kind != out.Kind { @@ -347,15 +347,15 @@ func isDirectlyConvertible(in, out *types.Type, preexisting conversions) bool { } return false } - convertible = convertible && isConvertible(inMember.Type, outMember.Type, preexisting) + convertible = convertible && isConvertible(inMember.Type, outMember.Type, manualConversions) } return convertible case types.Map: - return isConvertible(in.Key, out.Key, preexisting) && isConvertible(in.Elem, out.Elem, preexisting) + return isConvertible(in.Key, out.Key, manualConversions) && isConvertible(in.Elem, out.Elem, manualConversions) case types.Slice: - return isConvertible(in.Elem, out.Elem, preexisting) + return isConvertible(in.Elem, out.Elem, manualConversions) case types.Pointer: - return isConvertible(in.Elem, out.Elem, preexisting) + return isConvertible(in.Elem, out.Elem, manualConversions) } glog.Fatalf("All other types should be filtered before") return false @@ -389,25 +389,25 @@ const ( // genConversion produces a file with a autogenerated conversions. type genConversion struct { generator.DefaultGen - targetPackage string - preexisting conversions - defaulters defaulters - imports namer.ImportTracker - typesForInit []conversionType + targetPackage string + manualConversions conversionFuncMap + manualDefaulters defaulterFuncMap + imports namer.ImportTracker + typesForInit []conversionPair globalVariables map[string]interface{} } -func NewGenConversion(sanitizedName, targetPackage string, preexisting conversions, defaulters defaulters) generator.Generator { +func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap) generator.Generator { return &genConversion{ DefaultGen: generator.DefaultGen{ OptionalName: sanitizedName, }, - targetPackage: targetPackage, - preexisting: preexisting, - defaulters: defaulters, - imports: generator.NewImportTracker(), - typesForInit: make([]conversionType, 0), + targetPackage: targetPackage, + manualConversions: manualConversions, + manualDefaulters: manualDefaulters, + imports: generator.NewImportTracker(), + typesForInit: make([]conversionPair, 0), } } @@ -453,12 +453,12 @@ func (g *genConversion) Filter(c *generator.Context, t *types.Type) bool { // We explicitly return true if any conversion is possible - this needs // to be checked again while generating code for that type. convertible := false - if isConvertible(t, internalType, g.preexisting) { - g.typesForInit = append(g.typesForInit, conversionType{t, internalType}) + if isConvertible(t, internalType, g.manualConversions) { + g.typesForInit = append(g.typesForInit, conversionPair{t, internalType}) convertible = true } - if isConvertible(internalType, t, g.preexisting) { - g.typesForInit = append(g.typesForInit, conversionType{internalType, t}) + if isConvertible(internalType, t, g.manualConversions) { + g.typesForInit = append(g.typesForInit, conversionPair{internalType, t}) convertible = true } return convertible @@ -513,7 +513,7 @@ func (g *genConversion) funcNameTmpl(inType, outType *types.Type) string { } func (g *genConversion) preexists(inType, outType *types.Type) (*types.Type, bool) { - function, ok := g.preexisting[conversionType{inType, outType}] + function, ok := g.manualConversions[conversionPair{inType, outType}] return function, ok } @@ -547,10 +547,10 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error { func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error { sw := generator.NewSnippetWriter(w, c, "$", "$") internalType, _ := getInternalTypeFor(c, t) - if isDirectlyConvertible(t, internalType, g.preexisting) { + if isDirectlyConvertible(t, internalType, g.manualConversions) { g.generateConversion(t, internalType, sw) } - if isDirectlyConvertible(internalType, t, g.preexisting) { + if isDirectlyConvertible(internalType, t, g.manualConversions) { g.generateConversion(internalType, t, sw) } return sw.Error() @@ -561,14 +561,14 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene sw.Do(fmt.Sprintf("func auto%s(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", funcName), g.withGlobals(argsFromType(inType, outType))) // if no defaulter of form SetDefaults_XXX is defined, do not inline a check for defaulting. - if function, ok := g.defaulters[inType]; ok { + if function, ok := g.manualDefaulters[inType]; ok { sw.Do("$.|raw$(in)\n", function) } g.generateFor(inType, outType, sw) sw.Do("return nil\n", nil) sw.Do("}\n\n", nil) - // If there is no public preexisting Convert method, generate it. + // If there is no public manual Conversion method, generate it. if _, ok := g.preexists(inType, outType); !ok { sw.Do(fmt.Sprintf("func %s(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", funcName), g.withGlobals(argsFromType(inType, outType))) sw.Do(fmt.Sprintf("return auto%s(in, out, s)\n", funcName), argsFromType(inType, outType)) From 7fa1e87d66beafb6e07abe1f5571cc4f2620689a Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 25 Jun 2016 01:59:34 +0900 Subject: [PATCH 4/9] Simplify convertible check - same type is OK This is to clarify the code. No actual effect at the moment, but I manually verified this in the case of identical types. --- cmd/libs/go2idl/conversion-gen/generators/conversion.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index a2a21749b1..5f347f00d3 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -309,6 +309,9 @@ func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFunc return isConvertible(in, out.Underlying, manualConversions) } + if in == out { + return true + } if in.Kind != out.Kind { return false } @@ -318,12 +321,6 @@ func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFunc // We don't support conversion of other types yet. return false } - switch out.Kind { - case types.Builtin, types.Struct, types.Map, types.Slice, types.Pointer: - default: - // We don't support conversion of other types yet. - return false - } switch in.Kind { case types.Builtin: From 80490e0a55d1de9bde3f75eb14b6bbac1b6d6eec Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 26 Jun 2016 17:42:55 -0700 Subject: [PATCH 5/9] go2idl: Allow generators to AddDir() at runtime This is used subsequently to simplify the conversion generation, so each package can declare what peer-packages it uses, and have those imported dynamically, rather than having one mega list of packages to import and not really being clear why, for any given list item. --- cmd/libs/go2idl/generator/generator.go | 11 ++ cmd/libs/go2idl/parser/parse.go | 168 +++++++++++++++---------- 2 files changed, 116 insertions(+), 63 deletions(-) diff --git a/cmd/libs/go2idl/generator/generator.go b/cmd/libs/go2idl/generator/generator.go index bb197494c6..70e2af0afd 100644 --- a/cmd/libs/go2idl/generator/generator.go +++ b/cmd/libs/go2idl/generator/generator.go @@ -166,6 +166,9 @@ type Context struct { // If true, Execute* calls will just verify that the existing output is // correct. (You may set this after calling NewContext.) Verify bool + + // Allows generators to add packages at runtime. + builder *parser.Builder } // NewContext generates a context from the given builder, naming systems, and @@ -183,6 +186,7 @@ func NewContext(b *parser.Builder, nameSystems namer.NameSystems, canonicalOrder FileTypes: map[string]FileType{ GolangFileType: NewGolangFile(), }, + builder: b, } for name, systemNamer := range nameSystems { @@ -194,3 +198,10 @@ func NewContext(b *parser.Builder, nameSystems namer.NameSystems, canonicalOrder } return c, nil } + +// AddDir adds a Go package to the context. The specified path must be a single +// go package import path. GOPATH, GOROOT, and the location of your go binary +// (`which go`) will all be searched, in the normal Go fashion. +func (ctxt *Context) AddDir(path string) error { + return ctxt.builder.AddDirTo(path, &ctxt.Universe) +} diff --git a/cmd/libs/go2idl/parser/parse.go b/cmd/libs/go2idl/parser/parse.go index 331bfc92f4..2482d99580 100644 --- a/cmd/libs/go2idl/parser/parse.go +++ b/cmd/libs/go2idl/parser/parse.go @@ -45,7 +45,7 @@ type Builder struct { // map of package id to absolute path (to prevent overlap) absPaths map[string]string - // Set by makePackages, used by importer() and friends. + // Set by makePackage(), used by importer() and friends. pkgs map[string]*tc.Package // Map of package path to whether the user requested it or it was from @@ -167,6 +167,7 @@ func (b *Builder) addFile(pkg string, path string, src []byte, userRequested boo } else { b.absPaths[pkg] = dirPath } + b.parsed[pkg] = append(b.parsed[pkg], parsedFile{path, p}) b.userRequested[pkg] = userRequested for _, c := range p.Comments { @@ -232,6 +233,26 @@ func (b *Builder) AddDirRecursive(dir string) error { return nil } +// AddDirTo adds an entire directory to a given Universe. Unlike AddDir, this +// processes the package immediately, which makes it safe to use from within a +// generator (rather than just at init time. 'dir' must be a single go package. +// GOPATH, GOROOT, and the location of your go binary (`which go`) will all be +// searched if dir doesn't literally resolve. +func (b *Builder) AddDirTo(dir string, u *types.Universe) error { + if _, found := b.parsed[dir]; !found { + // We want all types from this package, as if they were directly added + // by the user. They WERE added by the user, in effect. + if err := b.addDir(dir, true); err != nil { + return err + } + } else { + // We already had this package, but we want it to be considered as if + // the user addid it directly. + b.userRequested[dir] = true + } + return b.findTypesIn(dir, u) +} + // The implementation of AddDir. A flag indicates whether this directory was // user-requested or just from following the import graph. func (b *Builder) addDir(dir string, userRequested bool) error { @@ -240,8 +261,10 @@ func (b *Builder) addDir(dir string, userRequested bool) error { return err } // Check in case this package was added (maybe dir was not canonical) - if _, alreadyAdded := b.parsed[dir]; alreadyAdded { - return nil + if wasRequested, wasAdded := b.userRequested[dir]; wasAdded { + if !userRequested || userRequested == wasRequested { + return nil + } } for _, n := range pkg.GoFiles { @@ -335,29 +358,35 @@ func (b *Builder) typeCheckPackage(id string) (*tc.Package, error) { return pkg, err } -func (b *Builder) makePackages() error { - b.pkgs = map[string]*tc.Package{} - +func (b *Builder) makeAllPackages() error { // Take a snapshot to iterate, since this will recursively mutate b.parsed. keys := []string{} for id := range b.parsed { keys = append(keys, id) } for _, id := range keys { - // We have to check here even though we made a new one above, - // because typeCheckPackage follows the import graph, which may - // cause a package to be filled before we get to it in this - // loop. - if _, done := b.pkgs[id]; done { - continue - } - if _, err := b.typeCheckPackage(id); err != nil { + if _, err := b.makePackage(id); err != nil { return err } } return nil } +func (b *Builder) makePackage(id string) (*tc.Package, error) { + if b.pkgs == nil { + b.pkgs = map[string]*tc.Package{} + } + + // We have to check here even though we made a new one above, + // because typeCheckPackage follows the import graph, which may + // cause a package to be filled before we get to it in this + // loop. + if pkg, done := b.pkgs[id]; done { + return pkg, nil + } + return b.typeCheckPackage(id) +} + // FindPackages fetches a list of the user-imported packages. func (b *Builder) FindPackages() []string { result := []string{} @@ -375,65 +404,78 @@ func (b *Builder) FindPackages() []string { // FindTypes finalizes the package imports, and searches through all the // packages for types. func (b *Builder) FindTypes() (types.Universe, error) { - if err := b.makePackages(); err != nil { + if err := b.makeAllPackages(); err != nil { return nil, err } u := types.Universe{} - for pkgPath, pkg := range b.pkgs { - if !b.userRequested[pkgPath] { - // Since walkType is recursive, all types that the - // packages they asked for depend on will be included. - // But we don't need to include all types in all - // *packages* they depend on. - continue + for pkgPath := range b.parsed { + if err := b.findTypesIn(pkgPath, &u); err != nil { + return nil, err } - - for _, f := range b.parsed[pkgPath] { - if strings.HasSuffix(f.name, "/doc.go") { - tp := u.Package(pkgPath) - for i := range f.file.Comments { - tp.Comments = append(tp.Comments, splitLines(f.file.Comments[i].Text())...) - } - if f.file.Doc != nil { - tp.DocComments = splitLines(f.file.Doc.Text()) - } - } - } - - s := pkg.Scope() - for _, n := range s.Names() { - obj := s.Lookup(n) - tn, ok := obj.(*tc.TypeName) - if ok { - t := b.walkType(u, nil, tn.Type()) - c1 := b.priorCommentLines(obj.Pos(), 1) - t.CommentLines = splitLines(c1.Text()) - if c1 == nil { - t.SecondClosestCommentLines = splitLines(b.priorCommentLines(obj.Pos(), 2).Text()) - } else { - t.SecondClosestCommentLines = splitLines(b.priorCommentLines(c1.List[0].Slash, 2).Text()) - } - } - tf, ok := obj.(*tc.Func) - // We only care about functions, not concrete/abstract methods. - if ok && tf.Type() != nil && tf.Type().(*tc.Signature).Recv() == nil { - b.addFunction(u, nil, tf) - } - tv, ok := obj.(*tc.Var) - if ok && !tv.IsField() { - b.addVariable(u, nil, tv) - } - } - for p := range b.importGraph[pkgPath] { - u.AddImports(pkgPath, p) - } - u.Package(pkgPath).Name = pkg.Name() } return u, nil } +// findTypesIn finalizes the package import and searches through the package +// for types. +func (b *Builder) findTypesIn(pkgPath string, u *types.Universe) error { + pkg, err := b.makePackage(pkgPath) + if err != nil { + return err + } + if !b.userRequested[pkgPath] { + // Since walkType is recursive, all types that the + // packages they asked for depend on will be included. + // But we don't need to include all types in all + // *packages* they depend on. + return nil + } + + for _, f := range b.parsed[pkgPath] { + if strings.HasSuffix(f.name, "/doc.go") { + tp := u.Package(pkgPath) + for i := range f.file.Comments { + tp.Comments = append(tp.Comments, splitLines(f.file.Comments[i].Text())...) + } + if f.file.Doc != nil { + tp.DocComments = splitLines(f.file.Doc.Text()) + } + } + } + + s := pkg.Scope() + for _, n := range s.Names() { + obj := s.Lookup(n) + tn, ok := obj.(*tc.TypeName) + if ok { + t := b.walkType(*u, nil, tn.Type()) + c1 := b.priorCommentLines(obj.Pos(), 1) + t.CommentLines = splitLines(c1.Text()) + if c1 == nil { + t.SecondClosestCommentLines = splitLines(b.priorCommentLines(obj.Pos(), 2).Text()) + } else { + t.SecondClosestCommentLines = splitLines(b.priorCommentLines(c1.List[0].Slash, 2).Text()) + } + } + tf, ok := obj.(*tc.Func) + // We only care about functions, not concrete/abstract methods. + if ok && tf.Type() != nil && tf.Type().(*tc.Signature).Recv() == nil { + b.addFunction(*u, nil, tf) + } + tv, ok := obj.(*tc.Var) + if ok && !tv.IsField() { + b.addVariable(*u, nil, tv) + } + } + for p := range b.importGraph[pkgPath] { + u.AddImports(pkgPath, p) + } + u.Package(pkgPath).Name = pkg.Name() + return nil +} + // if there's a comment on the line `lines` before pos, return its text, otherwise "". func (b *Builder) priorCommentLines(pos token.Pos, lines int) *ast.CommentGroup { position := b.fset.Position(pos) From 291b51ec50f9ea348748894386eec76ca494329b Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 6 Jun 2016 23:42:16 -0700 Subject: [PATCH 6/9] Use file tags to generate conversions This drives conversion generation from file tags like: // +conversion-gen=k8s.io/my/internal/version .. rather than hardcoded lists of packages. The only net change in generated code can be explained as correct. Previously it didn't know that conversion was available. --- .../conversion-gen/generators/conversion.go | 369 ++++++++++-------- .../go2idl/conversion-gen/generators/tags.go | 33 -- cmd/libs/go2idl/conversion-gen/main.go | 67 ++-- docs/devel/adding-an-APIGroup.md | 7 +- federation/apis/federation/v1beta1/doc.go | 2 +- hack/update-codegen.sh | 18 +- hack/verify-flags/known-flags.txt | 3 +- pkg/api/v1/doc.go | 2 +- pkg/api/v1/generated.proto | 4 +- pkg/api/v1/types.go | 4 +- pkg/apis/apps/v1alpha1/doc.go | 2 +- pkg/apis/authentication.k8s.io/v1beta1/doc.go | 2 +- pkg/apis/authorization/v1beta1/doc.go | 2 +- pkg/apis/autoscaling/v1/doc.go | 2 +- pkg/apis/batch/v1/doc.go | 2 +- pkg/apis/batch/v2alpha1/doc.go | 2 +- pkg/apis/certificates/v1alpha1/doc.go | 2 +- pkg/apis/componentconfig/v1alpha1/doc.go | 2 +- .../v1beta1/conversion_generated.go | 70 ++++ pkg/apis/extensions/v1beta1/doc.go | 4 +- pkg/apis/policy/v1alpha1/doc.go | 2 +- pkg/apis/rbac/v1alpha1/doc.go | 2 +- 22 files changed, 343 insertions(+), 260 deletions(-) delete mode 100644 cmd/libs/go2idl/conversion-gen/generators/tags.go diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index 5f347f00d3..670a1bdb5a 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -32,6 +32,19 @@ import ( "github.com/golang/glog" ) +// CustomArgs is used tby the go2idl framework to pass args specific to this +// generator. +type CustomArgs struct { + ExtraPeerDirs []string // Always consider these as last-ditch possibilities for conversions. +} + +// This is the comment tag that carries parameters for conversion generation. +const tagName = "k8s:conversion-gen" + +func extractTag(comments []string) []string { + return types.ExtractCommentTags("+", comments)[tagName] +} + // TODO: This is created only to reduce number of changes in a single PR. // Remove it and use PublicNamer instead. func conversionNamer() *namer.NameStrategy { @@ -67,25 +80,17 @@ func DefaultNameSystem() string { return "public" } -var fallbackPackages = []string{ - "k8s.io/kubernetes/pkg/api/unversioned", - "k8s.io/kubernetes/pkg/apis/extensions", - "k8s.io/kubernetes/pkg/apis/autoscaling", - "k8s.io/kubernetes/pkg/apis/batch", -} - -func getInternalTypeFor(context *generator.Context, t *types.Type) (*types.Type, bool) { - internalPackage := filepath.Dir(t.Name.Package) - if !context.Universe.Package(internalPackage).Has(t.Name.Name) { - for _, fallbackPackage := range fallbackPackages { - if fallbackPackage == t.Name.Package || !context.Universe.Package(fallbackPackage).Has(t.Name.Name) { - continue - } - return context.Universe.Package(fallbackPackage).Type(t.Name.Name), true +func getPeerTypeFor(context *generator.Context, t *types.Type, potenialPeerPkgs []string) *types.Type { + for _, ppp := range potenialPeerPkgs { + p := context.Universe.Package(ppp) + if p == nil { + continue + } + if p.Has(t.Name.Name) { + return p.Type(t.Name.Name) } - return nil, false } - return context.Universe.Package(internalPackage).Type(t.Name.Name), true + return nil } type conversionPair struct { @@ -97,109 +102,103 @@ type conversionPair struct { // the underlying type being "Func". type conversionFuncMap map[conversionPair]*types.Type -// Returns all manually-defined conversion functions that we are able to find. -func getManualConversionFunctions(context *generator.Context) conversionFuncMap { +// Returns all manually-defined conversion functions in the package. +func getManualConversionFunctions(context *generator.Context, pkg *types.Package, manualMap conversionFuncMap) { scopeName := types.Name{Package: conversionPackagePath, Name: "Scope"} errorName := types.Name{Package: "", Name: "error"} buffer := &bytes.Buffer{} sw := generator.NewSnippetWriter(buffer, context, "$", "$") - manualMap := make(conversionFuncMap) - for _, p := range context.Universe { - for _, f := range p.Functions { - if f.Underlying == nil || f.Underlying.Kind != types.Func { - glog.Errorf("Malformed function: %#v", f) - continue - } - if f.Underlying.Signature == nil { - glog.Errorf("Function without signature: %#v", f) - continue - } - signature := f.Underlying.Signature - // Check whether the function is conversion function. - // Note that all of them have signature: - // func Convert_inType_To_outType(inType, outType, conversion.Scope) error - if signature.Receiver != nil { - continue - } - if len(signature.Parameters) != 3 || signature.Parameters[2].Name != scopeName { - continue - } - if len(signature.Results) != 1 || signature.Results[0].Name != errorName { - continue - } - inType := signature.Parameters[0] - outType := signature.Parameters[1] - if inType.Kind != types.Pointer || outType.Kind != types.Pointer { - continue - } - // Now check if the name satisfies the convention. - args := argsFromType(inType.Elem, outType.Elem) - sw.Do("Convert_$.inType|public$_To_$.outType|public$", args) - if f.Name.Name == buffer.String() { - key := conversionPair{inType.Elem, outType.Elem} - if v, ok := manualMap[key]; ok && v != nil { - panic(fmt.Sprintf("duplicate static conversion defined: %#v", key)) - } - manualMap[key] = f - } - buffer.Reset() + for _, f := range pkg.Functions { + if f.Underlying == nil || f.Underlying.Kind != types.Func { + glog.Errorf("Malformed function: %#v", f) + continue } + if f.Underlying.Signature == nil { + glog.Errorf("Function without signature: %#v", f) + continue + } + signature := f.Underlying.Signature + // Check whether the function is conversion function. + // Note that all of them have signature: + // func Convert_inType_To_outType(inType, outType, conversion.Scope) error + if signature.Receiver != nil { + continue + } + if len(signature.Parameters) != 3 || signature.Parameters[2].Name != scopeName { + continue + } + if len(signature.Results) != 1 || signature.Results[0].Name != errorName { + continue + } + inType := signature.Parameters[0] + outType := signature.Parameters[1] + if inType.Kind != types.Pointer || outType.Kind != types.Pointer { + continue + } + // Now check if the name satisfies the convention. + args := argsFromType(inType.Elem, outType.Elem) + sw.Do("Convert_$.inType|public$_To_$.outType|public$", args) + if f.Name.Name == buffer.String() { + key := conversionPair{inType.Elem, outType.Elem} + // We might scan the same package twice, and that's OK. + if v, ok := manualMap[key]; ok && v != nil && v.Name.Package != pkg.Path { + panic(fmt.Sprintf("duplicate static conversion defined: %#v", key)) + } + manualMap[key] = f + } + buffer.Reset() } - return manualMap } // All of the types in conversions map are of type "DeclarationOf" with // the underlying type being "Func". type defaulterFuncMap map[*types.Type]*types.Type -// Returns all manually-defined defaulting functions that we are able to find. -func getManualDefaultingFunctions(context *generator.Context) defaulterFuncMap { +// Returns all manually-defined defaulting functions in the package. +func getManualDefaultingFunctions(context *generator.Context, pkg *types.Package, manualMap defaulterFuncMap) { buffer := &bytes.Buffer{} sw := generator.NewSnippetWriter(buffer, context, "$", "$") - manualMap := make(defaulterFuncMap) - for _, p := range context.Universe { - for _, f := range p.Functions { - if f.Underlying == nil || f.Underlying.Kind != types.Func { - glog.Errorf("Malformed function: %#v", f) - continue - } - if f.Underlying.Signature == nil { - glog.Errorf("Function without signature: %#v", f) - continue - } - signature := f.Underlying.Signature - // Check whether the function is conversion function. - // Note that all of them have signature: - // func Convert_inType_To_outType(inType, outType, conversion.Scope) error - if signature.Receiver != nil { - continue - } - if len(signature.Parameters) != 1 { - continue - } - if len(signature.Results) != 0 { - continue - } - inType := signature.Parameters[0] - if inType.Kind != types.Pointer { - continue - } - // Now check if the name satisfies the convention. - args := defaultingArgsFromType(inType.Elem) - sw.Do("$.inType|defaultfn$", args) - if f.Name.Name == buffer.String() { - key := inType.Elem - if v, ok := manualMap[key]; ok && v != nil { - panic(fmt.Sprintf("duplicate static defaulter defined: %#v", key)) - } - manualMap[key] = f - } - buffer.Reset() + for _, f := range pkg.Functions { + if f.Underlying == nil || f.Underlying.Kind != types.Func { + glog.Errorf("Malformed function: %#v", f) + continue } + if f.Underlying.Signature == nil { + glog.Errorf("Function without signature: %#v", f) + continue + } + signature := f.Underlying.Signature + // Check whether the function is conversion function. + // Note that all of them have signature: + // func Convert_inType_To_outType(inType, outType, conversion.Scope) error + if signature.Receiver != nil { + continue + } + if len(signature.Parameters) != 1 { + continue + } + if len(signature.Results) != 0 { + continue + } + inType := signature.Parameters[0] + if inType.Kind != types.Pointer { + continue + } + // Now check if the name satisfies the convention. + args := defaultingArgsFromType(inType.Elem) + sw.Do("$.inType|defaultfn$", args) + if f.Name.Name == buffer.String() { + key := inType.Elem + // We might scan the same package twice, and that's OK. + if v, ok := manualMap[key]; ok && v != nil && v.Name.Package != pkg.Path { + panic(fmt.Sprintf("duplicate static defaulter defined: %#v", key)) + } + manualMap[key] = f + } + buffer.Reset() } - return manualMap } func Packages(context *generator.Context, arguments *args.GeneratorArgs) generator.Packages { @@ -208,7 +207,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat glog.Fatalf("Failed loading boilerplate: %v", err) } - inputs := sets.NewString(arguments.InputDirs...) + inputs := sets.NewString(context.Inputs...) packages := generator.Packages{} header := append([]byte(fmt.Sprintf("// +build !%s\n\n", arguments.GeneratedBuildTag)), boilerplate...) header = append(header, []byte( @@ -217,65 +216,89 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat `)...) - // Compute all pre-existing conversion functions. - manualConversions := getManualConversionFunctions(context) - manualDefaulters := getManualDefaultingFunctions(context) + // Accumulate pre-existing conversion and default functions. + // TODO: This is too ad-hoc. We need a better way. + manualConversions := conversionFuncMap{} + manualDefaults := defaulterFuncMap{} // We are generating conversions only for packages that are explicitly - // passed as InputDir, and only for those that have a corresponding type - // (in the directory one above) and can be automatically converted to. - for _, p := range context.Universe { - path := p.Path - if !inputs.Has(path) { - continue - } - // Only generate conversions for package which explicitly requested it - // byt setting "+genversion=true" in their doc.go file. - filtered := false - if extractBoolTagOrDie("genconversion", false, p.DocComments) == true { - filtered = true - } - if !filtered { + // passed as InputDir. + for i := range inputs { + glog.V(5).Infof("considering pkg %q", i) + pkg := context.Universe[i] + if pkg == nil { + // If the input had no Go files, for example. continue } - convertibleType := false - for _, t := range p.Types { - // Check whether this type can be auto-converted to the internal - // version. - internalType, exists := getInternalTypeFor(context, t) - if !exists { - // There is no corresponding type in the internal package. + // Add conversion and defaulting functions. + getManualConversionFunctions(context, pkg, manualConversions) + getManualDefaultingFunctions(context, pkg, manualDefaults) + + // Only generate conversions for packages which explicitly request it + // by specifying one or more "+k8s:conversion-gen=" + // in their doc.go file. + peerPkgs := extractTag(pkg.Comments) + if peerPkgs != nil { + glog.V(5).Infof(" tags: %q", peerPkgs) + } else { + glog.V(5).Infof(" no tag") + continue + } + if customArgs, ok := arguments.CustomArgs.(*CustomArgs); ok { + if len(customArgs.ExtraPeerDirs) > 0 { + peerPkgs = append(peerPkgs, customArgs.ExtraPeerDirs...) + } + } + // Make sure our peer-packages are added and fully parsed. + for _, pp := range peerPkgs { + context.AddDir(pp) + getManualConversionFunctions(context, context.Universe[pp], manualConversions) + getManualDefaultingFunctions(context, context.Universe[pp], manualDefaults) + } + + pkgNeedsGeneration := false + for _, t := range pkg.Types { + // Check whether this type can be auto-converted to the peer + // package type. + peerType := getPeerTypeFor(context, t, peerPkgs) + if peerType == nil { + // We did not find a corresponding type. continue } - // We won't be able to convert to private type. - if namer.IsPrivateGoName(internalType.Name.Name) { + if namer.IsPrivateGoName(peerType.Name.Name) { + // We won't be able to convert to a private type. + glog.V(5).Infof(" found a peer type %v, but it is a private name", t) continue } + // If we can generate conversion in any direction, we should // generate this package. - if isConvertible(t, internalType, manualConversions) || isConvertible(internalType, t, manualConversions) { - convertibleType = true + if isConvertible(t, peerType, manualConversions) || isConvertible(peerType, t, manualConversions) { + pkgNeedsGeneration = true + break } } - - if convertibleType { - packages = append(packages, - &generator.DefaultPackage{ - PackageName: filepath.Base(path), - PackagePath: path, - HeaderText: header, - GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { - generators = []generator.Generator{} - generators = append( - generators, NewGenConversion("conversion_generated", path, manualConversions, manualDefaulters)) - return generators - }, - FilterFunc: func(c *generator.Context, t *types.Type) bool { - return t.Name.Package == path - }, - }) + if !pkgNeedsGeneration { + glog.V(5).Infof(" no viable conversions, not generating for this package") + continue } + + packages = append(packages, + &generator.DefaultPackage{ + PackageName: filepath.Base(pkg.Path), + PackagePath: pkg.Path, + HeaderText: header, + GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { + generators = []generator.Generator{} + generators = append( + generators, NewGenConversion(arguments.OutputFileBaseName, pkg.Path, manualConversions, manualDefaults, peerPkgs)) + return generators + }, + FilterFunc: func(c *generator.Context, t *types.Type) bool { + return t.Name.Package == pkg.Path + }, + }) } return packages } @@ -324,9 +347,6 @@ func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFunc switch in.Kind { case types.Builtin: - if in == out { - return true - } // TODO: Support more conversion types. return types.IsInteger(in) && types.IsInteger(out) case types.Struct: @@ -335,11 +355,11 @@ func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFunc // Check if there is an out member with that name. outMember, found := findMember(out, inMember.Name) if !found { - // Check if the member doesn't have comment: - // "+ genconversion=false" - // comment to ignore this field for conversion. + // Check if the member has opted out with: + // "+k8s:conversion-gen=false" // TODO: Switch to SecondClosestCommentLines. - if extractBoolTagOrDie("genconversion", true, inMember.CommentLines) == false { + if tagvals := extractTag(inMember.CommentLines); tagvals != nil && tagvals[0] == "false" { + glog.V(5).Infof("field %v.%s requests no conversion generation, skipping", in, inMember.Name) continue } return false @@ -387,6 +407,7 @@ const ( type genConversion struct { generator.DefaultGen targetPackage string + peerPackages []string manualConversions conversionFuncMap manualDefaulters defaulterFuncMap imports namer.ImportTracker @@ -395,12 +416,13 @@ type genConversion struct { globalVariables map[string]interface{} } -func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap) generator.Generator { +func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap, peerPkgs []string) generator.Generator { return &genConversion{ DefaultGen: generator.DefaultGen{ OptionalName: sanitizedName, }, targetPackage: targetPackage, + peerPackages: peerPkgs, manualConversions: manualConversions, manualDefaulters: manualDefaulters, imports: generator.NewImportTracker(), @@ -425,7 +447,13 @@ func (g *genConversion) convertibleOnlyWithinPackage(inType, outType *types.Type if t.Name.Package != g.targetPackage { return false } - if extractBoolTagOrDie("genconversion", true, t.CommentLines) == false { + // If the type has opted out, skip it. + tagvals := extractTag(t.CommentLines) + if tagvals != nil { + if tagvals[0] != "false" { + glog.Fatalf("Type %v: unsupported %s value: %q", t, tagName, tagvals[0]) + } + glog.V(5).Infof("type %v requests no conversion generation, skipping", t) return false } // TODO: Consider generating functions for other kinds too. @@ -440,22 +468,22 @@ func (g *genConversion) convertibleOnlyWithinPackage(inType, outType *types.Type } func (g *genConversion) Filter(c *generator.Context, t *types.Type) bool { - internalType, exists := getInternalTypeFor(c, t) - if !exists { + peerType := getPeerTypeFor(c, t, g.peerPackages) + if peerType == nil { return false } - if !g.convertibleOnlyWithinPackage(t, internalType) { + if !g.convertibleOnlyWithinPackage(t, peerType) { return false } // We explicitly return true if any conversion is possible - this needs // to be checked again while generating code for that type. convertible := false - if isConvertible(t, internalType, g.manualConversions) { - g.typesForInit = append(g.typesForInit, conversionPair{t, internalType}) + if isConvertible(t, peerType, g.manualConversions) { + g.typesForInit = append(g.typesForInit, conversionPair{t, peerType}) convertible = true } - if isConvertible(internalType, t, g.manualConversions) { - g.typesForInit = append(g.typesForInit, conversionPair{internalType, t}) + if isConvertible(peerType, t, g.manualConversions) { + g.typesForInit = append(g.typesForInit, conversionPair{peerType, t}) convertible = true } return convertible @@ -542,13 +570,14 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error { } func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error { + glog.V(5).Infof("generating for type %v", t) sw := generator.NewSnippetWriter(w, c, "$", "$") - internalType, _ := getInternalTypeFor(c, t) - if isDirectlyConvertible(t, internalType, g.manualConversions) { - g.generateConversion(t, internalType, sw) + peerType := getPeerTypeFor(c, t, g.peerPackages) + if isDirectlyConvertible(t, peerType, g.manualConversions) { + g.generateConversion(t, peerType, sw) } - if isDirectlyConvertible(internalType, t, g.manualConversions) { - g.generateConversion(internalType, t, sw) + if isDirectlyConvertible(peerType, t, g.manualConversions) { + g.generateConversion(peerType, t, sw) } return sw.Error() } @@ -681,7 +710,7 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip outMember, isOutMember := findMember(outType, m.Name) if !isOutMember { // Since this object wasn't filtered out, this means that - // this field has "genconversion=false" comment to ignore it. + // this field has "+k8s:conversion-gen=false" comment to ignore it. continue } t, outT := m.Type, outMember.Type diff --git a/cmd/libs/go2idl/conversion-gen/generators/tags.go b/cmd/libs/go2idl/conversion-gen/generators/tags.go deleted file mode 100644 index 032c501e3d..0000000000 --- a/cmd/libs/go2idl/conversion-gen/generators/tags.go +++ /dev/null @@ -1,33 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package generators - -import ( - "github.com/golang/glog" - "k8s.io/kubernetes/cmd/libs/go2idl/types" -) - -// extractBoolTagOrDie gets the comment-tags for the key and asserts that, if -// it exists, the value is boolean. If the tag did not exist, it returns -// defaultVal. -func extractBoolTagOrDie(key string, defaultVal bool, lines []string) bool { - val, err := types.ExtractSingleBoolCommentTag("+", key, defaultVal, lines) - if err != nil { - glog.Fatalf(err.Error()) - } - return val -} diff --git a/cmd/libs/go2idl/conversion-gen/main.go b/cmd/libs/go2idl/conversion-gen/main.go index 5b2dae836f..33a4001370 100644 --- a/cmd/libs/go2idl/conversion-gen/main.go +++ b/cmd/libs/go2idl/conversion-gen/main.go @@ -16,9 +16,22 @@ limitations under the License. // conversion-gen is a tool for auto-generating Conversion functions. // -// Structs in the input directories with the below line in their comments -// will be ignored during generation. -// // +genconversion=false +// Given a list of input directories, it will scan for "peer" packages and +// generate functions that efficiently convert between same-name types in each +// package. For any pair of types that has a +// `Convert___To__ +// +// When generating for a package, individual types or fields of structs may opt +// out of Conversion generation by specifying a comment on the of the form: +// // +k8s:conversion-gen=false package main import ( @@ -26,42 +39,30 @@ import ( "k8s.io/kubernetes/cmd/libs/go2idl/conversion-gen/generators" "github.com/golang/glog" + "github.com/spf13/pflag" ) func main() { arguments := args.Default() - // Override defaults. These are Kubernetes specific input locations. - arguments.InputDirs = []string{ - "k8s.io/kubernetes/pkg/api/v1", - "k8s.io/kubernetes/pkg/api", - "k8s.io/kubernetes/pkg/apis/authentication.k8s.io", - "k8s.io/kubernetes/pkg/apis/authentication.k8s.io/v1beta1", - "k8s.io/kubernetes/pkg/apis/authorization", - "k8s.io/kubernetes/pkg/apis/authorization/v1beta1", - "k8s.io/kubernetes/pkg/apis/autoscaling", - "k8s.io/kubernetes/pkg/apis/autoscaling/v1", - "k8s.io/kubernetes/pkg/apis/batch", - "k8s.io/kubernetes/pkg/apis/batch/v1", - "k8s.io/kubernetes/pkg/apis/batch/v2alpha1", - "k8s.io/kubernetes/pkg/apis/apps", - "k8s.io/kubernetes/pkg/apis/apps/v1alpha1", - "k8s.io/kubernetes/pkg/apis/certificates", - "k8s.io/kubernetes/pkg/apis/certificates/v1alpha1", - "k8s.io/kubernetes/pkg/apis/componentconfig", - "k8s.io/kubernetes/pkg/apis/componentconfig/v1alpha1", - "k8s.io/kubernetes/pkg/apis/policy", - "k8s.io/kubernetes/pkg/apis/policy/v1alpha1", - "k8s.io/kubernetes/pkg/apis/extensions", - "k8s.io/kubernetes/pkg/apis/extensions/v1beta1", - "k8s.io/kubernetes/pkg/apis/rbac", - "k8s.io/kubernetes/pkg/apis/rbac/v1alpha1", - "k8s.io/kubernetes/federation/apis/federation", - "k8s.io/kubernetes/federation/apis/federation/v1beta1", - "k8s.io/kubernetes/pkg/conversion", - "k8s.io/kubernetes/pkg/runtime", - } + // Override defaults. + arguments.OutputFileBaseName = "conversion_generated" + // Custom args. + customArgs := &generators.CustomArgs{ + ExtraPeerDirs: []string{ + "k8s.io/kubernetes/pkg/api", + "k8s.io/kubernetes/pkg/api/v1", + "k8s.io/kubernetes/pkg/api/unversioned", + "k8s.io/kubernetes/pkg/conversion", + "k8s.io/kubernetes/pkg/runtime", + }, + } + pflag.CommandLine.StringSliceVar(&customArgs.ExtraPeerDirs, "extra-peer-dirs", customArgs.ExtraPeerDirs, + "Comma-separated list of import paths which are considered, after tag-specified peers, for conversions.") + arguments.CustomArgs = customArgs + + // Run it. if err := arguments.Execute( generators.NameSystems(), generators.DefaultNameSystem(), diff --git a/docs/devel/adding-an-APIGroup.md b/docs/devel/adding-an-APIGroup.md index 63c4e2a2d6..cefa856414 100644 --- a/docs/devel/adding-an-APIGroup.md +++ b/docs/devel/adding-an-APIGroup.md @@ -79,9 +79,10 @@ cmd/libs/go2idl/ tool. 2. Make sure your pkg/apis/``/`` directory has a doc.go file with the comment `// +k8s:deepcopy-gen=package,register`, to catch the attention of our generation tools. - 3. Make sure your pkg/apis/``/`` directory has a doc.go file - with the comment `// +genconversion=true`, to catch the attention of our - gen-conversion script. + 3. Make sure your `pkg/apis//` directory has a doc.go file + with the comment `// +k8s:conversion-gen=`, to catch the + attention of our generation tools. For most APIs the only target you + need is `k8s.io/kubernetes/pkg/apis/` (your internal API). 4. Run hack/update-all.sh. 2. Generate files for Ugorji codec: diff --git a/federation/apis/federation/v1beta1/doc.go b/federation/apis/federation/v1beta1/doc.go index 8eebd4e879..3a4f89c982 100644 --- a/federation/apis/federation/v1beta1/doc.go +++ b/federation/apis/federation/v1beta1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/federation/apis/federation -// +genconversion=true package v1beta1 diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 9d5df48b90..2eba4de2f2 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -47,7 +47,6 @@ ${clientgen} --clientset-name="release_1_4" --input="api/v1,extensions/v1beta1,a # Clientgen for federation clientset. ${clientgen} --clientset-name=federation_internalclientset --clientset-path=k8s.io/kubernetes/federation/client/clientset_generated --input="../../federation/apis/federation/","api/" --included-types-overrides="api/Service" "$@" ${clientgen} --clientset-name=federation_release_1_4 --clientset-path=k8s.io/kubernetes/federation/client/clientset_generated --input="../../federation/apis/federation/v1beta1","api/v1" --included-types-overrides="api/v1/Service" "$@" -${conversiongen} "$@" ${setgen} "$@" # You may add additional calls of code generators like set-gen above. @@ -73,9 +72,22 @@ DEEP_COPY_DIRS=$( | xargs dirname \ | sort -u ) -INPUTS=$( +DEEPCOPY_INPUTS=$( for d in ${DEEP_COPY_DIRS}; do echo k8s.io/kubernetes/$d done | paste -sd, ) -${deepcopygen} -i ${INPUTS} +${deepcopygen} -i ${DEEPCOPY_INPUTS} + +CONVERSION_DIRS=$( + grep '^// *+k8s:conversion-gen=' ${ALL_K8S_TAG_FILES} \ + | cut -f1 -d: \ + | xargs dirname \ + | sort -u \ + ) +CONVERSION_INPUTS=$( + for d in ${CONVERSION_DIRS}; do + echo k8s.io/kubernetes/$d + done | paste -sd, + ) +${conversiongen} -i ${CONVERSION_INPUTS} diff --git a/hack/verify-flags/known-flags.txt b/hack/verify-flags/known-flags.txt index e85f40967c..b9dd7fcb57 100644 --- a/hack/verify-flags/known-flags.txt +++ b/hack/verify-flags/known-flags.txt @@ -150,13 +150,14 @@ executor-bindall executor-logv executor-path executor-suicide-timeout +exit-on-lock-contention experimental-flannel-overlay experimental-keystone-url experimental-nvidia-gpus experimental-prefix external-hostname external-ip -exit-on-lock-contention +extra-peer-dirs failover-timeout failure-domains fake-clientset diff --git a/pkg/api/v1/doc.go b/pkg/api/v1/doc.go index 0d81e264a0..8849ee1cb1 100644 --- a/pkg/api/v1/doc.go +++ b/pkg/api/v1/doc.go @@ -15,7 +15,7 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/api // Package v1 is the v1 version of the API. -// +genconversion=true package v1 diff --git a/pkg/api/v1/generated.proto b/pkg/api/v1/generated.proto index 86fbcabb3e..1aef723d98 100644 --- a/pkg/api/v1/generated.proto +++ b/pkg/api/v1/generated.proto @@ -2481,7 +2481,7 @@ message Secret { // It is provided as a write-only convenience method. // All keys and values are merged into the data field on write, overwriting any existing values. // It is never output when reading from the API. - // +genconversion=false + // +k8s:conversion-gen=false map stringData = 4; // Used to facilitate programmatic handling of secret data. @@ -2715,7 +2715,7 @@ message ServiceSpec { // API for compatibility until at least 8/20/2016. It will be removed from // any new API revisions. If both deprecatedPublicIPs *and* externalIPs are // set, deprecatedPublicIPs is used. - // +genconversion=false + // +k8s:conversion-gen=false repeated string deprecatedPublicIPs = 6; // Supports "ClientIP" and "None". Used to maintain session affinity. diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index 9cb0ac558a..d09101b8bd 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -2081,7 +2081,7 @@ type ServiceSpec struct { // API for compatibility until at least 8/20/2016. It will be removed from // any new API revisions. If both deprecatedPublicIPs *and* externalIPs are // set, deprecatedPublicIPs is used. - // +genconversion=false + // +k8s:conversion-gen=false DeprecatedPublicIPs []string `json:"deprecatedPublicIPs,omitempty" protobuf:"bytes,6,rep,name=deprecatedPublicIPs"` // Supports "ClientIP" and "None". Used to maintain session affinity. @@ -3096,7 +3096,7 @@ type Secret struct { // It is provided as a write-only convenience method. // All keys and values are merged into the data field on write, overwriting any existing values. // It is never output when reading from the API. - // +genconversion=false + // +k8s:conversion-gen=false StringData map[string]string `json:"stringData,omitempty" protobuf:"bytes,4,rep,name=stringData"` // Used to facilitate programmatic handling of secret data. diff --git a/pkg/apis/apps/v1alpha1/doc.go b/pkg/apis/apps/v1alpha1/doc.go index 97c790d9da..53d9fcabc4 100644 --- a/pkg/apis/apps/v1alpha1/doc.go +++ b/pkg/apis/apps/v1alpha1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/apps -// +genconversion=true package v1alpha1 diff --git a/pkg/apis/authentication.k8s.io/v1beta1/doc.go b/pkg/apis/authentication.k8s.io/v1beta1/doc.go index 8eebd4e879..25b76fd347 100644 --- a/pkg/apis/authentication.k8s.io/v1beta1/doc.go +++ b/pkg/apis/authentication.k8s.io/v1beta1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/authentication.k8s.io -// +genconversion=true package v1beta1 diff --git a/pkg/apis/authorization/v1beta1/doc.go b/pkg/apis/authorization/v1beta1/doc.go index 8eebd4e879..a6f2c27b10 100644 --- a/pkg/apis/authorization/v1beta1/doc.go +++ b/pkg/apis/authorization/v1beta1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/authorization -// +genconversion=true package v1beta1 diff --git a/pkg/apis/autoscaling/v1/doc.go b/pkg/apis/autoscaling/v1/doc.go index 0dc08faf0a..be1c70fdb5 100644 --- a/pkg/apis/autoscaling/v1/doc.go +++ b/pkg/apis/autoscaling/v1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/autoscaling -// +genconversion=true package v1 diff --git a/pkg/apis/batch/v1/doc.go b/pkg/apis/batch/v1/doc.go index 0dc08faf0a..5695b9e402 100644 --- a/pkg/apis/batch/v1/doc.go +++ b/pkg/apis/batch/v1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/batch -// +genconversion=true package v1 diff --git a/pkg/apis/batch/v2alpha1/doc.go b/pkg/apis/batch/v2alpha1/doc.go index c7cf93b9f9..76b5d32513 100644 --- a/pkg/apis/batch/v2alpha1/doc.go +++ b/pkg/apis/batch/v2alpha1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/batch -// +genconversion=true package v2alpha1 diff --git a/pkg/apis/certificates/v1alpha1/doc.go b/pkg/apis/certificates/v1alpha1/doc.go index 97c790d9da..3528c164f9 100644 --- a/pkg/apis/certificates/v1alpha1/doc.go +++ b/pkg/apis/certificates/v1alpha1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/certificates -// +genconversion=true package v1alpha1 diff --git a/pkg/apis/componentconfig/v1alpha1/doc.go b/pkg/apis/componentconfig/v1alpha1/doc.go index 97c790d9da..621e80613d 100644 --- a/pkg/apis/componentconfig/v1alpha1/doc.go +++ b/pkg/apis/componentconfig/v1alpha1/doc.go @@ -15,6 +15,6 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/componentconfig -// +genconversion=true package v1alpha1 diff --git a/pkg/apis/extensions/v1beta1/conversion_generated.go b/pkg/apis/extensions/v1beta1/conversion_generated.go index 727b26e886..6f4ff538bf 100644 --- a/pkg/apis/extensions/v1beta1/conversion_generated.go +++ b/pkg/apis/extensions/v1beta1/conversion_generated.go @@ -62,6 +62,8 @@ func init() { Convert_extensions_DeploymentStatus_To_v1beta1_DeploymentStatus, Convert_v1beta1_DeploymentStrategy_To_extensions_DeploymentStrategy, Convert_extensions_DeploymentStrategy_To_v1beta1_DeploymentStrategy, + Convert_v1beta1_ExportOptions_To_api_ExportOptions, + Convert_api_ExportOptions_To_v1beta1_ExportOptions, Convert_v1beta1_FSGroupStrategyOptions_To_extensions_FSGroupStrategyOptions, Convert_extensions_FSGroupStrategyOptions_To_v1beta1_FSGroupStrategyOptions, Convert_v1beta1_HTTPIngressPath_To_extensions_HTTPIngressPath, @@ -110,6 +112,8 @@ func init() { Convert_unversioned_LabelSelector_To_v1beta1_LabelSelector, Convert_v1beta1_LabelSelectorRequirement_To_unversioned_LabelSelectorRequirement, Convert_unversioned_LabelSelectorRequirement_To_v1beta1_LabelSelectorRequirement, + Convert_v1beta1_ListOptions_To_api_ListOptions, + Convert_api_ListOptions_To_v1beta1_ListOptions, Convert_v1beta1_NetworkPolicy_To_extensions_NetworkPolicy, Convert_extensions_NetworkPolicy_To_v1beta1_NetworkPolicy, Convert_v1beta1_NetworkPolicyIngressRule_To_extensions_NetworkPolicyIngressRule, @@ -644,6 +648,32 @@ func autoConvert_extensions_DeploymentStrategy_To_v1beta1_DeploymentStrategy(in return nil } +func autoConvert_v1beta1_ExportOptions_To_api_ExportOptions(in *ExportOptions, out *api.ExportOptions, s conversion.Scope) error { + if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil { + return err + } + out.Export = in.Export + out.Exact = in.Exact + return nil +} + +func Convert_v1beta1_ExportOptions_To_api_ExportOptions(in *ExportOptions, out *api.ExportOptions, s conversion.Scope) error { + return autoConvert_v1beta1_ExportOptions_To_api_ExportOptions(in, out, s) +} + +func autoConvert_api_ExportOptions_To_v1beta1_ExportOptions(in *api.ExportOptions, out *ExportOptions, s conversion.Scope) error { + if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil { + return err + } + out.Export = in.Export + out.Exact = in.Exact + return nil +} + +func Convert_api_ExportOptions_To_v1beta1_ExportOptions(in *api.ExportOptions, out *ExportOptions, s conversion.Scope) error { + return autoConvert_api_ExportOptions_To_v1beta1_ExportOptions(in, out, s) +} + func autoConvert_v1beta1_FSGroupStrategyOptions_To_extensions_FSGroupStrategyOptions(in *FSGroupStrategyOptions, out *extensions.FSGroupStrategyOptions, s conversion.Scope) error { out.Rule = extensions.FSGroupStrategyType(in.Rule) if in.Ranges != nil { @@ -1440,6 +1470,46 @@ func Convert_unversioned_LabelSelectorRequirement_To_v1beta1_LabelSelectorRequir return autoConvert_unversioned_LabelSelectorRequirement_To_v1beta1_LabelSelectorRequirement(in, out, s) } +func autoConvert_v1beta1_ListOptions_To_api_ListOptions(in *ListOptions, out *api.ListOptions, s conversion.Scope) error { + if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil { + return err + } + if err := api.Convert_string_To_labels_Selector(&in.LabelSelector, &out.LabelSelector, s); err != nil { + return err + } + if err := api.Convert_string_To_fields_Selector(&in.FieldSelector, &out.FieldSelector, s); err != nil { + return err + } + out.Watch = in.Watch + out.ResourceVersion = in.ResourceVersion + out.TimeoutSeconds = in.TimeoutSeconds + return nil +} + +func Convert_v1beta1_ListOptions_To_api_ListOptions(in *ListOptions, out *api.ListOptions, s conversion.Scope) error { + return autoConvert_v1beta1_ListOptions_To_api_ListOptions(in, out, s) +} + +func autoConvert_api_ListOptions_To_v1beta1_ListOptions(in *api.ListOptions, out *ListOptions, s conversion.Scope) error { + if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil { + return err + } + if err := api.Convert_labels_Selector_To_string(&in.LabelSelector, &out.LabelSelector, s); err != nil { + return err + } + if err := api.Convert_fields_Selector_To_string(&in.FieldSelector, &out.FieldSelector, s); err != nil { + return err + } + out.Watch = in.Watch + out.ResourceVersion = in.ResourceVersion + out.TimeoutSeconds = in.TimeoutSeconds + return nil +} + +func Convert_api_ListOptions_To_v1beta1_ListOptions(in *api.ListOptions, out *ListOptions, s conversion.Scope) error { + return autoConvert_api_ListOptions_To_v1beta1_ListOptions(in, out, s) +} + func autoConvert_v1beta1_NetworkPolicy_To_extensions_NetworkPolicy(in *NetworkPolicy, out *extensions.NetworkPolicy, s conversion.Scope) error { SetDefaults_NetworkPolicy(in) if err := api.Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(&in.TypeMeta, &out.TypeMeta, s); err != nil { diff --git a/pkg/apis/extensions/v1beta1/doc.go b/pkg/apis/extensions/v1beta1/doc.go index 8eebd4e879..dc8790024d 100644 --- a/pkg/apis/extensions/v1beta1/doc.go +++ b/pkg/apis/extensions/v1beta1/doc.go @@ -15,6 +15,8 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/extensions +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/autoscaling +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/batch -// +genconversion=true package v1beta1 diff --git a/pkg/apis/policy/v1alpha1/doc.go b/pkg/apis/policy/v1alpha1/doc.go index 37d78e2917..985d4bbf07 100644 --- a/pkg/apis/policy/v1alpha1/doc.go +++ b/pkg/apis/policy/v1alpha1/doc.go @@ -15,9 +15,9 @@ limitations under the License. */ // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/policy // Package policy is for any kind of policy object. Suitable examples, even if // they aren't all here, are PodDisruptionBudget, PodSecurityPolicy, // NetworkPolicy, etc. -// +genconversion=true package v1alpha1 diff --git a/pkg/apis/rbac/v1alpha1/doc.go b/pkg/apis/rbac/v1alpha1/doc.go index 35aef40894..e471bd384b 100644 --- a/pkg/apis/rbac/v1alpha1/doc.go +++ b/pkg/apis/rbac/v1alpha1/doc.go @@ -16,6 +16,6 @@ limitations under the License. // +groupName=rbac.authorization.k8s.io // +k8s:deepcopy-gen=package,register +// +k8s:conversion-gen=k8s.io/kubernetes/pkg/apis/rbac -// +genconversion=true package v1alpha1 From f63f168b514532377eb96cb25573c4a1a6ea7baf Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 27 Jun 2016 00:29:27 -0700 Subject: [PATCH 7/9] Comment and simplify a bit of conversion There are ample opportunities to optimize and streamline here. For example, there's no reason to have a function to convert IntStr to IntStr. Removing the function does generate the right assignment, but it is unclear whether the registered function is needed or not. I opted to leave it alone for now. Another example is Convert_Slice_byte_To_Slice_byte, which just seems silly. --- cmd/libs/go2idl/conversion-gen/generators/conversion.go | 5 +++++ pkg/api/conversion.go | 7 ++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index 670a1bdb5a..cfa62128b6 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -695,6 +695,11 @@ func (g *genConversion) doSlice(inType, outType *types.Type, sw *generator.Snipp funcName := g.funcNameTmpl(inType.Elem, outType.Elem) sw.Do(fmt.Sprintf("if err := %s(&(*in)[i], &(*out)[i], s); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) } else { + // TODO: This triggers on v1.ObjectMeta <-> api.ObjectMeta and + // similar because neither package is the target package, and + // we really don't know which package will have the conversion + // function defined. This fires on basically every object + // conversion outside of pkg/api/v1. sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) sw.Do("if err := s.Convert(&(*in)[i], &(*out)[i], 0); err != nil {\n", nil) } diff --git a/pkg/api/conversion.go b/pkg/api/conversion.go index 8a024a24b2..07585d8c35 100644 --- a/pkg/api/conversion.go +++ b/pkg/api/conversion.go @@ -100,15 +100,12 @@ func Convert_unversioned_TypeMeta_To_unversioned_TypeMeta(in, out *unversioned.T } func Convert_unversioned_ListMeta_To_unversioned_ListMeta(in, out *unversioned.ListMeta, s conversion.Scope) error { - out.ResourceVersion = in.ResourceVersion - out.SelfLink = in.SelfLink + *out = *in return nil } func Convert_intstr_IntOrString_To_intstr_IntOrString(in, out *intstr.IntOrString, s conversion.Scope) error { - out.Type = in.Type - out.IntVal = in.IntVal - out.StrVal = in.StrVal + *out = *in return nil } From 355794c303752ddc799d0bdaa53fd64335650da6 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 27 Jun 2016 01:27:05 -0700 Subject: [PATCH 8/9] Log errors or fail if conversion fail to generate This fixes PodSpec to generate cleanly. No other types only half-generate (so now we Fatalf), though several fail to generate at all (only Errorf for now). --- .../conversion-gen/generators/conversion.go | 15 +++ pkg/api/types.go | 3 + pkg/api/v1/conversion.go | 117 ++---------------- pkg/api/v1/conversion_generated.go | 84 +++++++++++++ pkg/api/v1/generated.proto | 4 + pkg/api/v1/types.go | 4 + 6 files changed, 118 insertions(+), 109 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index cfa62128b6..a9c93c52fb 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -573,12 +573,27 @@ func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.W glog.V(5).Infof("generating for type %v", t) sw := generator.NewSnippetWriter(w, c, "$", "$") peerType := getPeerTypeFor(c, t, g.peerPackages) + didForward, didBackward := false, false if isDirectlyConvertible(t, peerType, g.manualConversions) { + didForward = true g.generateConversion(t, peerType, sw) } if isDirectlyConvertible(peerType, t, g.manualConversions) { + didBackward = true g.generateConversion(peerType, t, sw) } + if didForward != didBackward { + glog.Fatalf("Could only generate one direction of conversion for %v <-> %v", t, peerType) + } + if !didForward && !didBackward { + // TODO: This should be fatal but we have at least 8 types that + // currently fail this. The right thing to do is to figure out why they + // can't be generated and mark those fields as + // +k8s:conversion-gen=false, and ONLY do manual conversions for those + // fields, with the manual Convert_...() calling autoConvert_...() + // first. + glog.Errorf("Warning: could not generate autoConvert functions for %v <-> %v", t, peerType) + } return sw.Error() } diff --git a/pkg/api/types.go b/pkg/api/types.go index 1c038d0dc1..7225e8fa89 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -1479,12 +1479,15 @@ type PodSecurityContext struct { // Use the host's network namespace. If this option is set, the ports that will be // used must be specified. // Optional: Default to false + // +k8s:conversion-gen=false HostNetwork bool `json:"hostNetwork,omitempty"` // Use the host's pid namespace. // Optional: Default to false. + // +k8s:conversion-gen=false HostPID bool `json:"hostPID,omitempty"` // Use the host's ipc namespace. // Optional: Default to false. + // +k8s:conversion-gen=false HostIPC bool `json:"hostIPC,omitempty"` // The SELinux context to be applied to all containers. // If unspecified, the container runtime will allocate a random SELinux context for each diff --git a/pkg/api/v1/conversion.go b/pkg/api/v1/conversion.go index 7e760a49da..642b1bc5e5 100644 --- a/pkg/api/v1/conversion.go +++ b/pkg/api/v1/conversion.go @@ -360,124 +360,34 @@ func Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(in *PodTemplateSpec, out // The following two PodSpec conversions are done here to support ServiceAccount // as an alias for ServiceAccountName. func Convert_api_PodSpec_To_v1_PodSpec(in *api.PodSpec, out *PodSpec, s conversion.Scope) error { - if in.Volumes != nil { - out.Volumes = make([]Volume, len(in.Volumes)) - for i := range in.Volumes { - if err := Convert_api_Volume_To_v1_Volume(&in.Volumes[i], &out.Volumes[i], s); err != nil { - return err - } - } - } else { - out.Volumes = nil - } - if in.InitContainers != nil { - out.InitContainers = make([]Container, len(in.InitContainers)) - for i := range in.InitContainers { - if err := Convert_api_Container_To_v1_Container(&in.InitContainers[i], &out.InitContainers[i], s); err != nil { - return err - } - } - } else { - out.InitContainers = nil - } - if in.Containers != nil { - out.Containers = make([]Container, len(in.Containers)) - for i := range in.Containers { - if err := Convert_api_Container_To_v1_Container(&in.Containers[i], &out.Containers[i], s); err != nil { - return err - } - } - } else { - out.Containers = nil + if err := autoConvert_api_PodSpec_To_v1_PodSpec(in, out, s); err != nil { + return err } - out.RestartPolicy = RestartPolicy(in.RestartPolicy) - out.TerminationGracePeriodSeconds = in.TerminationGracePeriodSeconds - out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds - out.DNSPolicy = DNSPolicy(in.DNSPolicy) - out.NodeSelector = in.NodeSelector - - out.ServiceAccountName = in.ServiceAccountName // DeprecatedServiceAccount is an alias for ServiceAccountName. out.DeprecatedServiceAccount = in.ServiceAccountName - out.NodeName = in.NodeName - if in.SecurityContext != nil { - out.SecurityContext = new(PodSecurityContext) - if err := Convert_api_PodSecurityContext_To_v1_PodSecurityContext(in.SecurityContext, out.SecurityContext, s); err != nil { - return err - } + if in.SecurityContext != nil { // the host namespace fields have to be handled here for backward compatibility // with v1.0.0 out.HostPID = in.SecurityContext.HostPID out.HostNetwork = in.SecurityContext.HostNetwork out.HostIPC = in.SecurityContext.HostIPC } - if in.ImagePullSecrets != nil { - out.ImagePullSecrets = make([]LocalObjectReference, len(in.ImagePullSecrets)) - for i := range in.ImagePullSecrets { - if err := Convert_api_LocalObjectReference_To_v1_LocalObjectReference(&in.ImagePullSecrets[i], &out.ImagePullSecrets[i], s); err != nil { - return err - } - } - } else { - out.ImagePullSecrets = nil - } - out.Hostname = in.Hostname - out.Subdomain = in.Subdomain + return nil } func Convert_v1_PodSpec_To_api_PodSpec(in *PodSpec, out *api.PodSpec, s conversion.Scope) error { - SetDefaults_PodSpec(in) - if in.Volumes != nil { - out.Volumes = make([]api.Volume, len(in.Volumes)) - for i := range in.Volumes { - if err := Convert_v1_Volume_To_api_Volume(&in.Volumes[i], &out.Volumes[i], s); err != nil { - return err - } - } - } else { - out.Volumes = nil + if err := autoConvert_v1_PodSpec_To_api_PodSpec(in, out, s); err != nil { + return err } - if in.InitContainers != nil { - out.InitContainers = make([]api.Container, len(in.InitContainers)) - for i := range in.InitContainers { - if err := Convert_v1_Container_To_api_Container(&in.InitContainers[i], &out.InitContainers[i], s); err != nil { - return err - } - } - } else { - out.InitContainers = nil - } - if in.Containers != nil { - out.Containers = make([]api.Container, len(in.Containers)) - for i := range in.Containers { - if err := Convert_v1_Container_To_api_Container(&in.Containers[i], &out.Containers[i], s); err != nil { - return err - } - } - } else { - out.Containers = nil - } - out.RestartPolicy = api.RestartPolicy(in.RestartPolicy) - out.TerminationGracePeriodSeconds = in.TerminationGracePeriodSeconds - out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds - out.DNSPolicy = api.DNSPolicy(in.DNSPolicy) - out.NodeSelector = in.NodeSelector + // We support DeprecatedServiceAccount as an alias for ServiceAccountName. // If both are specified, ServiceAccountName (the new field) wins. - out.ServiceAccountName = in.ServiceAccountName if in.ServiceAccountName == "" { out.ServiceAccountName = in.DeprecatedServiceAccount } - out.NodeName = in.NodeName - if in.SecurityContext != nil { - out.SecurityContext = new(api.PodSecurityContext) - if err := Convert_v1_PodSecurityContext_To_api_PodSecurityContext(in.SecurityContext, out.SecurityContext, s); err != nil { - return err - } - } // the host namespace fields have to be handled specially for backward compatibility // with v1.0.0 @@ -487,18 +397,7 @@ func Convert_v1_PodSpec_To_api_PodSpec(in *PodSpec, out *api.PodSpec, s conversi out.SecurityContext.HostNetwork = in.HostNetwork out.SecurityContext.HostPID = in.HostPID out.SecurityContext.HostIPC = in.HostIPC - if in.ImagePullSecrets != nil { - out.ImagePullSecrets = make([]api.LocalObjectReference, len(in.ImagePullSecrets)) - for i := range in.ImagePullSecrets { - if err := Convert_v1_LocalObjectReference_To_api_LocalObjectReference(&in.ImagePullSecrets[i], &out.ImagePullSecrets[i], s); err != nil { - return err - } - } - } else { - out.ImagePullSecrets = nil - } - out.Hostname = in.Hostname - out.Subdomain = in.Subdomain + return nil } diff --git a/pkg/api/v1/conversion_generated.go b/pkg/api/v1/conversion_generated.go index 6c2516fe00..a9cb349c0d 100644 --- a/pkg/api/v1/conversion_generated.go +++ b/pkg/api/v1/conversion_generated.go @@ -4785,6 +4785,90 @@ func autoConvert_v1_PodSecurityContext_To_api_PodSecurityContext(in *PodSecurity return nil } +func autoConvert_api_PodSecurityContext_To_v1_PodSecurityContext(in *api.PodSecurityContext, out *PodSecurityContext, s conversion.Scope) error { + if in.SELinuxOptions != nil { + in, out := &in.SELinuxOptions, &out.SELinuxOptions + *out = new(SELinuxOptions) + if err := Convert_api_SELinuxOptions_To_v1_SELinuxOptions(*in, *out, s); err != nil { + return err + } + } else { + out.SELinuxOptions = nil + } + out.RunAsUser = in.RunAsUser + out.RunAsNonRoot = in.RunAsNonRoot + out.SupplementalGroups = in.SupplementalGroups + out.FSGroup = in.FSGroup + return nil +} + +func autoConvert_v1_PodSpec_To_api_PodSpec(in *PodSpec, out *api.PodSpec, s conversion.Scope) error { + SetDefaults_PodSpec(in) + if in.Volumes != nil { + in, out := &in.Volumes, &out.Volumes + *out = make([]api.Volume, len(*in)) + for i := range *in { + if err := Convert_v1_Volume_To_api_Volume(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Volumes = nil + } + if in.InitContainers != nil { + in, out := &in.InitContainers, &out.InitContainers + *out = make([]api.Container, len(*in)) + for i := range *in { + if err := Convert_v1_Container_To_api_Container(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.InitContainers = nil + } + if in.Containers != nil { + in, out := &in.Containers, &out.Containers + *out = make([]api.Container, len(*in)) + for i := range *in { + if err := Convert_v1_Container_To_api_Container(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Containers = nil + } + out.RestartPolicy = api.RestartPolicy(in.RestartPolicy) + out.TerminationGracePeriodSeconds = in.TerminationGracePeriodSeconds + out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds + out.DNSPolicy = api.DNSPolicy(in.DNSPolicy) + out.NodeSelector = in.NodeSelector + out.ServiceAccountName = in.ServiceAccountName + out.NodeName = in.NodeName + if in.SecurityContext != nil { + in, out := &in.SecurityContext, &out.SecurityContext + *out = new(api.PodSecurityContext) + if err := Convert_v1_PodSecurityContext_To_api_PodSecurityContext(*in, *out, s); err != nil { + return err + } + } else { + out.SecurityContext = nil + } + if in.ImagePullSecrets != nil { + in, out := &in.ImagePullSecrets, &out.ImagePullSecrets + *out = make([]api.LocalObjectReference, len(*in)) + for i := range *in { + if err := Convert_v1_LocalObjectReference_To_api_LocalObjectReference(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.ImagePullSecrets = nil + } + out.Hostname = in.Hostname + out.Subdomain = in.Subdomain + return nil +} + func autoConvert_api_PodSpec_To_v1_PodSpec(in *api.PodSpec, out *PodSpec, s conversion.Scope) error { if in.Volumes != nil { in, out := &in.Volumes, &out.Volumes diff --git a/pkg/api/v1/generated.proto b/pkg/api/v1/generated.proto index 1aef723d98..f2101dfc9a 100644 --- a/pkg/api/v1/generated.proto +++ b/pkg/api/v1/generated.proto @@ -2089,6 +2089,7 @@ message PodSpec { // DeprecatedServiceAccount is a depreciated alias for ServiceAccountName. // Deprecated: Use serviceAccountName instead. + // +k8s:conversion-gen=false optional string serviceAccount = 9; // NodeName is a request to schedule this pod onto a specific node. If it is non-empty, @@ -2099,14 +2100,17 @@ message PodSpec { // Host networking requested for this pod. Use the host's network namespace. // If this option is set, the ports that will be used must be specified. // Default to false. + // +k8s:conversion-gen=false optional bool hostNetwork = 11; // Use the host's pid namespace. // Optional: Default to false. + // +k8s:conversion-gen=false optional bool hostPID = 12; // Use the host's ipc namespace. // Optional: Default to false. + // +k8s:conversion-gen=false optional bool hostIPC = 13; // SecurityContext holds pod-level security attributes and common container settings. diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index d09101b8bd..5dc3afceb5 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -1719,6 +1719,7 @@ type PodSpec struct { ServiceAccountName string `json:"serviceAccountName,omitempty" protobuf:"bytes,8,opt,name=serviceAccountName"` // DeprecatedServiceAccount is a depreciated alias for ServiceAccountName. // Deprecated: Use serviceAccountName instead. + // +k8s:conversion-gen=false DeprecatedServiceAccount string `json:"serviceAccount,omitempty" protobuf:"bytes,9,opt,name=serviceAccount"` // NodeName is a request to schedule this pod onto a specific node. If it is non-empty, @@ -1728,12 +1729,15 @@ type PodSpec struct { // Host networking requested for this pod. Use the host's network namespace. // If this option is set, the ports that will be used must be specified. // Default to false. + // +k8s:conversion-gen=false HostNetwork bool `json:"hostNetwork,omitempty" protobuf:"varint,11,opt,name=hostNetwork"` // Use the host's pid namespace. // Optional: Default to false. + // +k8s:conversion-gen=false HostPID bool `json:"hostPID,omitempty" protobuf:"varint,12,opt,name=hostPID"` // Use the host's ipc namespace. // Optional: Default to false. + // +k8s:conversion-gen=false HostIPC bool `json:"hostIPC,omitempty" protobuf:"varint,13,opt,name=hostIPC"` // SecurityContext holds pod-level security attributes and common container settings. // Optional: Defaults to empty. See type description for default values of each field. From a892b26bf8c4649501852667e02810b323b106f7 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 27 Jun 2016 01:33:46 -0700 Subject: [PATCH 9/9] small docs update --- docs/devel/api_changes.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/devel/api_changes.md b/docs/devel/api_changes.md index d5ead76024..35d7a545cb 100644 --- a/docs/devel/api_changes.md +++ b/docs/devel/api_changes.md @@ -522,9 +522,8 @@ At the moment, you'll have to make a new directory under `pkg/apis/`; copy the directory structure from `pkg/apis/extensions`. Add the new group/version to all of the `hack/{verify,update}-generated-{deep-copy,conversions,swagger}.sh` files in the appropriate places--it should just require adding your new group/version -to a bash array. You will also need to make sure your new types are imported by -the generation commands (`cmd/gendeepcopy/` & `cmd/genconversion`). These -instructions may not be complete and will be updated as we gain experience. +to a bash array. See [docs on adding an API group](adding-an-APIGroup.md) for +more. Adding API groups outside of the `pkg/apis/` directory is not currently supported, but is clearly desirable. The deep copy & conversion generators need