cmd/gorelease: support comparison of modules with different paths

For golang/go#39666

Change-Id: I7e69ef14a3f6b3996ceea2a70ec4ce1e33f912c6
Reviewed-on: https://go-review.googlesource.com/c/exp/+/238839
Trust: Jay Conrod <[email protected]>
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>

Author: Jay Conrod

cmd/gorelease/gorelease.go

@@ -64,8 +64,11 @@
 // "latest", or "none". If the version is "none", gorelease will not compare the
 // current version against any previous version; it will only validate the
 // current version. This is useful for checking the first release of a new major
-// version. If -base is not specified, gorelease will attempt to infer a base
-// version from the -version flag and available released versions.
+// version. The version may be preceded by a different module path and an '@',
+// like -base=example.com/mod/[email protected]. This is useful to compare against
+// an earlier major version or a fork. If -base is not specified, gorelease will
+// attempt to infer a base version from the -version flag and available released
+// versions.
 //
 // -version=version: The proposed version to be released. If specified,
 // gorelease will confirm whether this version is consistent with changes made
@@ -160,8 +163,8 @@ func runRelease(w io.Writer, dir string, args []string) (success bool, err error
 	fs := flag.NewFlagSet("gorelease", flag.ContinueOnError)
 	fs.Usage = func() {}
 	fs.SetOutput(ioutil.Discard)
-	var baseVersion, releaseVersion string
-	fs.StringVar(&baseVersion, "base", "", "previous version to compare against")
+	var baseOpt, releaseVersion string
+	fs.StringVar(&baseOpt, "base", "", "previous version to compare against")
 	fs.StringVar(&releaseVersion, "version", "", "proposed version to be released")
 	if err := fs.Parse(args); err != nil {
 		return false, &usageError{err: err}
@@ -170,6 +173,7 @@ func runRelease(w io.Writer, dir string, args []string) (success bool, err error
 	if len(fs.Args()) > 0 {
 		return false, usageErrorf("no arguments allowed")
 	}
+
 	if releaseVersion != "" {
 		if semver.Build(releaseVersion) != "" {
 			return false, usageErrorf("release version %q is not a canonical semantic version: build metadata is not supported", releaseVersion)
@@ -178,12 +182,26 @@ func runRelease(w io.Writer, dir string, args []string) (success bool, err error
 			return false, usageErrorf("release version %q is not a canonical semantic version", releaseVersion)
 		}
 	}
-	if baseVersion != "" && semver.Canonical(baseVersion) == baseVersion && releaseVersion != "" {
-		if cmp := semver.Compare(baseVersion, releaseVersion); cmp == 0 {
-			return false, usageErrorf("-base and -version must be different")
-		} else if cmp > 0 {
-			return false, usageErrorf("base version (%q) must be lower than release version (%q)", baseVersion, releaseVersion)
+
+	var baseModPath, baseVersion string
+	if at := strings.Index(baseOpt, "@"); at >= 0 {
+		baseModPath = baseOpt[:at]
+		baseVersion = baseOpt[at+1:]
+	} else if dot, slash := strings.Index(baseOpt, "."), strings.Index(baseOpt, "/"); dot >= 0 && slash >= 0 && dot < slash {
+		baseModPath = baseOpt
+	} else {
+		baseVersion = baseOpt
+	}
+	if baseModPath == "" {
+		if baseVersion != "" && semver.Canonical(baseVersion) == baseVersion && releaseVersion != "" {
+			if cmp := semver.Compare(baseOpt, releaseVersion); cmp == 0 {
+				return false, usageErrorf("-base and -version must be different")
+			} else if cmp > 0 {
+				return false, usageErrorf("base version (%q) must be lower than release version (%q)", baseVersion, releaseVersion)
+			}
 		}
+	} else if baseModPath != "" && baseVersion == "none" {
+		return false, usageErrorf(`base version (%q) cannot have version "none" with explicit module path`, baseOpt)
 	}
 
 	// Find the local module and repository root directories.
@@ -201,8 +219,12 @@ func runRelease(w io.Writer, dir string, args []string) (success bool, err error
 
 	// Find the base version if there is one, download it, and load packages from
 	// the module cache.
-	baseModPath := release.modPath // TODO(golang.org/issue/39666): allow different module path
-	base, err := loadDownloadedModule(baseModPath, baseVersion, releaseVersion)
+	var max string
+	if baseModPath == "" {
+		baseModPath = release.modPath
+		max = releaseVersion
+	}
+	base, err := loadDownloadedModule(baseModPath, baseVersion, max)
 	if err != nil {
 		return false, err
 	}
@@ -381,6 +403,9 @@ func loadLocalModule(modRoot, repoRoot, version string) (m moduleInfo, err error
 // to max will not be considered. Typically, loadDownloadedModule is used to
 // load the base version, and max is the release version.
 func loadDownloadedModule(modPath, version, max string) (m moduleInfo, err error) {
+	// TODO(#39666): support downloaded modules that are "soft forks", where the
+	// module path in go.mod is different from modPath.
+
 	// Check the module path and version.
 	// If the version is a query, resolve it to a canonical version.
 	m = moduleInfo{modPath: modPath}
@@ -474,18 +499,12 @@ func loadDownloadedModule(modPath, version, max string) (m moduleInfo, err error
 // version control tag to use (with an appropriate prefix, for modules not
 // in the repository root directory).
 func makeReleaseReport(base, release moduleInfo) (report, error) {
-	if base.modPath != release.modPath {
-		// TODO(golang.org/issue/39666): allow base and release path to be different.
-		panic(fmt.Sprintf("base module path %q is different than release module path %q", base.modPath, release.modPath))
-	}
-	modPath := release.modPath
-
 	// Compare each pair of packages.
 	// Ignore internal packages.
 	// If we don't have a base version to compare against,
 	// just check the new packages for errors.
 	shouldCompare := base.version != "none"
-	isInternal := func(pkgPath string) bool {
+	isInternal := func(modPath, pkgPath string) bool {
 		if !hasPathPrefix(pkgPath, modPath) {
 			panic(fmt.Sprintf("package %s not in module %s", pkgPath, modPath))
 		}
@@ -501,17 +520,17 @@ func makeReleaseReport(base, release moduleInfo) (report, error) {
 		base:    base,
 		release: release,
 	}
-	for _, pair := range zipPackages(base.pkgs, release.pkgs) {
+	for _, pair := range zipPackages(base.modPath, base.pkgs, release.modPath, release.pkgs) {
 		basePkg, releasePkg := pair.base, pair.release
 		switch {
 		case releasePkg == nil:
 			// Package removed
-			if !isInternal(basePkg.PkgPath) || len(basePkg.Errors) > 0 {
+			if internal := isInternal(base.modPath, basePkg.PkgPath); !internal || len(basePkg.Errors) > 0 {
 				pr := packageReport{
 					path:       basePkg.PkgPath,
 					baseErrors: basePkg.Errors,
 				}
-				if !isInternal(basePkg.PkgPath) {
+				if !internal {
 					pr.Report = apidiff.Report{
 						Changes: []apidiff.Change{{
 							Message:    "package removed",
@@ -524,12 +543,12 @@ func makeReleaseReport(base, release moduleInfo) (report, error) {
 
 		case basePkg == nil:
 			// Package added
-			if !isInternal(releasePkg.PkgPath) && shouldCompare || len(releasePkg.Errors) > 0 {
+			if internal := isInternal(release.modPath, releasePkg.PkgPath); !internal && shouldCompare || len(releasePkg.Errors) > 0 {
 				pr := packageReport{
 					path:          releasePkg.PkgPath,
 					releaseErrors: releasePkg.Errors,
 				}
-				if !isInternal(releasePkg.PkgPath) && shouldCompare {
+				if !internal && shouldCompare {
 					// If we aren't comparing against a base version, don't say
 					// "package added". Only report packages with errors.
 					pr.Report = apidiff.Report{
@@ -544,7 +563,10 @@ func makeReleaseReport(base, release moduleInfo) (report, error) {
 
 		default:
 			// Matched packages
-			if !isInternal(basePkg.PkgPath) && basePkg.Name != "main" && releasePkg.Name != "main" {
+			// Both packages are internal or neither; we only consider path components
+			// after the module path.
+			internal := isInternal(release.modPath, releasePkg.PkgPath)
+			if !internal && basePkg.Name != "main" && releasePkg.Name != "main" {
 				pr := packageReport{
 					path:          basePkg.PkgPath,
 					baseErrors:    basePkg.Errors,
@@ -558,7 +580,7 @@ func makeReleaseReport(base, release moduleInfo) (report, error) {
 
 	if release.version != "" {
 		r.validateVersion()
-	} else {
+	} else if r.similarModPaths() {
 		r.suggestVersion()
 	}
 
@@ -1056,24 +1078,27 @@ type packagePair struct {
 // If a package is in one list but not the other (because it was added or
 // removed between releases), a pair will be returned with a nil
 // base or release field.
-func zipPackages(basePkgs, releasePkgs []*packages.Package) []packagePair {
+func zipPackages(baseModPath string, basePkgs []*packages.Package, releaseModPath string, releasePkgs []*packages.Package) []packagePair {
 	baseIndex, releaseIndex := 0, 0
 	var pairs []packagePair
 	for baseIndex < len(basePkgs) || releaseIndex < len(releasePkgs) {
 		var basePkg, releasePkg *packages.Package
+		var baseSuffix, releaseSuffix string
 		if baseIndex < len(basePkgs) {
 			basePkg = basePkgs[baseIndex]
+			baseSuffix = trimPathPrefix(basePkg.PkgPath, baseModPath)
 		}
 		if releaseIndex < len(releasePkgs) {
 			releasePkg = releasePkgs[releaseIndex]
+			releaseSuffix = trimPathPrefix(releasePkg.PkgPath, releaseModPath)
 		}
 
 		var pair packagePair
-		if basePkg != nil && (releasePkg == nil || basePkg.PkgPath < releasePkg.PkgPath) {
+		if basePkg != nil && (releasePkg == nil || baseSuffix < releaseSuffix) {
 			// Package removed
 			pair = packagePair{basePkg, nil}
 			baseIndex++
-		} else if releasePkg != nil && (basePkg == nil || releasePkg.PkgPath < basePkg.PkgPath) {
+		} else if releasePkg != nil && (basePkg == nil || releaseSuffix < baseSuffix) {
 			// Package added
 			pair = packagePair{nil, releasePkg}
 			releaseIndex++

cmd/gorelease/report.go

@@ -65,10 +65,14 @@ func (r *report) Text(w io.Writer) error {
 		}
 	}
 
+	baseVersion := r.base.version
+	if r.base.modPath != r.release.modPath {
+		baseVersion = r.base.modPath + "@" + baseVersion
+	}
 	if r.base.versionInferred {
-		fmt.Fprintf(buf, "Inferred base version: %s\n", r.base.version)
+		fmt.Fprintf(buf, "Inferred base version: %s\n", baseVersion)
 	} else if r.base.versionQuery != "" {
-		fmt.Fprintf(buf, "Base version: %s (%s)\n", r.base.version, r.base.versionQuery)
+		fmt.Fprintf(buf, "Base version: %s (%s)\n", baseVersion, r.base.versionQuery)
 	}
 
 	if len(r.release.diagnostics) > 0 {
@@ -83,7 +87,7 @@ func (r *report) Text(w io.Writer) error {
 		} else {
 			fmt.Fprintf(buf, "Suggested version: %[1]s (with tag %[2]s%[1]s)\n", r.release.version, r.release.tagPrefix)
 		}
-	} else {
+	} else if r.release.version != "" && r.similarModPaths() {
 		if r.release.tagPrefix == "" {
 			fmt.Fprintf(buf, "%s is a valid semantic version for this release.\n", r.release.version)
 
@@ -175,7 +179,7 @@ which is required for major versions v2 or greater.`, major)
 	}
 
 	// Check that compatible / incompatible changes are consistent.
-	if semver.Major(r.base.version) == "v0" {
+	if semver.Major(r.base.version) == "v0" || r.base.modPath != r.release.modPath {
 		return
 	}
 	if r.haveIncompatibleChanges {
@@ -202,6 +206,11 @@ func (r *report) suggestVersion() {
 		r.release.versionInferred = true
 	}
 
+	if r.base.modPath != r.release.modPath {
+		setNotValid("Base module path is different from release.")
+		return
+	}
+
 	if r.haveReleaseErrors || r.haveBaseErrors {
 		setNotValid("Errors were found.")
 		return
@@ -245,6 +254,14 @@ func (r *report) suggestVersion() {
 	setVersion(fmt.Sprintf("v%s.%s.%s", major, minor, patch))
 }
 
+// similarModPaths returns true if r.base and r.release are versions of the same
+// module or different major versions of the same module.
+func (r *report) similarModPaths() bool {
+	basePath := strings.TrimSuffix(r.base.modPath, r.base.modPathMajor)
+	releasePath := strings.TrimSuffix(r.release.modPath, r.release.modPathMajor)
+	return basePath == releasePath
+}
+
 // requirementsChanged reports whether requirements have changed from base to
 // version.
 //

cmd/gorelease/testdata/basic/v1_fork_base_modpath_version_verify.test

@@ -0,0 +1,11 @@
+mod=example.com/basicfork
+base=example.com/[email protected]
+version=v1.1.2
+release=v1.1.2
+-- want --
+example.com/basic/a
+-------------------
+Incompatible changes:
+- A2: removed
+Compatible changes:
+- A3: added

cmd/gorelease/testdata/basic/v1_v2_base_modpath_query_verify.test

@@ -0,0 +1,17 @@
+mod=example.com/basic/v2
+base=example.com/basic@>=v1.1.0
+version=v2.0.1
+release=v2.0.1
+-- want --
+example.com/basic/a
+-------------------
+Incompatible changes:
+- A2: removed
+
+example.com/basic/b
+-------------------
+Incompatible changes:
+- package removed
+
+Base version: example.com/[email protected] (>=v1.1.0)
+v2.0.1 is a valid semantic version for this release.

cmd/gorelease/testdata/basic/v1_v2_base_modpath_suggest.test

@@ -0,0 +1,18 @@
+mod=example.com/basic/v2
+base=example.com/basic
+version=v2.1.0
+success=false
+-- want --
+example.com/basic/a
+-------------------
+Compatible changes:
+- A2: added
+
+example.com/basic/v2/b
+----------------------
+Compatible changes:
+- package added
+
+Inferred base version: example.com/[email protected]
+Cannot suggest a release version.
+Base module path is different from release.

cmd/gorelease/testdata/basic/v1_v2_base_modpath_verify.test

@@ -0,0 +1,17 @@
+mod=example.com/basic/v2
+base=example.com/basic
+version=v2.1.0
+release=v2.1.0
+-- want --
+example.com/basic/a
+-------------------
+Compatible changes:
+- A2: added
+
+example.com/basic/v2/b
+----------------------
+Compatible changes:
+- package added
+
+Inferred base version: example.com/[email protected]
+v2.1.0 is a valid semantic version for this release.

cmd/gorelease/testdata/basic/v1_v2_base_modpath_version_verify.test

@@ -0,0 +1,16 @@
+mod=example.com/basic/v2
+base=example.com/[email protected]
+version=v2.0.1
+release=v2.0.1
+-- want --
+example.com/basic/a
+-------------------
+Incompatible changes:
+- A2: removed
+
+example.com/basic/b
+-------------------
+Incompatible changes:
+- package removed
+
+v2.0.1 is a valid semantic version for this release.

cmd/gorelease/testdata/errors/base_modpath_none.test

@@ -0,0 +1,8 @@
+mod=example.com/basic/v2
+base=example.com/basic@none
+error=true
+
+-- want --
+usage: gorelease [-base=version] [-version=version]
+base version ("example.com/basic@none") cannot have version "none" with explicit module path
+For more information, run go doc golang.org/x/exp/cmd/gorelease

cmd/gorelease/testdata/internalcompat/README.txt

@@ -0,0 +1,20 @@
+Modules example.com/internalcompat/{a,b} are copies. One could be a fork
+of the other. An external package p exposes a type from a package q
+within the same module.
+
+gorelease should not report differences between these packages. The types
+are distinct, but they correspond (in apidiff terminology), which is the
+important property when considering differences between modules.
+
+There are three use cases to consider:
+
+1. One module substitutes for the other via a `replace` directive.
+   Only the replacement module is used, and the package paths are effectively
+   identical, so the types are not distinct.
+2. One module subsititutes for the other by rewriting `import` statements
+   globally. All references to the original type become references to the
+   new type, so there is no conflict.
+3. One module substitutes for the other by rewriting some `import` statements
+   but not others (for example, those within a specific consumer package).
+   In this case, the types are distinct, and even if there are no changes,
+   the types are not compatible.

cmd/gorelease/testdata/internalcompat/internalcompat.test

@@ -0,0 +1,6 @@
+mod=example.com/internalcompat/b
+version=v1.0.0
+release=v1.0.0
+base=example.com/internalcompat/[email protected]
+
+-- want --