feat(symbols): key Android uploads by R8's map id, and upload dSYMs from an Xcode build

cmd/symbols/android_mapid.go

@@ -0,0 +1,58 @@
+package symbols
+
+import (
+	"bufio"
+	"os"
+	"regexp"
+	"strings"
+)
+
+// The id R8 gives the mapping it produced.
+//
+// R8 writes "# pg_map_id: <hash of the mapping>" into every mapping's header, and
+// from AGP 8.12 it stamps that same id into each class's source file attribute — so
+// a shipped app reports "r8-map-id-<hash>" where a file name goes, on every frame of
+// every crash. Keying the upload by it puts the index exactly where symbolication
+// will look for a build that was asked to do nothing at all.
+//
+// It is read out of the header rather than recomputed here because the header is
+// R8's own statement of what it stamped. A hash computed on this side would have to
+// agree with R8's forever, and would be silently wrong the first time it didn't.
+
+// androidMapIDComment is the mapping header line carrying the id.
+const androidMapIDComment = "# pg_map_id:"
+
+// androidMapIDPattern is the shape of an id that may be keyed by: a hash, which is
+// the mapping's full SHA-256 from AGP 8.12 and a 7-character prefix of it before.
+var androidMapIDPattern = regexp.MustCompile(`^[0-9a-f]{7,64}$`)
+
+// androidMapID returns the id R8 recorded for a mapping, or "" when it recorded none
+// — a mapping written by ProGuard rather than R8, or one old enough to predate the
+// header. Best effort, like every other way of learning what a build shipped: an
+// upload with no id falls back to the Version Lane rather than failing over it.
+func androidMapID(mappingPath string) string {
+	file, err := os.Open(mappingPath)
+	if err != nil {
+		return ""
+	}
+	defer file.Close()
+
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		line := scanner.Text()
+		// The header is the comments before the first class, and a release mapping
+		// is tens of megabytes of what follows it.
+		if !strings.HasPrefix(line, "#") {
+			return ""
+		}
+		rest, ok := strings.CutPrefix(line, androidMapIDComment)
+		if !ok {
+			continue
+		}
+		if id := strings.TrimSpace(rest); androidMapIDPattern.MatchString(id) {
+			return id
+		}
+		return ""
+	}
+	return ""
+}

cmd/symbols/android_mapid_test.go

@@ -0,0 +1,82 @@
+package symbols
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The header R8 8.13 writes, trimmed to what is read from it.
+const testMapIDHeader = `# compiler: R8
+# compiler_version: 8.13.19
+# min_api: 23
+# common_typos_disable
+# {"id":"com.android.tools.r8.mapping","version":"2.2"}
+# pg_map_id: 92d0222f1a7a3b92fca00ddc75fbcf893c89be03e02286414d51abcfd9b02063
+# pg_map_hash: SHA-256 92d0222f1a7a3b92fca00ddc75fbcf893c89be03e02286414d51abcfd9b02063
+`
+
+const testMapID = "92d0222f1a7a3b92fca00ddc75fbcf893c89be03e02286414d51abcfd9b02063"
+
+func TestAndroidMapIDReadsTheHeader(t *testing.T) {
+	path := writeMappingFile(t, testMapIDHeader+testAndroidMapping)
+	assert.Equal(t, testMapID, androidMapID(path))
+}
+
+// R8 shortened the id before AGP 8.12, and a build being retraced today may well
+// have been produced by one of those.
+func TestAndroidMapIDReadsAShortID(t *testing.T) {
+	path := writeMappingFile(t, "# compiler: R8\n# pg_map_id: 92d0222\n"+testAndroidMapping)
+	assert.Equal(t, "92d0222", androidMapID(path))
+}
+
+// Nothing here may fail an upload: an id that cannot be read leaves the mapping on
+// the Version Lane, which is where it was before R8 recorded one.
+func TestAndroidMapIDWithoutOne(t *testing.T) {
+	cases := map[string]string{
+		"no header at all":         testAndroidMapping,
+		"a ProGuard mapping":       "com.example.app.CheckoutDemo -> a.b.c:\n",
+		"an id that is not a hash": "# compiler: R8\n# pg_map_id: release-7\n" + testAndroidMapping,
+		"an empty id":              "# compiler: R8\n# pg_map_id:\n" + testAndroidMapping,
+	}
+	for name, mapping := range cases {
+		assert.Emptyf(t, androidMapID(writeMappingFile(t, mapping)), "androidMapID of %s", name)
+	}
+
+	assert.Empty(t, androidMapID(filepath.Join(t.TempDir(), "nothing-here.txt")))
+}
+
+// The header ends at the first class, and what follows is tens of megabytes of a
+// build's own strings — including, in an app that has one, a class whose name would
+// read as a header line to anything still looking.
+func TestAndroidMapIDStopsAtTheFirstClass(t *testing.T) {
+	path := writeMappingFile(t, testAndroidMapping+"# pg_map_id: "+testMapID+"\n")
+	assert.Empty(t, androidMapID(path))
+}
+
+// The point of the id: a build that stamps nothing and is told nothing still uploads
+// to the lane its crashes will arrive on, because R8 recorded which mapping this is
+// and the shipped app reports the same thing on every frame.
+func TestBuildAndroidObjectsKeysByTheMapID(t *testing.T) {
+	path := writeMappingFile(t, testMapIDHeader+testAndroidMapping)
+
+	objects, err := buildAndroidObjects(path, "", "", false, "")
+	require.NoError(t, err)
+	require.Len(t, objects, 1)
+	assert.Equal(t, "_sym/android/id/"+testMapID+"/mapping.v1.index", objects[0].Key())
+	assert.True(t, objects[0].keyProvesContent, "the key is derived from the mapping it stores")
+}
+
+// An id the caller gave, or one read out of the packaged app, is a build saying what
+// it will report — which is the more specific answer, and the only one for a build
+// old enough that R8 stamps nothing into the app itself.
+func TestBuildAndroidObjectsPrefersAReportedID(t *testing.T) {
+	path := writeMappingFile(t, testMapIDHeader+testAndroidMapping)
+
+	objects, err := buildAndroidObjects(path, "", "deadbeef", false, "")
+	require.NoError(t, err)
+	require.Len(t, objects, 1)
+	assert.Equal(t, "_sym/android/id/deadbeef/mapping.v1.index", objects[0].Key())
+}

cmd/symbols/android_upload.go

@@ -126,15 +126,25 @@ func buildAndroidObjects(path, appVersion, symbolsID string, includeSources bool
 		return nil, err
 	}
 
-	lanes := androidLanes(build)
-	if len(lanes) == 0 {
-		return nil, fmt.Errorf("this build reports no symbols id and no app version, so there is no key a crash could be symbolicated under. Apply the LaunchDarkly Gradle plugin so the shipped app records its symbols id, or re-run with --%s <version>", appVersionFlag)
-	}
-
 	mapping, err := findAndroidMapping(build.Path)
 	if err != nil {
 		return nil, err
 	}
+	if build.SymbolsID == "" {
+		// R8's own id for this mapping, which the shipped app reports on every frame
+		// (see android_mapid.go). Below an id the app stamped, which is a build
+		// saying what it will report and so the more specific answer of the two.
+		if id := androidMapID(mapping); id != "" {
+			build.SymbolsID = id
+			fmt.Printf("Using symbols id %s, as recorded by R8 in the mapping\n", id)
+		}
+	}
+
+	lanes := androidLanes(build)
+	if len(lanes) == 0 {
+		return nil, fmt.Errorf("this mapping records no id of its own and the build reports no app version, so there is no key a crash could be symbolicated under. Re-run with --%s <version>", appVersionFlag)
+	}
+
 	index, err := buildAndroidIndex(mapping)
 	if err != nil {
 		return nil, err

cmd/symbols/apple_upload.go

@@ -54,13 +54,21 @@ func (m appleSymbolMap) label() string {
 // nothing. Source bundles borrow that UUID rather than being keyed by their own
 // contents — sources unreadable on the machine that uploaded first must still be able
 // to overwrite — so those are skipped only when their digest matches what is stored.
-func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources, skipExisting bool) error {
-	images, err := findDSYMImages(path)
+func uploadAppleDSYMs(apiKey, projectID string, upload appleUpload, backendURL string, includeSources, skipExisting bool) error {
+	images, err := findDSYMImages(upload.Path)
 	if err != nil {
 		return fmt.Errorf("failed to find dSYM files: %w", err)
 	}
 	if len(images) == 0 {
-		return fmt.Errorf("no .dSYM bundles found in %s, is this the correct path?", path)
+		if upload.FromXcode {
+			// Running from a build that produced no dSYM is ordinary — a Debug
+			// build's debug information stays in the binary — and a build phase
+			// that fails the build over it would be a phase every project has to
+			// guard. There is nothing to upload, which is not the same as an error.
+			fmt.Printf("This build produced no dSYM, so there is nothing to upload. Set Debug Information Format to \"DWARF with dSYM File\" for the configurations you ship.\n")
+			return nil
+		}
+		return fmt.Errorf("no .dSYM bundles found in %s, is this the correct path?", upload.Path)
 	}
 
 	maps, err := buildAppleMaps(images, includeSources)

cmd/symbols/apple_xcode.go

@@ -0,0 +1,50 @@
+package symbols
+
+import (
+	"fmt"
+	"os"
+)
+
+// Uploading from inside an Xcode build.
+//
+// The reliable moment to upload a dSYM is the build that produced it, which means
+// a Run Script phase. Xcode runs one with the whole build's settings in its
+// environment, so the phase can be told nothing and still know everything:
+// DWARF_DSYM_FOLDER_PATH is where this build put its dSYMs.
+//
+// Reading it here is what keeps the phase to a single line. Otherwise every
+// project writes the same two pieces of shell — a path that has to stay in step
+// with the project's configuration, and a guard against the configurations that
+// produce no dSYM — and gets to debug them inside a build phase, where the way you
+// find out is a failed build.
+
+// xcodeDSYMFolderEnv is the build setting naming the folder Xcode wrote this
+// build's dSYMs to. It is set for every configuration, including the ones whose
+// debug information format produces no dSYM at all.
+const xcodeDSYMFolderEnv = "DWARF_DSYM_FOLDER_PATH"
+
+// appleUpload is where an Apple upload reads its dSYMs from.
+type appleUpload struct {
+	Path string
+	// FromXcode records that the path came from the surrounding build rather than
+	// from the caller, which is what makes finding no dSYM there ordinary: it is
+	// the answer for a Debug build, and not something to fail over.
+	FromXcode bool
+}
+
+// resolveAppleUpload decides where to read dSYMs from. An explicit --path always
+// wins — a phase that wants to upload something other than what it just built can
+// still say so — and the build environment answers when there was no path to go on.
+func resolveAppleUpload(path string) appleUpload {
+	if path != defaultPath {
+		return appleUpload{Path: path}
+	}
+
+	folder := os.Getenv(xcodeDSYMFolderEnv)
+	if folder == "" {
+		return appleUpload{Path: path}
+	}
+
+	fmt.Printf("Using the dSYMs this Xcode build produced, from %s\n", folder)
+	return appleUpload{Path: folder, FromXcode: true}
+}

cmd/symbols/apple_xcode_test.go

@@ -0,0 +1,55 @@
+package symbols
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The point of reading the build environment: a phase that says nothing at all
+// still uploads what the build it is part of just produced.
+func TestResolveAppleUploadFromXcode(t *testing.T) {
+	t.Setenv(xcodeDSYMFolderEnv, "/dd/Build/Products/Release-iphoneos")
+
+	upload := resolveAppleUpload(defaultPath)
+	assert.Equal(t, "/dd/Build/Products/Release-iphoneos", upload.Path)
+	assert.True(t, upload.FromXcode)
+}
+
+// A phase that names a path means it: uploading a dSYM from somewhere other than
+// the build in progress has to stay possible.
+func TestResolveAppleUploadPrefersAnExplicitPath(t *testing.T) {
+	t.Setenv(xcodeDSYMFolderEnv, "/dd/Build/Products/Release-iphoneos")
+
+	upload := resolveAppleUpload("./archives/MyApp.xcarchive/dSYMs")
+	assert.Equal(t, "./archives/MyApp.xcarchive/dSYMs", upload.Path)
+	assert.False(t, upload.FromXcode)
+}
+
+func TestResolveAppleUploadOutsideXcode(t *testing.T) {
+	t.Setenv(xcodeDSYMFolderEnv, "")
+
+	upload := resolveAppleUpload(defaultPath)
+	assert.Equal(t, defaultPath, upload.Path)
+	assert.False(t, upload.FromXcode)
+}
+
+// Xcode sets the folder for every configuration, including the ones whose debug
+// information stays in the binary. Finding no dSYM there is the answer for a Debug
+// build, and a build phase that failed the build over it would be one every project
+// has to write a guard around.
+func TestUploadAppleDSYMsFromXcodeWithoutADSYM(t *testing.T) {
+	err := uploadAppleDSYMs("", "", appleUpload{Path: t.TempDir(), FromXcode: true}, "", false, false)
+	assert.NoError(t, err)
+}
+
+// Asked for a path with no dSYM under it, though, there is nothing else this could
+// have meant, so it is still reported.
+func TestUploadAppleDSYMsWithoutADSYM(t *testing.T) {
+	dir := t.TempDir()
+
+	err := uploadAppleDSYMs("", "", appleUpload{Path: dir}, "", false, false)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), dir)
+}

cmd/symbols/upload.go

@@ -224,8 +224,11 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error
 		// Apple dSYMs take a dedicated path: they are compiled to per-arch .dsymmap
 		// symbol maps keyed by build UUID, ignoring the version/symbols-id lanes.
 		if symbolType == typeAppleDSYM {
-			fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, path)
-			return uploadAppleDSYMs(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, backendUrl, viper.GetBool(includeSourcesFlag), skipExisting)
+			// A dSYM is best uploaded by the build that produced it, so where to
+			// read one from can come from the build itself. See apple_xcode.go.
+			upload := resolveAppleUpload(path)
+			fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, upload.Path)
+			return uploadAppleDSYMs(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, upload, backendUrl, viper.GetBool(includeSourcesFlag), skipExisting)
 		}
 
 		// Flutter/Dart symbols take a dedicated path too: each app.<platform>.symbols
@@ -700,10 +703,10 @@ func initFlags(cmd *cobra.Command) {
 	cmd.Flags().String(appVersionFlag, "", fmt.Sprintf("The current version of your deploy. With --type %s this is read from the packaged build when omitted", typeAndroid))
 	_ = viper.BindPFlag(appVersionFlag, cmd.Flags().Lookup(appVersionFlag))
 
-	cmd.Flags().String(symbolsIdFlag, "", fmt.Sprintf("The symbols id (launchdarkly.symbols_id.htlhash) to key uploads by (Symbols Id Lane). If omitted, a *.symbolsid sidecar next to the bundle is used when present, and with --type %s the id the packaged app reports", typeAndroid))
+	cmd.Flags().String(symbolsIdFlag, "", fmt.Sprintf("The symbols id (launchdarkly.symbols_id.htlhash) to key uploads by (Symbols Id Lane). If omitted, a *.symbolsid sidecar next to the bundle is used when present, and with --type %s the id the packaged app reports, or failing that the one R8 recorded in the mapping", typeAndroid))
 	_ = viper.BindPFlag(symbolsIdFlag, cmd.Flags().Lookup(symbolsIdFlag))
 
-	cmd.Flags().String(pathFlag, defaultPath, fmt.Sprintf("Sets the directory of where the symbol files are. With --type %s, run from your project root and the R8 mapping is found for you", typeAndroid))
+	cmd.Flags().String(pathFlag, defaultPath, fmt.Sprintf("Sets the directory of where the symbol files are. With --type %s, run from your project root and the R8 mapping is found for you; with --type %s, an Xcode build phase uploads what it just built", typeAndroid, typeAppleDSYM))
 	_ = viper.BindPFlag(pathFlag, cmd.Flags().Lookup(pathFlag))
 
 	cmd.Flags().String(basePathFlag, "", "An optional base path for the uploaded symbol files")