From 7ccd3e36d2b850d7ba573861a469064cdbfc07fa Mon Sep 17 00:00:00 2001 From: Elijah Oyekunle Date: Thu, 10 Oct 2019 09:23:59 +0100 Subject: [PATCH] add inverse import rules --- .travis.yml | 2 +- args/args.go | 3 + examples/import-boss/Makefile | 28 +++ .../import-boss/generators/import_restrict.go | 212 ++++++++++++++---- examples/import-boss/main.go | 57 ++++- examples/import-boss/tests/inverse/a/doc.go | 6 + examples/import-boss/tests/inverse/b/doc.go | 7 + examples/import-boss/tests/inverse/c/doc.go | 7 + examples/import-boss/tests/inverse/d/doc.go | 6 + examples/import-boss/tests/inverse/lib/doc.go | 6 + .../import-boss/tests/inverse/lib/lib_test.go | 5 + .../inverse/lib/nonprod/.import-restrictions | 9 + .../tests/inverse/lib/nonprod/doc.go | 2 + .../inverse/lib/private/.import-restrictions | 11 + .../tests/inverse/lib/private/doc.go | 1 + .../tests/inverse/lib/public/doc.go | 1 + .../tests/inverse/lib/quarantine/doc.go | 6 + .../tests/rules/a/.import-restrictions | 11 + examples/import-boss/tests/rules/a/doc.go | 5 + .../tests/rules/b/.import-restrictions | 11 + examples/import-boss/tests/rules/b/doc.go | 6 + examples/import-boss/tests/rules/c/doc.go | 1 + .../tests/rules/nested/.import-restrictions | 10 + .../import-boss/tests/rules/nested/doc.go | 5 + .../rules/nested/nested/.import-restrictions | 10 + .../tests/rules/nested/nested/doc.go | 5 + .../nested/nested/nested/.import-restrictions | 10 + .../tests/rules/nested/nested/nested/doc.go | 5 + .../rules/nested/nested/nested/inherit/doc.go | 5 + generator/default_package.go | 7 +- generator/execute.go | 12 +- generator/generator.go | 53 ++++- generator/transitive_closure.go | 65 ++++++ generator/transitive_closure_test.go | 64 ++++++ parser/parse.go | 14 +- 35 files changed, 596 insertions(+), 72 deletions(-) create mode 100644 examples/import-boss/Makefile create mode 100644 examples/import-boss/tests/inverse/a/doc.go create mode 100644 examples/import-boss/tests/inverse/b/doc.go create mode 100644 examples/import-boss/tests/inverse/c/doc.go create mode 100644 examples/import-boss/tests/inverse/d/doc.go create mode 100644 examples/import-boss/tests/inverse/lib/doc.go create mode 100644 examples/import-boss/tests/inverse/lib/lib_test.go create mode 100644 examples/import-boss/tests/inverse/lib/nonprod/.import-restrictions create mode 100644 examples/import-boss/tests/inverse/lib/nonprod/doc.go create mode 100644 examples/import-boss/tests/inverse/lib/private/.import-restrictions create mode 100644 examples/import-boss/tests/inverse/lib/private/doc.go create mode 100644 examples/import-boss/tests/inverse/lib/public/doc.go create mode 100644 examples/import-boss/tests/inverse/lib/quarantine/doc.go create mode 100644 examples/import-boss/tests/rules/a/.import-restrictions create mode 100644 examples/import-boss/tests/rules/a/doc.go create mode 100644 examples/import-boss/tests/rules/b/.import-restrictions create mode 100644 examples/import-boss/tests/rules/b/doc.go create mode 100644 examples/import-boss/tests/rules/c/doc.go create mode 100644 examples/import-boss/tests/rules/nested/.import-restrictions create mode 100644 examples/import-boss/tests/rules/nested/doc.go create mode 100644 examples/import-boss/tests/rules/nested/nested/.import-restrictions create mode 100644 examples/import-boss/tests/rules/nested/nested/doc.go create mode 100644 examples/import-boss/tests/rules/nested/nested/nested/.import-restrictions create mode 100644 examples/import-boss/tests/rules/nested/nested/nested/doc.go create mode 100644 examples/import-boss/tests/rules/nested/nested/nested/inherit/doc.go create mode 100644 generator/transitive_closure.go create mode 100644 generator/transitive_closure_test.go diff --git a/.travis.yml b/.travis.yml index 1fd593ac..3ed9b349 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,4 +13,4 @@ jobs: - go test -v ./... - stage: Verify examples script: - - go run ./examples/import-boss/main.go -i k8s.io/gengo/... --verify-only + - go run ./examples/import-boss/main.go -i $(go list k8s.io/gengo/... | grep -v import-boss/tests | paste -sd',' -) --verify-only diff --git a/args/args.go b/args/args.go index 7401098c..26c790e6 100644 --- a/args/args.go +++ b/args/args.go @@ -159,6 +159,9 @@ func (g *GeneratorArgs) InputIncludes(p *types.Package) bool { if strings.HasSuffix(d, "...") { d = strings.TrimSuffix(d, "...") } + if strings.HasPrefix(d, "./vendor/") { + d = strings.TrimPrefix(d, "./vendor/") + } if strings.HasPrefix(p.Path, d) { return true } diff --git a/examples/import-boss/Makefile b/examples/import-boss/Makefile new file mode 100644 index 00000000..b1e24786 --- /dev/null +++ b/examples/import-boss/Makefile @@ -0,0 +1,28 @@ +TOOL=import-boss + +all: test + +test: build test_rules test_inverse + +build: + @go build -o /tmp/$(TOOL) + +test_rules: + /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/a) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/b) + /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/c) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/nested) + /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/nested/nested) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/nested/nested/nested) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/rules/nested/nested/nested/inherit) + +test_inverse: + /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/a ./tests/inverse/lib/... | grep -v quarantine | paste -sd',' -) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/b ./tests/inverse/lib/... | grep -v quarantine | paste -sd',' -) + /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/c ./tests/inverse/lib/... | grep -v quarantine | paste -sd',' -) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/d ./tests/inverse/lib/... | grep -v quarantine | paste -sd',' -) + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/lib/... | paste -sd',' -) + + ! /tmp/$(TOOL) --logtostderr --v=4 -i $$(go list ./tests/inverse/lib/... | paste -sd',' -) + +.PHONY: build test test_rules test_inverse diff --git a/examples/import-boss/generators/import_restrict.go b/examples/import-boss/generators/import_restrict.go index ea5716b6..fb73c03b 100644 --- a/examples/import-boss/generators/import_restrict.go +++ b/examples/import-boss/generators/import_restrict.go @@ -38,6 +38,7 @@ import ( ) const ( + goModFile = "go.mod" importBossFileType = "import-boss" ) @@ -58,7 +59,7 @@ func DefaultNameSystem() string { func Packages(c *generator.Context, arguments *args.GeneratorArgs) generator.Packages { pkgs := generator.Packages{} c.FileTypes = map[string]generator.FileType{ - importBossFileType: importRuleFile{}, + importBossFileType: importRuleFile{c}, } for _, p := range c.Universe { @@ -70,6 +71,7 @@ func Packages(c *generator.Context, arguments *args.GeneratorArgs) generator.Pac pkgs = append(pkgs, &generator.DefaultPackage{ PackageName: p.Name, PackagePath: p.Path, + Source: p.SourcePath, // GeneratorFunc returns a list of generators. Each generator makes a // single file. GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { @@ -96,10 +98,19 @@ type Rule struct { ForbiddenPrefixes []string } +type InverseRule struct { + Rule + // True if the rule is to be applied to transitive imports. + Transitive bool +} + type fileFormat struct { CurrentImports []string - Rules []Rule + Rules []Rule + InverseRules []InverseRule + + path string } func readFile(path string) (*fileFormat, error) { @@ -113,6 +124,7 @@ func readFile(path string) (*fileFormat, error) { if err != nil { return nil, fmt.Errorf("couldn't unmarshal %v: %v", path, err) } + current.path = path return ¤t, nil } @@ -131,10 +143,12 @@ func writeFile(path string, ff *fileFormat) error { } // This does the actual checking, since it knows the literal destination file. -type importRuleFile struct{} +type importRuleFile struct { + context *generator.Context +} -func (importRuleFile) AssembleFile(f *generator.File, path string) error { - return nil +func (irf importRuleFile) AssembleFile(f *generator.File, path string) error { + return irf.VerifyFile(f, path) } // TODO: make a flag to enable this, or expose this information in some other way. @@ -169,62 +183,99 @@ func removeLastDir(path string) (newPath, removedDir string) { return filepath.Join(filepath.Dir(dir), file), filepath.Base(dir) } -// Keep going up a directory until we find an .import-restrictions file. -func recursiveRead(path string) (*fileFormat, string, error) { +// isGoModRoot checks if a directory is the root directory for a package +// by checking for the existence of a 'go.mod' file in that directory. +func isGoModRoot(path string) bool { + _, err := os.Stat(filepath.Join(filepath.Dir(path), goModFile)) + return err == nil +} + +// recursiveRead collects all '.import-restriction' files, between the current directory, +// and the package root when Go modules are enabled, or $GOPATH/src when they are not. +func recursiveRead(path string) ([]*fileFormat, error) { + restrictionFiles := make([]*fileFormat, 0) + for { if _, err := os.Stat(path); err == nil { - ff, err := readFile(path) - return ff, path, err + rules, err := readFile(path) + if err != nil { + return nil, err + } + + restrictionFiles = append(restrictionFiles, rules) } nextPath, removedDir := removeLastDir(path) - if nextPath == path || removedDir == "src" { + if nextPath == path || isGoModRoot(path) || removedDir == "src" { break } + path = nextPath } - return nil, "", nil + + return restrictionFiles, nil } -func (importRuleFile) VerifyFile(f *generator.File, path string) error { - rules, actualPath, err := recursiveRead(path) +func (irf importRuleFile) VerifyFile(f *generator.File, path string) error { + restrictionFiles, err := recursiveRead(filepath.Join(f.PackageSourcePath, f.Name)) if err != nil { return fmt.Errorf("error finding rules file: %v", err) } - if rules == nil { - // No restrictions on this directory. - return nil + if err := irf.verifyRules(restrictionFiles, f); err != nil { + return err + } + + return irf.verifyInverseRules(restrictionFiles, f) +} + +func (irf importRuleFile) verifyRules(restrictionFiles []*fileFormat, f *generator.File) error { + selectors := make([][]*regexp.Regexp, len(restrictionFiles)) + for i, restrictionFile := range restrictionFiles { + for _, r := range restrictionFile.Rules { + re, err := regexp.Compile(r.SelectorRegexp) + if err != nil { + return fmt.Errorf("regexp `%s` in file %q doesn't compile: %v", r.SelectorRegexp, restrictionFile.path, err) + } + + selectors[i] = append(selectors[i], re) + } } forbiddenImports := map[string]string{} allowedMismatchedImports := []string{} - for _, r := range rules.Rules { - re, err := regexp.Compile(r.SelectorRegexp) - if err != nil { - return fmt.Errorf("regexp `%s` in file %q doesn't compile: %v", r.SelectorRegexp, actualPath, err) - } - for v := range f.Imports { - klog.V(4).Infof("Checking %v matches %v: %v\n", r.SelectorRegexp, v, re.MatchString(v)) - if !re.MatchString(v) { - continue - } - for _, forbidden := range r.ForbiddenPrefixes { - klog.V(4).Infof("Checking %v against %v\n", v, forbidden) - if strings.HasPrefix(v, forbidden) { - forbiddenImports[v] = forbidden + + for v := range f.Imports { + explicitlyAllowed := false + + NextRestrictionFiles: + for i, rules := range restrictionFiles { + for j, r := range rules.Rules { + matching := selectors[i][j].MatchString(v) + klog.V(5).Infof("Checking %v matches %v: %v\n", r.SelectorRegexp, v, matching) + if !matching { + continue } - } - found := false - for _, allowed := range r.AllowedPrefixes { - klog.V(4).Infof("Checking %v against %v\n", v, allowed) - if strings.HasPrefix(v, allowed) { - found = true - break + for _, forbidden := range r.ForbiddenPrefixes { + klog.V(4).Infof("Checking %v against %v\n", v, forbidden) + if strings.HasPrefix(v, forbidden) { + forbiddenImports[v] = forbidden + } + } + for _, allowed := range r.AllowedPrefixes { + klog.V(4).Infof("Checking %v against %v\n", v, allowed) + if strings.HasPrefix(v, allowed) { + explicitlyAllowed = true + break + } + } + + if !explicitlyAllowed { + allowedMismatchedImports = append(allowedMismatchedImports, v) + } else { + klog.V(2).Infof("%v importing %v allowed by %v\n", f.PackagePath, v, restrictionFiles[i].path) + break NextRestrictionFiles } - } - if !found { - allowedMismatchedImports = append(allowedMismatchedImports, v) } } } @@ -243,8 +294,85 @@ func (importRuleFile) VerifyFile(f *generator.File, path string) error { } return errors.New(errorBuilder.String()) } - if len(rules.Rules) > 0 { - klog.V(2).Infof("%v passes rules found in %v\n", path, actualPath) + + return nil +} + +// verifyInverseRules checks that all packages that import a package are allowed to import it. +func (irf importRuleFile) verifyInverseRules(restrictionFiles []*fileFormat, f *generator.File) error { + // compile all Selector regex in all restriction files + selectors := make([][]*regexp.Regexp, len(restrictionFiles)) + for i, restrictionFile := range restrictionFiles { + for _, r := range restrictionFile.InverseRules { + re, err := regexp.Compile(r.SelectorRegexp) + if err != nil { + return fmt.Errorf("regexp `%s` in file %q doesn't compile: %v", r.SelectorRegexp, restrictionFile.path, err) + } + + selectors[i] = append(selectors[i], re) + } + } + + directImport := map[string]bool{} + for _, imp := range irf.context.IncomingImports()[f.PackagePath] { + directImport[imp] = true + } + + forbiddenImports := map[string]string{} + allowedMismatchedImports := []string{} + + for _, v := range irf.context.TransitiveIncomingImports()[f.PackagePath] { + explicitlyAllowed := false + + NextRestrictionFiles: + for i, rules := range restrictionFiles { + for j, r := range rules.InverseRules { + if !r.Transitive && !directImport[v] { + continue + } + + re := selectors[i][j] + matching := re.MatchString(v) + klog.V(4).Infof("Checking %v matches %v (importing %v: %v\n", r.SelectorRegexp, v, f.PackagePath, matching) + if !matching { + continue + } + for _, forbidden := range r.ForbiddenPrefixes { + klog.V(4).Infof("Checking %v against %v\n", v, forbidden) + if strings.HasPrefix(v, forbidden) { + forbiddenImports[v] = forbidden + } + } + for _, allowed := range r.AllowedPrefixes { + klog.V(4).Infof("Checking %v against %v\n", v, allowed) + if strings.HasPrefix(v, allowed) { + explicitlyAllowed = true + break + } + } + if !explicitlyAllowed { + allowedMismatchedImports = append(allowedMismatchedImports, v) + } else { + klog.V(2).Infof("%v importing %v allowed by %v\n", v, f.PackagePath, restrictionFiles[i].path) + break NextRestrictionFiles + } + } + } + } + + if len(forbiddenImports) > 0 || len(allowedMismatchedImports) > 0 { + var errorBuilder strings.Builder + for i, f := range forbiddenImports { + fmt.Fprintf(&errorBuilder, "(inverse): import %v has forbidden prefix %v\n", i, f) + } + if len(allowedMismatchedImports) > 0 { + sort.Sort(sort.StringSlice(allowedMismatchedImports)) + fmt.Fprintf(&errorBuilder, "(inverse): the following imports did not match any allowed prefix:\n") + for _, i := range allowedMismatchedImports { + fmt.Fprintf(&errorBuilder, " %v\n", i) + } + } + return errors.New(errorBuilder.String()) } return nil diff --git a/examples/import-boss/main.go b/examples/import-boss/main.go index 44b0d63e..3e5f31b4 100644 --- a/examples/import-boss/main.go +++ b/examples/import-boss/main.go @@ -16,24 +16,37 @@ limitations under the License. // import-boss enforces import restrictions in a given repository. // -// When a directory is verified, import-boss looks for a file called -// ".import-restrictions". If this file is not found, parent directories will be -// recursively searched. +// When a package is verified, import-boss looks for files called +// ".import-restrictions" in all directories between the package and +// the $GOPATH/src. // -// If an ".import-restrictions" file is found, then all imports of the package -// are checked against each "rule" in the file. A rule consists of three parts: +// All imports of the package are checked against each "rule" in the +// found restriction files, climbing up the directory tree until the +// import matches one of the rules. +// +// If the import does not match any of the rules, it is accepted. +// +// In analogy, all incoming imports of the package are checked against each +// "inverse rule" in the found restriction files, climbing up the directory +// tree until the import matches one of the rules. +// +// If the incoming import does not match any of the inverse rules, it is accepted. +// +// A rule consists of three parts: // * A SelectorRegexp, to select the import paths that the rule applies to. // * A list of AllowedPrefixes // * A list of ForbiddenPrefixes -// An import is allowed if it matches at least one allowed prefix and does not -// match any forbidden prefix. An example file looks like this: +// An import passes a rule of a matching selector if it matches at least one +// allowed prefix, but no forbidden prefix. +// +// An example file looks like this: // // { // "Rules": [ // { // "SelectorRegexp": "k8s[.]io", // "AllowedPrefixes": [ -// "k8s.io/gengo", +// "k8s.io/gengo/examples", // "k8s.io/kubernetes/third_party" // ], // "ForbiddenPrefixes": [ @@ -48,11 +61,37 @@ limitations under the License. // "" // ] // } +// ], +// "InverseRules": [{ +// "SelectorRegexp": "k8s[.]io", +// "AllowedPrefixes": [ +// "k8s.io/same-repo", +// "k8s.io/kubernetes/pkg/legacy" +// ], +// "ForbiddenPrefixes": [ +// "k8s.io/kubernetes/pkg/legacy/subpkg" +// ] +// }, +// { +// "SelectorRegexp": "k8s[.]io", +// "Transitive": true, +// "AllowedPrefixes": [ +// "k8s.io/ +// ], +// "ForbiddenPrefixes": [ +// "k8s.io/kubernetes/cmd/kubelet", +// "k8s.io/kubernetes/cmd/kubectl" +// ], // ] // } // -// Note the secound block explicitly matches the unsafe package, and forbids it +// Note the second (non-inverse) rule explicitly matches the unsafe package, and forbids it // ("" is a prefix of everything). +// +// An import from another package passes an inverse rule with a matching selector if +// it matches at least one allowed prefix, but no forbidden prefix. +// +// Note that the second InverseRule is transitive, the first only applies to direct imports. package main import ( diff --git a/examples/import-boss/tests/inverse/a/doc.go b/examples/import-boss/tests/inverse/a/doc.go new file mode 100644 index 00000000..4a831853 --- /dev/null +++ b/examples/import-boss/tests/inverse/a/doc.go @@ -0,0 +1,6 @@ +// a only imports public packages. +package a + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/public" +) diff --git a/examples/import-boss/tests/inverse/b/doc.go b/examples/import-boss/tests/inverse/b/doc.go new file mode 100644 index 00000000..c9044187 --- /dev/null +++ b/examples/import-boss/tests/inverse/b/doc.go @@ -0,0 +1,7 @@ +// b only imports public and private packages. The latter it shouldn't. +package b + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/private" + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/public" +) diff --git a/examples/import-boss/tests/inverse/c/doc.go b/examples/import-boss/tests/inverse/c/doc.go new file mode 100644 index 00000000..7438e478 --- /dev/null +++ b/examples/import-boss/tests/inverse/c/doc.go @@ -0,0 +1,7 @@ +// c imports the library root, which in turn imports the public and private packages. This is fine because +// the private package is not directly imported. +package c + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib" +) diff --git a/examples/import-boss/tests/inverse/d/doc.go b/examples/import-boss/tests/inverse/d/doc.go new file mode 100644 index 00000000..3e61b6ce --- /dev/null +++ b/examples/import-boss/tests/inverse/d/doc.go @@ -0,0 +1,6 @@ +// c imports non-prod code. It shouldn't. +package d + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/nonprod" +) diff --git a/examples/import-boss/tests/inverse/lib/doc.go b/examples/import-boss/tests/inverse/lib/doc.go new file mode 100644 index 00000000..6ed837cd --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/doc.go @@ -0,0 +1,6 @@ +package lib + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/private" + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/public" +) diff --git a/examples/import-boss/tests/inverse/lib/lib_test.go b/examples/import-boss/tests/inverse/lib/lib_test.go new file mode 100644 index 00000000..78a02b2a --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/lib_test.go @@ -0,0 +1,5 @@ +package lib + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/nonprod" +) diff --git a/examples/import-boss/tests/inverse/lib/nonprod/.import-restrictions b/examples/import-boss/tests/inverse/lib/nonprod/.import-restrictions new file mode 100644 index 00000000..c0e2acff --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/nonprod/.import-restrictions @@ -0,0 +1,9 @@ +{ + "InverseRules": [{ + "Transitive": true, + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/inverse/lib" + ] + }] +} diff --git a/examples/import-boss/tests/inverse/lib/nonprod/doc.go b/examples/import-boss/tests/inverse/lib/nonprod/doc.go new file mode 100644 index 00000000..aad4b53e --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/nonprod/doc.go @@ -0,0 +1,2 @@ +// nonprod is non-production code that should not be linked into any outside package. +package nonprod diff --git a/examples/import-boss/tests/inverse/lib/private/.import-restrictions b/examples/import-boss/tests/inverse/lib/private/.import-restrictions new file mode 100644 index 00000000..9bcbd35e --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/private/.import-restrictions @@ -0,0 +1,11 @@ +{ + "InverseRules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/inverse/lib" + ], + "ForbiddenPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/inverse/lib/quarantine" + ] + }] +} diff --git a/examples/import-boss/tests/inverse/lib/private/doc.go b/examples/import-boss/tests/inverse/lib/private/doc.go new file mode 100644 index 00000000..735e4dc8 --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/private/doc.go @@ -0,0 +1 @@ +package private diff --git a/examples/import-boss/tests/inverse/lib/public/doc.go b/examples/import-boss/tests/inverse/lib/public/doc.go new file mode 100644 index 00000000..9fd24e5c --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/public/doc.go @@ -0,0 +1 @@ +package public diff --git a/examples/import-boss/tests/inverse/lib/quarantine/doc.go b/examples/import-boss/tests/inverse/lib/quarantine/doc.go new file mode 100644 index 00000000..82d777e4 --- /dev/null +++ b/examples/import-boss/tests/inverse/lib/quarantine/doc.go @@ -0,0 +1,6 @@ +// quarantine is inside the library, but should not import the private package. But it does! +package quarantine + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/inverse/lib/private" +) diff --git a/examples/import-boss/tests/rules/a/.import-restrictions b/examples/import-boss/tests/rules/a/.import-restrictions new file mode 100644 index 00000000..0edffb47 --- /dev/null +++ b/examples/import-boss/tests/rules/a/.import-restrictions @@ -0,0 +1,11 @@ +{ + "Rules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/b" + ], + "ForbiddenPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/c" + ] + }] +} diff --git a/examples/import-boss/tests/rules/a/doc.go b/examples/import-boss/tests/rules/a/doc.go new file mode 100644 index 00000000..a8478f9a --- /dev/null +++ b/examples/import-boss/tests/rules/a/doc.go @@ -0,0 +1,5 @@ +package a + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/b" +) diff --git a/examples/import-boss/tests/rules/b/.import-restrictions b/examples/import-boss/tests/rules/b/.import-restrictions new file mode 100644 index 00000000..49f2c702 --- /dev/null +++ b/examples/import-boss/tests/rules/b/.import-restrictions @@ -0,0 +1,11 @@ +{ + "Rules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + "" + ], + "ForbiddenPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/c" + ] + }] +} diff --git a/examples/import-boss/tests/rules/b/doc.go b/examples/import-boss/tests/rules/b/doc.go new file mode 100644 index 00000000..9cc55a68 --- /dev/null +++ b/examples/import-boss/tests/rules/b/doc.go @@ -0,0 +1,6 @@ +// b only public and private packages. The latter it shouldn't. +package b + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/c" +) diff --git a/examples/import-boss/tests/rules/c/doc.go b/examples/import-boss/tests/rules/c/doc.go new file mode 100644 index 00000000..7f96c221 --- /dev/null +++ b/examples/import-boss/tests/rules/c/doc.go @@ -0,0 +1 @@ +package c diff --git a/examples/import-boss/tests/rules/nested/.import-restrictions b/examples/import-boss/tests/rules/nested/.import-restrictions new file mode 100644 index 00000000..e8c54579 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/.import-restrictions @@ -0,0 +1,10 @@ +{ + "Rules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + ], + "ForbiddenPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/c" + ] + }] +} diff --git a/examples/import-boss/tests/rules/nested/doc.go b/examples/import-boss/tests/rules/nested/doc.go new file mode 100644 index 00000000..45635d38 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/doc.go @@ -0,0 +1,5 @@ +package nested + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/c" +) diff --git a/examples/import-boss/tests/rules/nested/nested/.import-restrictions b/examples/import-boss/tests/rules/nested/nested/.import-restrictions new file mode 100644 index 00000000..c3264425 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/nested/.import-restrictions @@ -0,0 +1,10 @@ +{ + "Rules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/c" + ], + "ForbiddenPrefixes": [ + ] + }] +} diff --git a/examples/import-boss/tests/rules/nested/nested/doc.go b/examples/import-boss/tests/rules/nested/nested/doc.go new file mode 100644 index 00000000..45635d38 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/nested/doc.go @@ -0,0 +1,5 @@ +package nested + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/c" +) diff --git a/examples/import-boss/tests/rules/nested/nested/nested/.import-restrictions b/examples/import-boss/tests/rules/nested/nested/nested/.import-restrictions new file mode 100644 index 00000000..1d49424e --- /dev/null +++ b/examples/import-boss/tests/rules/nested/nested/nested/.import-restrictions @@ -0,0 +1,10 @@ +{ + "Rules": [{ + "SelectorRegexp": "k8s[.]io/gengo", + "AllowedPrefixes": [ + ], + "ForbiddenPrefixes": [ + "k8s.io/gengo/examples/import-boss/tests/rules/c" + ] + }] +} diff --git a/examples/import-boss/tests/rules/nested/nested/nested/doc.go b/examples/import-boss/tests/rules/nested/nested/nested/doc.go new file mode 100644 index 00000000..45635d38 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/nested/nested/doc.go @@ -0,0 +1,5 @@ +package nested + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/c" +) diff --git a/examples/import-boss/tests/rules/nested/nested/nested/inherit/doc.go b/examples/import-boss/tests/rules/nested/nested/nested/inherit/doc.go new file mode 100644 index 00000000..54c0f5d9 --- /dev/null +++ b/examples/import-boss/tests/rules/nested/nested/nested/inherit/doc.go @@ -0,0 +1,5 @@ +package inherit + +import ( + _ "k8s.io/gengo/examples/import-boss/tests/rules/c" +) diff --git a/generator/default_package.go b/generator/default_package.go index 11517fc6..dcf08832 100644 --- a/generator/default_package.go +++ b/generator/default_package.go @@ -26,6 +26,8 @@ type DefaultPackage struct { PackageName string // Import path of the package, and the location on disk of the package. PackagePath string + // The location of the package on disk. + Source string // Emitted at the top of every file. HeaderText []byte @@ -43,8 +45,9 @@ type DefaultPackage struct { FilterFunc func(*Context, *types.Type) bool } -func (d *DefaultPackage) Name() string { return d.PackageName } -func (d *DefaultPackage) Path() string { return d.PackagePath } +func (d *DefaultPackage) Name() string { return d.PackageName } +func (d *DefaultPackage) Path() string { return d.PackagePath } +func (d *DefaultPackage) SourcePath() string { return d.Source } func (d *DefaultPackage) Filter(c *Context, t *types.Type) bool { if d.FilterFunc != nil { diff --git a/generator/execute.go b/generator/execute.go index b5f5aaeb..d1b12258 100644 --- a/generator/execute.go +++ b/generator/execute.go @@ -233,11 +233,13 @@ func (c *Context) ExecutePackage(outDir string, p Package) error { if f == nil { // This is the first generator to reference this file, so start it. f = &File{ - Name: g.Filename(), - FileType: fileType, - PackageName: p.Name(), - Header: p.Header(g.Filename()), - Imports: map[string]struct{}{}, + Name: g.Filename(), + FileType: fileType, + PackageName: p.Name(), + PackagePath: p.Path(), + PackageSourcePath: p.SourcePath(), + Header: p.Header(g.Filename()), + Imports: map[string]struct{}{}, } files[f.Name] = f } else { diff --git a/generator/generator.go b/generator/generator.go index 05a3f65f..4b48f503 100644 --- a/generator/generator.go +++ b/generator/generator.go @@ -31,6 +31,8 @@ type Package interface { Name() string // Path returns the package import path. Path() string + // SourcePath returns the location of the package on disk. + SourcePath() string // Filter should return true if this package cares about this type. // Otherwise, this type will be omitted from the type ordering for @@ -50,14 +52,16 @@ type Package interface { } type File struct { - Name string - FileType string - PackageName string - Header []byte - Imports map[string]struct{} - Vars bytes.Buffer - Consts bytes.Buffer - Body bytes.Buffer + Name string + FileType string + PackageName string + Header []byte + PackagePath string + PackageSourcePath string + Imports map[string]struct{} + Vars bytes.Buffer + Consts bytes.Buffer + Body bytes.Buffer } type FileType interface { @@ -156,6 +160,12 @@ type Context struct { // All the types, in case you want to look up something. Universe types.Universe + // Incoming imports, i.e. packages importing the given package. + incomingImports map[string][]string + + // Incoming transitive imports, i.e. the transitive closure of IncomingImports + incomingTransitiveImports map[string][]string + // All the user-specified packages. This is after recursive expansion. Inputs []string @@ -203,11 +213,36 @@ func NewContext(b *parser.Builder, nameSystems namer.NameSystems, canonicalOrder return c, nil } +// IncomingImports returns the incoming imports for each package. The map is lazily computed. +func (ctxt *Context) IncomingImports() map[string][]string { + if ctxt.incomingImports == nil { + incoming := map[string][]string{} + for _, pkg := range ctxt.Universe { + for imp := range pkg.Imports { + incoming[imp] = append(incoming[imp], pkg.Path) + } + } + ctxt.incomingImports = incoming + } + return ctxt.incomingImports +} + +// TransitiveIncomingImports returns the transitive closure of the incoming imports for each package. +// The map is lazily computed. +func (ctxt *Context) TransitiveIncomingImports() map[string][]string { + if ctxt.incomingTransitiveImports == nil { + ctxt.incomingTransitiveImports = transitiveClosure(ctxt.IncomingImports()) + } + return ctxt.incomingTransitiveImports +} + // 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. // Deprecated. Please use AddDirectory. func (ctxt *Context) AddDir(path string) error { + ctxt.incomingImports = nil + ctxt.incomingTransitiveImports = nil return ctxt.builder.AddDirTo(path, &ctxt.Universe) } @@ -215,5 +250,7 @@ func (ctxt *Context) AddDir(path string) error { // 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) AddDirectory(path string) (*types.Package, error) { + ctxt.incomingImports = nil + ctxt.incomingTransitiveImports = nil return ctxt.builder.AddDirectoryTo(path, &ctxt.Universe) } diff --git a/generator/transitive_closure.go b/generator/transitive_closure.go new file mode 100644 index 00000000..385a49fc --- /dev/null +++ b/generator/transitive_closure.go @@ -0,0 +1,65 @@ +/* +Copyright 2019 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 generator + +import "sort" + +type edge struct { + from string + to string +} + +func transitiveClosure(in map[string][]string) map[string][]string { + adj := make(map[edge]bool) + imports := make(map[string]struct{}) + for from, tos := range in { + for _, to := range tos { + adj[edge{from, to}] = true + imports[to] = struct{}{} + } + } + + // Warshal's algorithm + for k := range in { + for i := range in { + if !adj[edge{i, k}] { + continue + } + for j := range imports { + if adj[edge{i, j}] { + continue + } + if adj[edge{k, j}] { + adj[edge{i, j}] = true + } + } + } + } + + out := make(map[string][]string, len(in)) + for i := range in { + for j := range imports { + if adj[edge{i, j}] { + out[i] = append(out[i], j) + } + } + + sort.Strings(out[i]) + } + + return out +} diff --git a/generator/transitive_closure_test.go b/generator/transitive_closure_test.go new file mode 100644 index 00000000..917f8f7a --- /dev/null +++ b/generator/transitive_closure_test.go @@ -0,0 +1,64 @@ +/* +Copyright 2019 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 generator + +import ( + "reflect" + "testing" +) + +func TestTransitiveClosure(t *testing.T) { + cases := []struct { + name string + in map[string][]string + expected map[string][]string + }{ + { + name: "no transition", + in: map[string][]string{ + "a": {"b"}, + "c": {"d"}, + }, + expected: map[string][]string{ + "a": {"b"}, + "c": {"d"}, + }, + }, + { + name: "simple", + in: map[string][]string{ + "a": {"b"}, + "b": {"c"}, + "c": {"d"}, + }, + expected: map[string][]string{ + "a": {"b", "c", "d"}, + "b": {"c", "d"}, + "c": {"d"}, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + out := transitiveClosure(c.in) + if !reflect.DeepEqual(c.expected, out) { + t.Errorf("expected: %#v, got %#v", c.expected, out) + } + }) + } +} diff --git a/parser/parse.go b/parser/parse.go index 6a3d53b2..1e457979 100644 --- a/parser/parse.go +++ b/parser/parse.go @@ -227,12 +227,16 @@ func (b *Builder) AddDirRecursive(dir string) error { klog.Warningf("Ignoring directory %v: %v", dir, err) } - // filepath.Walk includes the root dir, but we already did that, so we'll - // remove that prefix and rebuild a package import path. - prefix := b.buildPackages[dir].Dir + // filepath.Walk does not follow symlinks. We therefore evaluate symlinks and use that with + // filepath.Walk. + realPath, err := filepath.EvalSymlinks(b.buildPackages[dir].Dir) + if err != nil { + return err + } + fn := func(filePath string, info os.FileInfo, err error) error { if info != nil && info.IsDir() { - rel := filepath.ToSlash(strings.TrimPrefix(filePath, prefix)) + rel := filepath.ToSlash(strings.TrimPrefix(filePath, realPath)) if rel != "" { // Make a pkg path. pkg := path.Join(string(canonicalizeImportPath(b.buildPackages[dir].ImportPath)), rel) @@ -245,7 +249,7 @@ func (b *Builder) AddDirRecursive(dir string) error { } return nil } - if err := filepath.Walk(b.buildPackages[dir].Dir, fn); err != nil { + if err := filepath.Walk(realPath, fn); err != nil { return err } return nil