Merge pull request #695 from k8s-infra-cherrypick-robot/cherry-pick-687-to-release-0.9

🏃 Simplify the LoadRootsWithConfig logic

Author: k8s-ci-robot

pkg/loader/loader.go

@@ -323,7 +323,7 @@ func (l *loader) parseFile(filename string, src []byte) (*ast.File, error) {
 // populated.  Additional information, like ASTs and type-checking information,
 // can be accessed via methods on individual packages.
 func LoadRoots(roots ...string) ([]*Package, error) {
-	return LoadRootsWithConfig(nil, roots...)
+	return LoadRootsWithConfig(&packages.Config{}, roots...)
 }
 
 // LoadRootsWithConfig functions like LoadRoots, except that it allows passing
@@ -366,41 +366,30 @@ func LoadRoots(roots ...string) ([]*Package, error) {
 //
 //     5. Load the filesystem path roots and return the load packages for the
 //        package/module roots AND the filesystem path roots.
-//
-//nolint:gocyclo
-func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Package, retErr error) {
+func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, error) {
+	l := &loader{
+		cfg:      cfg,
+		packages: make(map[*packages.Package]*Package),
+	}
+	l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
+	if l.cfg.Fset == nil {
+		l.cfg.Fset = token.NewFileSet()
+	}
+	// put our build flags first so that callers can override them
+	l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
+
+	// Visit the import graphs of the loaded, root packages. If an imported
+	// package refers to another loaded, root package, then replace the
+	// instance of the imported package with a reference to the loaded, root
+	// package. This is required to make kubebuilder markers work correctly
+	// when multiple root paths are loaded and types from one path reference
+	// types from another root path.
 	defer func() {
-		if retErr != nil {
-			return
-		}
-		for i := range result {
-			visitImports(result, result[i], nil)
+		for i := range l.Roots {
+			visitImports(l.Roots, l.Roots[i], nil)
 		}
 	}()
 
-	newLoader := func(cfg *packages.Config, cfgDir string) *loader {
-		if cfg == nil {
-			cfg = &packages.Config{
-				Dir: cfgDir,
-			}
-		}
-		l := &loader{
-			cfg:      cfg,
-			packages: map[*packages.Package]*Package{},
-		}
-		l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
-		if l.cfg.Fset == nil {
-			l.cfg.Fset = token.NewFileSet()
-		}
-		// put our build flags first so that callers can override them
-		l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
-
-		return l
-	}
-
-	// initialize the default loader
-	l := newLoader(cfg, "")
-
 	// uniquePkgIDs is used to keep track of the discovered packages to be nice
 	// and try and prevent packages from showing up twice when nested module
 	// support is enabled. there is not harm that comes from this per se, but
@@ -412,7 +401,7 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 	// if validatePkgFn is nil, a package will be returned in the slice,
 	// otherwise the package is only returned if the result of
 	// validatePkgFn(pkg.ID) is truthy
-	loadPackages := func(l *loader, roots ...string) ([]*Package, error) {
+	loadPackages := func(roots ...string) ([]*Package, error) {
 		rawPkgs, err := packages.Load(l.cfg, roots...)
 		if err != nil {
 			return nil, err
@@ -430,11 +419,12 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 
 	// if no roots were provided then load the current package and return early
 	if len(roots) == 0 {
-		pkgs, err := loadPackages(l)
+		pkgs, err := loadPackages()
 		if err != nil {
 			return nil, err
 		}
-		return pkgs, nil
+		l.Roots = append(l.Roots, pkgs...)
+		return l.Roots, nil
 	}
 
 	// pkgRoots is a slice of roots that are package/modules and fspRoots
@@ -460,39 +450,34 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 	// system path roots due to them needing a custom, calculated value for the
 	// cfg.Dir field
 	if len(pkgRoots) > 0 {
-		pkgs, err := loadPackages(l, pkgRoots...)
+		pkgs, err := loadPackages(pkgRoots...)
 		if err != nil {
 			return nil, err
 		}
-		result = append(result, pkgs...)
+		l.Roots = append(l.Roots, pkgs...)
 	}
 
 	// if there are no filesystem path roots then go ahead and return early
 	if len(fspRoots) == 0 {
-		return result, nil
+		return l.Roots, nil
 	}
 
 	//
 	// at this point we are handling filesystem path roots
 	//
 
-	// store the value of cfg.Dir so we can use it later if it is non-empty.
-	// we need to store it now as the value of cfg.Dir will be updated by
-	// a loop below
-	var cfgDir string
-	if cfg != nil {
-		cfgDir = cfg.Dir
-	}
-
 	// ensure the cfg.Dir field is reset to its original value upon
 	// returning from this function. it should honestly be fine if it is
 	// not given most callers will not send in the cfg parameter directly,
 	// as it's largely for testing, but still, let's be good stewards.
 	defer func(d string) {
-		if cfg != nil {
-			cfg.Dir = d
-		}
-	}(cfgDir)
+		cfg.Dir = d
+	}(cfg.Dir)
+
+	// store the value of cfg.Dir so we can use it later if it is non-empty.
+	// we need to store it now as the value of cfg.Dir will be updated by
+	// a loop below
+	cfgDir := cfg.Dir
 
 	// addNestedGoModulesToRoots is given to filepath.WalkDir and adds the
 	// directory part of p to the list of filesystem path roots IFF p is the
@@ -591,16 +576,9 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 			b = "."
 		}
 
-		// create the loader for this filesystem path. this is required in order
-		// to properly load the files as AST later in various workflows,
-		// including CRD generation
-		var fspLoader *loader
-		if cfg == nil {
-			fspLoader = newLoader(nil, d)
-		} else {
-			fspLoader = l
-			fspLoader.cfg.Dir = d
-		}
+		// update the loader configuration's Dir field to the directory part of
+		// the root
+		l.cfg.Dir = d
 
 		// update the root to be "./..." or "./."
 		// (with OS-specific filepath separator). please note filepath.Join
@@ -609,14 +587,14 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 		r = fmt.Sprintf(".%s%s", string(filepath.Separator), b)
 
 		// load the packages from the roots
-		pkgs, err := loadPackages(fspLoader, r)
+		pkgs, err := loadPackages(r)
 		if err != nil {
 			return nil, err
 		}
-		result = append(result, pkgs...)
+		l.Roots = append(l.Roots, pkgs...)
 	}
 
-	return result, nil
+	return l.Roots, nil
 }
 
 // visitImports walks a dependency graph, replacing imported package
@@ -634,8 +612,8 @@ func visitImports(rootPkgs []*Package, pkg *Package, seen sets.String) {
 			}
 		}
 		if !seen.Has(importedPkgID) {
-			visitImports(rootPkgs, importedPkg, seen)
 			seen.Insert(importedPkgID)
+			visitImports(rootPkgs, importedPkg, seen)
 		}
 	}
 }