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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>

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,20 +35,25 @@ 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)
 	}
@@ -58,7 +61,7 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 		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,11 +39,11 @@ 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)
 	}
@@ -53,7 +51,7 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 		panic(err)
 	}
 	defer func() {
-		if err := os.Chdir(origWD); err != nil {
+		if err := os.Chdir(oldDir); err != nil {
 			panic(err)
 		}
 	}()
@@ -63,28 +61,28 @@ 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",
@@ -92,7 +90,7 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 		"-template", "fragment",
 		"-dr", "INSTALLDIR",
 		"-var", "var.SourceDir",
-		"-out", appfiles,
+		"-out", appFiles,
 	); err != nil {
 		return "", err
 	}
@@ -114,7 +112,7 @@ 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,
@@ -122,30 +120,30 @@ func ConstructInstaller(_ context.Context, workDir, tgzPath string, opt Installe
 		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,6 +236,8 @@ 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...)
@@ -245,35 +245,22 @@ func run(name string, args ...string) error {
 	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