internal/installer/windowsmsi: fix duplicate workDir in paths

ConstructInstaller already changes working directory to workDir, so it
shouldn't be used again in relative paths. This wasn't spotted earlier
because it happens not to be an issue when workDir is an absolute path,
as was produced by t.TempDir().

Also apply some code simplifications, consistency improvements,
and dead code removal.

Even though the installer packages don't need cgo, they'll be imported
elsewhere where cgo is possibly enabled, so it's not viable to use the
!cgo build constraints. Drop them for now.

For golang/go#63147.

Change-Id: Id60c05fa67988e7548b70a55e712dbabfb7d85d7
Reviewed-on: https://go-review.googlesource.com/c/build/+/554057
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>

internal/installer/darwinpkg/darwinpkg.go

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build !cgo
-
 // Package darwinpkg encodes the process of building a macOS PKG
 // installer from the given Go toolchain .tar.gz binary archive.
 package darwinpkg
@@ -37,28 +35,33 @@ type InstallerOptions struct {
 // ConstructInstaller constructs an installer for the provided Go toolchain .tar.gz
 // binary archive using workDir as a working directory, and returns the output path.
 //
-// It's intended to run on a macOS system with Xcode installed.
+// It's intended to run on a macOS system, with Xcode tools available in $PATH.
 func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt InstallerOptions) (pkgPath string, _ error) {
 	var errs []error
+	for _, dep := range [...]string{"pkgbuild", "productbuild"} {
+		if _, err := exec.LookPath(dep); err != nil {
+			errs = append(errs, fmt.Errorf("dependency %q is not in PATH", dep))
+		}
+	}
 	if opt.GOARCH == "" {
 		errs = append(errs, fmt.Errorf("GOARCH is empty"))
 	}
 	if opt.MinMacOSVersion == "" {
 		errs = append(errs, fmt.Errorf("MinMacOSVersion is empty"))
 	}
-	if len(errs) > 0 {
-		return "", errors.Join(errs...)
+	if err := errors.Join(errs...); err != nil {
+		return "", err
 	}
 
-	origWD, err := os.Getwd()
+	oldDir, err := os.Getwd()
 	if err != nil {
 		panic(err)
 	}
 	if err := os.Chdir(workDir); err != nil {
 		panic(err)
 	}
 	defer func() {
-		if err := os.Chdir(origWD); err != nil {
+		if err := os.Chdir(oldDir); err != nil {
 			panic(err)
 		}
 	}()

internal/installer/darwinpkg/darwinpkg_test.go

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build !cgo
-
 package darwinpkg_test
 
 import (

internal/installer/windowsmsi/windowsmsi.go

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build !cgo
-
 // Package windowsmsi encodes the process of building a Windows MSI
 // installer from the given Go toolchain .tar.gz binary archive.
 package windowsmsi
@@ -41,19 +39,19 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 	if opt.GOARCH == "" {
 		errs = append(errs, fmt.Errorf("GOARCH is empty"))
 	}
-	if len(errs) > 0 {
-		return "", errors.Join(errs...)
+	if err := errors.Join(errs...); err != nil {
+		return "", err
 	}
 
-	origWD, err := os.Getwd()
+	oldDir, err := os.Getwd()
 	if err != nil {
 		panic(err)
 	}
 	if err := os.Chdir(workDir); err != nil {
 		panic(err)
 	}
 	defer func() {
-		if err := os.Chdir(origWD); err != nil {
+		if err := os.Chdir(oldDir); err != nil {
 			panic(err)
 		}
 	}()
@@ -63,36 +61,36 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 	version := readVERSION("go")
 
 	fmt.Println("\nInstalling WiX tools.")
-	wix := filepath.Join(workDir, "wix")
+	const wixDir = "wix"
 	switch opt.GOARCH {
 	default:
-		if err := installWix(wixRelease311, wix); err != nil {
+		if err := installWix(wixRelease311, wixDir); err != nil {
 			return "", err
 		}
 	case "arm", "arm64":
-		if err := installWix(wixRelease314, wix); err != nil {
+		if err := installWix(wixRelease314, wixDir); err != nil {
 			return "", err
 		}
 	}
 
-	fmt.Println("\nWriting out windows data used by the packaging process.")
-	win := filepath.Join(workDir, "windows")
-	if err := writeDataFiles(windowsData, win); err != nil {
+	fmt.Println("\nWriting out installer data used by the packaging process.")
+	const winDir = "windows"
+	if err := writeDataFiles(windowsData, winDir); err != nil {
 		return "", err
 	}
 
 	fmt.Println("\nGathering files (running wix heat).")
-	goDir := filepath.Join(workDir, "go")
-	appfiles := filepath.Join(win, "AppFiles.wxs")
-	if err := runDir(win, filepath.Join(wix, "heat"),
+	const goDir = "go"
+	appFiles := filepath.Join(winDir, "AppFiles.wxs")
+	if err := run(filepath.Join(wixDir, "heat"),
 		"dir", goDir,
 		"-nologo",
 		"-gg", "-g1", "-srd", "-sfrag", "-sreg",
 		"-cg", "AppFiles",
 		"-template", "fragment",
 		"-dr", "INSTALLDIR",
 		"-var", "var.SourceDir",
-		"-out", appfiles,
+		"-out", appFiles,
 	); err != nil {
 		return "", err
 	}
@@ -114,38 +112,38 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 	default:
 		panic("unknown arch for windows " + opt.GOARCH)
 	}
-	if err := runDir(win, filepath.Join(wix, "candle"),
+	if err := run(filepath.Join(wixDir, "candle"),
 		"-nologo",
 		"-arch", msArch,
 		"-dGoVersion="+version,
 		fmt.Sprintf("-dGoMajorVersion=%v", verMajor),
 		fmt.Sprintf("-dWixGoVersion=1.%v.%v", verMajor, verMinor),
 		"-dArch="+opt.GOARCH,
 		"-dSourceDir="+goDir,
-		filepath.Join(win, "installer.wxs"),
-		appfiles,
+		filepath.Join(winDir, "installer.wxs"),
+		appFiles,
 	); err != nil {
 		return "", err
 	}
 
 	fmt.Println("\nLinking the .msi installer (running wix light).")
-	msi := filepath.Join(workDir, "msi")
-	if err := os.Mkdir(msi, 0755); err != nil {
+	if err := os.Mkdir("msi-out", 0755); err != nil {
 		return "", err
 	}
-	if err := runDir(win, filepath.Join(wix, "light"),
+	if err := run(filepath.Join(wixDir, "light"),
+		"-b", winDir,
 		"-nologo",
 		"-dcl:high",
 		"-ext", "WixUIExtension",
 		"-ext", "WixUtilExtension",
 		"AppFiles.wixobj",
 		"installer.wixobj",
-		"-o", filepath.Join(msi, version+"-unsigned.msi"),
+		"-o", filepath.Join("msi-out", version+"-unsigned.msi"),
 	); err != nil {
 		return "", err
 	}
 
-	return filepath.Join(msi, version+"-unsigned.msi"), nil
+	return filepath.Join(workDir, "msi-out", version+"-unsigned.msi"), nil
 }
 
 type wixRelease struct {
@@ -238,42 +236,31 @@ func putTar(tgz, dir string) {
 	}
 }
 
+// run runs the specified command.
+// It prints the command line.
 func run(name string, args ...string) error {
 	fmt.Printf("$ %s %s\n", name, strings.Join(args, " "))
 	cmd := exec.Command(name, args...)
 	cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr
 	return cmd.Run()
 }
 
-func runDir(dir, name string, args ...string) error {
-	fmt.Printf("%s $ %s %s\n", filepath.Base(dir), name, strings.Join(args, " "))
-	cmd := exec.Command(name, args...)
-	cmd.Dir = dir
-	if dir != "" {
-		cmd.Env = append(os.Environ(), "PWD="+dir)
-	}
-	cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr
-	return cmd.Run()
-}
-
-var versionRe = regexp.MustCompile(`^go1\.(\d+(\.\d+)?)`)
+var versionRE = regexp.MustCompile(`^go1\.(\d+\.\d+)`)
 
-// splitVersion splits a Go version string such as "go1.9" or "go1.10.2" (as matched by versionRe)
+// splitVersion splits a Go version string such as "go1.23.4" (as matched by versionRE)
 // into its parts: major and minor.
 func splitVersion(v string) (major, minor int) {
-	m := versionRe.FindStringSubmatch(v)
-	if m == nil {
-		return
+	m := versionRE.FindStringSubmatch(v)
+	if len(m) < 2 {
+		return 0, 0
 	}
 	parts := strings.Split(m[1], ".")
-	if len(parts) >= 1 {
-		major, _ = strconv.Atoi(parts[0])
-
-		if len(parts) >= 2 {
-			minor, _ = strconv.Atoi(parts[1])
-		}
+	if len(parts) < 2 {
+		return 0, 0
 	}
-	return
+	major, _ = strconv.Atoi(parts[0])
+	minor, _ = strconv.Atoi(parts[1])
+	return major, minor
 }
 
 const storageBase = "https://storage.googleapis.com/go-builder-data/release/"

internal/installer/windowsmsi/windowsmsi_test.go

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build !cgo
-
 package windowsmsi
 
 import (
@@ -38,18 +36,16 @@ func TestConstructInstaller(t *testing.T) {
 
 func TestSplitVersion(t *testing.T) {
 	// Test splitVersion.
-	for _, tt := range []struct {
+	for _, tc := range [...]struct {
 		v            string
-		minor, patch int
+		major, minor int
 	}{
-		{"go1", 0, 0},
-		{"go1.34", 34, 0},
+		{"go1.34.0", 34, 0},
 		{"go1.34.7", 34, 7},
 	} {
-		minor, patch := splitVersion(tt.v)
-		if minor != tt.minor || patch != tt.patch {
-			t.Errorf("splitVersion(%q) = %v, %v; want %v, %v",
-				tt.v, minor, patch, tt.minor, tt.patch)
+		major, minor := splitVersion(tc.v)
+		if major != tc.major || minor != tc.minor {
+			t.Errorf("splitVersion(%q) = %v, %v; want %v, %v", tc.v, major, minor, tc.major, tc.minor)
 		}
 	}
 }

internal/untar/untar.go

@@ -14,7 +14,6 @@ import (
 	"io/fs"
 	"log"
 	"os"
-	"path"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -138,17 +137,6 @@ func untar(r io.Reader, dir string) (err error) {
 	return nil
 }
 
-func validRelativeDir(dir string) bool {
-	if strings.Contains(dir, `\`) || path.IsAbs(dir) {
-		return false
-	}
-	dir = path.Clean(dir)
-	if strings.HasPrefix(dir, "../") || strings.HasSuffix(dir, "/..") || dir == ".." {
-		return false
-	}
-	return true
-}
-
 func validRelPath(p string) bool {
 	if p == "" || strings.Contains(p, `\`) || strings.HasPrefix(p, "/") || strings.Contains(p, "../") {
 		return false