cmd/go/internal/modfetch: disable go.mod conversion for transitive deps

Earlier dependency management tools (dep, glide, godeps, and so on)
all wrote some kind of lock file saying which version of each
dependency had been (or should be) copied into the vendor directory.
When invoked in a new repo without a go.mod file, vgo can initialize
go.mod by converting a lock file from one of those earlier systems.

A key part of the design of Go modules (inherited from dep and others)
is the automatic incorporation of constraints from dependencies into
the top-level build, so that if A requires B, then A/go.mod and
B/go.mod both have an effect on the versions chosen for A's build.

Because of the auto-conversion, I thought of files like Gopkg.lock as
being like "go.mod in different syntax", and so if A requires B and
there's no B/go.mod but there is a B/Gopkg.lock, then the
auto-converted B/Gopkg.lock gets used as if it were B/go.mod.

On further consideration, this is too much to ask of the conversion.
Stop.

if A requires B, both have Gopkg.lock but not go.mod, and we're
working in the repo for A, then it's fine to use A/Gopkg.lock to
create an initial A/go.mod. If there are problems with the A/go.mod,
we can edit that file to fix them. It's not fine to use B/Gopkg.lock
to create an effective B/go.mod for use during the build.
If there are any problems with the conversion in B, there is no
way for us (working on A) to override it.

Typical problems include:

1. Lock files that have not been properly pruned, resulting in
spurious dependencies.

2. Module requirement cycles are impossible to express
when using commit hashes as identifiers. Instead "tailspins"
have been recorded instead.

Because the lock files being used as the source for the
auto-conversion all include transitive dependencies anyway,
converting A/Gopkg.lock should suffice anyway.

Move conversion code (still used for main module)
out of modfetch into modconv.

Change-Id: I45e33345a38a60f3c4a1eba166150ee1a66e5816
Reviewed-on: https://go-review.googlesource.com/120999
Reviewed-by: Bryan C. Mills <bcmills@google.com>

vendor/cmd/go/internal/modfetch/convert.go → vendor/cmd/go/internal/modconv/convert.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package modfetch
+package modconv
 
 import (
 	"fmt"
@@ -11,7 +11,7 @@ import (
 	"strings"
 	"sync"
 
-	"cmd/go/internal/modconv"
+	"cmd/go/internal/modfetch"
 	"cmd/go/internal/modfile"
 	"cmd/go/internal/module"
 	"cmd/go/internal/par"
@@ -26,9 +26,9 @@ func ConvertLegacyConfig(f *modfile.File, file string, data []byte) error {
 	if i >= 0 {
 		j = strings.LastIndex(file[:i], "/")
 	}
-	convert := modconv.Converters[file[i+1:]]
+	convert := Converters[file[i+1:]]
 	if convert == nil && j != -2 {
-		convert = modconv.Converters[file[j+1:]]
+		convert = Converters[file[j+1:]]
 	}
 	if convert == nil {
 		return fmt.Errorf("unknown legacy config file %s", file)
@@ -55,7 +55,7 @@ func ConvertLegacyConfig(f *modfile.File, file string, data []byte) error {
 	)
 	work.Do(10, func(item interface{}) {
 		r := item.(module.Version)
-		repo, info, err := ImportRepoRev(r.Path, r.Version)
+		repo, info, err := modfetch.ImportRepoRev(r.Path, r.Version)
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "vgo: converting %s: stat %s@%s: %v\n", file, r.Path, r.Version, err)
 			return

vendor/cmd/go/internal/modfetch/convert_test.go → vendor/cmd/go/internal/modconv/convert_test.go

@@ -2,19 +2,49 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package modfetch
+package modconv
 
 import (
 	"bytes"
+	"fmt"
 	"internal/testenv"
+	"io/ioutil"
+	"log"
+	"os"
+	"os/exec"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"cmd/go/internal/cfg"
-	"cmd/go/internal/modconv"
+	"cmd/go/internal/modfetch"
+	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/modfile"
+	"cmd/go/internal/module"
 )
 
+func TestMain(m *testing.M) {
+	os.Exit(testMain(m))
+}
+
+func testMain(m *testing.M) int {
+	if _, err := exec.LookPath("git"); err != nil {
+		fmt.Fprintln(os.Stderr, "skipping because git binary not found")
+		fmt.Println("PASS")
+		return 0
+	}
+
+	dir, err := ioutil.TempDir("", "modconv-test-")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+	modfetch.SrcMod = filepath.Join(dir, "src/mod")
+	codehost.WorkRoot = filepath.Join(dir, "codework")
+
+	return m.Run()
+}
+
 func TestConvertLegacyConfig(t *testing.T) {
 	testenv.MustHaveExternalNetwork(t)
 
@@ -117,23 +147,32 @@ func TestConvertLegacyConfig(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			repo, err := Lookup(tt.path)
-			if err != nil {
-				t.Fatal(err)
-			}
 
-			out, err := repo.GoMod(tt.vers)
+			dir, err := modfetch.Download(module.Version{Path: tt.path, Version: tt.vers})
 			if err != nil {
 				t.Fatal(err)
 			}
-			prefix := modconv.Prefix + "\n"
-			if !bytes.HasPrefix(out, []byte(prefix)) {
-				t.Fatalf("go.mod missing prefix %q:\n%s", prefix, out)
-			}
-			out = out[len(prefix):]
-			if !bytes.Equal(out, want) {
-				t.Fatalf("final go.mod:\n%s\n\nwant:\n%s", out, want)
+
+			for name := range Converters {
+				file := filepath.Join(dir, name)
+				data, err := ioutil.ReadFile(file)
+				if err == nil {
+					f := new(modfile.File)
+					f.AddModuleStmt(tt.path)
+					if err := ConvertLegacyConfig(f, filepath.ToSlash(file), data); err != nil {
+						t.Fatal(err)
+					}
+					out, err := f.Format()
+					if err != nil {
+						t.Fatalf("format after conversion: %v", err)
+					}
+					if !bytes.Equal(out, want) {
+						t.Fatalf("final go.mod:\n%s\n\nwant:\n%s", out, want)
+					}
+					return
+				}
 			}
+			t.Fatalf("no converter found for %s@%s", tt.path, tt.vers)
 		})
 	}
 }

vendor/cmd/go/internal/modconv/modconv.go

@@ -22,4 +22,4 @@ var Converters = map[string]func(string, []byte) (*modfile.File, error){
 // for dependencies before caching them.
 // In case of bugs in the converter, if we bump this version number,
 // then all the cached copies will be ignored.
-const Prefix = "//vgo 0.0.4\n"
+const Prefix = "//vgo 0.0.5\n"

vendor/cmd/go/internal/modfetch/cache.go

@@ -13,7 +13,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"cmd/go/internal/modconv"
 	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/par"
 	"cmd/go/internal/semver"
@@ -248,7 +247,12 @@ func readDiskStatByHash(path, rev string) (file string, info *RevInfo, err error
 	return "", nil, errNotCached
 }
 
-var vgoVersion = []byte(modconv.Prefix)
+// oldVgoPrefix is the prefix in the old auto-generated cached go.mod files.
+// We stopped trying to auto-generate the go.mod files. Now we use a trivial
+// go.mod with only a module line, and we've dropped the version prefix
+// entirely. If we see a version prefix, that means we're looking at an old copy
+// and should ignore it.
+var oldVgoPrefix = []byte("//vgo 0.0.")
 
 // readDiskGoMod reads a cached stat result from disk,
 // returning the name of the cache file and the result.
@@ -257,15 +261,8 @@ var vgoVersion = []byte(modconv.Prefix)
 func readDiskGoMod(path, rev string) (file string, data []byte, err error) {
 	file, data, err = readDiskCache(path, rev, "mod")
 
-	// If go.mod has a //vgo comment at the start,
-	// it was auto-converted from a legacy lock file.
-	// The auto-conversion details may have bugs and
-	// may be fixed in newer versions of vgo.
-	// We ignore cached go.mod files if they do not match
-	// our own vgoVersion.
-	// This use of "vgo" appears in disk files and must be preserved
-	// even once we excise most of the mentions of vgo from the code.
-	if err == nil && bytes.HasPrefix(data, vgoVersion[:len("//vgo")]) && !bytes.HasPrefix(data, vgoVersion) {
+	// If the file has an old auto-conversion prefix, pretend it's not there.
+	if bytes.HasPrefix(data, oldVgoPrefix) {
 		err = errNotCached
 		data = nil
 	}

vendor/cmd/go/internal/modfetch/coderepo.go

@@ -17,7 +17,6 @@ import (
 	"strings"
 	"time"
 
-	"cmd/go/internal/modconv"
 	"cmd/go/internal/modfetch/codehost"
 	"cmd/go/internal/modfile"
 	"cmd/go/internal/module"
@@ -268,41 +267,15 @@ func (r *codeRepo) GoMod(version string) (data []byte, err error) {
 	return data, nil
 }
 
-var altConfigs = []string{
-	"Gopkg.lock",
-
-	"GLOCKFILE",
-	"Godeps/Godeps.json",
-	"dependencies.tsv",
-	"glide.lock",
-	"vendor.conf",
-	"vendor.yml",
-	"vendor/manifest",
-	"vendor/vendor.json",
-}
-
 func (r *codeRepo) legacyGoMod(rev, dir string) []byte {
-	mf := new(modfile.File)
-	mf.AddModuleStmt(r.modPath)
-	for _, file := range altConfigs {
-		data, err := r.code.ReadFile(rev, path.Join(dir, file), codehost.MaxGoMod)
-		if err != nil {
-			continue
-		}
-		convert := modconv.Converters[file]
-		if convert == nil {
-			continue
-		}
-		if err := ConvertLegacyConfig(mf, path.Join(r.codeRoot+"@"+rev, dir, file), data); err != nil {
-			continue
-		}
-		break
-	}
-	data, err := mf.Format()
-	if err != nil {
-		return []byte(fmt.Sprintf("%s\nmodule %q\n", modconv.Prefix, r.modPath))
-	}
-	return append([]byte(modconv.Prefix+"\n"), data...)
+	// We used to try to build a go.mod reflecting pre-existing
+	// package management metadata files, but the conversion
+	// was inherently imperfect (because those files don't have
+	// exactly the same semantics as go.mod) and, when done
+	// for dependencies in the middle of a build, impossible to
+	// correct. So we stopped.
+	// Return a fake go.mod that simply declares the module path.
+	return []byte(fmt.Sprintf("module %s\n", modfile.AutoQuote(r.modPath)))
 }
 
 func (r *codeRepo) modPrefix(rev string) string {

vendor/cmd/go/internal/modfetch/coderepo_test.go

@@ -274,7 +274,7 @@ var codeRepoTests = []struct {
 		name:    "d670f9405373e636a5a2765eea47fac0c9bc91a4",
 		short:   "d670f9405373",
 		time:    time.Date(2018, 1, 9, 11, 43, 31, 0, time.UTC),
-		gomod:   "//vgo 0.0.4\n\nmodule gopkg.in/yaml.v2\n",
+		gomod:   "module gopkg.in/yaml.v2\n",
 	},
 	{
 		path:    "gopkg.in/check.v1",
@@ -283,7 +283,7 @@ var codeRepoTests = []struct {
 		name:    "20d25e2804050c1cd24a7eea1e7a6447dd0e74ec",
 		short:   "20d25e280405",
 		time:    time.Date(2016, 12, 8, 18, 13, 25, 0, time.UTC),
-		gomod:   "//vgo 0.0.4\n\nmodule gopkg.in/check.v1\n",
+		gomod:   "module gopkg.in/check.v1\n",
 	},
 	{
 		path:    "gopkg.in/yaml.v2",
@@ -301,7 +301,7 @@ var codeRepoTests = []struct {
 		name:    "ede458df7cd0fdca520df19a33158086a8a68e81",
 		short:   "ede458df7cd0",
 		time:    time.Date(2018, 4, 17, 19, 43, 22, 0, time.UTC),
-		gomod:   "//vgo 0.0.4\n\nmodule vcs-test.golang.org/go/mod/gitrepo1\n",
+		gomod:   "module vcs-test.golang.org/go/mod/gitrepo1\n",
 	},
 	{
 		path:    "gopkg.in/natefinch/lumberjack.v2",
@@ -310,7 +310,7 @@ var codeRepoTests = []struct {
 		name:    "a96e63847dc3c67d17befa69c303767e2f84e54f",
 		short:   "a96e63847dc3",
 		time:    time.Date(2017, 5, 31, 16, 3, 50, 0, time.UTC),
-		gomod:   "//vgo 0.0.4\n\nmodule gopkg.in/natefinch/lumberjack.v2\n",
+		gomod:   "module gopkg.in/natefinch/lumberjack.v2\n",
 	},
 	{
 		path: "gopkg.in/natefinch/lumberjack.v2",
@@ -325,7 +325,7 @@ var codeRepoTests = []struct {
 		name:    "a96e63847dc3c67d17befa69c303767e2f84e54f",
 		short:   "a96e63847dc3",
 		time:    time.Date(2017, 5, 31, 16, 3, 50, 0, time.UTC),
-		gomod:   "//vgo 0.0.4\n\nmodule gopkg.in/natefinch/lumberjack.v2\n",
+		gomod:   "module gopkg.in/natefinch/lumberjack.v2\n",
 	},
 }
 

vendor/cmd/go/internal/vgo/init.go

@@ -264,7 +264,7 @@ func legacyModInit() {
 			}
 			fmt.Fprintf(os.Stderr, "vgo: copying requirements from %s\n", cfg)
 			cfg = filepath.ToSlash(cfg)
-			if err := modfetch.ConvertLegacyConfig(modFile, cfg, data); err != nil {
+			if err := modconv.ConvertLegacyConfig(modFile, cfg, data); err != nil {
 				base.Fatalf("vgo: %v", err)
 			}
 			if len(modFile.Syntax.Stmt) == 1 {

vendor/cmd/go/internal/vgo/load.go

@@ -17,7 +17,6 @@ import (
 	"cmd/go/internal/base"
 	"cmd/go/internal/cfg"
 	"cmd/go/internal/imports"
-	"cmd/go/internal/modconv"
 	"cmd/go/internal/modfetch"
 	"cmd/go/internal/modfile"
 	"cmd/go/internal/module"
@@ -458,8 +457,6 @@ func (r *mvsReqs) Required(mod module.Version) ([]module.Version, error) {
 	return c.list, c.err
 }
 
-var vgoVersion = []byte(modconv.Prefix)
-
 func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) {
 	if mod == Target {
 		var list []module.Version