diff --git a/cmd/symbols/android_mapid.go b/cmd/symbols/android_mapid.go new file mode 100644 index 00000000..caf10092 --- /dev/null +++ b/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: " 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-" 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 "" +} diff --git a/cmd/symbols/android_mapid_test.go b/cmd/symbols/android_mapid_test.go new file mode 100644 index 00000000..b36081cc --- /dev/null +++ b/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()) +} diff --git a/cmd/symbols/android_upload.go b/cmd/symbols/android_upload.go index 0e684c70..1ccc5792 100644 --- a/cmd/symbols/android_upload.go +++ b/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 ", 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 ", appVersionFlag) + } + index, err := buildAndroidIndex(mapping) if err != nil { return nil, err diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index 79d30049..21cbf774 100644 --- a/cmd/symbols/apple_upload.go +++ b/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) diff --git a/cmd/symbols/apple_xcode.go b/cmd/symbols/apple_xcode.go new file mode 100644 index 00000000..75cdb922 --- /dev/null +++ b/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} +} diff --git a/cmd/symbols/apple_xcode_test.go b/cmd/symbols/apple_xcode_test.go new file mode 100644 index 00000000..d0cde2d3 --- /dev/null +++ b/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) +} diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index 157a5457..980b95e3 100644 --- a/cmd/symbols/upload.go +++ b/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..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")