Only set arch on bin dir when we are building for Windows (#1851)

pkg/packaging/detectLauncherVersion.go

@@ -19,7 +19,7 @@ func (p *PackageOptions) detectLauncherVersion(ctx context.Context) error {
 	logger := log.With(ctxlog.FromContext(ctx), "library", "detectLauncherVersion")
 	level.Debug(logger).Log("msg", "Attempting launcher autodetection")
 
-	launcherPath := p.launcherLocation(filepath.Join(p.packageRoot, p.binDir, string(p.target.Arch)))
+	launcherPath := p.launcherLocation()
 
 	stdout, err := p.execOut(ctx, launcherPath, "-version")
 	if err != nil {
@@ -49,17 +49,20 @@ func (p *PackageOptions) detectLauncherVersion(ctx context.Context) error {
 // launcherLocation returns the location of the launcher binary within `binDir`. For darwin,
 // it may be in an app bundle -- we check to see if the binary exists there first, and then
 // fall back to the common location if it doesn't.
-func (p *PackageOptions) launcherLocation(binDir string) string {
+func (p *PackageOptions) launcherLocation() string {
 	if p.target.Platform == Darwin {
-		// We want /usr/local/Kolide.app, not /usr/local/bin/universal/Kolide.app, so we use Dir to strip out `bin`
-		// and universal
-		appBundleBinaryPath := filepath.Join(filepath.Dir(filepath.Dir(binDir)), "Kolide.app", "Contents", "MacOS", "launcher")
+		// We want /usr/local/Kolide.app, not /usr/local/bin/Kolide.app, so we use Dir to strip out `bin`
+		appBundleBinaryPath := filepath.Join(p.packageRoot, filepath.Dir(p.binDir), "Kolide.app", "Contents", "MacOS", "launcher")
 		if info, err := os.Stat(appBundleBinaryPath); err == nil && !info.IsDir() {
 			return appBundleBinaryPath
 		}
 	}
 
-	return filepath.Join(binDir, p.target.PlatformBinaryName("launcher"))
+	if p.target.Platform == Windows {
+		return filepath.Join(p.packageRoot, p.binDir, string(p.target.Arch), p.target.PlatformBinaryName("launcher"))
+	}
+
+	return filepath.Join(p.packageRoot, p.binDir, p.target.PlatformBinaryName("launcher"))
 }
 
 // formatVersion formats the version according to the packaging requirements of the target platform

pkg/packaging/detectLauncherVersion_test.go

@@ -85,19 +85,25 @@ func TestFormatVersion(t *testing.T) {
 	}
 }
 
-func TestLauncherLocation(t *testing.T) {
+func TestLauncherLocation_Darwin(t *testing.T) {
 	t.Parallel()
 
-	pDarwin := &PackageOptions{target: Target{Platform: Darwin}}
+	pDarwin := &PackageOptions{
+		target: Target{
+			Platform: Darwin,
+		},
+		packageRoot: t.TempDir(),
+		binDir:      "bin",
+	}
 
 	// First, test that if the app bundle doesn't exist, we fall back to the top-level binary
-	require.Equal(t, filepath.Join("some", "dir", "launcher"), pDarwin.launcherLocation(filepath.Join("some", "dir")))
+	expectedFallbackLauncherLocation := filepath.Join(pDarwin.packageRoot, pDarwin.binDir, "launcher")
+	require.Equal(t, expectedFallbackLauncherLocation, pDarwin.launcherLocation())
 
 	// Create a temp directory with an app bundle in it
-	tmpDir := t.TempDir()
-	binDir := filepath.Join(tmpDir, "bin", "universal")
+	binDir := filepath.Join(pDarwin.packageRoot, pDarwin.binDir)
 	require.NoError(t, os.MkdirAll(binDir, 0755))
-	baseDir := filepath.Join(tmpDir, "Kolide.app", "Contents", "MacOS")
+	baseDir := filepath.Join(pDarwin.packageRoot, "Kolide.app", "Contents", "MacOS")
 	require.NoError(t, os.MkdirAll(baseDir, 0755))
 	expectedLauncherBinaryPath := filepath.Join(baseDir, "launcher")
 	f, err := os.Create(expectedLauncherBinaryPath)
@@ -106,13 +112,38 @@ func TestLauncherLocation(t *testing.T) {
 	defer os.Remove(expectedLauncherBinaryPath)
 
 	// Now, confirm that we find the binary inside the app bundle
-	require.Equal(t, expectedLauncherBinaryPath, pDarwin.launcherLocation(binDir))
+	require.Equal(t, expectedLauncherBinaryPath, pDarwin.launcherLocation())
+}
+
+func TestLauncherLocation_Windows(t *testing.T) {
+	t.Parallel()
+
+	pWindows := &PackageOptions{
+		target: Target{
+			Platform: Windows,
+			Arch:     Amd64,
+		},
+		packageRoot: t.TempDir(),
+		binDir:      "bin",
+	}
 
 	// No file check for windows, just expect the binary in the given location
-	pWindows := &PackageOptions{target: Target{Platform: Windows}}
-	require.Equal(t, filepath.Join("some", "dir", "launcher.exe"), pWindows.launcherLocation(filepath.Join("some", "dir")))
+	expectedLauncherLocation := filepath.Join(pWindows.packageRoot, pWindows.binDir, string(pWindows.target.Arch), "launcher.exe")
+	require.Equal(t, expectedLauncherLocation, pWindows.launcherLocation())
+}
+
+func TestLauncherLocation_Linux(t *testing.T) {
+	t.Parallel()
+
+	pLinux := &PackageOptions{
+		target: Target{
+			Platform: Linux,
+		},
+		packageRoot: t.TempDir(),
+		binDir:      "bin",
+	}
 
-	// Same as for windows: no file check, just expect the binary in the given location
-	pLinux := &PackageOptions{target: Target{Platform: Linux}}
-	require.Equal(t, filepath.Join("some", "dir", "launcher"), pLinux.launcherLocation(filepath.Join("some", "dir")))
+	// No file check for Linux, just expect the binary in the given location
+	expectedLauncherLocation := filepath.Join(pLinux.packageRoot, pLinux.binDir, "launcher")
+	require.Equal(t, expectedLauncherLocation, pLinux.launcherLocation())
 }

pkg/packaging/fetch.go

@@ -38,9 +38,11 @@ func FetchBinary(ctx context.Context, localCacheDir, name, binaryName, channelOr
 		return "", errors.New("empty cache dir argument")
 	}
 
-	// put binaries in arch specific directory to avoid naming collisions in wix msi building
+	// put binaries in arch specific directory for Windows to avoid naming collisions in wix msi building
 	// where a single destination will have multiple, mutally exclusive sources
-	localCacheDir = filepath.Join(localCacheDir, string(target.Arch))
+	if target.Platform == Windows {
+		localCacheDir = filepath.Join(localCacheDir, string(target.Arch))
+	}
 
 	if err := os.MkdirAll(localCacheDir, fsutil.DirMode); err != nil {
 		return "", fmt.Errorf("could not create cache directory: %w", err)

pkg/packaging/packaging.go

@@ -379,15 +379,15 @@ func (p *PackageOptions) getBinary(ctx context.Context, symbolicName, binaryName
 
 		// Create a symlink from <bin dir>/<binary> to the actual binary location within the app bundle
 		target := filepath.Join("..", appBundleName, "Contents", "MacOS", binaryName)
-		symlinkFile := filepath.Join(p.packageRoot, p.binDir, binaryName)
+		symlinkFile := p.fullPathToBareBinary(binaryName)
 		if err := os.Symlink(target, symlinkFile); err != nil {
 			return fmt.Errorf("could not create symlink after copying app bundle: %w", err)
 		}
 
 		return nil
 	}
 
-	binPath := filepath.Join(p.packageRoot, p.binDir, string(p.target.Arch), binaryName)
+	binPath := p.fullPathToBareBinary(binaryName)
 	if err := os.MkdirAll(filepath.Dir(binPath), fsutil.DirMode); err != nil {
 		return fmt.Errorf("could not create directory for binary %s: %w", binaryName, err)
 	}
@@ -402,6 +402,16 @@ func (p *PackageOptions) getBinary(ctx context.Context, symbolicName, binaryName
 	return nil
 }
 
+// fullPathToBareBinary returns the path to the binary (including arch only on Windows).
+// On macOS, this location is a symlink to inside the app bundle.
+func (p *PackageOptions) fullPathToBareBinary(binaryName string) string {
+	if p.target.Platform == Windows {
+		return filepath.Join(p.packageRoot, p.binDir, string(p.target.Arch), binaryName)
+	}
+
+	return filepath.Join(p.packageRoot, p.binDir, binaryName)
+}
+
 func (p *PackageOptions) makePackage(ctx context.Context) error {
 	ctx, span := trace.StartSpan(ctx, "packaging.makePackage")
 	defer span.End()

pkg/packaging/packaging_test.go

@@ -277,3 +277,65 @@ func testedTargets() []Target {
 		},
 	}
 }
+
+func Test_fullPathToBareBinary(t *testing.T) {
+	t.Parallel()
+
+	for _, tt := range []struct {
+		testCaseName string
+		binaryName   string
+		identifier   string
+		packageRoot  string
+		binDir       string
+		target       Target
+		expectedPath string
+	}{
+		{
+			testCaseName: "darwin",
+			binaryName:   "launcher",
+			packageRoot:  filepath.Join("test", "root"),
+			binDir:       filepath.Join("usr", "local", "test-identifier", "bin"),
+			target: Target{
+				Platform: Darwin,
+				Arch:     Arm64,
+			},
+			expectedPath: filepath.Join("test", "root", "usr", "local", "test-identifier", "bin", "launcher"),
+		},
+		{
+			testCaseName: "linux",
+			binaryName:   "launcher",
+			packageRoot:  filepath.Join("test", "root"),
+			binDir:       filepath.Join("usr", "local", "test-identifier", "bin"),
+			target: Target{
+				Platform: Linux,
+				Arch:     Amd64,
+			},
+			expectedPath: filepath.Join("test", "root", "usr", "local", "test-identifier", "bin", "launcher"),
+		},
+		{
+			testCaseName: "windows",
+			binaryName:   "launcher.exe",
+			packageRoot:  filepath.Join("test", "root"),
+			binDir:       filepath.Join("Launcher-test-identifier", "bin"),
+			target: Target{
+				Platform: Windows,
+				Arch:     Amd64,
+			},
+			expectedPath: filepath.Join("test", "root", "Launcher-test-identifier", "bin", "amd64", "launcher.exe"),
+		},
+	} {
+		tt := tt
+		t.Run(tt.testCaseName, func(t *testing.T) {
+			t.Parallel()
+
+			p := &PackageOptions{
+				packageRoot: tt.packageRoot,
+				binDir:      tt.binDir,
+				target:      tt.target,
+			}
+
+			actualPath := p.fullPathToBareBinary(tt.binaryName)
+			require.Equal(t, tt.expectedPath, actualPath)
+		})
+	}
+}