simplify our handling of "go list" errors

First, teach scripts/gen-go-std-tables.sh to omit test packages,
since runtime/metrics_test would always result in an error.
Instead, make transformLinkname explicitly skip that package,
leaving a comment about a potential improvement if needed.

Second, the only remaining "not found" error we had was "maps" on 1.20,
so rewrite that check based on ImportPath and GoVersionSemver.

Third, detect packages with the "exclude all Go files" error
by looking at CompiledGoFiles and IgnoredGoFiles, which is less brittle.
This means that we are no longer doing any filtering on pkg.Error.Err,
which means we are less likely to break with Go error message changes.

Fourth, the check on pkg.Incomplete is now obsolete given the above,
meaning that the CompiledGoFiles length check is plenty.

Finally, stop trying to be clever about how we print errors.
Now that we're no longer skipping packages based on pkg.Error values,
printing pkg.DepsErrors was causing duplicate messages in the output.
Simply print pkg.Error values with only minimal tweaks:
including the position if there is any, and avoiding double newlines.

Overall, this makes our logic a lot less complicated,
and garble still works the way we want it to.

go_std_tables.go

@@ -1,6 +1,6 @@
 // Code generated by scripts/gen-go-std-tables.sh; DO NOT EDIT.
 
-// Generated from Go version devel go1.21-0fd6ae548f Fri Apr 28 20:33:34 2023 +0000.
+// Generated from Go version devel go1.21-3637132233 Sat Jun 3 01:13:08 2023 +0000.
 
 package main
 
@@ -41,7 +41,6 @@ var runtimeLinknamed = []string{
 	"runtime/coverage",
 	"runtime/debug",
 	"runtime/metrics",
-	"runtime/metrics_test",
 	"runtime/pprof",
 	"runtime/trace",
 	"sync",

main.go

@@ -1102,6 +1102,13 @@ func (tf *transformer) transformLinkname(localName, newName string) (string, str
 		pkgPath := newName[:pkgSplit]
 		pkgSplit++ // skip over the dot
 
+		if strings.HasSuffix(pkgPath, "_test") {
+			// runtime uses a go:linkname to metrics_test;
+			// we don't need this to work for now on regular builds,
+			// though we might need to rethink this if we want "go test std" to work.
+			continue
+		}
+
 		var err error
 		lpkg, err = listPackage(tf.curPkg, pkgPath)
 		if err == nil {

scripts/gen-go-std-tables.sh

@@ -12,7 +12,7 @@ runtime_and_deps=$(go list -deps runtime)
 # This resulting list is what we need to "go list" when obfuscating the runtime,
 # as they are the packages that we may be missing.
 runtime_linknamed=$(comm -23 <(
-	sed -rn 's@//go:linkname .* ([^.]*)\.[^.]*@\1@p' "${goroot}"/src/runtime/*.go | grep -vE '^main|^runtime\.' | sort -u
+	sed -rn 's@//go:linkname .* ([^.]*)\.[^.]*@\1@p' "${goroot}"/src/runtime/*.go | grep -vE '^main|^runtime\.|_test$' | sort -u
 ) <(
 	# Note that we assume this is constant across platforms.
 	go list -deps runtime | sort -u

shared.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"golang.org/x/mod/module"
+	"golang.org/x/mod/semver"
 )
 
 //go:generate ./scripts/gen-go-std-tables.sh
@@ -151,12 +152,10 @@ type listedPackage struct {
 
 	Dir             string
 	CompiledGoFiles []string
+	IgnoredGoFiles  []string
 	Imports         []string
 
-	Incomplete bool
+	Error *packageError // to report package loading errors to the user
-	// These two exist to report package loading errors to the user.
-	Error      *packageError
-	DepsErrors []*packageError
 
 	// The fields below are not part of 'go list', but are still reused
 	// between garble processes. Use "Garble" as a prefix to ensure no
@@ -174,6 +173,7 @@ type listedPackage struct {
 }
 
 type packageError struct {
+	Pos string
 	Err string
 }
 
@@ -254,58 +254,30 @@ func appendListedPackages(packages []string, mainBuild bool) error {
 	if sharedCache.ListedPackages == nil {
 		sharedCache.ListedPackages = make(map[string]*listedPackage)
 	}
-	var pkgErrors []string
+	var pkgErrors strings.Builder
 	for dec.More() {
 		var pkg listedPackage
 		if err := dec.Decode(&pkg); err != nil {
 			return err
 		}
 
-		// Sometimes cmd/go sets Error without setting Incomplete per the docs.
-		// TODO: remove the workaround once https://go.dev/issue/57724 is fixed.
-		if pkg.Error != nil || pkg.DepsErrors != nil {
-			pkg.Incomplete = true
-		}
-
 		if perr := pkg.Error; perr != nil {
-			switch {
+			if pkg.Standard && len(pkg.CompiledGoFiles) == 0 && len(pkg.IgnoredGoFiles) > 0 {
-			// All errors in non-std packages are fatal,
+				// Some packages in runtimeLinknamed need a build tag to be importable,
-			// but only some errors in std packages are.
+				// like crypto/internal/boring/fipstls with boringcrypto,
-			case strings.Contains(pkg.ImportPath, "."):
+				// so any pkg.Error should be ignored when the build tag isn't set.
-				fallthrough
+			} else if pkg.ImportPath == "maps" && semver.Compare(sharedCache.GoVersionSemver, "v1.21") < 0 {
-			default:
+				// "maps" was added in Go 1.21, so Go 1.20 runs into a "not found" error.
-				// Error messages sometimes include a trailing newline.
+			} else {
-				pkgErrors = append(pkgErrors, strings.TrimSpace(perr.Err))
+				if pkgErrors.Len() > 0 {
-
+					pkgErrors.WriteString("\n")
-			// Some packages in runtimeLinknamed are OS-specific,
+				}
-			// like crypto/internal/boring/fipstls, so "no Go files"
+				if perr.Pos != "" {
-			// for the current OS can be ignored safely as an error.
+					pkgErrors.WriteString(perr.Pos)
-			case pkg.Standard && strings.Contains(perr.Err, "build constraints exclude all Go files"):
+					pkgErrors.WriteString(": ")
-			// Some packages in runtimeLinknamed are recent,
-			// like "arena", so older Go versions that we support
-			// do not yet have them and that's OK.
-			// Note that pkg.Standard is false for them.
-			// Note that Go 1.21 is swapping "GOROOT" for "std".
-			// TODO(mvdan): We try to list test packages like runtime/metrics_test, which always fail.
-			case strings.Contains(perr.Err, "is not in GOROOT"):
-			case strings.Contains(perr.Err, "is not in std"):
-			case strings.Contains(perr.Err, "cannot find package"):
-			}
-		}
-		if len(pkg.DepsErrors) > 0 {
-			for i, derr := range pkg.DepsErrors {
-				// When an error in DepsErrors starts with a "# pkg/path" line,
-				// it's an error that we're already printing via that package's Error field.
-				// Otherwise, the error is that we couldn't find that package at all,
-				// so we do need to print it here as that package won't be listed.
-				if i == 0 {
-					if strings.HasPrefix(derr.Err, "# ") {
-						break
-					}
-					pkgErrors = append(pkgErrors, "# "+pkg.ImportPath)
 				}
 				// Error messages sometimes include a trailing newline.
-				pkgErrors = append(pkgErrors, strings.TrimSpace(derr.Err))
+				pkgErrors.WriteString(strings.TrimRight(perr.Err, "\n"))
 			}
 		}
 
@@ -329,8 +301,8 @@ func appendListedPackages(packages []string, mainBuild bool) error {
 	if err := cmd.Wait(); err != nil {
 		return fmt.Errorf("go list error: %v:\nargs: %q\n%s", err, args, stderr.Bytes())
 	}
-	if len(pkgErrors) > 0 {
+	if pkgErrors.Len() > 0 {
-		return errors.New(strings.Join(pkgErrors, "\n"))
+		return errors.New(pkgErrors.String())
 	}
 
 	anyToObfuscate := false
@@ -345,11 +317,7 @@ func appendListedPackages(packages []string, mainBuild bool) error {
 			// "unknown pc" crashes on windows in the cgo test otherwise.
 			path == "runtime/cgo":
 
-		// We can't obfuscate packages which weren't loaded.
+		// No point in obfuscating empty packages, like OS-specific ones that don't match.
-		// This can happen since we ignore some pkg.Error messages above.
-		case pkg.Incomplete:
-
-		// No point in obfuscating empty packages.
 		case len(pkg.CompiledGoFiles) == 0:
 
 		// Test main packages like "foo/bar.test" are always obfuscated,

testdata/script/list_error.txtar

@@ -5,13 +5,11 @@
 -- stderr-go1.20.golden --
 # test/main/broken
 broken${/}broken.go:5:16: cannot use 123 (untyped int constant) as string value in variable declaration
-# test/main/imports_missing
+imports_missing${/}imports.go:5:8: package test/main/missing is not in GOROOT (${GOROOT}${/}src${/}test${/}main${/}missing)
-package test/main/missing is not in GOROOT (${GOROOT}${/}src${/}test${/}main${/}missing)
 -- stderr-go1.21.golden --
 # test/main/broken
 broken${/}broken.go:5:16: cannot use 123 (untyped int constant) as string value in variable declaration
-# test/main/imports_missing
+imports_missing${/}imports.go:5:8: package test/main/missing is not in std (${GOROOT}${/}src${/}test${/}main${/}missing)
-package test/main/missing is not in std (${GOROOT}${/}src${/}test${/}main${/}missing)
 -- go.mod --
 module test/main