From bc4e533d494b33c56063aa4a21e75e90c9fee844 Mon Sep 17 00:00:00 2001 From: marpio Date: Mon, 1 Oct 2018 20:53:17 +0200 Subject: [PATCH] Use go mod download -json (#710) * use go mod download -json * annotate json, fix build * add comment * address review feedback --- pkg/module/all_test.go | 5 +- pkg/module/disk_ref.go | 110 ---------------------------- pkg/module/disk_ref_test.go | 110 ---------------------------- pkg/module/go_get_fetcher.go | 111 ++++++++++++++++------------- pkg/module/go_get_fetcher_test.go | 36 ++-------- pkg/module/zip_read_closer.go | 50 +++++++++++++ pkg/module/zip_read_closer_test.go | 56 +++++++++++++++ pkg/paths/encode.go | 38 ---------- pkg/paths/encode_decode_test.go | 46 ------------ pkg/paths/go_get.go | 11 --- 10 files changed, 178 insertions(+), 395 deletions(-) delete mode 100644 pkg/module/disk_ref.go delete mode 100644 pkg/module/disk_ref_test.go create mode 100644 pkg/module/zip_read_closer.go create mode 100644 pkg/module/zip_read_closer_test.go delete mode 100644 pkg/paths/encode.go delete mode 100644 pkg/paths/encode_decode_test.go delete mode 100644 pkg/paths/go_get.go diff --git a/pkg/module/all_test.go b/pkg/module/all_test.go index eacee33c..57415fdd 100644 --- a/pkg/module/all_test.go +++ b/pkg/module/all_test.go @@ -10,8 +10,9 @@ import ( const ( // these values need to point to a real repository that has a tag - repoURI = "github.com/arschles/assert" - version = "v1.0.0" + // github.com/NYTimes/gizmo is a example of a path that needs to be encoded so we can cover that case as well + repoURI = "github.com/NYTimes/gizmo" + version = "v0.1.4" ) type ModuleSuite struct { diff --git a/pkg/module/disk_ref.go b/pkg/module/disk_ref.go deleted file mode 100644 index e0c56b76..00000000 --- a/pkg/module/disk_ref.go +++ /dev/null @@ -1,110 +0,0 @@ -package module - -import ( - "fmt" - "io" - "os" - "path/filepath" - - "github.com/gomods/athens/pkg/errors" - "github.com/gomods/athens/pkg/paths" - "github.com/gomods/athens/pkg/storage" - "github.com/spf13/afero" -) - -// diskRef is a Ref implementation for modules on disk. It is not safe to use concurrently. -// -// Do not create this struct directly. use newDiskRef -type diskRef struct { - root string - module string - fs afero.Fs - version string -} - -type zipReadCloser struct { - zip io.ReadCloser - ref *diskRef -} - -// Close closes the zip file handle and clears up disk space used by the underlying disk ref -// It is the caller's responsibility to call this method to free up utilized disk space -func (rc *zipReadCloser) Close() error { - rc.zip.Close() - return ClearFiles(rc.ref.fs, rc.ref.root) -} - -func (rc *zipReadCloser) Read(p []byte) (n int, err error) { - return rc.zip.Read(p) -} - -func newDiskRef(fs afero.Fs, root, module, version string) *diskRef { - return &diskRef{ - fs: fs, - root: root, - module: module, - version: version, - } -} - -// ClearFiles deletes all data from the given fs at path root -// This function must be called when zip is closed to cleanup the entire GOPATH created by the diskref -func ClearFiles(fs afero.Fs, root string) error { - const op errors.Op = "module.ClearFiles" - // This is required because vgo ensures dependencies are read-only - // See https://github.com/golang/go/issues/24111 and - // https://go-review.googlesource.com/c/vgo/+/96978 - walkFn := func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - return fs.Chmod(path, 0770) - } - err := afero.Walk(fs, root, walkFn) - if err != nil { - return errors.E(op, err) - } - err = fs.RemoveAll(root) - if err != nil { - return errors.E(op, err) - } - return nil -} - -// read is the Ref interface implementation. -func (d *diskRef) Read() (*storage.Version, error) { - const op errors.Op = "diskRef.Read" - var ver storage.Version - - encodedModulePath, err := paths.EncodePath(d.module) - if err != nil { - return nil, errors.E(op, err, errors.KindBadRequest) - } - packagePath := getPackagePath(d.root, encodedModulePath) - - infoFile := filepath.Join(packagePath, fmt.Sprintf("%s.info", d.version)) - info, err := afero.ReadFile(d.fs, infoFile) - if err != nil { - return nil, errors.E(op, err) - } - ver.Info = info - - modFile := filepath.Join(packagePath, fmt.Sprintf("%s.mod", d.version)) - mod, err := afero.ReadFile(d.fs, modFile) - if err != nil { - return nil, errors.E(op, err) - } - ver.Mod = mod - - sourceFile, err := d.fs.Open(filepath.Join(packagePath, fmt.Sprintf("%s.zip", d.version))) - if err != nil { - return nil, errors.E(op, err) - } - // note: don't close sourceFile here so that the caller can read directly from disk. - // - // if we close, then the caller will panic, and the alternative to make this work is - // that we read into memory and return an io.ReadCloser that reads out of memory - ver.Zip = &zipReadCloser{sourceFile, d} - - return &ver, nil -} diff --git a/pkg/module/disk_ref_test.go b/pkg/module/disk_ref_test.go deleted file mode 100644 index 32d112f2..00000000 --- a/pkg/module/disk_ref_test.go +++ /dev/null @@ -1,110 +0,0 @@ -package module - -import ( - "fmt" - "io/ioutil" - "path/filepath" - - "github.com/spf13/afero" -) - -func (m *ModuleSuite) TestDiskRefReadAndClear() { - const ( - root = "testroot" - version = "v1.0.0" - info = "testinfo" - mod = "testmod" - zip = "testzip" - ) - r := m.Require() - - packagePath := getPackagePath(root, mod) - // create a new disk ref using the filesystem - diskRef := newDiskRef(m.fs, root, mod, version) - - // ensure that reading fails, because there are no files - ver, err := diskRef.Read() - r.Nil(ver) - r.NotNil(err) - - // create all the files the disk ref expects - r.NoError(createAndWriteFile(m.fs, filepath.Join(packagePath, version+".info"), info)) - r.NoError(createAndWriteFile(m.fs, filepath.Join(packagePath, version+".mod"), mod)) - r.NoError(createAndWriteFile(m.fs, filepath.Join(packagePath, version+".zip"), zip)) - - // read from the disk ref - this time it should succeed - ver, err = diskRef.Read() - r.NoError(err) - r.Equal(info, string(ver.Info)) - r.Equal(mod, string(ver.Mod)) - zipBytes, err := ioutil.ReadAll(ver.Zip) - r.NoError(err) - r.Equal(zip, string(zipBytes)) - - // Validate that the root dir still exists - fInfo, err := m.fs.Stat(root) - r.NotNil(fInfo) - r.Nil(err) - - // close the version's zip file (which also cleans up the underlying diskref's GOPATH) and expect it to fail again - r.NoError(ver.Zip.Close()) - ver, err = diskRef.Read() - r.Nil(ver) - r.NotNil(err) - - // The root dir should not exist after a clear - fInfo, err = m.fs.Stat(root) - r.Nil(fInfo) - r.NotNil(err) - -} - -func (m *ModuleSuite) TestDiskRefClearFail() { - root := "testroot" - r := m.Require() - // This should fail because we haven't created any files - err := ClearFiles(m.fs, root) - r.EqualError(err, "open testroot: file does not exist") -} - -func (m *ModuleSuite) TestDiskRefClearSuccess() { - const ( - root = "testroot" - mod = "testmod" - file = "testfile" - info = "testinfo" - ) - r := m.Require() - - // Create a single file - packagePath := getPackagePath(root, mod) - filePath := filepath.Join(packagePath, file) - r.NoError(createAndWriteFile(m.fs, filePath, info)) - - // Validate the file exists - _, err := m.fs.Stat(filePath) - r.NoError(err) - - // Now clear the files - err = ClearFiles(m.fs, root) - r.NoError(err) - - // Validate the file has been deleted - _, err = m.fs.Stat(filePath) - expErr := fmt.Sprintf("open %s: file does not exist", filePath) - r.EqualError(err, expErr) -} - -// creates filename with fs, writes data to the file, and closes the file, -// -// returns a non-nil error if anything went wrong. the file will be closed -// regardless of what this function returns -func createAndWriteFile(fs afero.Fs, filename, data string) error { - fileHandle, err := fs.Create(filename) - if err != nil { - return err - } - defer fileHandle.Close() - _, err = fileHandle.Write([]byte(data)) - return err -} diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go index 7004fc07..6b801f07 100644 --- a/pkg/module/go_get_fetcher.go +++ b/pkg/module/go_get_fetcher.go @@ -3,6 +3,7 @@ package module import ( "bytes" "context" + "encoding/json" "fmt" "os" "os/exec" @@ -12,7 +13,6 @@ import ( "github.com/gomods/athens/pkg/errors" "github.com/gomods/athens/pkg/observ" - "github.com/gomods/athens/pkg/paths" "github.com/gomods/athens/pkg/storage" "github.com/spf13/afero" ) @@ -22,6 +22,18 @@ type goGetFetcher struct { goBinaryName string } +type goModule struct { + Path string `json:"path"` // module path + Version string `json:"version"` // module version + Error string `json:"error"` // error loading module + Info string `json:"info"` // absolute path to cached .info file + GoMod string `json:"goMod"` // absolute path to cached .mod file + Zip string `json:"zip"` // absolute path to cached .zip file + Dir string `json:"dir"` // absolute path to cached source root directory + Sum string `json:"sum"` // checksum for path, version (as in go.sum) + GoModSum string `json:"goModSum"` // checksum for go.mod (as in go.sum) +} + // NewGoGetFetcher creates fetcher which uses go get tool to fetch modules func NewGoGetFetcher(goBinaryName string, fs afero.Fs) (Fetcher, error) { const op errors.Op = "module.NewGoGetFetcher" @@ -59,14 +71,36 @@ func (g *goGetFetcher) Fetch(ctx context.Context, mod, ver string) (*storage.Ver return nil, errors.E(op, err) } - err = getSources(g.goBinaryName, g.fs, goPathRoot, modPath, mod, ver) + m, err := downloadModule(g.goBinaryName, g.fs, goPathRoot, modPath, mod, ver) if err != nil { ClearFiles(g.fs, goPathRoot) return nil, errors.E(op, err) } - dr := newDiskRef(g.fs, goPathRoot, mod, ver) - return dr.Read() + var storageVer storage.Version + info, err := afero.ReadFile(g.fs, m.Info) + if err != nil { + return nil, errors.E(op, err) + } + storageVer.Info = info + + gomod, err := afero.ReadFile(g.fs, m.GoMod) + if err != nil { + return nil, errors.E(op, err) + } + storageVer.Mod = gomod + + zip, err := g.fs.Open(m.Zip) + if err != nil { + return nil, errors.E(op, err) + } + // note: don't close zip here so that the caller can read directly from disk. + // + // if we close, then the caller will panic, and the alternative to make this work is + // that we read into memory and return an io.ReadCloser that reads out of memory + storageVer.Zip = &zipReadCloser{zip, g.fs, goPathRoot} + + return &storageVer, nil } // Dummy Hacky thing makes vgo not to complain @@ -86,38 +120,39 @@ func Dummy(fs afero.Fs, repoRoot string) error { return nil } -// given a filesystem, gopath, repository root, module and version, runs 'vgo get' +// given a filesystem, gopath, repository root, module and version, runs 'go mod download -json' // on module@version from the repoRoot with GOPATH=gopath, and returns a non-nil error if anything went wrong. -func getSources(goBinaryName string, fs afero.Fs, gopath, repoRoot, module, version string) error { - const op errors.Op = "module.getSources" +func downloadModule(goBinaryName string, fs afero.Fs, gopath, repoRoot, module, version string) (goModule, error) { + const op errors.Op = "module.downloadModule" uri := strings.TrimSuffix(module, "/") fullURI := fmt.Sprintf("%s@%s", uri, version) - cmd := exec.Command(goBinaryName, "mod", "download", fullURI) + cmd := exec.Command(goBinaryName, "mod", "download", "-json", fullURI) cmd.Env = PrepareEnv(gopath) cmd.Dir = repoRoot - o, err := cmd.CombinedOutput() + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + cmd.Stdout = stdout + cmd.Stderr = stderr + err := cmd.Run() if err != nil { - errMsg := fmt.Sprintf("%v : %s", err, o) + err = fmt.Errorf("%v: %s", err, stderr) // github quota exceeded - if isLimitHit(o) { - return errors.E(op, errMsg, errors.KindRateLimit) + if isLimitHit(err.Error()) { + return goModule{}, errors.E(op, err, errors.KindRateLimit) } - // another error in the output - return errors.E(op, errMsg) - } - // make sure the expected files exist - encmod, err := paths.EncodePath(module) - if err != nil { - return errors.E(op, err) - } - packagePath := getPackagePath(gopath, encmod) - err = checkFiles(fs, packagePath, version) - if err != nil { - return errors.E(op, err) + return goModule{}, errors.E(op, err) } - return nil + var m goModule + if err = json.NewDecoder(stdout).Decode(&m); err != nil { + return goModule{}, errors.E(op, err) + } + if m.Error != "" { + return goModule{}, errors.E(op, m.Error) + } + + return m, nil } // PrepareEnv will return all the appropriate @@ -146,25 +181,8 @@ func PrepareEnv(gopath string) []string { return cmdEnv } -func checkFiles(fs afero.Fs, path, version string) error { - const op errors.Op = "module.checkFiles" - if _, err := fs.Stat(filepath.Join(path, version+".mod")); err != nil { - return errors.E(op, fmt.Sprintf("%s.mod not found in %s", version, path)) - } - - if _, err := fs.Stat(filepath.Join(path, version+".zip")); err != nil { - return errors.E(op, fmt.Sprintf("%s.mod not found in %s", version, path)) - } - - if _, err := fs.Stat(filepath.Join(path, version+".info")); err != nil { - return errors.E(op, fmt.Sprintf("%s.mod not found in %s", version, path)) - } - - return nil -} - -func isLimitHit(o []byte) bool { - return bytes.Contains(o, []byte("403 response from api.github.com")) +func isLimitHit(o string) bool { + return strings.Contains(o, "403 response from api.github.com") } // getRepoDirName takes a raw repository URI and a version and creates a directory name that the @@ -174,11 +192,6 @@ func getRepoDirName(repoURI, version string) string { return fmt.Sprintf("%s-%s", escapedURI, version) } -// getPackagePath returns the path to the module cache given the gopath and module name -func getPackagePath(gopath, module string) string { - return filepath.Join(gopath, "pkg", "mod", "cache", "download", module, "@v") -} - func validGoBinary(name string) error { const op errors.Op = "module.validGoBinary" err := exec.Command(name).Run() diff --git a/pkg/module/go_get_fetcher_test.go b/pkg/module/go_get_fetcher_test.go index 874e572e..1edf8b59 100644 --- a/pkg/module/go_get_fetcher_test.go +++ b/pkg/module/go_get_fetcher_test.go @@ -2,11 +2,9 @@ package module import ( "context" - "fmt" "io/ioutil" - "log" + "runtime" - "github.com/gobuffalo/envy" "github.com/spf13/afero" "github.com/stretchr/testify/assert" ) @@ -25,7 +23,11 @@ func (s *ModuleSuite) TestGoGetFetcherError() { fetcher, err := NewGoGetFetcher("invalidpath", afero.NewOsFs()) assert.Nil(s.T(), fetcher) - assert.EqualError(s.T(), err, "exec: \"invalidpath\": executable file not found in $PATH") + if runtime.GOOS == "windows" { + assert.EqualError(s.T(), err, "exec: \"invalidpath\": executable file not found in %PATH%") + } else { + assert.EqualError(s.T(), err, "exec: \"invalidpath\": executable file not found in $PATH") + } } func (s *ModuleSuite) TestGoGetFetcherFetch() { @@ -46,30 +48,6 @@ func (s *ModuleSuite) TestGoGetFetcherFetch() { r.NoError(err) r.True(len(zipBytes) > 0) - // close the version's zip file (which also cleans up the underlying diskref's GOPATH) and expect it to fail again + // close the version's zip file (which also cleans up the underlying GOPATH) and expect it to fail again r.NoError(ver.Zip.Close()) } - -func ExampleFetcher() { - repoURI := "github.com/arschles/assert" - version := "v1.0.0" - goBinaryName := envy.Get("GO_BINARY_PATH", "go") - fetcher, err := NewGoGetFetcher(goBinaryName, afero.NewOsFs()) - if err != nil { - log.Fatal(err) - } - versionData, err := fetcher.Fetch(ctx, repoURI, version) - // handle errors if any - if err != nil { - return - } - // Close the handle to versionData.Zip once done - // This will also handle cleanup so it's important to call Close - defer versionData.Zip.Close() - if err != nil { - return - } - // Do something with versionData - fmt.Println(string(versionData.Mod)) - // Output: module github.com/arschles/assert -} diff --git a/pkg/module/zip_read_closer.go b/pkg/module/zip_read_closer.go new file mode 100644 index 00000000..724f216f --- /dev/null +++ b/pkg/module/zip_read_closer.go @@ -0,0 +1,50 @@ +package module + +import ( + "io" + "os" + + "github.com/gomods/athens/pkg/errors" + "github.com/spf13/afero" +) + +type zipReadCloser struct { + zip io.ReadCloser + fs afero.Fs + goPath string +} + +// Close closes the zip file handle and clears up disk space used by the underlying disk ref +// It is the caller's responsibility to call this method to free up utilized disk space +func (rc *zipReadCloser) Close() error { + rc.zip.Close() + return ClearFiles(rc.fs, rc.goPath) +} + +func (rc *zipReadCloser) Read(p []byte) (n int, err error) { + return rc.zip.Read(p) +} + +// ClearFiles deletes all data from the given fs at path root +// This function must be called when zip is closed to cleanup the entire GOPATH created by the diskref +func ClearFiles(fs afero.Fs, root string) error { + const op errors.Op = "module.ClearFiles" + // This is required because vgo ensures dependencies are read-only + // See https://github.com/golang/go/issues/24111 and + // https://go-review.googlesource.com/c/vgo/+/96978 + walkFn := func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + return fs.Chmod(path, 0770) + } + err := afero.Walk(fs, root, walkFn) + if err != nil { + return errors.E(op, err) + } + err = fs.RemoveAll(root) + if err != nil { + return errors.E(op, err) + } + return nil +} diff --git a/pkg/module/zip_read_closer_test.go b/pkg/module/zip_read_closer_test.go new file mode 100644 index 00000000..dcc87102 --- /dev/null +++ b/pkg/module/zip_read_closer_test.go @@ -0,0 +1,56 @@ +package module + +import ( + "path/filepath" + + "github.com/spf13/afero" +) + +func (m *ModuleSuite) TestZipReadCloser() { + const ( + root = "testroot" + version = "v1.0.0" + info = "testinfo" + mod = "testmod" + zip = "testzip" + ) + r := m.Require() + + fs := afero.NewMemMapFs() + gopath, err := afero.TempDir(fs, "", "athens-test") + r.NoError(err) + packagePath := filepath.Join(gopath, "pkg", "mod", "cache", "download", mod, "@v") + // create all the files the disk ref expects + r.NoError(createAndWriteFile(fs, filepath.Join(packagePath, version+".info"), info)) + r.NoError(createAndWriteFile(fs, filepath.Join(packagePath, version+".mod"), mod)) + r.NoError(createAndWriteFile(fs, filepath.Join(packagePath, version+".zip"), zip)) + + ziprc, err := fs.Open(filepath.Join(packagePath, version+".zip")) + r.NoError(err) + cl := &zipReadCloser{fs: fs, goPath: gopath, zip: ziprc} + + fInfo, err := fs.Stat(gopath) + r.NotNil(fInfo) + r.Nil(err) + + r.NoError(cl.Close()) + + // The root dir should not exist after a clear + fInfo, err = fs.Stat(gopath) + r.Nil(fInfo) + r.NotNil(err) +} + +// creates filename with fs, writes data to the file, and closes the file, +// +// returns a non-nil error if anything went wrong. the file will be closed +// regardless of what this function returns +func createAndWriteFile(fs afero.Fs, filename, data string) error { + fileHandle, err := fs.Create(filename) + if err != nil { + return err + } + defer fileHandle.Close() + _, err = fileHandle.Write([]byte(data)) + return err +} diff --git a/pkg/paths/encode.go b/pkg/paths/encode.go deleted file mode 100644 index a3f96e58..00000000 --- a/pkg/paths/encode.go +++ /dev/null @@ -1,38 +0,0 @@ -package paths - -import ( - "unicode/utf8" - - "github.com/gomods/athens/pkg/errors" -) - -// EncodePath is ripped from cmd/go -- it replaces upper case -// letters with bang+lowercase. -func EncodePath(s string) (encoding string, err error) { - const op errors.Op = "paths.EncodePath" - haveUpper := false - for _, r := range s { - if r == '!' || r >= utf8.RuneSelf { - // This should be disallowed by CheckPath, but diagnose anyway. - // The correctness of the encoding loop below depends on it. - return "", errors.E(op, "internal error: inconsistency in EncodePath") - } - if 'A' <= r && r <= 'Z' { - haveUpper = true - } - } - - if !haveUpper { - return s, nil - } - - var buf []byte - for _, r := range s { - if 'A' <= r && r <= 'Z' { - buf = append(buf, '!', byte(r+'a'-'A')) - } else { - buf = append(buf, byte(r)) - } - } - return string(buf), nil -} diff --git a/pkg/paths/encode_decode_test.go b/pkg/paths/encode_decode_test.go deleted file mode 100644 index d3dd5c77..00000000 --- a/pkg/paths/encode_decode_test.go +++ /dev/null @@ -1,46 +0,0 @@ -package paths - -import "testing" - -var testCases = []struct { - name, decoded, encoded string -}{ - { - "happy path", - "github.com/a/b", - "github.com/a/b", - }, - { - "capital letters", - "github.com/NYTimes/gizmo", - "github.com/!n!y!times/gizmo", - }, -} - -func TestEncodeDecode(t *testing.T) { - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - testEncodeDecode(t, tc.decoded, tc.encoded) - }) - } -} - -func testEncodeDecode(t *testing.T, exDec, exEnc string) { - t.Helper() - enc, err := EncodePath(exDec) - if err != nil { - t.Fatal(err) - } - dec, err := DecodePath(exEnc) - if err != nil { - t.Fatal(err) - } - - if exEnc != enc { - t.Fatalf("expected %v and %v to be equal", exEnc, enc) - } - - if exDec != dec { - t.Fatalf("expected %v and %v to be equal", exDec, dec) - } -} diff --git a/pkg/paths/go_get.go b/pkg/paths/go_get.go deleted file mode 100644 index 81976517..00000000 --- a/pkg/paths/go_get.go +++ /dev/null @@ -1,11 +0,0 @@ -package paths - -import ( - "net/url" - "strings" -) - -// IsGoGet returns true if u has ?go-get=1 in its query string -func IsGoGet(u *url.URL) bool { - return strings.Contains(u.Query().Get("go-get"), "1") -}