#17 Replace Go internals with go/build
Merged 10 months ago by qulogic. Opened a year ago by qulogic.
qulogic/golist remove-internals  into  master

file modified
+1 -5

@@ -8,11 +8,7 @@ 

  Building

  --------

  

- Building requires linking Go compiler sources inside the project:

- 

- ```sh

- $ ln -s $(go env GOROOT)/src/cmd/go/internal/ pkg/util/internal

- ```

+ Building requires github.com/urfave/cli, github.com/golang/glog.

  

  Licensing

  ---------

file modified
+75 -190

@@ -1,59 +1,36 @@ 

  package util

  

  import (

- 	"fmt"

+ 	"go/build"

  	"go/parser"

  	"go/token"

  	"os"

- 	"os/exec"

  	"path"

  	"path/filepath"

  	"regexp"

  	"sort"

  	"strings"

- 

- 	"pagure.io/golist/pkg/util/internal/load"

- 	"pagure.io/golist/pkg/util/internal/work"

- 	"github.com/golang/glog"

- 

- 	// initialize the load.ModInit function

- 	"pagure.io/golist/pkg/util/internal/modload"

  )

  

- func exists(path string) (bool, error) {

- 	_, err := os.Stat(path)

- 	if err == nil {

- 		return true, nil

- 	}

- 	if os.IsNotExist(err) {

- 		return false, nil

- 	}

- 	return true, err

- }

- 

- func findPackageLocation(packagePath string) (string, string, error) {

- 	gopath := os.Getenv("GOPATH")

- 	if gopath == "" {

- 		return "", "", fmt.Errorf("GOPATH not set")

- 	}

- 

- 	var abspath, pathPrefix string

- 

- 	// first, find the absolute path

- 	for _, gpath := range strings.Split(gopath, ":") {

- 		abspath = path.Join(gpath, "src", packagePath)

- 

- 		if e, err := exists(abspath); err == nil && e {

- 			pathPrefix = path.Join(gpath, "src")

- 			break

- 		}

- 	}

- 

- 	if pathPrefix == "" {

- 		return "", "", fmt.Errorf("Path %v not found on %v", packagePath, gopath)

- 	}

- 

- 	return abspath, pathPrefix, nil

+ // IsStandardImportPath reports whether $GOROOT/src/path should be considered

+ // part of the standard distribution. For historical reasons we allow people to add

+ // their own code to $GOROOT instead of using $GOPATH, but we assume that

+ // code will start with a domain name (dot in the first element).

+ //

+ // Note that this function is meant to evaluate whether a directory found in GOROOT

+ // should be treated as part of the standard library. It should not be used to decide

+ // that a directory found in GOPATH should be rejected: directories in GOPATH

+ // need not have dots in the first element, and they just take their chances

+ // with future collisions in the standard library.

+ //

+ // This function was copied from src/cmd/go/internal/search/search.go in Go's code.

+ func isStandardImportPath(path string) bool {

+ 	i := strings.Index(path, "/")

+ 	if i < 0 {

+ 		i = len(path)

+ 	}

+ 	elem := path[:i]

+ 	return !strings.Contains(elem, ".")

  }

  

  // Ignore specifies a set of resources to ignore

@@ -88,7 +65,7 @@ 

  }

  

  type PackageInfoCollector struct {

- 	packageInfos   map[string]*load.PackagePublic

+ 	packageInfos   map[string]*build.Package

  	mainFiles      map[string][]string

  	packagePath    string

  	ignore         *Ignore

@@ -106,7 +83,7 @@ 

  

  func NewPackageInfoCollector(ignore *Ignore, extensions []string) *PackageInfoCollector {

  	return &PackageInfoCollector{

- 		packageInfos:  make(map[string]*load.PackagePublic),

+ 		packageInfos:  make(map[string]*build.Package),

  		mainFiles:     make(map[string][]string),

  		isStdPackages: make(map[string]bool),

  		ignore:        ignore,

@@ -114,29 +91,52 @@ 

  	}

  }

  

+ func (p *PackageInfoCollector) isStandardPackage(pkg *build.Package) bool {

+ 	return pkg.Goroot && pkg.ImportPath != "" && isStandardImportPath(pkg.ImportPath)

+ }

+ 

  func (p *PackageInfoCollector) isStandard(pkg string) (bool, error) {

  	if is, exists := p.isStdPackages[pkg]; exists {

  		return is, nil

  	}

- 	pkgInfo, err := ListPackage(pkg)

+ 	pkgInfo, err := build.Import(pkg, "", build.IgnoreVendor)

  	if err != nil {

  		return false, err

  	}

  

- 	p.isStdPackages[pkg] = pkgInfo.Standard

+ 	p.isStdPackages[pkg] = p.isStandardPackage(pkgInfo)

  

- 	return pkgInfo.Standard, nil

+ 	return p.isStdPackages[pkg], nil

+ }

+ 

+ func (p *PackageInfoCollector) resolveImports(baseDir string, imports []string) []string {

+ 	var paths []string

+ 	for _, importPath := range imports {

+ 		importedPackage, err := build.Import(importPath, baseDir, 0)

+ 		if err == nil {

+ 			paths = append(paths, importedPackage.ImportPath)

+ 		} else {

+ 			// Just assume we haven't 'go get'd it yet.

+ 			paths = append(paths, importPath)

+ 		}

+ 	}

+ 	return paths

  }

  

  func (p *PackageInfoCollector) CollectPackageInfos(packagePath string) error {

- 	abspath, pathPrefix, err := findPackageLocation(packagePath)

+ 	pkg, err := build.Import(packagePath, "", build.IgnoreVendor)

  	if err != nil {

- 		return err

+ 		// Don't worry about lack of Go files; there may be Go files in

+ 		// subdirectories.  We just want to find the absolute path in

+ 		// GOPATH/GOROOT, but return error if not found at all.

+ 		if _, ok := err.(*build.NoGoError); !ok {

+ 			return err

+ 		}

  	}

  

  	p.otherResources = &OtherResources{}

  

- 	if err := filepath.Walk(abspath+"/", func(path string, info os.FileInfo, err error) error {

+ 	if err := filepath.Walk(pkg.Dir+"/", func(path string, info os.FileInfo, err error) error {

  		if !info.IsDir() {

  			// Cited from https://golang.org/cmd/go/#hdr-Package_lists:

  			//   Directory and file names that begin with "." or "_" are ignored

@@ -162,7 +162,7 @@ 

  			return nil

  		}

  

- 		relPath := path[len(pathPrefix)+1:]

+ 		relPath := path[len(pkg.SrcRoot)+1:]

  		if strings.HasSuffix(relPath, "/") {

  			relPath = relPath[:len(relPath)-1]

  		}

@@ -178,20 +178,19 @@ 

  			return nil

  		}

  

- 		pkgInfo, err := ListPackage(relPath)

+ 		pkgInfo, err := build.ImportDir(path, build.IgnoreVendor)

  		if err != nil {

- 			if strings.Contains(err.Error(), "no Go files in") {

- 				return nil

- 			}

- 			// TODO(jchaloup): remove later

- 			if strings.Contains(err.Error(), "build constraints exclude all Go files in") {

+ 			if _, ok := err.(*build.NoGoError); ok {

  				return nil

  			}

  			panic(err)

- 			return nil

  		}

  

  		if len(pkgInfo.GoFiles) > 0 || len(pkgInfo.CgoFiles) > 0 {

+ 			// Show vendor-expanded paths in listing

+ 			pkgInfo.TestImports = p.resolveImports(pkg.Dir, pkgInfo.TestImports)

+ 			pkgInfo.XTestImports = p.resolveImports(pkg.Dir, pkgInfo.XTestImports)

+ 

  			p.packageInfos[relPath] = pkgInfo

  		}

  

@@ -318,32 +317,22 @@ 

  func (p *PackageInfoCollector) CollectProjectDeps(standard bool, skipSelf bool, tests bool) ([]string, error) {

  	imports := make(map[string]struct{})

  

- 	if tests {

- 		for _, info := range p.packageInfos {

- 			for _, item := range info.TestImports {

- 				if item == "C" {

- 					continue

- 				}

- 				if pos := strings.LastIndex(item, "/vendor/"); pos != -1 {

- 					item = item[pos+8:]

- 				}

- 				if _, ok := imports[item]; !ok {

- 					imports[item] = struct{}{}

- 				}

+ 	for _, info := range p.packageInfos {

+ 		var pkgImports []string

+ 		if tests {

+ 			pkgImports = info.TestImports

+ 		} else {

+ 			pkgImports = info.Imports

+ 		}

+ 		for _, item := range pkgImports {

+ 			if item == "C" {

+ 				continue

  			}

- 		}

- 	} else {

- 		for _, info := range p.packageInfos {

- 			for _, item := range info.Imports {

- 				if item == "C" {

- 					continue

- 				}

- 				if pos := strings.LastIndex(item, "/vendor/"); pos != -1 {

- 					item = item[pos+8:]

- 				}

- 				if _, ok := imports[item]; !ok {

- 					imports[item] = struct{}{}

- 				}

+ 			if pos := strings.LastIndex(item, "/vendor/"); pos != -1 {

+ 				item = item[pos+8:]

+ 			}

+ 			if _, ok := imports[item]; !ok {

+ 				imports[item] = struct{}{}

  			}

  		}

  	}

@@ -359,9 +348,9 @@ 

  			continue

  		}

  

- 		pkgInfo, err := ListPackage(relPath)

+ 		pkgInfo, err := build.Import(relPath, "", build.IgnoreVendor)

  		// assuming the stdlib is always processed properly

- 		if !standard && err == nil && pkgInfo.Standard {

+ 		if !standard && err == nil && p.isStandardPackage(pkgInfo) {

  			continue

  		}

  

@@ -413,110 +402,6 @@ 

  	return entryPoints, nil

  }

  

- func ListPackage(path string) (*load.PackagePublic, error) {

- 	// TODO(jchaloup): more things need to be init most likely

- 	work.BuildInit()

- 	load.ModInit = modload.Init

- 	d := load.PackagesAndErrors([]string{path})

- 	if d == nil {

- 		return nil, fmt.Errorf("No package listing found for %v", path)

- 	}

- 

- 	pkg := d[0]

- 	if pkg.Error != nil {

- 		return nil, pkg.Error

- 	}

- 	// Show vendor-expanded paths in listing

- 	pkg.TestImports = pkg.Resolve(pkg.TestImports)

- 	pkg.XTestImports = pkg.Resolve(pkg.XTestImports)

- 	return &pkg.PackagePublic, nil

- }

- 

- func ListGoFiles(packagePath string, cgo bool) ([]string, error) {

- 

- 	collectFiles := func(output string) []string {

- 		line := strings.Split(string(output), "\n")[0]

- 		line = line[1 : len(line)-1]

- 		if line == "" {

- 			return nil

- 		}

- 		return strings.Split(line, " ")

- 	}

- 	// check GOPATH/packagePath

- 	filter := "{{.GoFiles}}"

- 	if cgo {

- 		filter = "{{.CgoFiles}}"

- 	}

- 	cmd := exec.Command("go", "list", "-f", filter, packagePath)

- 	output, e := cmd.CombinedOutput()

- 	if e == nil {

- 		return collectFiles(string(output)), nil

- 	}

- 

- 	if strings.Contains(string(output), "no buildable Go source files in") {

- 		return nil, nil

- 	}

- 

- 	// if strings.Contains(string(output), "no Go files in") {

- 	// 	return nil, nil

- 	// }

- 

- 	return nil, fmt.Errorf("%v: %v, %v", strings.Join(cmd.Args, " "), e, string(output))

- }

- 

- func GetPackageFiles(packageRoot, packagePath string) (files []string, packageLocation string, err error) {

- 

- 	files, ppath, e := func() ([]string, string, error) {

- 		var searched []string

- 		// First searched the vendor directories

- 		pathParts := strings.Split(packageRoot, string(os.PathSeparator))

- 		for i := len(pathParts); i >= 0; i-- {

- 			vendorpath := path.Join(path.Join(pathParts[:i]...), "vendor", packagePath)

- 			glog.V(1).Infof("Checking %v directory", vendorpath)

- 			if l, e := ListGoFiles(vendorpath, false); e == nil {

- 				glog.V(1).Infof("Found %v directory", vendorpath)

- 				return l, vendorpath, e

- 			}

- 			searched = append(searched, vendorpath)

- 		}

- 

- 		glog.V(1).Infof("Checking %v directory", packagePath)

- 		if l, e := ListGoFiles(packagePath, false); e == nil {

- 			glog.V(1).Infof("Found %v directory", packagePath)

- 			return l, packagePath, e

- 		}

- 		searched = append(searched, packagePath)

- 

- 		return nil, "", fmt.Errorf("Unable to find %q in any of:\n\t\t%v\n", packagePath, strings.Join(searched, "\n\t\t"))

- 	}()

- 

- 	if e != nil {

- 		return nil, "", e

- 	}

- 

- 	// cgo files enabled?

- 	cgoFiles, e := ListGoFiles(ppath, true)

- 	if e != nil {

- 		return nil, "", e

- 	}

- 

- 	if len(cgoFiles) > 0 {

- 		files = append(files, cgoFiles...)

- 	}

- 

- 	{

- 		cmd := exec.Command("go", "list", "-f", "{{.Dir}}", ppath)

- 		output, err := cmd.CombinedOutput()

- 		if err != nil {

- 			return nil, "", fmt.Errorf("go list -f {{.Dir}} %v failed: %v", ppath, err)

- 		}

- 		lines := strings.Split(string(output), "\n")

- 		packageLocation = string(lines[0])

- 	}

- 

- 	return files, packageLocation, nil

- }

- 

  type ProjectData struct {

  	Packages     []string            `json:"packages"`

  	Dependencies map[string][]string `json:"dependencies"`

There are several benefits:

  • This searches all GOPATH and GOROOT, and returns error message more consistent with regular go tools.
  • No need to mess with symlinks.
  • No need to worry about ABI breaks due to using internal code.
  • No crashes due to failing to initialize things related to the above, e.g., it fixes #7.

This is based on #16, or it wouldn't compile otherwise.

There are still some difference that I am investigating, so it's not perfect just yet.

5 new commits added

  • Simplify code duplication in CollectProjectDeps.
  • Replace findPackageLocation by go.build.Import.
  • Replace Go internals with go/build.
  • Remove unused functions and make one private.
  • Fix compile against latest urfave/cli.
a year ago

So for tests, I searched the specs for all that definegoipath and cloned all of them (note this is not all Go packages.) Then ran golist as packaged and golist from this PR and compared the results for all possible arguments.

Current golist crashes on:

  • github.com/gengo/grpc-gateway
  • github.com/shurcooL/githubql

but this PR works again.

There are only 2 differences out of 510 packages with goipath (out of approximately 642 that mention golang at all):

  • github.com/cespare/xxhash
  • github.com/google/go-github

Both of these report themselves in the --imported section, but the new one includes the version (i.e., github.com/cespare/xxhash -> github.com/cespare/xxhash/v2). I assume this might be a problem, since we don't add it in the provides, but I don't know for sure. Can you confirm @nim?

As a note, the original code ran a resolve on the test imports, but not the regular imports. I tried adding a resolve on the regular imports as well, but it doesn't help. I also tried setting GO111MODULE=on, and while it does stop it from erroring, it still returns the same /v2 import path back.

The v2 is module versioning. That's something we need to support to have working Go packaging. It's good that your PR starts picking it up when the previous code does not.

However you are right that just doing it that way is going to break things. It needs changes in other parts of golist and in the macro code (and the current go-rpm-macros code should be in a good enough shape to allow those changes but that ’s not what is in Fedora).

Looking at how github.com/cespare/xxhash handled the module transition, assuming it is representative of the conversion of a real-world codebase to modules, here is the list of the things I see needing doing (paper analysis, real work implementation and testing is needed)

  • we have the new module descriptors. That means golist needs to include the go.mod file as part of the project files, otherwise it will not end up in the -devel package and rpm will (rightfully) ignore it when generating deps

  • then once the go.mod file is in the devel package we need to check the golist call that generates the package provides actually sees it (should probably just work with the code in go-rpm-macros and will almost certainly fail in some case with the code in Fedora because some of the approximations I fixed this year and which are still in the Fedora macro code will definitely hurt module parsing)

  • then we have the fact that modules break the identity between the URL and the import path. The code is still at https://github.com/cespare/xxhash but the logical name is now github.com/cespare/xxhash/v2. No matter this is a case that is handled nicely by go-rpm-macros, that just means you need to declare github.com/cespare/xxhash/v2 as goipath and https://github.com/cespare/xxhash as forgeurl

  • the module says v2 depends on v1 which means you need a separate v1 spec and package with just github.com/cespare/xxhash/ goipath (and be careful to pull from the V1 branch not the v2 one). If you package the v2 code as github.com/cespare/xxhash/ you are doing a mistake, that's not its real import path

And that should be sufficient to have github.com/cespare/xxhash/ work in a module world. However, this is a simple module conversion with a single go.mod per git repo, and no version constrains in this module file

  • but, what happens when a single git repo ships several go.mod files with a logical name that has nothing to do with the URL path ? I suspect things will break badly. the latest go-rpm-macros code is a lot more flexible, but it still assumes some form of identity between the filesystem layout and the import layout. How badly will this identity break with modules? I have no idea.

  • and then you have the version qualifiers in the module files, we need to map them rpm side one way or another

Ok, after more work evolving to module support seems a bit simpler than I though, because go mod need requires people to clean up their mess and that's also the assumption of the current go-rpm-macros.

Though it will definitely require the coding of a helper in go, since the json structure go mod edit outputs is pretty much unworkable in shell, lua is not available at this stage of the rpm build, and I don't think we need to inject a fourth language in our go tooling (after lua+shell due to how rpm works, and golang for golist).

The 1000-dollar question is how much this new helper can subsume current golist functions, or if we end up with 2 helpers because go module is almost there for our needs but not completely.

rebased onto 6652d08

10 months ago

I'm going to merge this, as those two packages are failing due to the way they use Go modules, and getting testing working everywhere is more important. They can just be patched to work right in the meantime. Of course, as more things move to modules, this will be harder to do, but I guess you're working on that or we can get to it later.

Pull-Request has been merged by qulogic

10 months ago
Metadata