From bb4d442326bfa067cd2bc3e52753492f2c3da41c Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 18:02:09 -0700 Subject: [PATCH 01/10] feat(symbols): find the Android mapping and app version in the build `ldcli symbols upload --type android` needed a --path pointing at a directory holding mapping.txt and an --app-version matching what the app reports, so every project had to add build script code to stage the mapping somewhere and plumb its version into CI. The Android Gradle Plugin already writes both: R8's mapping at /build/outputs/mapping//mapping.txt, and the version it packaged in output-metadata.json beside the APK. Read them, and the command works from an Android project root with no flags and no build script at all. Several obfuscated variants is an error naming them rather than a guess, since only one of them is the build being shipped. An Android mapping is now also stored as mapping.txt however deep it was found. Symbolication reads it at /mapping.txt, so a mapping uploaded from a nested path was previously keyed somewhere nothing looks. Co-authored-by: Cursor --- cmd/symbols/android_discover.go | 194 +++++++++++++++++++++++++ cmd/symbols/android_discover_test.go | 208 +++++++++++++++++++++++++++ cmd/symbols/upload.go | 27 +++- 3 files changed, 426 insertions(+), 3 deletions(-) create mode 100644 cmd/symbols/android_discover.go create mode 100644 cmd/symbols/android_discover_test.go diff --git a/cmd/symbols/android_discover.go b/cmd/symbols/android_discover.go new file mode 100644 index 00000000..dcd69124 --- /dev/null +++ b/cmd/symbols/android_discover.go @@ -0,0 +1,194 @@ +package symbols + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// Android build discovery. +// +// The Android Gradle Plugin already writes everything an upload needs, in +// well-known places: R8's mapping at +// /build/outputs/mapping//mapping.txt, and the version that +// shipped in /build/outputs/{apk,bundle}/**/output-metadata.json. Reading +// both here means `ldcli symbols upload --type android` works from an Android +// project root with no --path, no --app-version, and no build script staging +// files for it. + +const ( + // androidOutputMetadataName is the JSON manifest AGP writes beside each + // packaged APK/AAB, recording the variant and the versionName/versionCode it + // was built with. + androidOutputMetadataName = "output-metadata.json" + + // androidMappingGlobs are the paths R8 writes a mapping to, relative to the + // directory being searched: a module's own build dir, then one and two levels + // of nesting so a project root or a grouped module (features/foo) both match. + // Deliberately a fixed set of shapes rather than a recursive walk, so an + // unrelated mapping.txt somewhere in the tree can't be mistaken for a build. + androidMappingGlob1 = "build/outputs/mapping/*/" + androidMappingFileName + androidMappingGlob2 = "*/build/outputs/mapping/*/" + androidMappingFileName + androidMappingGlob3 = "*/*/build/outputs/mapping/*/" + androidMappingFileName +) + +// resolveAndroidBuild fills in from the build itself whatever the command line +// left out: the mapping to upload, and the version to key it by. Both are +// returned unchanged when path is not an AGP output tree, or when the caller +// already said which mapping and version it means. +func resolveAndroidBuild(path, appVersion string) (string, string, error) { + info, err := os.Stat(path) + if err != nil || !info.IsDir() { + // A path naming one file is already an answer; a path naming nothing is + // reported by the ordinary search, whose error says what was looked for. + return path, appVersion, nil + } + + build, err := discoverAndroidBuild(path) + if err != nil || build == nil { + return path, appVersion, err + } + + fmt.Printf("Found the %s mapping at %s\n", build.Variant, build.MappingPath) + if appVersion == "" { + if version := build.AppVersion(); version != "" { + appVersion = version + fmt.Printf("Using app version %s, as packaged for %s\n", version, build.Variant) + } + } + return build.MappingPath, appVersion, nil +} + +// androidBuild is one obfuscated variant found in an AGP output tree. +type androidBuild struct { + // MappingPath is the mapping.txt R8 produced for the variant. + MappingPath string + // Variant is the AGP variant name (composeRelease), which is both the + // directory the mapping sits in and how output-metadata.json identifies a build. + Variant string + // BuildDir is the module's build directory, the root of the outputs tree the + // mapping and the packaged app share. + BuildDir string +} + +// discoverAndroidBuild finds the obfuscated build under root, so a mapping never +// has to be pointed at or copied somewhere for upload. +// +// Returns nil when root is not an AGP output tree, which leaves the caller on its +// ordinary search: this only adds a shortcut for the conventional layout, it does +// not take away the ability to upload a mapping from anywhere. Several variants +// is an error rather than a guess, since each is a different app build and only +// one of them is the one being shipped. +func discoverAndroidBuild(root string) (*androidBuild, error) { + builds, err := findAndroidBuilds(root) + if err != nil || len(builds) == 0 { + return nil, err + } + if len(builds) > 1 { + paths := make([]string, 0, len(builds)) + for _, b := range builds { + paths = append(paths, fmt.Sprintf(" %s (%s)", b.MappingPath, b.Variant)) + } + return nil, fmt.Errorf( + "found %d obfuscated Android variants under %s; pass --%s to pick the one you are shipping:\n%s", + len(builds), root, pathFlag, strings.Join(paths, "\n"), + ) + } + return builds[0], nil +} + +// findAndroidBuilds returns every variant with a non-empty mapping under root, +// ordered by path so the ambiguity error reads the same on every run. An empty +// mapping is skipped: AGP writes one for a variant R8 left unobfuscated, and it +// would retrace nothing. +func findAndroidBuilds(root string) ([]*androidBuild, error) { + var matches []string + for _, glob := range []string{androidMappingGlob1, androidMappingGlob2, androidMappingGlob3} { + found, err := filepath.Glob(filepath.Join(root, glob)) + if err != nil { + // The globs are constants, so the only error the pattern can raise is + // impossible here; surfacing it keeps that assumption honest. + return nil, err + } + matches = append(matches, found...) + } + sort.Strings(matches) + + builds := make([]*androidBuild, 0, len(matches)) + for _, mappingPath := range matches { + info, err := os.Stat(mappingPath) + if err != nil || info.IsDir() || info.Size() == 0 { + continue + } + variantDir := filepath.Dir(mappingPath) + builds = append(builds, &androidBuild{ + MappingPath: mappingPath, + Variant: filepath.Base(variantDir), + // /outputs/mapping//mapping.txt, so the build dir is + // three levels up from the variant directory. + BuildDir: filepath.Dir(filepath.Dir(filepath.Dir(variantDir))), + }) + } + return builds, nil +} + +// AppVersion returns the versionName the variant was packaged with, or "" when +// the app has not been packaged (mapping but no APK/AAB) or AGP recorded no +// version. It is the same string the app reports at runtime, which is what makes +// it usable as the Version Lane key. +// +// Note this is the app's version, and the SDK only reports it when the app passes +// it as the observability service version; a build that leaves that at its default +// reports the SDK's version instead, and needs a symbols id to be symbolicated. +func (b *androidBuild) AppVersion() string { + for _, metadataPath := range b.outputMetadataPaths() { + content, err := os.ReadFile(metadataPath) + if err != nil { + continue + } + var metadata struct { + VariantName string `json:"variantName"` + Elements []struct { + VersionName string `json:"versionName"` + } `json:"elements"` + } + if err := json.Unmarshal(content, &metadata); err != nil { + continue + } + // A module builds many variants into one outputs tree, so the metadata has + // to name this one: taking another variant's version would key the upload + // to a build these symbols do not describe. + if metadata.VariantName != b.Variant { + continue + } + for _, element := range metadata.Elements { + if element.VersionName != "" { + return element.VersionName + } + } + } + return "" +} + +// outputMetadataPaths are where AGP writes packaging metadata, which is under the +// APK's flavor/buildType directories rather than the variant directory the mapping +// uses — hence the wildcards, with the variant identified by the file's contents. +func (b *androidBuild) outputMetadataPaths() []string { + var paths []string + for _, glob := range []string{ + filepath.Join("outputs", "apk", "*", "*", androidOutputMetadataName), + filepath.Join("outputs", "apk", "*", androidOutputMetadataName), + filepath.Join("outputs", "bundle", "*", androidOutputMetadataName), + } { + found, err := filepath.Glob(filepath.Join(b.BuildDir, glob)) + if err != nil { + continue + } + paths = append(paths, found...) + } + sort.Strings(paths) + return paths +} diff --git a/cmd/symbols/android_discover_test.go b/cmd/symbols/android_discover_test.go new file mode 100644 index 00000000..8ee7a19e --- /dev/null +++ b/cmd/symbols/android_discover_test.go @@ -0,0 +1,208 @@ +package symbols + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writeAndroidMapping lays out the AGP output an obfuscated build leaves behind: +// module/build/outputs/mapping//mapping.txt. +func writeAndroidMapping(t *testing.T, root, module, variant, content string) string { + t.Helper() + dir := filepath.Join(root, module, "build", "outputs", "mapping", variant) + require.NoError(t, os.MkdirAll(dir, 0o755)) + path := filepath.Join(dir, androidMappingFileName) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + return path +} + +// writeAndroidOutputMetadata lays out what AGP records when it packages the app, +// under the flavor/buildType directories the APK is written to. +func writeAndroidOutputMetadata(t *testing.T, root, module, variant, versionName string, apkDirs ...string) { + t.Helper() + dir := filepath.Join(append([]string{root, module, "build", "outputs", "apk"}, apkDirs...)...) + require.NoError(t, os.MkdirAll(dir, 0o755)) + metadata := `{ + "version": 3, + "artifactType": {"type": "APK", "kind": "Directory"}, + "applicationId": "com.example.app", + "variantName": "` + variant + `", + "elements": [{"type": "SINGLE", "versionCode": 7, "versionName": "` + versionName + `", "outputFile": "app.apk"}] + }` + require.NoError(t, os.WriteFile(filepath.Join(dir, androidOutputMetadataName), []byte(metadata), 0o644)) +} + +func TestDiscoverAndroidBuildFromProjectRoot(t *testing.T) { + root := t.TempDir() + mapping := writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, mapping, build.MappingPath) + assert.Equal(t, "composeRelease", build.Variant) + assert.Equal(t, filepath.Join(root, "app", "build"), build.BuildDir) +} + +// Someone in the module directory rather than the project root is pointing at the +// same build, and gets the same answer. +func TestDiscoverAndroidBuildFromModuleDirectory(t *testing.T) { + root := t.TempDir() + mapping := writeAndroidMapping(t, root, "app", "release", "com.example.App -> a.a:\n") + + build, err := discoverAndroidBuild(filepath.Join(root, "app")) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, mapping, build.MappingPath) + assert.Equal(t, "release", build.Variant) +} + +func TestDiscoverAndroidBuildFromNestedModule(t *testing.T) { + root := t.TempDir() + mapping := writeAndroidMapping(t, root, filepath.Join("features", "checkout"), "release", "com.example.App -> a.a:\n") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, mapping, build.MappingPath) +} + +// Each variant is a different app, and uploading the wrong one symbolicates every +// frame into the wrong place, so this asks rather than picks. +func TestDiscoverAndroidBuildRefusesToGuessBetweenVariants(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidMapping(t, root, "app", "javaRelease", "com.example.App -> a.a:\n") + + _, err := discoverAndroidBuild(root) + require.Error(t, err) + assert.Contains(t, err.Error(), "composeRelease") + assert.Contains(t, err.Error(), "javaRelease") + assert.Contains(t, err.Error(), "--"+pathFlag) +} + +// R8 writes an empty mapping for a variant it did not obfuscate. There is nothing +// to retrace with, so it is not a build worth finding — and finding it would make +// a real variant beside it look ambiguous. +func TestDiscoverAndroidBuildIgnoresEmptyMapping(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "debug", "") + mapping := writeAndroidMapping(t, root, "app", "release", "com.example.App -> a.a:\n") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, mapping, build.MappingPath) +} + +// A mapping.txt that is not in an AGP output tree — one staged by a build script, +// say — is left to the ordinary search rather than claimed here. +func TestDiscoverAndroidBuildIgnoresUnconventionalLayout(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "app", "build", "symbols", "composeRelease") + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, androidMappingFileName), []byte("x -> a:\n"), 0o644)) + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + assert.Nil(t, build) +} + +func TestAndroidBuildAppVersion(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, "1.0.1", build.AppVersion()) +} + +// Without product flavors the APK lands one directory shallower. +func TestAndroidBuildAppVersionWithoutFlavors(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "release", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "release", "2.3.4", "release") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, "2.3.4", build.AppVersion()) +} + +// One outputs tree holds every variant the module has ever packaged. Taking a +// version from the wrong one would key these symbols to a build they do not describe. +func TestAndroidBuildAppVersionIgnoresOtherVariants(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "javaRelease", "9.9.9", "java", "release") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Empty(t, build.AppVersion()) +} + +// A mapping with no packaged app beside it still uploads; it just has no version +// to be keyed by. +func TestAndroidBuildAppVersionMissing(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Empty(t, build.AppVersion()) +} + +func TestResolveAndroidBuildFillsInPathAndVersion(t *testing.T) { + root := t.TempDir() + mapping := writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + + path, version, err := resolveAndroidBuild(root, "") + require.NoError(t, err) + assert.Equal(t, mapping, path) + assert.Equal(t, "1.0.1", version) +} + +// What the caller asked for wins: an explicit version is the one the app reports, +// whatever the build was packaged as. +func TestResolveAndroidBuildKeepsExplicitVersion(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + + _, version, err := resolveAndroidBuild(root, "2.0.0") + require.NoError(t, err) + assert.Equal(t, "2.0.0", version) +} + +// An explicit --path names the mapping, so there is nothing to discover. +func TestResolveAndroidBuildLeavesExplicitFileAlone(t *testing.T) { + root := t.TempDir() + mapping := writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + + path, version, err := resolveAndroidBuild(mapping, "") + require.NoError(t, err) + assert.Equal(t, mapping, path) + assert.Empty(t, version) +} + +// The mapping is stored as mapping.txt however deep it was found, because that is +// where symbolication reads it. +func TestGetAllSymbolFilesAndroidFlattensName(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + + files, err := getAllSymbolFiles(root, typeAndroid) + require.NoError(t, err) + require.Len(t, files, 1) + assert.Equal(t, androidMappingFileName, files[0].Name) + assert.Equal(t, "1.2.3/mapping.txt", getS3Key(androidSymbolsIDPrefix, "", "1.2.3", "", files[0].Name)) +} diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index dce58f72..fe3d4285 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -235,6 +235,14 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return uploadFlutterSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, backendUrl, skipExisting) } + // An Android project already knows where R8 put the mapping and what version + // it shipped, so read both out of the build instead of asking for them. + if symbolType == typeAndroid { + if path, appVersion, err = resolveAndroidBuild(path, appVersion); err != nil { + return err + } + } + symbolsIDPrefix := symbolsIDPrefixForType(symbolType) fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, path) @@ -430,6 +438,15 @@ func isSymbolUploadFile(symbolType, name string) bool { return isReactNativeUploadFile(name) } +// uploadName is the name an artifact is stored under, given its path relative to +// the directory searched. +func uploadName(symbolType, relPath string) string { + if symbolType == typeAndroid { + return filepath.Base(relPath) + } + return relPath +} + func getAllSymbolFiles(path, symbolType string) ([]SymbolFile, error) { var files []SymbolFile @@ -468,7 +485,11 @@ func getAllSymbolFiles(path, symbolType string) ([]SymbolFile, error) { files = append(files, SymbolFile{ Path: filePath, - Name: relPath, + // Symbolication reads an Android mapping at /mapping.txt, so + // the object is named for the file alone however deep it was found. + // A React Native bundle keeps its path, which is part of how a map + // is matched to the bundle that references it. + Name: uploadName(symbolType, relPath), }) } @@ -690,13 +711,13 @@ func initFlags(cmd *cobra.Command) { _ = cmd.Flags().SetAnnotation(cliflags.ProjectFlag, "required", []string{"true"}) _ = viper.BindPFlag(cliflags.ProjectFlag, cmd.Flags().Lookup(cliflags.ProjectFlag)) - cmd.Flags().String(appVersionFlag, "", "The current version of your deploy") + 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, "", "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") _ = viper.BindPFlag(symbolsIdFlag, cmd.Flags().Lookup(symbolsIdFlag)) - cmd.Flags().String(pathFlag, defaultPath, "Sets the directory of where the symbol files are") + 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)) _ = viper.BindPFlag(pathFlag, cmd.Flags().Lookup(pathFlag)) cmd.Flags().String(basePathFlag, "", "An optional base path for the uploaded symbol files") From 6bdb31ae979be0ecbe7f5dffa74060f0b2a14e00 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 18:05:48 -0700 Subject: [PATCH 02/10] feat(symbols): read the Android symbols id out of the packaged app A build that stamps a content-derived symbols id into assets/ld_symbols_id.txt had to also stage a mapping.txt.symbolsid sidecar next to a copy of the mapping, purely so the upload could be keyed by the same id the app reports. The packaged APK/AAB already carries that asset, and it is the app that will run: what it carries is exactly what will be reported. Reading it there needs nothing handed over by the build and leaves no way for the two to disagree, so the staging step goes away and a stamped build uploads on the Symbols Id Lane with no flags. An id that does not match the 32-hex-char shape the SDK reports is ignored rather than keyed on, since it names a lane nothing would ever ask for. Co-authored-by: Cursor --- cmd/symbols/android_discover.go | 111 +++++++++++------- cmd/symbols/android_discover_test.go | 16 +-- cmd/symbols/android_symbolsid.go | 87 ++++++++++++++ cmd/symbols/android_symbolsid_test.go | 161 ++++++++++++++++++++++++++ cmd/symbols/upload.go | 13 ++- 5 files changed, 333 insertions(+), 55 deletions(-) create mode 100644 cmd/symbols/android_symbolsid.go create mode 100644 cmd/symbols/android_symbolsid_test.go diff --git a/cmd/symbols/android_discover.go b/cmd/symbols/android_discover.go index dcd69124..cf118fdb 100644 --- a/cmd/symbols/android_discover.go +++ b/cmd/symbols/android_discover.go @@ -35,31 +35,44 @@ const ( androidMappingGlob3 = "*/*/build/outputs/mapping/*/" + androidMappingFileName ) +// androidUpload is what an Android upload has to know about a build: which +// mapping to send, and how symbolication will look it up again. +type androidUpload struct { + Path string + AppVersion string + SymbolsID string +} + // resolveAndroidBuild fills in from the build itself whatever the command line -// left out: the mapping to upload, and the version to key it by. Both are -// returned unchanged when path is not an AGP output tree, or when the caller -// already said which mapping and version it means. -func resolveAndroidBuild(path, appVersion string) (string, string, error) { - info, err := os.Stat(path) +// left out. Everything is returned unchanged when the path is not an AGP output +// tree, or when the caller already said what it means. +func resolveAndroidBuild(upload androidUpload) (androidUpload, error) { + info, err := os.Stat(upload.Path) if err != nil || !info.IsDir() { // A path naming one file is already an answer; a path naming nothing is // reported by the ordinary search, whose error says what was looked for. - return path, appVersion, nil + return upload, nil } - build, err := discoverAndroidBuild(path) + build, err := discoverAndroidBuild(upload.Path) if err != nil || build == nil { - return path, appVersion, err + return upload, err } + upload.Path = build.MappingPath fmt.Printf("Found the %s mapping at %s\n", build.Variant, build.MappingPath) - if appVersion == "" { + if upload.AppVersion == "" { if version := build.AppVersion(); version != "" { - appVersion = version + upload.AppVersion = version fmt.Printf("Using app version %s, as packaged for %s\n", version, build.Variant) } } - return build.MappingPath, appVersion, nil + if upload.SymbolsID == "" { + // Read from the app that ships rather than derived here, so the id keyed on + // is the id reported. See android_symbolsid.go. + upload.SymbolsID = build.SymbolsID() + } + return upload, nil } // androidBuild is one obfuscated variant found in an AGP output tree. @@ -144,7 +157,43 @@ func findAndroidBuilds(root string) ([]*androidBuild, error) { // it as the observability service version; a build that leaves that at its default // reports the SDK's version instead, and needs a symbols id to be symbolicated. func (b *androidBuild) AppVersion() string { - for _, metadataPath := range b.outputMetadataPaths() { + for _, app := range b.packagedApps() { + if app.VersionName != "" { + return app.VersionName + } + } + return "" +} + +// androidPackagedApp is an APK or AAB AGP built for the variant, named by the +// output metadata written beside it. +type androidPackagedApp struct { + Path string + VersionName string +} + +// packagedApps reads AGP's packaging metadata for this variant. The metadata is +// written under the APK's flavor/buildType directories rather than the variant +// directory the mapping uses — hence the wildcards, with the variant identified +// by each file's contents. A module builds every variant into one outputs tree, +// so reading another's would describe a different app. +func (b *androidBuild) packagedApps() []androidPackagedApp { + var metadataPaths []string + for _, glob := range []string{ + filepath.Join("outputs", "apk", "*", "*", androidOutputMetadataName), + filepath.Join("outputs", "apk", "*", androidOutputMetadataName), + filepath.Join("outputs", "bundle", "*", androidOutputMetadataName), + } { + found, err := filepath.Glob(filepath.Join(b.BuildDir, glob)) + if err != nil { + continue + } + metadataPaths = append(metadataPaths, found...) + } + sort.Strings(metadataPaths) + + var apps []androidPackagedApp + for _, metadataPath := range metadataPaths { content, err := os.ReadFile(metadataPath) if err != nil { continue @@ -153,42 +202,20 @@ func (b *androidBuild) AppVersion() string { VariantName string `json:"variantName"` Elements []struct { VersionName string `json:"versionName"` + OutputFile string `json:"outputFile"` } `json:"elements"` } - if err := json.Unmarshal(content, &metadata); err != nil { - continue - } - // A module builds many variants into one outputs tree, so the metadata has - // to name this one: taking another variant's version would key the upload - // to a build these symbols do not describe. - if metadata.VariantName != b.Variant { + if err := json.Unmarshal(content, &metadata); err != nil || metadata.VariantName != b.Variant { continue } for _, element := range metadata.Elements { - if element.VersionName != "" { - return element.VersionName + app := androidPackagedApp{VersionName: element.VersionName} + if element.OutputFile != "" { + // outputFile is recorded relative to the metadata, as a bare name. + app.Path = filepath.Join(filepath.Dir(metadataPath), filepath.Base(element.OutputFile)) } + apps = append(apps, app) } } - return "" -} - -// outputMetadataPaths are where AGP writes packaging metadata, which is under the -// APK's flavor/buildType directories rather than the variant directory the mapping -// uses — hence the wildcards, with the variant identified by the file's contents. -func (b *androidBuild) outputMetadataPaths() []string { - var paths []string - for _, glob := range []string{ - filepath.Join("outputs", "apk", "*", "*", androidOutputMetadataName), - filepath.Join("outputs", "apk", "*", androidOutputMetadataName), - filepath.Join("outputs", "bundle", "*", androidOutputMetadataName), - } { - found, err := filepath.Glob(filepath.Join(b.BuildDir, glob)) - if err != nil { - continue - } - paths = append(paths, found...) - } - sort.Strings(paths) - return paths + return apps } diff --git a/cmd/symbols/android_discover_test.go b/cmd/symbols/android_discover_test.go index 8ee7a19e..3d3e42ab 100644 --- a/cmd/symbols/android_discover_test.go +++ b/cmd/symbols/android_discover_test.go @@ -165,10 +165,10 @@ func TestResolveAndroidBuildFillsInPathAndVersion(t *testing.T) { mapping := writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") - path, version, err := resolveAndroidBuild(root, "") + resolved, err := resolveAndroidBuild(androidUpload{Path: root}) require.NoError(t, err) - assert.Equal(t, mapping, path) - assert.Equal(t, "1.0.1", version) + assert.Equal(t, mapping, resolved.Path) + assert.Equal(t, "1.0.1", resolved.AppVersion) } // What the caller asked for wins: an explicit version is the one the app reports, @@ -178,9 +178,9 @@ func TestResolveAndroidBuildKeepsExplicitVersion(t *testing.T) { writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") - _, version, err := resolveAndroidBuild(root, "2.0.0") + resolved, err := resolveAndroidBuild(androidUpload{Path: root, AppVersion: "2.0.0"}) require.NoError(t, err) - assert.Equal(t, "2.0.0", version) + assert.Equal(t, "2.0.0", resolved.AppVersion) } // An explicit --path names the mapping, so there is nothing to discover. @@ -188,10 +188,10 @@ func TestResolveAndroidBuildLeavesExplicitFileAlone(t *testing.T) { root := t.TempDir() mapping := writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") - path, version, err := resolveAndroidBuild(mapping, "") + resolved, err := resolveAndroidBuild(androidUpload{Path: mapping}) require.NoError(t, err) - assert.Equal(t, mapping, path) - assert.Empty(t, version) + assert.Equal(t, mapping, resolved.Path) + assert.Empty(t, resolved.AppVersion) } // The mapping is stored as mapping.txt however deep it was found, because that is diff --git a/cmd/symbols/android_symbolsid.go b/cmd/symbols/android_symbolsid.go new file mode 100644 index 00000000..602afaa6 --- /dev/null +++ b/cmd/symbols/android_symbolsid.go @@ -0,0 +1,87 @@ +package symbols + +import ( + "archive/zip" + "io" + "regexp" + "strings" +) + +// The symbols id an Android build reports. +// +// A release build stamps a content-derived id of its mapping into +// assets/ld_symbols_id.txt, and the SDK reports it as +// `launchdarkly.symbols_id.htlhash` so symbolication can find the mapping by +// content instead of by app version. Uploading on that lane means keying the +// object by the same id. +// +// The id is read out of the packaged app rather than from a file the build staged +// beside the mapping, because the packaged app is the thing that will run: what it +// carries is exactly what will be reported. That leaves nothing for a build script +// to hand over, and no way for the two to disagree. + +const ( + // androidSymbolsIDAsset is the asset the build writes the id to, packaged at + // assets/ in an APK and base/assets/ in an app bundle. + androidSymbolsIDAsset = "ld_symbols_id.txt" + + apkSymbolsIDEntry = "assets/" + androidSymbolsIDAsset + aabSymbolsIDEntry = "base/assets/" + androidSymbolsIDAsset + + // symbolsIDAssetMaxBytes caps how much of the asset is read. The id is 32 + // bytes; anything remotely larger is not one, and is not worth reading. + symbolsIDAssetMaxBytes = 128 +) + +// symbolsIDPattern is the shape of a symbols id: the first 16 bytes of a SHA-256 +// as lowercase hex. It matches the check the SDK applies before reporting one, so +// an id the app would discard is not uploaded to a lane nothing will ask for. +var symbolsIDPattern = regexp.MustCompile(`^[0-9a-f]{32}$`) + +// SymbolsID returns the symbols id the variant's packaged app will report, or "" +// when the build does not stamp one — in which case the mapping belongs on the +// Version Lane, as before. +func (b *androidBuild) SymbolsID() string { + for _, app := range b.packagedApps() { + if app.Path == "" { + continue + } + if id := symbolsIDFromPackagedApp(app.Path); id != "" { + return id + } + } + return "" +} + +// symbolsIDFromPackagedApp reads the stamped id out of an APK or AAB, both of +// which are zips. Best-effort: an app that stamps no id, or a file that cannot be +// read as one, yields "" so the upload falls back to the Version Lane rather than +// failing a build over it. +func symbolsIDFromPackagedApp(path string) string { + archive, err := zip.OpenReader(path) + if err != nil { + return "" + } + defer archive.Close() + + for _, file := range archive.File { + if file.Name != apkSymbolsIDEntry && file.Name != aabSymbolsIDEntry { + continue + } + reader, err := file.Open() + if err != nil { + return "" + } + content, err := io.ReadAll(io.LimitReader(reader, symbolsIDAssetMaxBytes)) + _ = reader.Close() + if err != nil { + return "" + } + id := strings.TrimSpace(string(content)) + if symbolsIDPattern.MatchString(id) { + return id + } + return "" + } + return "" +} diff --git a/cmd/symbols/android_symbolsid_test.go b/cmd/symbols/android_symbolsid_test.go new file mode 100644 index 00000000..3b714b63 --- /dev/null +++ b/cmd/symbols/android_symbolsid_test.go @@ -0,0 +1,161 @@ +package symbols + +import ( + "archive/zip" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writeZip builds an APK/AAB stand-in: both are zips, and only one entry matters +// here. +func writeZip(t *testing.T, path string, entries map[string]string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + file, err := os.Create(path) + require.NoError(t, err) + defer file.Close() + + archive := zip.NewWriter(file) + for name, content := range entries { + entry, err := archive.Create(name) + require.NoError(t, err) + _, err = entry.Write([]byte(content)) + require.NoError(t, err) + } + require.NoError(t, archive.Close()) +} + +// writeAndroidApk packages an app beside the metadata that names it, which is how +// the APK for a variant is found. +func writeAndroidApk(t *testing.T, root, module string, entries map[string]string, apkDirs ...string) string { + t.Helper() + path := filepath.Join(append([]string{root, module, "build", "outputs", "apk"}, append(apkDirs, "app.apk")...)...) + writeZip(t, path, entries) + return path +} + +const stampedSymbolsID = "7e0d66142a85de6c6b2850dcbba5f066" + +func TestSymbolsIDFromPackagedAPK(t *testing.T) { + path := filepath.Join(t.TempDir(), "app.apk") + writeZip(t, path, map[string]string{ + "AndroidManifest.xml": "binary", + apkSymbolsIDEntry: stampedSymbolsID + "\n", + }) + + assert.Equal(t, stampedSymbolsID, symbolsIDFromPackagedApp(path)) +} + +func TestSymbolsIDFromAppBundle(t *testing.T) { + path := filepath.Join(t.TempDir(), "app.aab") + writeZip(t, path, map[string]string{aabSymbolsIDEntry: stampedSymbolsID}) + + assert.Equal(t, stampedSymbolsID, symbolsIDFromPackagedApp(path)) +} + +// An app that does not stamp an id has nothing to be keyed by, and belongs on the +// Version Lane. +func TestSymbolsIDFromPackagedAppWithoutAsset(t *testing.T) { + path := filepath.Join(t.TempDir(), "app.apk") + writeZip(t, path, map[string]string{"AndroidManifest.xml": "binary"}) + + assert.Empty(t, symbolsIDFromPackagedApp(path)) +} + +// The SDK reports only a 32-hex-char id, so anything else names a lane that will +// never be asked for — and an id is part of a storage key, which is not somewhere +// arbitrary text from a file belongs. +func TestSymbolsIDFromPackagedAppRejectsMalformedID(t *testing.T) { + for _, id := range []string{ + "", + " ", + "not-a-symbols-id", + "7E0D66142A85DE6C6B2850DCBBA5F066", + "7e0d66142a85de6c6b2850dcbba5f0", + "../../../../etc/passwd", + } { + path := filepath.Join(t.TempDir(), "app.apk") + writeZip(t, path, map[string]string{apkSymbolsIDEntry: id}) + assert.Empty(t, symbolsIDFromPackagedApp(path), "expected rejected: %q", id) + } +} + +func TestSymbolsIDFromPackagedAppUnreadable(t *testing.T) { + dir := t.TempDir() + notAZip := filepath.Join(dir, "app.apk") + require.NoError(t, os.WriteFile(notAZip, []byte("this is not a zip"), 0o644)) + + assert.Empty(t, symbolsIDFromPackagedApp(notAZip)) + assert.Empty(t, symbolsIDFromPackagedApp(filepath.Join(dir, "missing.apk"))) +} + +func TestAndroidBuildSymbolsID(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + writeAndroidApk(t, root, "app", map[string]string{apkSymbolsIDEntry: stampedSymbolsID}, "compose", "release") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Equal(t, stampedSymbolsID, build.SymbolsID()) +} + +// Another variant's APK describes another build, whose id would send this mapping +// to a lane that build's app already occupies. +func TestAndroidBuildSymbolsIDIgnoresOtherVariants(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "javaRelease", "1.0.1", "java", "release") + writeAndroidApk(t, root, "app", map[string]string{apkSymbolsIDEntry: stampedSymbolsID}, "java", "release") + + build, err := discoverAndroidBuild(root) + require.NoError(t, err) + require.NotNil(t, build) + assert.Empty(t, build.SymbolsID()) +} + +func TestResolveAndroidBuildUsesStampedSymbolsID(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + writeAndroidApk(t, root, "app", map[string]string{apkSymbolsIDEntry: stampedSymbolsID}, "compose", "release") + + resolved, err := resolveAndroidBuild(androidUpload{Path: root}) + require.NoError(t, err) + assert.Equal(t, stampedSymbolsID, resolved.SymbolsID) + + // The id fully addresses the mapping, so it supersedes the version. + assert.Equal(t, + "_sym/android/id/"+stampedSymbolsID+"/mapping.txt", + getS3Key(androidSymbolsIDPrefix, resolved.SymbolsID, resolved.AppVersion, "", androidMappingFileName), + ) +} + +func TestResolveAndroidBuildKeepsExplicitSymbolsID(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + writeAndroidApk(t, root, "app", map[string]string{apkSymbolsIDEntry: stampedSymbolsID}, "compose", "release") + + const explicit = "0123456789abcdef0123456789abcdef" + resolved, err := resolveAndroidBuild(androidUpload{Path: root, SymbolsID: explicit}) + require.NoError(t, err) + assert.Equal(t, explicit, resolved.SymbolsID) +} + +// A mapping with no packaged app beside it still uploads, on the Version Lane. +func TestResolveAndroidBuildWithoutPackagedApp(t *testing.T) { + root := t.TempDir() + writeAndroidMapping(t, root, "app", "composeRelease", "com.example.App -> a.a:\n") + writeAndroidOutputMetadata(t, root, "app", "composeRelease", "1.0.1", "compose", "release") + + resolved, err := resolveAndroidBuild(androidUpload{Path: root}) + require.NoError(t, err) + assert.Empty(t, resolved.SymbolsID) + assert.Equal(t, "1.0.1", resolved.AppVersion) +} diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index fe3d4285..c371e5e7 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -235,12 +235,15 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return uploadFlutterSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, backendUrl, skipExisting) } - // An Android project already knows where R8 put the mapping and what version - // it shipped, so read both out of the build instead of asking for them. + // An Android project already knows where R8 put the mapping, what version it + // shipped, and which symbols id the app reports, so read all three out of + // the build instead of asking for them. if symbolType == typeAndroid { - if path, appVersion, err = resolveAndroidBuild(path, appVersion); err != nil { - return err + resolved, aErr := resolveAndroidBuild(androidUpload{Path: path, AppVersion: appVersion, SymbolsID: symbolsID}) + if aErr != nil { + return aErr } + path, appVersion, symbolsID = resolved.Path, resolved.AppVersion, resolved.SymbolsID } symbolsIDPrefix := symbolsIDPrefixForType(symbolType) @@ -714,7 +717,7 @@ 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, "", "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") + 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)) _ = 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)) From 1b66af86757bb9385f4654b68b481742e5004879 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 19:06:39 -0700 Subject: [PATCH 03/10] feat(symbols): gzip what an upload sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A release R8 mapping is tens of megabytes of text and every build pushes it again: the e2e app's is 61.3 MB, which gzips to 5.0 MB in under half a second. React Native source maps and Apple symbol maps compress on the same order. Each object is marked Content-Encoding: gzip, so it stays self-describing — storage returns the header on read, an HTTP client inflates it in transit, and the backend inflates whatever still arrives compressed. Artifacts held in memory are compressed before an upload URL is asked for rather than at the point of sending, because the digest that proves an object is already stored is compared against the ETag of the stored bytes; hashing the artifact instead would never match and every source bundle would be re-sent. Files are compressed as they are read, streamed through the compressor so a large mapping is never held in memory whole. Anything that comes out of gzip no smaller is sent as it is, so a .srcbundle, which already gzips its own entries, is not wrapped a second time. Co-authored-by: Cursor --- cmd/symbols/apple_upload.go | 33 ++++---- cmd/symbols/compress.go | 90 +++++++++++++++++++++ cmd/symbols/compress_test.go | 142 ++++++++++++++++++++++++++++++++++ cmd/symbols/flutter_upload.go | 4 +- cmd/symbols/upload.go | 61 +++++++++++++-- 5 files changed, 304 insertions(+), 26 deletions(-) create mode 100644 cmd/symbols/compress.go create mode 100644 cmd/symbols/compress_test.go diff --git a/cmd/symbols/apple_upload.go b/cmd/symbols/apple_upload.go index ef5f083f..79d30049 100644 --- a/cmd/symbols/apple_upload.go +++ b/cmd/symbols/apple_upload.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "io/fs" - "net/http" "os" "path/filepath" "strings" @@ -74,12 +73,16 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources keys := make([]string, len(maps)) digests := make([]string, len(maps)) + bodies := make([]uploadBody, len(maps)) for i, m := range maps { keys[i] = m.Key + // Compressed here rather than at the point of sending, so a digest below + // describes the bytes that get stored. + bodies[i] = compressBody(m.Data) if m.Kind == kindSources { // Sources are keyed by their image's UUID rather than by their own // contents, so only a digest can show that re-sending them is a no-op. - digests[i] = contentDigest(m.Data) + digests[i] = contentDigest(bodies[i].Data) } } @@ -100,7 +103,7 @@ func uploadAppleDSYMs(apiKey, projectID, path, backendURL string, includeSources skipped++ continue } - if err := uploadBytes(m.Data, uploadURLs[i], m.label()); err != nil { + if err := uploadBytes(bodies[i], uploadURLs[i], m.label()); err != nil { return fmt.Errorf("failed to upload symbol map for %s: %w", m.UUID, err) } } @@ -246,23 +249,19 @@ func archLabel(cpuType uint32) string { } } -func uploadBytes(data []byte, uploadURL, name string) error { - req, err := http.NewRequest("PUT", uploadURL, bytes.NewReader(data)) - if err != nil { +// uploadBytes sends an artifact built in memory. It takes an already-compressed +// body rather than raw bytes so that a caller which sends a digest hashes exactly +// what gets stored; see compress.go. +func uploadBytes(body uploadBody, uploadURL, name string) error { + if err := putObject(uploadURL, bytes.NewReader(body.Data), int64(len(body.Data)), body.Encoding); err != nil { return err } - client := &http.Client{} - resp, err := client.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("upload failed with status code: %d", resp.StatusCode) + if body.Encoding == gzipEncoding { + fmt.Printf("[LaunchDarkly] Uploaded symbol map %s (%s gzipped to %s)\n", + name, byteSize(int64(body.RawSize)), byteSize(int64(len(body.Data)))) + return nil } - - fmt.Printf("[LaunchDarkly] Uploaded symbol map %s\n", name) + fmt.Printf("[LaunchDarkly] Uploaded symbol map %s (%s)\n", name, byteSize(int64(len(body.Data)))) return nil } diff --git a/cmd/symbols/compress.go b/cmd/symbols/compress.go new file mode 100644 index 00000000..d7fa2023 --- /dev/null +++ b/cmd/symbols/compress.go @@ -0,0 +1,90 @@ +package symbols + +import ( + "bytes" + "compress/gzip" + "fmt" + "io" + "os" +) + +// Uploads are gzipped. +// +// A release R8 mapping is tens of megabytes of text, a React Native source map +// several, and every build sends one again; compressed, that is roughly an eighth +// of the bytes. The object is marked Content-Encoding: gzip, which keeps it +// self-describing: storage returns the header on read, an HTTP client inflates it +// in transit, and the symbolication reader inflates whatever still arrives +// compressed. +// +// Compression is settled before an upload URL is requested rather than at the point +// of sending, because the digest that proves an object is already stored has to be +// the digest of the bytes that get stored. + +// gzipEncoding is the Content-Encoding an upload is marked with. +const gzipEncoding = "gzip" + +// uploadBody is what a PUT sends: the bytes, and how they are encoded. +type uploadBody struct { + Data []byte + Encoding string + // RawSize is the artifact's size before compression, for reporting. + RawSize int +} + +// compressBody gzips an artifact held in memory, keeping it as it is when +// compressing gains nothing — a .srcbundle already gzips its own entries, and +// wrapping one again would only put a reader a step further from it. +func compressBody(data []byte) uploadBody { + compressed, err := gzipBytes(data) + if err != nil || len(compressed) >= len(data) { + return uploadBody{Data: data, RawSize: len(data)} + } + return uploadBody{Data: compressed, Encoding: gzipEncoding, RawSize: len(data)} +} + +func gzipBytes(data []byte) ([]byte, error) { + var buf bytes.Buffer + writer := gzip.NewWriter(&buf) + if _, err := writer.Write(data); err != nil { + return nil, err + } + if err := writer.Close(); err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +// gzipFile compresses a file on disk, streaming it through the compressor so that a +// mapping large enough to be worth compressing is never held in memory whole. +func gzipFile(path string) ([]byte, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + defer file.Close() + + var buf bytes.Buffer + writer := gzip.NewWriter(&buf) + if _, err := io.Copy(writer, file); err != nil { + return nil, err + } + if err := writer.Close(); err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +// byteSize renders a size the way upload progress reports it. +func byteSize(n int64) string { + const unit = 1024 + if n < unit { + return fmt.Sprintf("%d B", n) + } + div, exp := int64(unit), 0 + for rest := n / unit; rest >= unit; rest /= unit { + div *= unit + exp++ + } + return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) +} diff --git a/cmd/symbols/compress_test.go b/cmd/symbols/compress_test.go new file mode 100644 index 00000000..6e46a8d6 --- /dev/null +++ b/cmd/symbols/compress_test.go @@ -0,0 +1,142 @@ +package symbols + +import ( + "bytes" + "compress/gzip" + "crypto/rand" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// receivedUpload is what a presigned PUT looked like on the wire. +type receivedUpload struct { + Encoding string + Length int64 + Body []byte +} + +// uploadTestServer stands in for the presigned URL, recording the one PUT made to it. +func uploadTestServer(t *testing.T) (*httptest.Server, *receivedUpload) { + t.Helper() + var got receivedUpload + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got.Encoding = r.Header.Get("Content-Encoding") + got.Length = r.ContentLength + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + got.Body = body + })) + t.Cleanup(srv.Close) + return srv, &got +} + +func ungzip(t *testing.T, data []byte) []byte { + t.Helper() + reader, err := gzip.NewReader(bytes.NewReader(data)) + require.NoError(t, err) + defer reader.Close() + out, err := io.ReadAll(reader) + require.NoError(t, err) + return out +} + +func TestUploadFileSendsGzip(t *testing.T) { + mapping := strings.Repeat("com.example.app.Class -> a.b.c:\n 12:12:void method() -> a\n", 500) + path := filepath.Join(t.TempDir(), androidMappingFileName) + require.NoError(t, os.WriteFile(path, []byte(mapping), 0o644)) + + srv, got := uploadTestServer(t) + stdout, _ := captureOutput(t, func() { + require.NoError(t, uploadFile(path, srv.URL, androidMappingFileName)) + }) + + assert.Equal(t, gzipEncoding, got.Encoding) + assert.Equal(t, mapping, string(ungzip(t, got.Body)), "the artifact has to survive the round trip") + assert.Less(t, len(got.Body), len(mapping)/10, "a mapping should compress by at least 10x") + // S3 will not take a chunked upload, so the length has to be known up front. + assert.Equal(t, int64(len(got.Body)), got.Length) + assert.Contains(t, stdout, "gzipped to") +} + +// An artifact that is already compressed can come out of gzip bigger. Storing that +// costs bytes and puts a reader a step further from the artifact, for nothing. +func TestUploadFileSendsIncompressibleFileAsIs(t *testing.T) { + incompressible := make([]byte, 8192) + _, err := rand.Read(incompressible) + require.NoError(t, err) + path := filepath.Join(t.TempDir(), "sources.srcbundle") + require.NoError(t, os.WriteFile(path, incompressible, 0o644)) + + srv, got := uploadTestServer(t) + _, _ = captureOutput(t, func() { + require.NoError(t, uploadFile(path, srv.URL, "sources.srcbundle")) + }) + + assert.Empty(t, got.Encoding) + assert.Equal(t, incompressible, got.Body) + assert.Equal(t, int64(len(incompressible)), got.Length) +} + +func TestUploadFileReportsAMissingFile(t *testing.T) { + srv, _ := uploadTestServer(t) + assert.Error(t, uploadFile(filepath.Join(t.TempDir(), "absent.txt"), srv.URL, "absent.txt")) +} + +// The digest that proves an object is already stored is compared against the ETag of +// the stored bytes, so it has to be taken after compression. Hashing the artifact +// instead would never match, and every source bundle would be re-sent forever. +func TestUploadBytesDigestDescribesTheStoredBytes(t *testing.T) { + body := compressBody([]byte(strings.Repeat("package com.example;\n", 500))) + require.Equal(t, gzipEncoding, body.Encoding, "this fixture is meant to compress") + + srv, got := uploadTestServer(t) + _, _ = captureOutput(t, func() { + require.NoError(t, uploadBytes(body, srv.URL, androidSourceBundleName)) + }) + + assert.Equal(t, gzipEncoding, got.Encoding) + assert.Equal(t, contentDigest(body.Data), contentDigest(got.Body)) +} + +func TestCompressBody(t *testing.T) { + data := []byte(strings.Repeat("com.example.App -> a.a:\n", 500)) + + body := compressBody(data) + assert.Equal(t, gzipEncoding, body.Encoding) + assert.Equal(t, len(data), body.RawSize) + assert.Equal(t, data, ungzip(t, body.Data)) +} + +// A .srcbundle gzips its own entries, so there is nothing left for another pass to +// take out and the artifact is sent as it is. +func TestCompressBodyKeepsIncompressibleData(t *testing.T) { + data := make([]byte, 4096) + _, err := rand.Read(data) + require.NoError(t, err) + + body := compressBody(data) + assert.Empty(t, body.Encoding) + assert.Equal(t, data, body.Data) +} + +func TestCompressBodyOnEmptyData(t *testing.T) { + body := compressBody(nil) + assert.Empty(t, body.Encoding) + assert.Empty(t, body.Data) +} + +func TestByteSize(t *testing.T) { + assert.Equal(t, "0 B", byteSize(0)) + assert.Equal(t, "512 B", byteSize(512)) + assert.Equal(t, "1.0 KB", byteSize(1024)) + assert.Equal(t, "1.5 MB", byteSize(1024*1024*3/2)) + assert.Equal(t, "61.3 MB", byteSize(64266122)) +} diff --git a/cmd/symbols/flutter_upload.go b/cmd/symbols/flutter_upload.go index ec0fab60..636e392a 100644 --- a/cmd/symbols/flutter_upload.go +++ b/cmd/symbols/flutter_upload.go @@ -76,7 +76,9 @@ func uploadFlutterSymbols(apiKey, projectID, path, appVersion, backendURL string skipped++ continue } - if err := uploadBytes(u.Data, uploadURLs[i], u.Label); err != nil { + // Nothing here carries a digest, so unlike the Apple uploader this can wait + // until a map is known to be going, and skip the work for one that isn't. + if err := uploadBytes(compressBody(u.Data), uploadURLs[i], u.Label); err != nil { return fmt.Errorf("failed to upload symbol map %s: %w", u.Label, err) } } diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index c371e5e7..a791e1ab 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -291,7 +291,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // mapping's id and not the bundle's, an object being there says nothing // about these sources — a source-only change keeps the mapping's id — so the // bundle is skipped only when its digest matches what is already stored. - var sourceBundle []byte + var sourceBundle *uploadBody if symbolType == typeAndroid && viper.GetBool(includeSourcesFlag) { sourceRoot := viper.GetString(sourcePathFlag) data, count, bErr := buildAndroidSourceBundle(sourceRoot) @@ -302,7 +302,10 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // Not fatal: the mapping alone still retraces, just without source. fmt.Printf("No .java/.kt sources found under %s; skipping source bundle\n", sourceRoot) } else { - sourceBundle = data + // Compressed here rather than at the point of sending, so the digest + // below describes the bytes that get stored. + body := compressBody(data) + sourceBundle = &body bundleName := filepath.Join(filepath.Dir(files[0].Name), androidSourceBundleName) s3Keys = append(s3Keys, getS3Key(symbolsIDPrefix, firstSymbolsID, appVersion, basePath, bundleName)) fmt.Printf("Built source bundle from %s (%d files, %d bytes)\n", sourceRoot, count, len(data)) @@ -314,7 +317,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error // and we never have to read a large mapping back off disk to hash it. digests := make([]string, len(s3Keys)) if sourceBundle != nil { - digests[len(s3Keys)-1] = contentDigest(sourceBundle) + digests[len(s3Keys)-1] = contentDigest(sourceBundle.Data) } uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, digests, backendUrl, skipExisting) @@ -344,7 +347,7 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error if alreadyUploaded(uploadUrls[len(files)]) { fmt.Printf("Skipping %s, already uploaded\n", androidSourceBundleName) skipped++ - } else if err := uploadBytes(sourceBundle, uploadUrls[len(files)], androidSourceBundleName); err != nil { + } else if err := uploadBytes(*sourceBundle, uploadUrls[len(files)], androidSourceBundleName); err != nil { return fmt.Errorf("failed to upload source bundle: %w", err) } } @@ -677,17 +680,61 @@ func requestSymbolUploadUrls(apiKey, projectID string, paths, digests []string, return urlsResp.Data.GetSymbolUploadUrls, nil } +// uploadFile sends a file on disk, gzipped. Nothing keyed to a file carries a +// digest — those keys are derived from the artifact's own contents — so unlike an +// artifact held in memory this can be compressed here, as it is read. func uploadFile(filePath, uploadUrl, name string) error { - fileContent, err := os.ReadFile(filePath) + info, err := os.Stat(filePath) if err != nil { return err } - req, err := http.NewRequest("PUT", uploadUrl, bytes.NewBuffer(fileContent)) + compressed, err := gzipFile(filePath) if err != nil { return err } + // Compressing an artifact that is already compressed can make it bigger; send + // the file as it is rather than pay to store the difference. + if int64(len(compressed)) >= info.Size() { + file, err := os.Open(filePath) + if err != nil { + return err + } + defer file.Close() + + if err := putObject(uploadUrl, file, info.Size(), ""); err != nil { + return err + } + fmt.Printf("[LaunchDarkly] Uploaded %s to %s (%s)\n", filePath, name, byteSize(info.Size())) + return nil + } + + if err := putObject(uploadUrl, bytes.NewReader(compressed), int64(len(compressed)), gzipEncoding); err != nil { + return err + } + fmt.Printf("[LaunchDarkly] Uploaded %s to %s (%s gzipped to %s)\n", + filePath, name, byteSize(info.Size()), byteSize(int64(len(compressed)))) + return nil +} + +// putObject PUTs a body to a presigned URL. +// +// The length is passed explicitly because S3 will not take a chunked upload, and +// net/http only measures a body it recognises — a file streamed from disk would +// otherwise be sent with no Content-Length at all. +func putObject(uploadURL string, body io.Reader, length int64, encoding string) error { + req, err := http.NewRequest("PUT", uploadURL, body) + if err != nil { + return err + } + req.ContentLength = length + if encoding != "" { + // Stored as object metadata and handed back on read, so the object says how + // to read it instead of the reader having to guess. + req.Header.Set("Content-Encoding", encoding) + } + client := &http.Client{} resp, err := client.Do(req) if err != nil { @@ -698,8 +745,6 @@ func uploadFile(filePath, uploadUrl, name string) error { if resp.StatusCode != http.StatusOK { return fmt.Errorf("upload failed with status code: %d", resp.StatusCode) } - - fmt.Printf("[LaunchDarkly] Uploaded %s to %s\n", filePath, name) return nil } From c63b50e08a0cf23e81f45524bedce13fe8d75d21 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 23:10:32 -0700 Subject: [PATCH 04/10] feat(symbols): build the R8 index in the CLI instead of uploading the mapping An Android upload sent R8's mapping.txt as it was, tens of megabytes of text, and the backend parsed it into a random-access index on the first crash of every build that ever crashed. The mapping is here, on the machine that just produced it, so the index is built here too and the text is never stored: `upload` and `generate` now write mapping.v1.index to the Symbols Id and Version lanes. The mapping is streamed through r8index.EncodeFrom rather than parsed into memory, so a build machine does not have to find hundreds of megabytes to produce a few. A mapping that yields no index is an error: symbolication reads only the index, so storing anything else would leave a build looking like it has symbols while every crash arrives obfuscated. internal/symbols/r8index is a verbatim copy of the backend's package, the way dsymmap and srcbundle already are, and srcbundle.Builder now compresses on add so that a per-class bundle costs its compressed size rather than its raw one. Co-authored-by: Cursor --- cmd/symbols/android_sources.go | 8 +- cmd/symbols/android_upload.go | 249 ++++++++++++++++++ cmd/symbols/android_upload_test.go | 272 +++++++++++++++++++ cmd/symbols/generate.go | 42 ++- cmd/symbols/upload.go | 120 ++------- cmd/symbols/upload_test.go | 5 - internal/symbols/r8index/codec_test.go | 324 +++++++++++++++++++++++ internal/symbols/r8index/encode.go | 194 ++++++++++++++ internal/symbols/r8index/parse.go | 336 ++++++++++++++++++++++++ internal/symbols/r8index/parse_test.go | 279 ++++++++++++++++++++ internal/symbols/r8index/r8index.go | 177 +++++++++++++ internal/symbols/r8index/read.go | 267 +++++++++++++++++++ internal/symbols/r8index/stream_test.go | 174 ++++++++++++ internal/symbols/srcbundle/srcbundle.go | 59 ++++- 14 files changed, 2388 insertions(+), 118 deletions(-) create mode 100644 cmd/symbols/android_upload.go create mode 100644 cmd/symbols/android_upload_test.go create mode 100644 internal/symbols/r8index/codec_test.go create mode 100644 internal/symbols/r8index/encode.go create mode 100644 internal/symbols/r8index/parse.go create mode 100644 internal/symbols/r8index/parse_test.go create mode 100644 internal/symbols/r8index/r8index.go create mode 100644 internal/symbols/r8index/read.go create mode 100644 internal/symbols/r8index/stream_test.go diff --git a/cmd/symbols/android_sources.go b/cmd/symbols/android_sources.go index db0f3cba..05bb2cdf 100644 --- a/cmd/symbols/android_sources.go +++ b/cmd/symbols/android_sources.go @@ -13,9 +13,9 @@ import ( "github.com/launchdarkly/ldcli/internal/symbols/srcbundle" ) -// androidSourceBundleName is the object name of the source bundle uploaded beside -// an R8 mapping, so a build's mapping and its sources share one key prefix: -// _sym/android/id//mapping.txt and .../sources.srcbundle. +// androidSourceBundleName is the object name of the source bundle uploaded beside a +// build's mapping index, so an index and the sources behind it share one key prefix: +// _sym/android/id//mapping.v1.index and .../sources.srcbundle. const androidSourceBundleName = "sources.srcbundle" // androidSourceExtensions are the JVM source types R8 stack frames can point at. @@ -167,7 +167,7 @@ func androidSourceRank(sourceSet string) int { // androidTestSourceSets are the source sets holding test code. Both unit tests // and instrumented tests are excluded: neither is compiled into the app whose -// mapping.txt is being uploaded, so no retraced frame can point at them, and a +// mapping is being indexed, so no retraced frame can point at them, and a // test-only class sharing a production class's package and file name would // otherwise compete for its bundle key. var androidTestSourceSets = []string{"test", "androidTest"} diff --git a/cmd/symbols/android_upload.go b/cmd/symbols/android_upload.go new file mode 100644 index 00000000..0e684c70 --- /dev/null +++ b/cmd/symbols/android_upload.go @@ -0,0 +1,249 @@ +package symbols + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/launchdarkly/ldcli/internal/symbols/r8index" +) + +// Android symbol uploads. +// +// R8 writes a mapping.txt: tens of megabytes of text recording, for one build, what +// each obfuscated class, method and line was called in the source. Symbolication needs +// a few dozen of those answers per crash, so what gets uploaded is not the text but an +// index over it (internal/symbols/r8index), built here — on the machine that just +// produced the mapping and can read it once — rather than by the backend on the first +// crash of every build that ever crashes. +// +// A build's objects go to as many as two lanes. The Symbols Id Lane is keyed by the id +// the shipped app reports, which is what a crash arrives with. The Version Lane is +// keyed by the app version, and is what symbolication falls back to for a crash that +// reports no id. + +const ( + // androidIndexFileName is the object symbolication reads for an Android build. + // The format version is part of the name so that changing the format means + // writing a different object rather than teaching a reader to guess, and so that + // an index can be skipped on existence: for one mapping and one version of this + // format, the bytes can only be these. + androidIndexFileName = "mapping.v1.index" +) + +// androidLane is one storage prefix a build's objects are stored under. +type androidLane struct { + prefix string + label string + // keyProvesContent is true when the prefix is derived from the build the object + // came from, so an object already at the key can only be these same bytes. It is + // false for the Version Lane, where what is there is the last build's. + keyProvesContent bool +} + +func (l androidLane) key(name string) string { + return fmt.Sprintf("%s/%s", l.prefix, name) +} + +// androidObject is one artifact to store at one key. +type androidObject struct { + Lane androidLane + Name string + Data []byte + // keyProvesContent is the lane's rule narrowed to this object: a source bundle is + // keyed by the mapping's id rather than its own contents, so even in the Id Lane + // the key says nothing about which sources are stored under it. + keyProvesContent bool +} + +func (o androidObject) Key() string { return o.Lane.key(o.Name) } + +func (o androidObject) Label() string { return fmt.Sprintf("%s (%s)", o.Name, o.Lane.label) } + +// uploadAndroidSymbols indexes the R8 mapping for a build and uploads it, along with +// the app's sources when they were asked for. +func uploadAndroidSymbols(apiKey, projectID, path, appVersion, symbolsID, backendURL string, includeSources bool, sourceRoot string, skipExisting bool) error { + fmt.Printf("Starting to upload %s symbols from %s\n", typeAndroid, path) + + objects, err := buildAndroidObjects(path, appVersion, symbolsID, includeSources, sourceRoot) + if err != nil { + return err + } + + // Compressed before URLs are requested rather than at the point of sending, + // because a digest has to describe the bytes that get stored. Bodies are shared + // across lanes, so the same object is not compressed once per key. An index gzips + // its own blocks, so compressBody declines to wrap it again. + bodies := make(map[string]uploadBody, 2) + keys := make([]string, len(objects)) + digests := make([]string, len(objects)) + for i, object := range objects { + body, ok := bodies[object.Name] + if !ok { + body = compressBody(object.Data) + bodies[object.Name] = body + } + keys[i] = object.Key() + if !object.keyProvesContent { + digests[i] = contentDigest(body.Data) + } + } + + uploadURLs, err := getSymbolUploadUrls(apiKey, projectID, keys, digests, backendURL, skipExisting) + if err != nil { + return fmt.Errorf("failed to get upload URLs: %w", err) + } + // One URL per requested key, in order; a short list would misalign the pairing. + if len(uploadURLs) != len(objects) { + return fmt.Errorf("expected %d upload URLs but received %d", len(objects), len(uploadURLs)) + } + + skipped := 0 + for i, object := range objects { + if alreadyUploaded(uploadURLs[i]) { + fmt.Printf("Skipping %s, already uploaded\n", object.Label()) + skipped++ + continue + } + if err := uploadBytes(bodies[object.Name], uploadURLs[i], object.Label()); err != nil { + return fmt.Errorf("failed to upload %s: %w", object.Label(), err) + } + } + + reportUploadSummary(skipped) + return nil +} + +// buildAndroidObjects compiles the build's mapping into an index and pairs it with +// every key it has to be stored under, which is also what `symbols generate` writes. +func buildAndroidObjects(path, appVersion, symbolsID string, includeSources bool, sourceRoot string) ([]androidObject, error) { + // An Android project already knows where R8 put the mapping, what version it + // shipped, and which symbols id the app reports, so read all three out of the + // build instead of asking for them. + build, err := resolveAndroidBuild(androidUpload{Path: path, AppVersion: appVersion, SymbolsID: symbolsID}) + if err != nil { + 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 + } + index, err := buildAndroidIndex(mapping) + if err != nil { + return nil, err + } + + sources, err := androidSources(includeSources, sourceRoot) + if err != nil { + return nil, err + } + + var objects []androidObject + for i, lane := range lanes { + objects = append(objects, androidObject{ + Lane: lane, + Name: androidIndexFileName, + Data: index, + keyProvesContent: lane.keyProvesContent, + }) + + // Sources go on the lane symbolication resolves first, because that is the + // lane it reads them from: the index a frame was retraced with and the source + // shown behind it have to come from one upload, or a frame would carry this + // build's line numbers under another build's code. + if sources != nil && i == 0 { + objects = append(objects, androidObject{Lane: lane, Name: androidSourceBundleName, Data: sources}) + } + } + return objects, nil +} + +// androidLanes returns the lanes to store a build's objects under, in the order +// symbolication tries them. +func androidLanes(build androidUpload) []androidLane { + var lanes []androidLane + if build.SymbolsID != "" { + lanes = append(lanes, androidLane{ + prefix: fmt.Sprintf("%s/%s", androidSymbolsIDPrefix, build.SymbolsID), + label: "Symbols Id Lane", + keyProvesContent: true, + }) + } + if build.AppVersion != "" { + lanes = append(lanes, androidLane{prefix: build.AppVersion, label: "Version Lane"}) + } + return lanes +} + +// androidSources packs the app's sources into the bundle stored beside the index, so +// the errors page can show the code around each retraced frame. Off unless asked for: +// it ships source to LaunchDarkly. Finding none is not fatal — the index alone still +// retraces, just without the code behind a frame. +func androidSources(includeSources bool, sourceRoot string) ([]byte, error) { + if !includeSources { + return nil, nil + } + + data, count, err := buildAndroidSourceBundle(sourceRoot) + if err != nil { + return nil, err + } + if data == nil { + fmt.Printf("No .java/.kt sources found under %s; skipping source bundle\n", sourceRoot) + return nil, nil + } + + fmt.Printf("Built source bundle from %s (%d files, %s)\n", sourceRoot, count, byteSize(int64(len(data)))) + return data, nil +} + +// findAndroidMapping resolves a path to the one mapping to index. More than one is +// refused rather than chosen between: a build has a single mapping, and the keys above +// can only describe one of them. +func findAndroidMapping(path string) (string, error) { + files, err := getAllSymbolFiles(path, typeAndroid) + if err != nil { + return "", fmt.Errorf("failed to find symbol files: %w", err) + } + if len(files) > 1 { + found := make([]string, len(files)) + for i, file := range files { + found[i] = file.Path + } + return "", fmt.Errorf("found %d mappings under %s (%s), which cannot all be from one build; point --%s at the one that shipped", len(files), path, strings.Join(found, ", "), pathFlag) + } + return files[0].Path, nil +} + +// buildAndroidIndex compiles a mapping.txt into the index symbolication reads, +// streaming it off disk so that a release mapping is never held in memory whole. +func buildAndroidIndex(path string) ([]byte, error) { + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("failed to read %s: %w", path, err) + } + defer file.Close() + + index, err := r8index.EncodeFrom(file) + if err != nil { + // Reported here rather than fallen back from: symbolication reads only the + // index, so storing anything else would leave a build looking like it has + // symbols while every crash arrives obfuscated. + return nil, fmt.Errorf("failed to index %s: %w. Check that this is an R8/ProGuard mapping from a minified build", path, err) + } + + size := int64(0) + if info, err := file.Stat(); err == nil { + size = info.Size() + } + fmt.Printf("Indexed %s (%s of mapping into a %s index)\n", + filepath.Base(path), byteSize(size), byteSize(int64(len(index)))) + return index, nil +} diff --git a/cmd/symbols/android_upload_test.go b/cmd/symbols/android_upload_test.go new file mode 100644 index 00000000..624bef0b --- /dev/null +++ b/cmd/symbols/android_upload_test.go @@ -0,0 +1,272 @@ +package symbols + +import ( + "bytes" + "compress/gzip" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/launchdarkly/ldcli/internal/symbols/r8index" +) + +const testAndroidMapping = `# compiler: R8 +com.example.app.CheckoutDemo -> a.b.c: +# {"id":"sourceFile","fileName":"SymbolicationDemo.kt"} + 12:14:int startCheckout(java.lang.String):40:42 -> a +` + +// writeAndroidMapping puts a mapping on disk and returns its path, which is the input +// an Android upload starts from. +func writeMappingFile(t *testing.T, mapping string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, androidMappingFileName) + require.NoError(t, os.WriteFile(path, []byte(mapping), 0o644)) + return path +} + +// What the backend reads is the index, on both lanes: a build's own id, and the app +// version a crash without an id falls back to. The mapping text itself is never stored, +// because nothing reads it. +func TestBuildAndroidObjectsIndexesBothLanes(t *testing.T) { + path := writeMappingFile(t, testAndroidMapping) + + objects, err := buildAndroidObjects(path, "1.2.3", "deadbeef", false, "") + require.NoError(t, err) + require.Len(t, objects, 2) + + assert.Equal(t, "_sym/android/id/deadbeef/mapping.v1.index", objects[0].Key()) + assert.Equal(t, "1.2.3/mapping.v1.index", objects[1].Key()) + assert.Equal(t, objects[0].Data, objects[1].Data, "one build, one index") + + for _, object := range objects { + assert.NotContains(t, object.Key(), androidMappingFileName) + } + + // An object is only worth storing if it answers what symbolication asks. + ix, err := r8index.Open(objects[0].Data) + require.NoError(t, err) + frames := ix.Retrace("a.b.c", "a", 13) + require.Len(t, frames, 1) + assert.Equal(t, "com.example.app.CheckoutDemo", frames[0].Class) + assert.Equal(t, "startCheckout", frames[0].Method) + assert.Equal(t, 41, frames[0].Line) + assert.Equal(t, "SymbolicationDemo.kt", ix.SourceFile("com.example.app.CheckoutDemo")) +} + +// The Id Lane is the one symbolication tries first, so its key is the one that proves +// what is stored under it. A Version Lane object is the last build's. +func TestBuildAndroidObjectsOnlyIdLaneKeyProvesContent(t *testing.T) { + objects, err := buildAndroidObjects(writeMappingFile(t, testAndroidMapping), "1.2.3", "deadbeef", false, "") + require.NoError(t, err) + require.Len(t, objects, 2) + + assert.True(t, objects[0].keyProvesContent) + assert.False(t, objects[1].keyProvesContent) +} + +// Either lane on its own is a complete upload: an app that reports an id needs no +// version, and a build with no id is still symbolicated by the version it shipped. +func TestBuildAndroidObjectsOneLane(t *testing.T) { + path := writeMappingFile(t, testAndroidMapping) + + idOnly, err := buildAndroidObjects(path, "", "deadbeef", false, "") + require.NoError(t, err) + require.Len(t, idOnly, 1) + assert.Equal(t, "_sym/android/id/deadbeef/mapping.v1.index", idOnly[0].Key()) + + versionOnly, err := buildAndroidObjects(path, "1.2.3", "", false, "") + require.NoError(t, err) + require.Len(t, versionOnly, 1) + assert.Equal(t, "1.2.3/mapping.v1.index", versionOnly[0].Key()) +} + +// With neither an id nor a version there is no key a crash could ever look under, so +// the upload is refused rather than written somewhere nothing reads. +func TestBuildAndroidObjectsRequiresALane(t *testing.T) { + _, err := buildAndroidObjects(writeMappingFile(t, testAndroidMapping), "", "", false, "") + require.Error(t, err) + assert.Contains(t, err.Error(), appVersionFlag) +} + +// A mapping that yields no index is reported here. Uploading nothing usable would +// leave the build looking symbolicated while every crash arrives obfuscated. +func TestBuildAndroidObjectsRejectsUnusableMapping(t *testing.T) { + path := writeMappingFile(t, "this is not a mapping\n") + + _, err := buildAndroidObjects(path, "1.2.3", "deadbeef", false, "") + require.Error(t, err) + assert.Contains(t, err.Error(), androidMappingFileName) + assert.Contains(t, err.Error(), "minified") +} + +// Sources go on the lane symbolication resolves first, since that is where it looks +// for them; the fallback lane getting an index without sources still retraces. +func TestBuildAndroidObjectsPlacesSourcesOnTheFirstLane(t *testing.T) { + sourceRoot := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(sourceRoot, "main/java/com/example/app"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(sourceRoot, "main/java/com/example/app/CheckoutDemo.kt"), + []byte("package com.example.app\n\nobject CheckoutDemo\n"), 0o644)) + + objects, err := buildAndroidObjects(writeMappingFile(t, testAndroidMapping), "1.2.3", "deadbeef", true, sourceRoot) + require.NoError(t, err) + require.Len(t, objects, 3) + + assert.Equal(t, "_sym/android/id/deadbeef/mapping.v1.index", objects[0].Key()) + assert.Equal(t, "_sym/android/id/deadbeef/"+androidSourceBundleName, objects[1].Key()) + assert.Equal(t, "1.2.3/mapping.v1.index", objects[2].Key()) + + // The bundle is keyed by the mapping's id rather than its own contents, so its + // key cannot stand in for its bytes even in the Id Lane. + assert.False(t, objects[1].keyProvesContent) +} + +// Without --include-sources nothing but the index is stored. +func TestBuildAndroidObjectsWithoutSources(t *testing.T) { + objects, err := buildAndroidObjects(writeMappingFile(t, testAndroidMapping), "1.2.3", "deadbeef", false, t.TempDir()) + require.NoError(t, err) + for _, object := range objects { + assert.NotContains(t, object.Key(), androidSourceBundleName) + } +} + +// Two mappings cannot be from one build, and the keys describe one build, so the +// ambiguity is reported rather than resolved by picking. +func TestFindAndroidMappingRefusesSeveral(t *testing.T) { + dir := t.TempDir() + for _, variant := range []string{"release", "composeRelease"} { + sub := filepath.Join(dir, variant) + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, androidMappingFileName), []byte(testAndroidMapping), 0o644)) + } + + _, err := findAndroidMapping(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), pathFlag) +} + +// androidUploadServer answers the URL request with a presigned URL per key pointing at +// itself, and records what is PUT to each. skip names the keys to answer with "", +// which is how the backend says it already has one. +type androidUploadServer struct { + *httptest.Server + requested []capturedRequest + stored map[string][]byte + encodings map[string]string +} + +func newAndroidUploadServer(t *testing.T, skip ...string) *androidUploadServer { + t.Helper() + srv := &androidUploadServer{stored: map[string][]byte{}, encodings: map[string]string{}} + skipped := make(map[string]bool, len(skip)) + for _, key := range skip { + skipped[key] = true + } + + srv.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPut { + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + key := strings.TrimPrefix(r.URL.Path, "/put/") + srv.stored[key] = body + srv.encodings[key] = r.Header.Get("Content-Encoding") + return + } + + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + var captured capturedRequest + require.NoError(t, json.Unmarshal(body, &captured)) + srv.requested = append(srv.requested, captured) + + paths, ok := captured.Variables["paths"].([]interface{}) + require.True(t, ok, "a URL request always names the keys it wants") + urls := make([]string, 0, len(paths)) + for _, path := range paths { + key := path.(string) + if skipped[key] { + urls = append(urls, "") + continue + } + urls = append(urls, srv.URL+"/put/"+key) + } + + answer, err := json.Marshal(map[string]interface{}{ + "data": map[string]interface{}{"get_symbol_upload_urls_ld": urls}, + }) + require.NoError(t, err) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(answer) + })) + t.Cleanup(srv.Close) + return srv +} + +// End to end over a fake backend: the index reaches both lanes, and the Version Lane +// key — which proves nothing about its contents — carries a digest of exactly the bytes +// that were sent. +func TestUploadAndroidSymbolsSendsIndexToBothLanes(t *testing.T) { + srv := newAndroidUploadServer(t) + path := writeMappingFile(t, testAndroidMapping) + + _, _ = captureOutput(t, func() { + require.NoError(t, uploadAndroidSymbols("key", "proj", path, "1.2.3", "deadbeef", srv.URL, false, "", true)) + }) + + idKey := "_sym/android/id/deadbeef/mapping.v1.index" + versionKey := "1.2.3/mapping.v1.index" + require.Contains(t, srv.stored, idKey) + require.Contains(t, srv.stored, versionKey) + assert.Equal(t, srv.stored[idKey], srv.stored[versionKey]) + + // The stored bytes are the index, whatever the transport did to them. + stored := srv.stored[idKey] + if srv.encodings[idKey] == gzipEncoding { + stored = gunzip(t, stored) + } + _, err := r8index.Open(stored) + require.NoError(t, err) + + require.Len(t, srv.requested, 1) + digests, ok := srv.requested[0].Variables[digestsArgument].([]interface{}) + require.True(t, ok, "the Version Lane copy needs a digest to be settled by") + assert.Equal(t, "", digests[0], "the Id Lane key already proves what is under it") + assert.Equal(t, contentDigest(srv.stored[versionKey]), digests[1], + "the digest has to describe the bytes that get stored") +} + +// A build re-uploaded unchanged skips the Id Lane copy by key alone, and the Version +// Lane copy still goes: that key is a version, and only the digest could have settled +// it. +func TestUploadAndroidSymbolsSkipsWhatTheBackendHas(t *testing.T) { + idKey := "_sym/android/id/deadbeef/mapping.v1.index" + srv := newAndroidUploadServer(t, idKey) + path := writeMappingFile(t, testAndroidMapping) + + stdout, _ := captureOutput(t, func() { + require.NoError(t, uploadAndroidSymbols("key", "proj", path, "1.2.3", "deadbeef", srv.URL, false, "", true)) + }) + + assert.NotContains(t, srv.stored, idKey) + assert.Contains(t, srv.stored, "1.2.3/mapping.v1.index") + assert.Contains(t, stdout, "Skipping") +} + +func gunzip(t *testing.T, data []byte) []byte { + t.Helper() + reader, err := gzip.NewReader(bytes.NewReader(data)) + require.NoError(t, err) + out, err := io.ReadAll(reader) + require.NoError(t, err) + return out +} diff --git a/cmd/symbols/generate.go b/cmd/symbols/generate.go index 32b68a6b..834011f6 100644 --- a/cmd/symbols/generate.go +++ b/cmd/symbols/generate.go @@ -85,6 +85,12 @@ func generateRunE() func(cmd *cobra.Command, args []string) error { return generateFlutterSymbols(path, viper.GetString(appVersionFlag), outputDir) } + // An Android mapping compiles to the index symbolication reads, on the Id and + // Version lanes, which is why generating one is not a copy either. + if symbolType == typeAndroid { + return generateAndroidSymbols(path, outputDir) + } + return generateSymbolFiles(symbolType, path, outputDir) } } @@ -136,9 +142,36 @@ func generateFlutterSymbols(path, appVersion, outputDir string) error { return nil } -// generateSymbolFiles discovers React Native or Android artifacts and copies -// each one to outputDir under the same storage key `symbols upload` would use, -// so the generated folder matches what the backend expects. +// generateAndroidSymbols indexes the discovered R8 mapping and writes it under +// outputDir at the same storage keys `symbols upload` would store it at. +// +// The objects are written uncompressed: an upload gzips a body to send it, and a +// folder is read from disk rather than fetched. +func generateAndroidSymbols(path, outputDir string) error { + objects, err := buildAndroidObjects( + path, + viper.GetString(appVersionFlag), + viper.GetString(symbolsIdFlag), + viper.GetBool(includeSourcesFlag), + viper.GetString(sourcePathFlag), + ) + if err != nil { + return err + } + + for _, object := range objects { + if err := writeSymbolFile(outputDir, object.Key(), object.Data); err != nil { + return fmt.Errorf("failed to write %s: %w", object.Label(), err) + } + } + + fmt.Printf("Successfully generated %d symbol file(s) in %s\n", len(objects), outputDir) + return nil +} + +// generateSymbolFiles discovers React Native artifacts and copies each one to +// outputDir under the same storage key `symbols upload` would use, so the generated +// folder matches what the backend expects. func generateSymbolFiles(symbolType, path, outputDir string) error { files, err := getAllSymbolFiles(path, symbolType) if err != nil { @@ -151,14 +184,13 @@ func generateSymbolFiles(symbolType, path, outputDir string) error { symbolsID := viper.GetString(symbolsIdFlag) appVersion := viper.GetString(appVersionFlag) basePath := viper.GetString(basePathFlag) - symbolsIDPrefix := symbolsIDPrefixForType(symbolType) for _, file := range files { fileSymbolsID := symbolsID if fileSymbolsID == "" { fileSymbolsID = symbolsIDForArtifact(file.Path) } - key := getS3Key(symbolsIDPrefix, fileSymbolsID, appVersion, basePath, file.Name) + key := getS3Key(reactNativeSymbolsIDPrefix, fileSymbolsID, appVersion, basePath, file.Name) data, err := os.ReadFile(file.Path) if err != nil { diff --git a/cmd/symbols/upload.go b/cmd/symbols/upload.go index a791e1ab..157a5457 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -61,7 +61,7 @@ const ( reactNativeSymbolsIDPrefix = "_sym/js/id" // androidSymbolsIDPrefix is the equivalent Symbols Id Lane segment for Android - // R8 / ProGuard mappings. Keys become _sym/android/id//mapping.txt. + // builds. Keys become _sym/android/id//mapping.v1.index. androidSymbolsIDPrefix = "_sym/android/id" // symbolsIDSidecarSuffix names the file written next to an artifact to record @@ -70,16 +70,17 @@ const ( // manual --symbols-id. symbolsIDSidecarSuffix = ".symbolsid" - // androidMappingFileName is the R8/ProGuard mapping file `ldcli` discovers - // for --type android. + // androidMappingFileName is the R8/ProGuard mapping `ldcli` discovers for + // --type android and indexes. The mapping itself is not uploaded; see + // android_upload.go. androidMappingFileName = "mapping.txt" // typeReactNative uploads React Native Hermes/Metro sourcemaps (ordinary // JavaScript sourcemaps). typeReactNative = "react-native" - // typeAndroid uploads an Android R8/ProGuard `mapping.txt` for Java/Kotlin - // stack-trace retrace. + // typeAndroid indexes an Android R8/ProGuard `mapping.txt` and uploads the + // index, for Java/Kotlin stack-trace retrace. typeAndroid = "android" // typeAppleDSYM compiles Apple dSYM debug info into per-architecture .dsymmap @@ -235,22 +236,15 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return uploadFlutterSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, backendUrl, skipExisting) } - // An Android project already knows where R8 put the mapping, what version it - // shipped, and which symbols id the app reports, so read all three out of - // the build instead of asking for them. + // Android takes a dedicated path as well: the R8 mapping is compiled into the + // index symbolication reads, and stored under the lanes a crash can arrive on. if symbolType == typeAndroid { - resolved, aErr := resolveAndroidBuild(androidUpload{Path: path, AppVersion: appVersion, SymbolsID: symbolsID}) - if aErr != nil { - return aErr - } - path, appVersion, symbolsID = resolved.Path, resolved.AppVersion, resolved.SymbolsID + return uploadAndroidSymbols(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, path, appVersion, symbolsID, backendUrl, viper.GetBool(includeSourcesFlag), viper.GetString(sourcePathFlag), skipExisting) } - symbolsIDPrefix := symbolsIDPrefixForType(symbolType) - fmt.Printf("Starting to upload %s symbols from %s\n", symbolType, path) if symbolsID != "" { - fmt.Printf("Using symbols id %s for all files (Symbols Id Lane: %s/%s)\n", symbolsID, symbolsIDPrefix, symbolsID) + fmt.Printf("Using symbols id %s for all files (Symbols Id Lane: %s/%s)\n", symbolsID, reactNativeSymbolsIDPrefix, symbolsID) } files, err := getAllSymbolFiles(path, symbolType) @@ -262,70 +256,32 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return fmt.Errorf("no symbol files found in %s, is this the correct path?", path) } - // Symbols Id Lane: resolve the id per file so a single upload of multiple - // platforms (e.g. iOS + Android maps in one dir) keys each artifact by the - // id its app reports. An explicit --symbols-id overrides all files; - // otherwise each artifact's *.symbolsid sidecar (or its sibling's — see - // symbolsIDForArtifact) is used, falling back to the Version Lane - // (version+basePath) when there is none. + // Symbols Id Lane: resolve the id per file so one upload of a build's bundle + // and its map keys both by the id the app reports. An explicit --symbols-id + // overrides all files; otherwise each artifact's *.symbolsid sidecar (or its + // sibling's — see symbolsIDForArtifact) is used, falling back to the Version + // Lane (version+basePath) when there is none. s3Keys := make([]string, 0, len(files)) - firstSymbolsID := "" - for i, file := range files { + for _, file := range files { fileSymbolsID := symbolsID if fileSymbolsID == "" { fileSymbolsID = symbolsIDForArtifact(file.Path) if fileSymbolsID != "" { - fmt.Printf("Using symbols id %s for %s (Symbols Id Lane: %s/%s)\n", fileSymbolsID, file.Name, symbolsIDPrefix, fileSymbolsID) + fmt.Printf("Using symbols id %s for %s (Symbols Id Lane: %s/%s)\n", fileSymbolsID, file.Name, reactNativeSymbolsIDPrefix, fileSymbolsID) } } - if i == 0 { - firstSymbolsID = fileSymbolsID - } - s3Keys = append(s3Keys, getS3Key(symbolsIDPrefix, fileSymbolsID, appVersion, basePath, file.Name)) + s3Keys = append(s3Keys, getS3Key(reactNativeSymbolsIDPrefix, fileSymbolsID, appVersion, basePath, file.Name)) } - // Android sources are packed into one bundle uploaded beside the mapping, - // on the same lane, so the errors page can show the code around each - // retraced frame. Keyed off the first mapping's lane: a build has one - // mapping, and its sources are the app's sources. Because that key is the - // mapping's id and not the bundle's, an object being there says nothing - // about these sources — a source-only change keeps the mapping's id — so the - // bundle is skipped only when its digest matches what is already stored. - var sourceBundle *uploadBody - if symbolType == typeAndroid && viper.GetBool(includeSourcesFlag) { - sourceRoot := viper.GetString(sourcePathFlag) - data, count, bErr := buildAndroidSourceBundle(sourceRoot) - if bErr != nil { - return bErr - } - if data == nil { - // Not fatal: the mapping alone still retraces, just without source. - fmt.Printf("No .java/.kt sources found under %s; skipping source bundle\n", sourceRoot) - } else { - // Compressed here rather than at the point of sending, so the digest - // below describes the bytes that get stored. - body := compressBody(data) - sourceBundle = &body - bundleName := filepath.Join(filepath.Dir(files[0].Name), androidSourceBundleName) - s3Keys = append(s3Keys, getS3Key(symbolsIDPrefix, firstSymbolsID, appVersion, basePath, bundleName)) - fmt.Printf("Built source bundle from %s (%d files, %d bytes)\n", sourceRoot, count, len(data)) - } - } - - // Only the source bundle needs a digest: every other key here is derived from - // the artifact's own content, so the backend settles those by existence alone - // and we never have to read a large mapping back off disk to hash it. - digests := make([]string, len(s3Keys)) - if sourceBundle != nil { - digests[len(s3Keys)-1] = contentDigest(sourceBundle.Data) - } - - uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, digests, backendUrl, skipExisting) + // No digests: a JavaScript map is keyed either by its own content id or by a + // Version Lane key the backend re-presigns so it can overwrite, so nothing + // here needs a hash to settle whether it is already stored. + uploadUrls, err := getSymbolUploadUrls(viper.GetString(cliflags.AccessTokenFlag), projectResult.ID, s3Keys, nil, backendUrl, skipExisting) if err != nil { return fmt.Errorf("failed to get upload URLs: %w", err) } - // The loops below pair each requested key with uploadUrls[i], so a short + // The loop below pairs each requested key with uploadUrls[i], so a short // list would panic. Require one URL per requested key. if len(uploadUrls) != len(s3Keys) { return fmt.Errorf("expected %d upload URLs but received %d", len(s3Keys), len(uploadUrls)) @@ -342,15 +298,6 @@ func runE(client resources.Client) func(cmd *cobra.Command, args []string) error return fmt.Errorf("failed to upload file %s: %w", file.Path, err) } } - if sourceBundle != nil { - // The bundle's key was appended last, so it takes the final URL. - if alreadyUploaded(uploadUrls[len(files)]) { - fmt.Printf("Skipping %s, already uploaded\n", androidSourceBundleName) - skipped++ - } else if err := uploadBytes(*sourceBundle, uploadUrls[len(files)], androidSourceBundleName); err != nil { - return fmt.Errorf("failed to upload source bundle: %w", err) - } - } reportUploadSummary(skipped) return nil @@ -417,15 +364,6 @@ func isSupportedType(symbolType string) bool { return symbolType == typeReactNative || symbolType == typeAndroid || symbolType == typeAppleDSYM || symbolType == typeFlutter } -// symbolsIDPrefixForType picks the Symbols Id Lane storage segment for the symbol -// type so JS and Android maps never collide in the same symbols-id namespace. -func symbolsIDPrefixForType(symbolType string) string { - if symbolType == typeAndroid { - return androidSymbolsIDPrefix - } - return reactNativeSymbolsIDPrefix -} - func isReactNativeUploadFile(name string) bool { for _, suffix := range reactNativeUploadSuffixes { if strings.HasSuffix(name, suffix) { @@ -435,8 +373,8 @@ func isReactNativeUploadFile(name string) bool { return false } -// isSymbolUploadFile reports whether a discovered file should be uploaded for -// the given symbol type: React Native bundles/maps, or an Android mapping.txt. +// isSymbolUploadFile reports whether a discovered file is an input for the given +// symbol type: React Native bundles and maps, or an Android mapping.txt. func isSymbolUploadFile(symbolType, name string) bool { if symbolType == typeAndroid { return filepath.Base(name) == androidMappingFileName @@ -491,10 +429,10 @@ func getAllSymbolFiles(path, symbolType string) ([]SymbolFile, error) { files = append(files, SymbolFile{ Path: filePath, - // Symbolication reads an Android mapping at /mapping.txt, so - // the object is named for the file alone however deep it was found. - // A React Native bundle keeps its path, which is part of how a map - // is matched to the bundle that references it. + // An Android mapping is named for the file alone however deep it was + // found, since what it is stored as is decided by the indexer. A + // React Native bundle keeps its path, which is part of how a map is + // matched to the bundle that references it. Name: uploadName(symbolType, relPath), }) } diff --git a/cmd/symbols/upload_test.go b/cmd/symbols/upload_test.go index 117565ad..04cc87be 100644 --- a/cmd/symbols/upload_test.go +++ b/cmd/symbols/upload_test.go @@ -181,11 +181,6 @@ func TestGetS3KeySymbolsID(t *testing.T) { getS3Key(androidSymbolsIDPrefix, symbolsID, "1.0.0", "", "mapping.txt")) } -func TestSymbolsIDPrefixForType(t *testing.T) { - assert.Equal(t, "_sym/js/id", symbolsIDPrefixForType(typeReactNative)) - assert.Equal(t, "_sym/android/id", symbolsIDPrefixForType(typeAndroid)) -} - func TestReadSymbolsIDFile(t *testing.T) { tempDir, err := os.MkdirTemp("", "symbols-id") assert.NoError(t, err) diff --git a/internal/symbols/r8index/codec_test.go b/internal/symbols/r8index/codec_test.go new file mode 100644 index 00000000..1300dfa6 --- /dev/null +++ b/internal/symbols/r8index/codec_test.go @@ -0,0 +1,324 @@ +package r8index + +import ( + "fmt" + "os" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// assertIndexMatchesMapping is what the format has to be judged on: for every +// question the enhancer can ask, an index answers what the parsed mapping answers. +// Both resolve through the same class.retrace, so what is really under test is +// whether a class survives the round trip intact. +func assertIndexMatchesMapping(t *testing.T, m *Mapping) { + t.Helper() + + raw, err := Encode(m) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + for obf, c := range m.classes { + gotName, gotOK := ix.RetraceClass(obf) + wantName, wantOK := m.RetraceClass(obf) + assert.Equal(t, wantOK, gotOK, "RetraceClass(%q)", obf) + assert.Equal(t, wantName, gotName, "RetraceClass(%q)", obf) + + for method, members := range c.members { + for _, line := range probeLines(members) { + assert.Equal(t, m.Retrace(obf, method, line), ix.Retrace(obf, method, line), + "Retrace(%q, %q, %d)", obf, method, line) + } + } + // A name the class does not remap still deobfuscates the class. + assert.Equal(t, m.Retrace(obf, "notAMethod", 1), ix.Retrace(obf, "notAMethod", 1), + "Retrace(%q, unmapped)", obf) + } + + for original, file := range m.sourceFiles { + assert.Equal(t, file, ix.SourceFile(original), "SourceFile(%q)", original) + } + + // A class outside the mapping reads as absent, which is how an unobfuscated + // frame passes through untouched. + assert.Nil(t, ix.Retrace("not.In.Mapping", "a", 1)) + _, ok := ix.RetraceClass("not.In.Mapping") + assert.False(t, ok) + assert.Empty(t, ix.SourceFile("not.In.Mapping")) +} + +// probeLines covers each range's edges and the lines outside it, since which entry a +// line falls in is what decides the frame. +func probeLines(members []member) []int { + lines := []int{0, 1, 7, 1000} + for _, mem := range members { + if mem.hasRange { + lines = append(lines, + mem.minStart-1, mem.minStart, + (mem.minStart+mem.minEnd)/2, + mem.minEnd, mem.minEnd+1) + } + } + return lines +} + +func TestIndexMatchesParsedMapping(t *testing.T) { + assertIndexMatchesMapping(t, parsedSampleMapping(t)) +} + +// The synthetic lambda mapping is the hard case: a synthesized class, a marker R8 +// writes only on a member's first occurrence, and members prefixed with the class's +// own residual name. +func TestIndexMatchesSyntheticLambdaMapping(t *testing.T) { + assertIndexMatchesMapping(t, Parse([]byte(syntheticLambdaMapping))) +} + +func TestIndexMatchesMappingWithSourceFiles(t *testing.T) { + assertIndexMatchesMapping(t, Parse([]byte(`com.example.app.Checkout -> a.b.c: +# {"id":"sourceFile","fileName":"Checkout.kt"} + 1:3:void run():2:2 -> a + 1:3:void com.example.app.Helper.help():2:2 -> a +com.example.app.Helper -> a.b.d: +# {"id":"sourceFile","fileName":"Helpers.kt"} + 1:1:void help():2:2 -> a +`))) +} + +// One name being a prefix of another is where a search that compares in place gets it +// wrong, and source files are looked up by original name, where nesting makes shared +// prefixes ordinary. +func TestIndexSourceFileNamePrefixes(t *testing.T) { + raw, err := Encode(Parse([]byte(`com.example.Cart -> a: +# {"id":"sourceFile","fileName":"Cart.kt"} + 1:1:void a():1:1 -> a +com.example.CartItem -> b: +# {"id":"sourceFile","fileName":"CartItem.kt"} + 1:1:void a():1:1 -> a +com.example.Cart$Line -> c: +# {"id":"sourceFile","fileName":"Cart.kt"} + 1:1:void a():1:1 -> a +`))) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + assert.Equal(t, "Cart.kt", ix.SourceFile("com.example.Cart")) + assert.Equal(t, "CartItem.kt", ix.SourceFile("com.example.CartItem")) + assert.Equal(t, "Cart.kt", ix.SourceFile("com.example.Cart$Line")) + assert.Empty(t, ix.SourceFile("com.example.Car"), "a shorter name is not a match") + assert.Empty(t, ix.SourceFile("com.example.CartItems"), "a longer name is not a match") + assert.Empty(t, ix.SourceFile("")) +} + +// Equal mappings must encode to equal bytes, so an index can be addressed by the +// content id of the mapping it was built from. +func TestEncodingIsDeterministic(t *testing.T) { + first, err := Encode(Parse([]byte(sampleMapping))) + require.NoError(t, err) + second, err := Encode(Parse([]byte(sampleMapping))) + require.NoError(t, err) + assert.Equal(t, first, second) +} + +func TestEncodeRejectsNilMapping(t *testing.T) { + _, err := Encode(nil) + assert.Error(t, err) +} + +// A block is bytes from storage, so every truncation of one has to read as unusable +// rather than panic or invent a class. +func TestDecodeClassRejectsTruncatedBlocks(t *testing.T) { + m := Parse([]byte(sampleMapping)) + block := encodeClass(m.classes["a.b.c"]) + + full, ok := decodeClass(block) + require.True(t, ok) + require.Equal(t, "com.example.app.UserService", full.originalName) + + for i := 0; i < len(block); i++ { + c, ok := decodeClass(block[:i]) + if ok { + // A prefix that happens to decode must still be a coherent class, not + // one carrying members it never got the bytes for. + assert.NotNil(t, c) + continue + } + assert.Nil(t, c, "truncated at %d", i) + } +} + +// A count is the one field that sizes an allocation, so it is bounded by the bytes +// that are left rather than trusted. +func TestDecodeClassRejectsImplausibleCounts(t *testing.T) { + // "" original name, no flags, then a method count of 2^32. + block := append(appendString(nil, ""), 0) + block = append(block, 0xff, 0xff, 0xff, 0xff, 0x0f) + + c, ok := decodeClass(block) + assert.False(t, ok) + assert.Nil(t, c) +} + +func TestOpenRejectsGarbage(t *testing.T) { + for _, raw := range [][]byte{nil, {}, []byte("SRCB"), []byte("not a bundle at all")} { + _, err := Open(raw) + assert.Error(t, err) + } +} + +// A block that survives the bundle's checks but decodes to nothing usable reads as a +// missing class, and is remembered as one. +func TestUnreadableBlockIsAMiss(t *testing.T) { + raw, err := Encode(Parse([]byte(sampleMapping))) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + assert.Nil(t, ix.Retrace("a.b.missing", "a", 1)) + assert.Nil(t, ix.Retrace("a.b.missing", "a", 1), "a miss is memoized, not re-read") + assert.NotNil(t, ix.Retrace("a.b.c", "a", 2), "other classes still resolve") +} + +// An index queried for every class must not end up holding every class, or it would +// have become the structure it exists to avoid. +func TestMemoIsBounded(t *testing.T) { + var mapping []byte + for i := 0; i < memoLimit*2; i++ { + mapping = append(mapping, fmt.Sprintf("com.example.C%d -> a%d:\n 1:1:void run():2:2 -> a\n", i, i)...) + } + raw, err := Encode(Parse(mapping)) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + for i := 0; i < memoLimit*2; i++ { + frames := ix.Retrace(fmt.Sprintf("a%d", i), "a", 1) + require.Len(t, frames, 1, "class %d", i) + assert.Equal(t, fmt.Sprintf("com.example.C%d", i), frames[0].Class) + } + assert.LessOrEqual(t, len(ix.classes), memoLimit) +} + +// The lookups have to be safe on an index that was never opened, since that is what a +// build with no symbols uploaded looks like. +func TestNilIndexAnswersNothing(t *testing.T) { + var ix *Index + assert.Nil(t, ix.Retrace("a", "b", 1)) + _, ok := ix.RetraceClass("a") + assert.False(t, ok) + assert.Empty(t, ix.SourceFile("a")) +} + +// realMapping reads the mapping named by R8_MAPPING, for the opt-in tests below: +// +// R8_MAPPING=/path/to/mapping.txt go test ./backend/stacktraces/r8index/ -run RealMapping -v +func realMapping(t *testing.T) []byte { + t.Helper() + path := os.Getenv("R8_MAPPING") + if path == "" { + t.Skip("set R8_MAPPING to a release mapping.txt") + } + data, err := os.ReadFile(path) + require.NoError(t, err) + return data +} + +func heapAllocMB() float64 { + runtime.GC() + var ms runtime.MemStats + runtime.ReadMemStats(&ms) + return float64(ms.HeapAlloc) / (1 << 20) +} + +// TestRealMapping checks every class in a real build decodes to what the parser +// produced. Two lines per method name rather than every line of every entry: the +// exhaustive comparison is done on the small mappings above, and a real mapping has +// 480k entries whose blocks would be inflated once per probe. +func TestRealMapping(t *testing.T) { + data := realMapping(t) + m := Parse(data) + require.NotZero(t, m.Classes()) + + raw, err := Encode(m) + require.NoError(t, err) + fmt.Printf("\nmapping.txt : %.1f MB\nindex : %.1f MB (%.0f%% of the mapping)\nclasses : %d\n", + float64(len(data))/(1<<20), float64(len(raw))/(1<<20), + 100*float64(len(raw))/float64(len(data)), m.Classes()) + + ix, err := Open(raw) + require.NoError(t, err) + + lookups := 0 + for obf, c := range m.classes { + gotName, gotOK := ix.RetraceClass(obf) + wantName, wantOK := m.RetraceClass(obf) + require.Equal(t, wantOK, gotOK, obf) + require.Equal(t, wantName, gotName, obf) + + for method, members := range c.members { + line := 1 + if len(members) > 0 && members[0].hasRange { + line = members[0].minStart + } + for _, probe := range []int{line, line + 1} { + require.Equal(t, m.Retrace(obf, method, probe), ix.Retrace(obf, method, probe), + "Retrace(%q, %q, %d)", obf, method, probe) + lookups++ + } + } + } + for original, file := range m.sourceFiles { + require.Equal(t, file, ix.SourceFile(original), original) + } + fmt.Printf("compared : %d lookups over %d classes\n", lookups, m.Classes()) +} + +// TestRealMappingFootprint is the number the format exists for: what has to stay in +// memory to answer lookups for one build. Each half takes its own baseline before it +// allocates anything, so neither is measured against the other's leftovers. +func TestRealMappingFootprint(t *testing.T) { + path := os.Getenv("R8_MAPPING") + if path == "" { + t.Skip("set R8_MAPPING to a release mapping.txt") + } + + var withMapping, withIndex float64 + + // The parsed mapping and the bytes it was built from, which is what a parse + // holds: the names are slices of that text rather than copies of it. + t.Run("parsed mapping", func(t *testing.T) { + base := heapAllocMB() + data, err := os.ReadFile(path) + require.NoError(t, err) + m := Parse(data) + require.NotZero(t, m.Classes()) + + withMapping = heapAllocMB() - base + runtime.KeepAlive(m) + runtime.KeepAlive(data) + }) + + // An index, with the mapping it was built from already collected. + t.Run("index", func(t *testing.T) { + base := heapAllocMB() + raw := func() []byte { + data, err := os.ReadFile(path) + require.NoError(t, err) + raw, err := Encode(Parse(data)) + require.NoError(t, err) + return raw + }() + ix, err := Open(raw) + require.NoError(t, err) + + withIndex = heapAllocMB() - base + runtime.KeepAlive(ix) + }) + + fmt.Printf("\nretained, parsed mapping + its bytes : %6.1f MB\nretained, index : %6.1f MB\nratio : %6.1fx\n", + withMapping, withIndex, withMapping/withIndex) +} diff --git a/internal/symbols/r8index/encode.go b/internal/symbols/r8index/encode.go new file mode 100644 index 00000000..2cb4ee67 --- /dev/null +++ b/internal/symbols/r8index/encode.go @@ -0,0 +1,194 @@ +package r8index + +// Writing an index. +// +// The container is a srcbundle: a sorted, string-keyed, individually gzipped blob +// store whose reads are bounds-checked and whose misses are misses rather than +// panics. What this file adds is the per-class encoding, so that a lookup is a +// binary search plus one small inflate. +// +// Class blocks are keyed by obfuscated name. The source-file table cannot live in +// them because it is keyed by *original* name: an inlined frame reports a class that +// is not the block it was found in, so there would be no block to look in. It gets +// one entry of its own, laid out as a sorted array searched in place so that holding +// it costs bytes rather than an entry per class in the build. +// +// Blocks are written in sorted order with sorted members, so equal mappings encode +// to equal bytes — which is what lets an index be addressed by the content id of the +// mapping it was built from. + +import ( + "bufio" + "bytes" + "encoding/binary" + "io" + "maps" + "slices" + + "github.com/launchdarkly/ldcli/internal/symbols/srcbundle" + e "github.com/pkg/errors" +) + +const ( + // classPrefix namespaces class blocks so the source-file table can share the + // bundle without an obfuscated name ever colliding with it. + classPrefix = "c/" + // sourceKey holds the original-name -> source-file table. + sourceKey = "s" + + // srcRecSize is one source-file record: two u32 offsets into the entry. + srcRecSize = 8 +) + +// EncodeFrom reads a mapping.txt and writes its index without ever holding the whole +// thing: each class is encoded and compressed as its last line is read, and then +// dropped, so what stays live is the index being built rather than every class in the +// build. On a 61 MB release mapping that is a peak of ~100 MB against the ~400 MB of +// parsing it and encoding the result, most of what is left being garbage the +// collector has not needed to take yet. A build machine should not have to find +// 400 MB to produce a 5 MB file. +// +// Reports an error for input with no classes in it, which is a file that is not a +// mapping (or a build that did not obfuscate). Uploading its index would leave the +// build looking symbolicated when nothing could be retraced. +func EncodeFrom(r io.Reader) ([]byte, error) { + var b srcbundle.Builder + classes := 0 + sc := newScanner(func(obf string, c *class) { + b.Add(classPrefix+obf, encodeClass(c)) + classes++ + }) + + lines := bufio.NewScanner(r) + // A mapping line is one member, but a signature with deeply generic arguments + // can be long, and the default 64 KB would fail the whole upload over one. + lines.Buffer(make([]byte, 0, 64*1024), 4*1024*1024) + for lines.Scan() { + sc.line(lines.Text()) + } + if err := lines.Err(); err != nil { + return nil, e.Wrap(err, "r8index: reading mapping") + } + sc.flush() + + if classes == 0 { + return nil, e.New("r8index: no classes in mapping") + } + return finishIndex(&b, sc.sourceFiles) +} + +// Encode encodes an already-parsed mapping. EncodeFrom is what a producer should +// reach for; this exists for a caller that has a Mapping anyway, and for tests that +// state their input as mapping text. +func Encode(m *Mapping) ([]byte, error) { + if m == nil { + return nil, e.New("r8index: nil mapping") + } + + var b srcbundle.Builder + for _, obf := range slices.Sorted(maps.Keys(m.classes)) { + if c := m.classes[obf]; c != nil { + b.Add(classPrefix+obf, encodeClass(c)) + } + } + return finishIndex(&b, m.sourceFiles) +} + +// finishIndex adds the source-file table and encodes the bundle. Shared so that an +// index is assembled in exactly one place however its classes were produced. +func finishIndex(b *srcbundle.Builder, sourceFiles map[string]string) ([]byte, error) { + b.Add(sourceKey, encodeSourceFileTable(sourceFiles)) + + var buf bytes.Buffer + if err := b.Encode(&buf); err != nil { + return nil, e.Wrap(err, "r8index: encoding bundle") + } + return buf.Bytes(), nil +} + +func encodeClass(c *class) []byte { + out := appendString(make([]byte, 0, 256), c.originalName) + var flags byte + if c.synthesized { + flags |= 1 + } + out = append(out, flags) + + out = binary.AppendUvarint(out, uint64(len(c.members))) + for _, name := range slices.Sorted(maps.Keys(c.members)) { + entries := c.members[name] + out = appendString(out, name) + out = binary.AppendUvarint(out, uint64(len(entries))) + for _, mem := range entries { + out = appendMember(out, mem) + } + } + return out +} + +// appendMember writes one member entry. Line numbers are signed varints because +// they come from Atoi over a file we did not write, and only the numbers a member +// actually carries are written at all. +func appendMember(out []byte, mem member) []byte { + var flags byte + if mem.hasRange { + flags |= 1 + } + if mem.hasOrig { + flags |= 2 + } + if mem.hasOrigEnd { + flags |= 4 + } + if mem.synthesized { + flags |= 8 + } + out = append(out, flags) + + if mem.hasRange { + out = binary.AppendVarint(out, int64(mem.minStart)) + out = binary.AppendVarint(out, int64(mem.minEnd)) + } + if mem.hasOrig { + out = binary.AppendVarint(out, int64(mem.origStart)) + } + if mem.hasOrigEnd { + out = binary.AppendVarint(out, int64(mem.origEnd)) + } + return appendString(appendString(out, mem.origClass), mem.origMethod) +} + +func appendString(out []byte, s string) []byte { + return append(binary.AppendUvarint(out, uint64(len(s))), s...) +} + +// encodeSourceFileTable lays the table out as a count, a sorted array of two +// offsets per class, then the strings — so a lookup binary searches the array and +// compares against the bytes in place, without building a map of every class in +// the build to answer a handful of frames. +func encodeSourceFileTable(files map[string]string) []byte { + names := slices.Sorted(maps.Keys(files)) + strtabStart := 4 + len(names)*srcRecSize + + var strtab []byte + interned := make(map[string]uint32, len(names)) + put := func(s string) uint32 { + if off, ok := interned[s]; ok { + return off + } + off := uint32(strtabStart + len(strtab)) + strtab = append(append(strtab, s...), 0) + interned[s] = off + return off + } + + out := make([]byte, strtabStart) + binary.LittleEndian.PutUint32(out, uint32(len(names))) + for i, name := range names { + nameOff, fileOff := put(name), put(files[name]) + rec := out[4+i*srcRecSize:] + binary.LittleEndian.PutUint32(rec[0:], nameOff) + binary.LittleEndian.PutUint32(rec[4:], fileOff) + } + return append(out, strtab...) +} diff --git a/internal/symbols/r8index/parse.go b/internal/symbols/r8index/parse.go new file mode 100644 index 00000000..aae8cb50 --- /dev/null +++ b/internal/symbols/r8index/parse.go @@ -0,0 +1,336 @@ +package r8index + +import ( + "strconv" + "strings" +) + +// Mapping is a whole mapping.txt parsed into memory. Encode turns one into an +// index, and symbolication reads the index instead — so in production this form +// only exists in the CLI, for as long as it takes to write one. +type Mapping struct { + // classes is keyed by the *obfuscated* class name (dotted). + classes map[string]*class + // sourceFiles maps an *original* (deobfuscated) class name to the source file + // it was compiled from, as recorded by R8's sourceFile metadata. Keyed by the + // original name because that is what a retraced frame reports — and because an + // inlined frame's class differs from the obfuscated class whose entry it was + // found under. + sourceFiles map[string]string +} + +// Retrace resolves an obfuscated (class, method, line) to one or more original +// frames. Returns nil when the class isn't in the mapping (name wasn't obfuscated, +// or a stale mapping) so the caller can pass the frame through. +func (m *Mapping) Retrace(obfClass, obfMethod string, line int) []Frame { + if m == nil { + return nil + } + c := m.classes[obfClass] + if c == nil { + return nil + } + return c.retrace(obfMethod, line) +} + +// RetraceClass deobfuscates a bare class name, with no method or line to narrow it +// down. An exception type needs exactly this: R8 renames a throwable's class like +// any other, so the reported type is obfuscated even though the class never has to +// appear in a frame. Reports false when the class isn't in the mapping, which is +// also how an unobfuscated name passes through untouched. +func (m *Mapping) RetraceClass(obfClass string) (string, bool) { + if m == nil { + return "", false + } + c := m.classes[obfClass] + if c == nil || c.originalName == "" { + return "", false + } + return c.originalName, true +} + +// SourceFile reports the file an *original* class name was compiled from, per R8's +// metadata, or "" when the build recorded none. +func (m *Mapping) SourceFile(originalClass string) string { + if m == nil { + return "" + } + return m.sourceFiles[originalClass] +} + +// Classes reports how many entries were parsed, which is how a caller tells a +// mapping it can symbolicate from one that parsed into nothing. +func (m *Mapping) Classes() int { + if m == nil { + return 0 + } + return len(m.classes) +} + +// Parse parses a whole mapping.txt into memory. Never returns nil: a file that is +// not a mapping at all reads as a mapping with no classes, which is what callers +// check. +func Parse(data []byte) *Mapping { + m := &Mapping{classes: map[string]*class{}} + sc := newScanner(func(obf string, c *class) { m.classes[obf] = c }) + sc.scan(data) + m.sourceFiles = sc.sourceFiles + return m +} + +// scanner reads the R8/ProGuard mapping.txt grammar, handing over each class as it +// completes. Grammar (per class): +// +// -> : +// -> (ignored) +// () -> (name only) +// :: [.]()[:[:]] -> +// +// Handing classes over one at a time is what lets EncodeFrom write an index without +// ever holding the whole mapping (see encode.go). The source-file table is kept +// here rather than streamed: it is one small map, and both callers need all of it. +type scanner struct { + onClass func(obf string, c *class) + sourceFiles map[string]string + + current *class + currentObf string + // A comment annotates the entry directly above it, so the marker it carries has + // to land on that entry: on the class while no member has been read yet, on a + // member once one has, and on neither for the lines skipped below. That last + // case is what keeps a synthesized *field* — ordinary classes have them — from + // marking its whole class as R8-generated. + // + // The member is held as key + index rather than a pointer because appending to + // the slice can move what a pointer refers to. + commentTargetsClass bool + lastMemberKey string + lastMemberIdx int +} + +func newScanner(onClass func(obf string, c *class)) *scanner { + return &scanner{onClass: onClass, sourceFiles: map[string]string{}} +} + +func (s *scanner) scan(data []byte) { + for _, raw := range strings.Split(string(data), "\n") { + s.line(raw) + } + s.flush() +} + +// flush hands over the class in hand, which is complete once anything else starts. +func (s *scanner) flush() { + if s.current != nil { + s.onClass(s.currentObf, s.current) + s.current, s.currentObf = nil, "" + } +} + +func (s *scanner) line(raw string) { + line := strings.TrimRight(raw, "\r") + if line == "" { + return + } + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") { + // Most comments are R8 bookkeeping, but two carry things nothing else + // records: sourceFile is the only authoritative statement of which file a + // class was compiled from (it can't be derived from the class name, see the + // enhancer's androidKnownSourceFile), and the synthesized marker is the only + // way to tell R8's own scaffolding from code somebody wrote. + if s.current == nil { + return + } + if f := parseSourceFileComment(trimmed); f != "" { + if _, seen := s.sourceFiles[s.current.originalName]; !seen { + s.sourceFiles[s.current.originalName] = f + } + } + if parseSynthesizedComment(trimmed) { + switch { + case s.commentTargetsClass: + s.current.synthesized = true + case s.lastMemberKey != "": + s.current.members[s.lastMemberKey][s.lastMemberIdx].synthesized = true + } + } + return + } + if line[0] != ' ' && line[0] != '\t' { + // Class header: " -> :" + s.flush() + s.commentTargetsClass = false + s.lastMemberKey = "" + if orig, obf, ok := parseClassHeader(line); ok { + s.current = &class{originalName: orig, members: map[string][]member{}} + s.currentObf = obf + s.commentTargetsClass = true + } + return + } + if s.current == nil { + return + } + s.commentTargetsClass = false + s.lastMemberKey = "" + if obf, mem, ok := parseMember(strings.TrimSpace(line)); ok { + s.current.members[obf] = append(s.current.members[obf], mem) + s.lastMemberKey, s.lastMemberIdx = obf, len(s.current.members[obf])-1 + } +} + +func parseClassHeader(line string) (original, obfuscated string, ok bool) { + if !strings.HasSuffix(line, ":") { + return "", "", false + } + body := strings.TrimSuffix(line, ":") + arrow := strings.Index(body, " -> ") + if arrow < 0 { + return "", "", false + } + original = strings.TrimSpace(body[:arrow]) + obfuscated = strings.TrimSpace(body[arrow+len(" -> "):]) + if original == "" || obfuscated == "" { + return "", "", false + } + return original, obfuscated, true +} + +// parseMember parses a single (already-trimmed) member line. Field lines and +// anything without a method signature "(...)" are ignored (ok=false). +func parseMember(line string) (obfName string, mem member, ok bool) { + arrow := strings.LastIndex(line, " -> ") + if arrow < 0 { + return "", member{}, false + } + left := line[:arrow] + obfName = strings.TrimSpace(line[arrow+len(" -> "):]) + if obfName == "" { + return "", member{}, false + } + + // Optional leading "::" obfuscated line range. + if s, e, rest, has := parseLeadingLineRange(left); has { + mem.hasRange = true + mem.minStart = s + mem.minEnd = e + left = rest + } + + // left is now " [.]()[:[:]]". + // Drop the return type (everything up to the first space). + sp := strings.Index(left, " ") + if sp < 0 { + return "", member{}, false + } + sig := left[sp+1:] + + paren := strings.Index(sig, "(") + if paren < 0 { + // A field (no parens): not retraceable. + return "", member{}, false + } + name := sig[:paren] + + // Trailing ":[:]" after the closing paren. + if closing := strings.Index(sig, ")"); closing >= 0 { + if tail := sig[closing+1:]; strings.HasPrefix(tail, ":") { + parseOriginalLineRange(tail[1:], &mem) + } + } + + // name may be "." for inlined frames. + if dot := strings.LastIndex(name, "."); dot >= 0 { + mem.origClass = name[:dot] + mem.origMethod = name[dot+1:] + } else { + mem.origMethod = name + } + if mem.origMethod == "" { + return "", member{}, false + } + return obfName, mem, true +} + +// parseLeadingLineRange consumes a "::" prefix if present. +func parseLeadingLineRange(s string) (start, end int, rest string, ok bool) { + first := strings.Index(s, ":") + if first <= 0 { + return 0, 0, s, false + } + start, err := strconv.Atoi(s[:first]) + if err != nil { + return 0, 0, s, false + } + after := s[first+1:] + second := strings.Index(after, ":") + if second <= 0 { + return 0, 0, s, false + } + end, err = strconv.Atoi(after[:second]) + if err != nil { + return 0, 0, s, false + } + return start, end, after[second+1:], true +} + +// parseOriginalLineRange fills mem from a trailing "[:]". +func parseOriginalLineRange(s string, mem *member) { + parts := strings.Split(s, ":") + if len(parts) == 0 { + return + } + start, err := strconv.Atoi(strings.TrimSpace(parts[0])) + if err != nil { + return + } + mem.hasOrig = true + mem.origStart = start + if len(parts) > 1 { + if end, err := strconv.Atoi(strings.TrimSpace(parts[1])); err == nil { + mem.hasOrigEnd = true + mem.origEnd = end + } + } +} + +// parseSourceFileComment extracts the file name from R8's sourceFile metadata +// comment, which sits directly under a class header: +// +// # {"id":"sourceFile","fileName":"SymbolicationDemo.kt"} +// +// Returns "" for any other comment, and for R8's own placeholders — a synthetic +// class ("R8$$SyntheticClass") or a renamed one ("SourceFile") names no real file, +// so there is nothing to prefer over the name the enhancer derives. +func parseSourceFileComment(comment string) string { + if !strings.Contains(comment, `"sourceFile"`) { + return "" + } + const key = `"fileName":"` + i := strings.Index(comment, key) + if i < 0 { + return "" + } + rest := comment[i+len(key):] + end := strings.IndexByte(rest, '"') + if end < 0 { + return "" + } + name := rest[:end] + if !isSourceFileName(name) { + return "" + } + return name +} + +// parseSynthesizedComment reports whether a comment is R8's marker for an entry it +// generated itself rather than compiled from source: +// +// # {"id":"com.android.tools.r8.synthesized"} +// +// R8 writes it under a class header for a wholly synthesized class (a desugared +// lambda), and under a member line for a synthesized member. +func parseSynthesizedComment(comment string) bool { + return strings.Contains(comment, `"com.android.tools.r8.synthesized"`) +} diff --git a/internal/symbols/r8index/parse_test.go b/internal/symbols/r8index/parse_test.go new file mode 100644 index 00000000..dc081898 --- /dev/null +++ b/internal/symbols/r8index/parse_test.go @@ -0,0 +1,279 @@ +package r8index + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const sampleMapping = `# compiler: R8 +com.example.app.UserService -> a.b.c: + 1:3:com.example.app.User loadUser(java.lang.String):40:42 -> a + 5:5:void validate():60:60 -> b + void helper() -> d +com.example.app.MainActivity -> a.b.d: + 10:10:void onCreate(android.os.Bundle):100:100 -> a + 20:20:void handleClick():5:5 -> b + 20:20:void com.example.app.Analytics.track():15:15 -> b +` + +// Verbatim R8 v2.2 output (e2e/android release build) for the class a Kotlin lambda +// desugars into. Three details this locks in: the class-level synthesized marker, R8 +// repeating the marker only on the *first* occurrence of a synthetic member (so 52:64 +// arrives unmarked), and the residual "g5." package R8 writes on the synthetic +// class's own member lines. +const syntheticLambdaMapping = `com.example.androidobservability.MainActivityKt$$ExternalSyntheticLambda10 -> g5.g: +# {"id":"sourceFile","fileName":"R8$$SyntheticClass"} +# {"id":"com.android.tools.r8.synthesized"} + int g5.MainActivityKt$$ExternalSyntheticLambda10.$r8$classId -> g + # {"id":"com.android.tools.r8.synthesized"} + 1:1:void g5.MainActivityKt$$ExternalSyntheticLambda10.(g5.MainActivityViewModel,int):0:0 -> + # {"id":"com.android.tools.r8.synthesized"} + 47:49:java.lang.Object g5.MainActivityKt$$ExternalSyntheticLambda10.invoke():0 -> invoke + # {"id":"com.android.tools.r8.synthesized"} + 52:64:int com.example.androidobservability.CartPricing.computeTotal(java.lang.String):19:19 -> invoke + 52:64:int com.example.androidobservability.CartPricing.priceOrder(java.lang.String):16 -> invoke + 52:64:int com.example.androidobservability.CheckoutDemo.startCheckout(java.lang.String):12 -> invoke + 52:64:kotlin.Unit com.example.androidobservability.MainActivityKt.ErrorButtons$lambda$59$lambda$58(com.example.androidobservability.MainActivityViewModel):452 -> invoke + 52:64:java.lang.Object g5.MainActivityKt$$ExternalSyntheticLambda10.invoke():0 -> invoke +` + +func parsedSampleMapping(t *testing.T) *Mapping { + t.Helper() + m := Parse([]byte(sampleMapping)) + require.NotNil(t, m) + require.Equal(t, 2, m.Classes()) + return m +} + +func TestParseClasses(t *testing.T) { + m := parsedSampleMapping(t) + assert.Contains(t, m.classes, "a.b.c") + assert.Contains(t, m.classes, "a.b.d") + assert.Equal(t, "com.example.app.UserService", m.classes["a.b.c"].originalName) + assert.Equal(t, "com.example.app.MainActivity", m.classes["a.b.d"].originalName) +} + +// A file that is not a mapping parses into nothing rather than failing, which is +// what a caller checks before uploading or symbolicating. +func TestParseRejectsNonMapping(t *testing.T) { + for _, in := range []string{"", "not a mapping", "# compiler: R8\n", "\n\n"} { + m := Parse([]byte(in)) + require.NotNil(t, m) + assert.Zero(t, m.Classes(), "%q", in) + } +} + +func TestRetraceLinearRange(t *testing.T) { + m := parsedSampleMapping(t) + // obf line 2 in range 1:3 -> original 40 + (2-1) = 41. + frames := m.Retrace("a.b.c", "a", 2) + assert.Len(t, frames, 1) + assert.Equal(t, "com.example.app.UserService", frames[0].Class) + assert.Equal(t, "loadUser", frames[0].Method) + assert.Equal(t, 41, frames[0].Line) +} + +func TestRetraceCollapsedRange(t *testing.T) { + m := parsedSampleMapping(t) + // 5:5 -> 60:60 maps to the single original line regardless of input. + frames := m.Retrace("a.b.c", "b", 5) + assert.Len(t, frames, 1) + assert.Equal(t, "validate", frames[0].Method) + assert.Equal(t, 60, frames[0].Line) +} + +func TestRetraceInlineExpansion(t *testing.T) { + m := parsedSampleMapping(t) + // obf a.b.d.b at line 20 has two entries in the same range: the enclosing + // handleClick and the inlined Analytics.track. + frames := m.Retrace("a.b.d", "b", 20) + assert.Len(t, frames, 2) + assert.Equal(t, "com.example.app.MainActivity", frames[0].Class) + assert.Equal(t, "handleClick", frames[0].Method) + assert.Equal(t, 5, frames[0].Line) + // Inlined frame keeps its own (original) class. + assert.Equal(t, "com.example.app.Analytics", frames[1].Class) + assert.Equal(t, "track", frames[1].Method) + assert.Equal(t, 15, frames[1].Line) +} + +func TestRetraceUnknownClassAndMethod(t *testing.T) { + m := parsedSampleMapping(t) + // Unknown class -> nil (frame passes through unchanged). + assert.Nil(t, m.Retrace("x.y.z", "a", 1)) + // Known class, method not in mapping -> class deobfuscated, method kept. + frames := m.Retrace("a.b.c", "zzz", 1) + assert.Len(t, frames, 1) + assert.Equal(t, "com.example.app.UserService", frames[0].Class) + assert.Equal(t, "zzz", frames[0].Method) +} + +func TestRetraceNameOnlyFallback(t *testing.T) { + m := parsedSampleMapping(t) + // "d" only has a name mapping (helper, no line range); line is preserved. + frames := m.Retrace("a.b.c", "d", 99) + assert.Len(t, frames, 1) + assert.Equal(t, "helper", frames[0].Method) + assert.Equal(t, 99, frames[0].Line) +} + +// Real R8 v2.2 output (from the e2e/android release build): the whole CheckoutDemo +// chain was inlined into a synthetic lambda's `invoke`, so a single runtime frame +// "g5.g.invoke(SourceFile:57)" must expand to the three original frames. Locks the +// parser + inline retrace against actual R8 output. +func TestRetraceRealInlinedOutput(t *testing.T) { + mapping := "com.example.androidobservability.MainActivityKt$$ExternalSyntheticLambda10 -> g5.g:\n" + + " 52:64:int com.example.androidobservability.CartPricing.computeTotal(java.lang.String):19:19 -> invoke\n" + + " 52:64:int com.example.androidobservability.CartPricing.priceOrder(java.lang.String):16 -> invoke\n" + + " 52:64:int com.example.androidobservability.CheckoutDemo.startCheckout(java.lang.String):12 -> invoke\n" + + frames := Parse([]byte(mapping)).Retrace("g5.g", "invoke", 57) + assert.Len(t, frames, 3) + assert.Equal(t, "com.example.androidobservability.CartPricing", frames[0].Class) + assert.Equal(t, "computeTotal", frames[0].Method) + assert.Equal(t, 19, frames[0].Line) + assert.Equal(t, "priceOrder", frames[1].Method) + assert.Equal(t, 16, frames[1].Line) + assert.Equal(t, "com.example.androidobservability.CheckoutDemo", frames[2].Class) + assert.Equal(t, "startCheckout", frames[2].Method) + assert.Equal(t, 12, frames[2].Line) +} + +// R8 renames a throwable's class like any other, so a reported exception type +// arrives obfuscated even though that class need not appear in any frame. +// androidx.core.Kept stands in for a class R8 chose to leave alone. +const exceptionTypeMapping = `# compiler: R8 +com.example.app.PaymentFailedException -> g5.a: +com.example.app.CheckoutActivity -> a.b.e: + 12:12:void pay():88:88 -> a +androidx.core.Kept -> androidx.core.Kept: +` + +func TestRetraceClass(t *testing.T) { + m := Parse([]byte(exceptionTypeMapping)) + require.NotNil(t, m) + + original, ok := m.RetraceClass("g5.a") + assert.True(t, ok) + assert.Equal(t, "com.example.app.PaymentFailedException", original) + + _, ok = m.RetraceClass("java.lang.IllegalStateException") + assert.False(t, ok, "a class outside the mapping has nothing to retrace to") +} + +// --- R8 sourceFile metadata --- + +func TestParseSourceFileComment(t *testing.T) { + cases := []struct { + name, in, want string + }{ + {"kotlin file", `# {"id":"sourceFile","fileName":"SymbolicationDemo.kt"}`, "SymbolicationDemo.kt"}, + {"java file", `# {"id":"sourceFile","fileName":"UserService.java"}`, "UserService.java"}, + // R8's own placeholders name no real file, so the derived guess is no worse. + {"synthetic class", `# {"id":"sourceFile","fileName":"R8$$SyntheticClass"}`, ""}, + {"renamed", `# {"id":"sourceFile","fileName":"SourceFile"}`, ""}, + {"other metadata", `# {"id":"com.android.tools.r8.synthesized"}`, ""}, + {"not json", `# compiler: R8`, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, parseSourceFileComment(c.in)) + }) + } +} + +// The metadata comment sits under the *obfuscated* class header but describes the +// original class, which is the name a retraced frame reports. +func TestParseReadsSourceFileMetadata(t *testing.T) { + m := Parse([]byte(`# compiler: R8 +com.example.app.CheckoutDemo -> a.b.c: +# {"id":"sourceFile","fileName":"SymbolicationDemo.kt"} + 1:3:int startCheckout(java.lang.String):12:12 -> a +com.example.app.Synthetic -> a.b.d: +# {"id":"sourceFile","fileName":"R8$$SyntheticClass"} + 1:1:void run():0:0 -> a +`)) + + assert.Equal(t, "SymbolicationDemo.kt", m.SourceFile("com.example.app.CheckoutDemo")) + assert.Empty(t, m.SourceFile("com.example.app.Synthetic"), "placeholder is not recorded") +} + +// --- R8 synthesized (compiler-generated) frames --- + +func TestParseSynthesizedComment(t *testing.T) { + cases := []struct { + name string + in string + want bool + }{ + {"marker", `# {"id":"com.android.tools.r8.synthesized"}`, true}, + {"source file", `# {"id":"sourceFile","fileName":"Checkout.kt"}`, false}, + {"residual signature", `# {"id":"com.android.tools.r8.residualsignature","signature":"(I)V"}`, false}, + {"header", `# compiler: R8`, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, parseSynthesizedComment(c.in)) + }) + } +} + +// A comment describes the entry above it, so where the marker lands depends on what +// was last read. The field case is the one that bites: ordinary classes carry +// synthesized fields, and attributing one to the class would declare the whole class +// compiler-generated. +func TestParseSynthesizedScope(t *testing.T) { + m := Parse([]byte(`com.example.app.Lambda$$ExternalSyntheticLambda0 -> a.b.c: +# {"id":"com.android.tools.r8.synthesized"} + 1:1:void run():0:0 -> a +com.example.app.Checkout -> a.b.d: + int field -> f + # {"id":"com.android.tools.r8.synthesized"} + 1:3:void pay():40:40 -> a + 5:7:void bridge():0:0 -> b + # {"id":"com.android.tools.r8.synthesized"} +`)) + + assert.True(t, m.classes["a.b.c"].synthesized, "class-level marker") + assert.False(t, m.classes["a.b.d"].synthesized, "a synthesized field must not mark its class") + assert.False(t, m.classes["a.b.d"].members["a"][0].synthesized, "unmarked member") + assert.True(t, m.classes["a.b.d"].members["b"][0].synthesized, "member-level marker") +} + +// The frame the device reports is the synthetic lambda's own `invoke`, expanded by R8 +// into the five real frames it inlined plus itself. Only that last one is +// scaffolding, and it is reported under the package from the class header rather than +// the residual "g5." R8 wrote on the member line. +func TestRetraceMarksSyntheticFrame(t *testing.T) { + frames := Parse([]byte(syntheticLambdaMapping)).Retrace("g5.g", "invoke", 57) + + require.Len(t, frames, 5) + for i, want := range []string{ + "com.example.androidobservability.CartPricing.computeTotal", + "com.example.androidobservability.CartPricing.priceOrder", + "com.example.androidobservability.CheckoutDemo.startCheckout", + "com.example.androidobservability.MainActivityKt.ErrorButtons$lambda$59$lambda$58", + } { + assert.Equal(t, want, frames[i].Class+"."+frames[i].Method) + assert.False(t, frames[i].Synthetic, "%s is code someone wrote", want) + } + + synthetic := frames[4] + assert.True(t, synthetic.Synthetic) + assert.Equal(t, "com.example.androidobservability.MainActivityKt$$ExternalSyntheticLambda10", synthetic.Class) + assert.Equal(t, "invoke", synthetic.Method) + assert.False(t, synthetic.Inlined, "the synthetic class's own frame is the physical one") +} + +// The lookups have to be safe on a mapping that was never loaded, since that is what +// a build with no symbols uploaded looks like. +func TestNilMappingAnswersNothing(t *testing.T) { + var m *Mapping + assert.Nil(t, m.Retrace("a", "b", 1)) + _, ok := m.RetraceClass("a") + assert.False(t, ok) + assert.Empty(t, m.SourceFile("a")) + assert.Zero(t, m.Classes()) +} diff --git a/internal/symbols/r8index/r8index.go b/internal/symbols/r8index/r8index.go new file mode 100644 index 00000000..2de34853 --- /dev/null +++ b/internal/symbols/r8index/r8index.go @@ -0,0 +1,177 @@ +// Package r8index reads and writes the symbolication data for an R8-obfuscated +// Android build: the `mapping.txt` R8 emits, and a random-access index over it. +// +// `ldcli symbols upload --type android` parses a build's mapping and uploads the +// index; the backend Android enhancer reads it. The two sides live in separate +// repos and duplicate this package rather than share a module, the way dsymmap and +// srcbundle already do: this directory is identical to +// backend/stacktraces/r8index in the observability repo apart from the srcbundle +// import path, and the version in the artifact's file name plus a golden file +// checked in on both sides guard against drift. +// +// Symbolication asks a mapping three things — retrace a frame, retrace a bare +// class name, name the file a class was compiled from — and one stack trace asks +// them about a few dozen classes. Answering out of the text means building all of +// it: a 61 MB release mapping becomes 10.7k classes, 50.5k method names and 480k +// member entries, retaining ~129 MB (the objects, plus the text their names alias) +// and leaving roughly a million pointers for every GC cycle to walk. An index +// answers the same three questions while decoding only the classes a trace +// mentions, holding ~5 MB and one small inflate per class. +// +// Unlike a JavaScript source map (file, line, column), R8 retrace works on (class, +// method, line): a JVM frame is ".(File:line)" and carries no column. +// +// A Mapping (parsed text) and an Index (encoded) answer identically, and resolve a +// frame through the same code below — so what the round trip has to preserve is a +// class, not an answer. +package r8index + +import "strings" + +// Frame is one original frame a retrace resolved to. A single obfuscated frame can +// produce several, when R8 inlined callees into the method the device reported. +type Frame struct { + Class string + Method string + Line int + // Inlined marks a frame R8 recorded as an inlined callee rather than the + // physical method the device reported. Such a frame's class — and therefore + // its source file — is not the reported frame's, so nothing the device said + // about the file applies to it. + Inlined bool + // Synthetic marks a frame belonging to code R8 generated. There is no source + // file to show for it and nothing it explains, so it is reported to the UI as + // background rather than dropped — a frame the device really executed. + Synthetic bool +} + +// class is one entry of a mapping, in the form both the parser and the decoder +// produce. +type class struct { + originalName string + // synthesized marks a class R8 generated rather than compiled from source, + // per its class-level "com.android.tools.r8.synthesized" marker. Kotlin + // lambdas desugar into one of these ("$$ExternalSyntheticLambda7"), + // and they show up in real stack traces even though no such class was written. + synthesized bool + // members is keyed by the *obfuscated* method name; a name can have several + // entries (overloads, distinct line ranges, inlined callees). + members map[string][]member +} + +type member struct { + hasRange bool + minStart int + minEnd int + // origClass is non-empty only for inlined frames, where R8 prefixes the + // original signature with the class the inlined code came from. + origClass string + origMethod string + hasOrig bool + origStart int + hasOrigEnd bool + origEnd int + // synthesized mirrors class.synthesized for a single member. R8 only writes + // the marker on a member's *first* occurrence, so repeated entries for the + // same synthetic method come through unmarked — which is why the class-level + // flag, not this one, is what a frame's synthetic-ness is decided by. + synthesized bool +} + +// retrace resolves a frame within one class. Both forms of a mapping call this, +// which is the whole point of decoding a block into the same type the parser +// builds: only how the class was obtained differs. +func (c *class) retrace(obfMethod string, line int) []Frame { + members := c.members[obfMethod] + if len(members) == 0 { + // Class known but method name not remapped (e.g. a kept/native method): + // still deobfuscate the class. + return []Frame{{ + Class: c.originalName, + Method: obfMethod, + Line: line, + Synthetic: c.synthesized, + }} + } + + // Prefer entries whose obfuscated line range covers the frame's line; these + // carry the precise original line (and any inlining). + var matched []member + for _, mem := range members { + if mem.hasRange && line >= mem.minStart && line <= mem.minEnd { + matched = append(matched, mem) + } + } + if len(matched) == 0 { + // No ranged match: fall back to the first name mapping so the method is + // still deobfuscated even without exact line info. + return []Frame{c.frameFor(members[0], line)} + } + + out := make([]Frame, 0, len(matched)) + for _, mem := range matched { + out = append(out, c.frameFor(mem, mem.originalLine(line))) + } + return out +} + +// frameFor builds the frame one member entry describes, at an already-resolved +// original line. +func (c *class) frameFor(mem member, line int) Frame { + name, own := c.resolveClass(mem) + return Frame{ + Class: name, + Method: mem.origMethod, + Line: line, + Inlined: !own, + Synthetic: mem.synthesized || (c.synthesized && own), + } +} + +// resolveClass returns the class a member entry belongs to, and whether that is +// the mapping entry's own class rather than an inlined callee's. +// +// A class writes its own methods bare and prefixes only the foreign classes it +// inlined, so a prefix normally means "a different class". A synthesized class is +// the exception: R8 prefixes even its own methods, and with the class's *residual* +// name ("g5.MainActivityKt$$ExternalSyntheticLambda10"), which carries the +// obfuscated package. The header's original name is the one worth reporting, so +// that a synthetic frame at least names the package it really came from. +func (c *class) resolveClass(mem member) (name string, own bool) { + if mem.origClass == "" { + return c.originalName, true + } + if c.synthesized && simpleClassName(mem.origClass) == simpleClassName(c.originalName) { + return c.originalName, true + } + return mem.origClass, false +} + +// simpleClassName drops the package from a (possibly nested) class name. +func simpleClassName(name string) string { + if i := strings.LastIndex(name, "."); i >= 0 { + return name[i+1:] + } + return name +} + +// originalLine maps an obfuscated line to the original line for a ranged member. +// A linear range (original span == obfuscated span) shifts by the offset; a +// collapsed range (inlined / single original line) maps everything to the start. +func (mem member) originalLine(line int) int { + if !mem.hasOrig { + return line + } + if mem.hasOrigEnd && (mem.origEnd-mem.origStart) == (mem.minEnd-mem.minStart) { + return mem.origStart + (line - mem.minStart) + } + return mem.origStart +} + +// isSourceFileName reports whether a name is a source file a JVM build compiles +// from, as opposed to one of R8's placeholders. Duplicated from the enhancer's +// copy so this package stays self-contained enough to be copied between repos. +func isSourceFileName(name string) bool { + lower := strings.ToLower(name) + return strings.HasSuffix(lower, ".java") || strings.HasSuffix(lower, ".kt") +} diff --git a/internal/symbols/r8index/read.go b/internal/symbols/r8index/read.go new file mode 100644 index 00000000..3ddd7338 --- /dev/null +++ b/internal/symbols/r8index/read.go @@ -0,0 +1,267 @@ +package r8index + +// Reading an index: a binary search for the class a frame names, one inflate of its +// block, and the same retrace the parsed form runs. + +import ( + "bytes" + "encoding/binary" + "sort" + "sync" + + "github.com/launchdarkly/ldcli/internal/symbols/srcbundle" + e "github.com/pkg/errors" +) + +// memoLimit caps the decoded classes an index keeps. The classes a build's traces +// hit are a small, stable set that gets in first, and the cap is what keeps an index +// that is queried for long enough from growing into the parsed mapping it exists to +// avoid. +const memoLimit = 512 + +// Index answers a mapping's lookups out of encoded bytes. Blocks are read through +// srcbundle.Read rather than File so the only thing held is the bounded memo below. +type Index struct { + bundle *srcbundle.Bundle + + mu sync.Mutex + classes map[string]*class +} + +// Open returns a view over raw, which must stay alive and unmodified for as long as +// the index is used — srcbundle.Open does not copy. +func Open(raw []byte) (*Index, error) { + bundle, err := srcbundle.Open(raw) + if err != nil { + return nil, e.Wrap(err, "r8index: opening bundle") + } + return &Index{bundle: bundle}, nil +} + +// Retrace resolves an obfuscated (class, method, line), as Mapping.Retrace does. +func (ix *Index) Retrace(obfClass, obfMethod string, line int) []Frame { + if ix == nil { + return nil + } + c := ix.class(obfClass) + if c == nil { + return nil + } + return c.retrace(obfMethod, line) +} + +// RetraceClass deobfuscates a bare class name, as Mapping.RetraceClass does. +func (ix *Index) RetraceClass(obfClass string) (string, bool) { + if ix == nil { + return "", false + } + c := ix.class(obfClass) + if c == nil || c.originalName == "" { + return "", false + } + return c.originalName, true +} + +// SourceFile reports the file an *original* class name was compiled from. This is +// the one entry worth memoizing (via File rather than Read): there is exactly one of +// it, every frame needs it, and it is searched where it lies rather than decoded +// into anything. +func (ix *Index) SourceFile(originalClass string) string { + if ix == nil { + return "" + } + table, ok := ix.bundle.File(sourceKey) + if !ok || len(table) < 4 { + return "" + } + count := int(binary.LittleEndian.Uint32(table)) + if count < 0 || 4+count*srcRecSize > len(table) { + return "" + } + + nameOff := func(i int) uint32 { + return binary.LittleEndian.Uint32(table[4+i*srcRecSize:]) + } + i := sort.Search(count, func(i int) bool { + return compareString(table, nameOff(i), originalClass) >= 0 + }) + if i >= count || compareString(table, nameOff(i), originalClass) != 0 { + return "" + } + return tableString(table, binary.LittleEndian.Uint32(table[4+i*srcRecSize+4:])) +} + +// class decodes one class block. A missing or unreadable block is remembered as a +// miss, so a frame from a stale mapping costs one lookup rather than one per frame. +func (ix *Index) class(obfClass string) *class { + ix.mu.Lock() + defer ix.mu.Unlock() + + if c, ok := ix.classes[obfClass]; ok { + return c + } + var c *class + if block, ok := ix.bundle.Read(classPrefix + obfClass); ok { + if decoded, ok := decodeClass(block); ok { + c = decoded + } + } + if ix.classes == nil { + ix.classes = make(map[string]*class) + } + if len(ix.classes) < memoLimit { + ix.classes[obfClass] = c + } + return c +} + +func decodeClass(data []byte) (*class, bool) { + cur := &cursor{buf: data} + c := &class{originalName: cur.string()} + c.synthesized = cur.byte()&1 != 0 + + // A count is bounded by what is left to read, so a corrupt one cannot ask for + // an allocation the block could not possibly describe. + methods, ok := cur.count() + if !ok { + return nil, false + } + c.members = make(map[string][]member, methods) + for range methods { + name := cur.string() + entries, ok := cur.count() + if !ok { + return nil, false + } + members := make([]member, 0, entries) + for range entries { + members = append(members, decodeMember(cur)) + } + if cur.err { + return nil, false + } + c.members[name] = members + } + if cur.err { + return nil, false + } + return c, true +} + +func decodeMember(cur *cursor) member { + flags := cur.byte() + var mem member + mem.hasRange = flags&1 != 0 + mem.hasOrig = flags&2 != 0 + mem.hasOrigEnd = flags&4 != 0 + mem.synthesized = flags&8 != 0 + + if mem.hasRange { + mem.minStart = int(cur.varint()) + mem.minEnd = int(cur.varint()) + } + if mem.hasOrig { + mem.origStart = int(cur.varint()) + } + if mem.hasOrigEnd { + mem.origEnd = int(cur.varint()) + } + mem.origClass = cur.string() + mem.origMethod = cur.string() + return mem +} + +// cursor reads a block, latching the first read that ran past the end so callers can +// decode straight through and check once. +type cursor struct { + buf []byte + err bool +} + +func (c *cursor) byte() byte { + if len(c.buf) < 1 { + c.err = true + return 0 + } + b := c.buf[0] + c.buf = c.buf[1:] + return b +} + +func (c *cursor) uvarint() uint64 { + v, n := binary.Uvarint(c.buf) + if n <= 0 { + c.err = true + return 0 + } + c.buf = c.buf[n:] + return v +} + +func (c *cursor) varint() int64 { + v, n := binary.Varint(c.buf) + if n <= 0 { + c.err = true + return 0 + } + c.buf = c.buf[n:] + return v +} + +func (c *cursor) string() string { + n := c.uvarint() + if c.err || n > uint64(len(c.buf)) { + c.err = true + return "" + } + s := string(c.buf[:n]) + c.buf = c.buf[n:] + return s +} + +// count reads a length that is about to size an allocation, rejecting one larger +// than the bytes left could describe. +func (c *cursor) count() (int, bool) { + n := c.uvarint() + if c.err || n > uint64(len(c.buf)) { + c.err = true + return 0, false + } + return int(n), true +} + +// compareString compares the NUL-terminated string at off against s without +// building a string to compare with. +func compareString(table []byte, off uint32, s string) int { + i := int(off) + if i >= len(table) { + return -1 + } + for k := 0; k < len(s); k++ { + if i >= len(table) || table[i] == 0 { + return -1 + } + if table[i] != s[k] { + if table[i] < s[k] { + return -1 + } + return 1 + } + i++ + } + if i < len(table) && table[i] != 0 { + return 1 + } + return 0 +} + +func tableString(table []byte, off uint32) string { + if int(off) >= len(table) { + return "" + } + s := table[off:] + if i := bytes.IndexByte(s, 0); i >= 0 { + return string(s[:i]) + } + return string(s) +} diff --git a/internal/symbols/r8index/stream_test.go b/internal/symbols/r8index/stream_test.go new file mode 100644 index 00000000..47426ed0 --- /dev/null +++ b/internal/symbols/r8index/stream_test.go @@ -0,0 +1,174 @@ +package r8index + +import ( + "bytes" + "fmt" + "os" + "runtime" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Streaming is an optimisation, so it has to be invisible: the same mapping has to +// encode to the same bytes whichever way it was read. Anything else would make the +// index no longer addressable by the mapping's content id. +func TestEncodeFromMatchesEncode(t *testing.T) { + for name, mapping := range map[string]string{ + "sample": sampleMapping, + "synthetic lambda": syntheticLambdaMapping, + "exception types": exceptionTypeMapping, + "crlf line ends": strings.ReplaceAll(sampleMapping, "\n", "\r\n"), + "no trailing newline": strings.TrimSuffix(`com.example.Cart -> a: +# {"id":"sourceFile","fileName":"Cart.kt"} + 1:1:void a():1:1 -> a +`, "\n"), + } { + t.Run(name, func(t *testing.T) { + want, err := Encode(Parse([]byte(mapping))) + require.NoError(t, err) + + got, err := EncodeFrom(strings.NewReader(mapping)) + require.NoError(t, err) + + assert.Equal(t, want, got) + }) + } +} + +// A file that is not a mapping has to be refused rather than turned into an index of +// nothing: the upload would otherwise succeed and the build would look symbolicated. +func TestEncodeFromRejectsInputWithoutClasses(t *testing.T) { + for name, in := range map[string]string{ + "empty": "", + "prose": "this is not a mapping\n", + "header only": "# compiler: R8\n# compiler_version: 8.5.35\n", + "members only": " 1:3:void run():2:2 -> a\n", + } { + t.Run(name, func(t *testing.T) { + _, err := EncodeFrom(strings.NewReader(in)) + assert.Error(t, err) + }) + } +} + +// A line longer than bufio's default must not fail the upload, since how long a +// member line gets is up to the signatures in somebody's build. +func TestEncodeFromReadsVeryLongLines(t *testing.T) { + args := strings.Repeat("java.lang.String,", 8*1024) + mapping := fmt.Sprintf("com.example.Wide -> a:\n 1:1:void run(%s):2:2 -> a\n", args) + require.Greater(t, len(mapping), 64*1024) + + raw, err := EncodeFrom(strings.NewReader(mapping)) + require.NoError(t, err) + + ix, err := Open(raw) + require.NoError(t, err) + frames := ix.Retrace("a", "a", 1) + require.Len(t, frames, 1) + assert.Equal(t, "com.example.Wide", frames[0].Class) + assert.Equal(t, "run", frames[0].Method) +} + +func TestEncodeFromReportsReadFailure(t *testing.T) { + _, err := EncodeFrom(failingReader{}) + assert.Error(t, err) +} + +type failingReader struct{} + +func (failingReader) Read([]byte) (int, error) { return 0, assert.AnError } + +// The point of streaming: what it costs to write an index should be the index rather +// than the mapping. What matters is the high-water mark, not what is left at the end +// — both ways end holding the same index — so this samples the live heap while the +// work runs instead of measuring it afterwards. +// +// R8_MAPPING=/path/to/mapping.txt go test ./backend/stacktraces/r8index/ -run RealMappingStream -v +func TestRealMappingStreamFootprint(t *testing.T) { + data := realMapping(t) + + var viaParse, viaStream []byte + parseAndEncode := peakHeapMB(func() { + var err error + viaParse, err = Encode(Parse(data)) + require.NoError(t, err) + }) + stream := peakHeapMB(func() { + var err error + viaStream, err = EncodeFrom(bytes.NewReader(data)) + require.NoError(t, err) + }) + + require.Equal(t, viaParse, viaStream, "streaming must not change the bytes") + + fmt.Printf("\nmapping.txt : %6.1f MB\npeak, parse then encode : %6.1f MB\npeak, streaming : %6.1f MB\n", + float64(len(data))/(1<<20), parseAndEncode, stream) + assert.Less(t, stream, parseAndEncode, "streaming should peak below the parse it replaces") +} + +// peakHeapMB is the highest live heap seen while f ran, over the bytes already +// allocated when it started. Sampled rather than read at the end, because the whole +// question is what the middle of the run needed. +func peakHeapMB(f func()) float64 { + runtime.GC() + var start runtime.MemStats + runtime.ReadMemStats(&start) + + var peak atomic.Uint64 + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + defer close(stopped) + for { + var ms runtime.MemStats + runtime.ReadMemStats(&ms) + if ms.HeapAlloc > peak.Load() { + peak.Store(ms.HeapAlloc) + } + select { + case <-done: + return + case <-time.After(time.Millisecond): + } + } + }() + + f() + close(done) + <-stopped + + if peak.Load() < start.HeapAlloc { + return 0 + } + return float64(peak.Load()-start.HeapAlloc) / (1 << 20) +} + +func TestEncodeFromRealMappingIsReadable(t *testing.T) { + path := os.Getenv("R8_MAPPING") + if path == "" { + t.Skip("set R8_MAPPING to a release mapping.txt") + } + file, err := os.Open(path) + require.NoError(t, err) + defer file.Close() + + raw, err := EncodeFrom(file) + require.NoError(t, err) + + ix, err := Open(raw) + require.NoError(t, err) + + // Every class the parser found has to be there, answering the same thing. + m := Parse(realMapping(t)) + for obf := range m.classes { + wantName, wantOK := m.RetraceClass(obf) + gotName, gotOK := ix.RetraceClass(obf) + require.Equal(t, wantOK, gotOK, obf) + require.Equal(t, wantName, gotName, obf) + } +} diff --git a/internal/symbols/srcbundle/srcbundle.go b/internal/symbols/srcbundle/srcbundle.go index 9258237f..15848b9b 100644 --- a/internal/symbols/srcbundle/srcbundle.go +++ b/internal/symbols/srcbundle/srcbundle.go @@ -82,7 +82,18 @@ var le = binary.LittleEndian // Builder accumulates source files for one image and encodes them. Add is // keyed by the same path string the sibling .dsymmap records. type Builder struct { - files map[string][]byte + files map[string]entry + // err latches the first compression failure, so Add can stay a statement. + err error +} + +// entry is one added file, already compressed. Holding the compressed form is what +// bounds a builder to roughly the size of what it is about to write: an R8 mapping +// index adds an entry per class, and those blocks together are several times the +// bundle they end up in. +type entry struct { + data []byte + rawLen int } // Add registers one source file's contents under path. Empty paths and repeats @@ -92,12 +103,28 @@ func (b *Builder) Add(path string, content []byte) { return } if b.files == nil { - b.files = make(map[string][]byte) + b.files = make(map[string]entry) } if _, ok := b.files[path]; ok { return } - b.files[path] = content + var buf bytes.Buffer + zw := gzip.NewWriter(&buf) + if _, err := zw.Write(content); err != nil { + b.latch(e.Wrapf(err, "srcbundle: compressing %s", path)) + return + } + if err := zw.Close(); err != nil { + b.latch(e.Wrapf(err, "srcbundle: finishing %s", path)) + return + } + b.files[path] = entry{data: buf.Bytes(), rawLen: len(content)} +} + +func (b *Builder) latch(err error) { + if b.err == nil { + b.err = err + } } // Len is the number of distinct files added. @@ -105,6 +132,10 @@ func (b *Builder) Len() int { return len(b.files) } // Encode writes the bundle. Paths are sorted so the reader can binary search. func (b *Builder) Encode(w io.Writer) error { + if b.err != nil { + return b.err + } + paths := make([]string, 0, len(b.files)) for p := range b.files { paths = append(paths, p) @@ -121,21 +152,15 @@ func (b *Builder) Encode(w io.Writer) error { strtab.WriteString(p) strtab.WriteByte(0) - raw := b.files[p] + file := b.files[p] dataOff := uint32(payload.Len()) - zw := gzip.NewWriter(&payload) - if _, err := zw.Write(raw); err != nil { - return e.Wrapf(err, "srcbundle: compressing %s", p) - } - if err := zw.Close(); err != nil { - return e.Wrapf(err, "srcbundle: finishing %s", p) - } + payload.Write(file.data) rec := make([]byte, indexRecSize) le.PutUint32(rec[0:], pathOff) le.PutUint32(rec[4:], dataOff) - le.PutUint32(rec[8:], uint32(payload.Len())-dataOff) - le.PutUint32(rec[12:], uint32(len(raw))) + le.PutUint32(rec[8:], uint32(len(file.data))) + le.PutUint32(rec[12:], uint32(file.rawLen)) index = append(index, rec...) } @@ -284,6 +309,14 @@ func (b *Bundle) File(path string) ([]byte, bool) { return out, true } +// Read is File without the memo, for a caller that caches what it decoded itself, +// or that queries enough distinct entries that remembering every one of them would +// grow into the whole bundle. It touches only the immutable view, so it is safe to +// call concurrently. +func (b *Bundle) Read(path string) ([]byte, bool) { + return b.readFile(path) +} + func (b *Bundle) readFile(path string) ([]byte, bool) { i, ok := b.find(path) if !ok { From 799bbd6c747d6d1f25f95469d30ccc7be5f82a45 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Thu, 30 Jul 2026 23:18:02 -0700 Subject: [PATCH 05/10] test(symbols): pin the R8 index format with a golden fixture The r8index package exists twice, here and in the backend that reads what this writes, and an index is only interchangeable if the two copies agree byte for byte. Two copies cannot import each other's tests, so what they share is a fixture: a readable mapping and the exact index bytes it encodes to, identical in both repos. Either side drifting now fails here rather than in production, where the symptom would be an unreadable index or a silently wrong frame. Co-authored-by: Cursor --- internal/symbols/r8index/golden_test.go | 115 ++++++++++++++++++ .../r8index/testdata/golden.mapping.txt | 22 ++++ .../symbols/r8index/testdata/golden.v1.index | Bin 0 -> 730 bytes 3 files changed, 137 insertions(+) create mode 100644 internal/symbols/r8index/golden_test.go create mode 100644 internal/symbols/r8index/testdata/golden.mapping.txt create mode 100644 internal/symbols/r8index/testdata/golden.v1.index diff --git a/internal/symbols/r8index/golden_test.go b/internal/symbols/r8index/golden_test.go new file mode 100644 index 00000000..af5e62b2 --- /dev/null +++ b/internal/symbols/r8index/golden_test.go @@ -0,0 +1,115 @@ +package r8index + +// The golden fixture: one mapping, and the exact bytes it encodes to. +// +// This package exists twice, in the CLI that writes indexes and the backend that reads +// them, and an index is only interchangeable between them if the two copies agree byte +// for byte. Two copies cannot import each other's tests, so what they share is a +// fixture: this file and the testdata beside it are identical in both repos, and either +// side drifting fails there rather than in production, where the symptom would be an +// unreadable index or a silently wrong frame. +// +// After an intentional format change, regenerate with +// +// go test ./... -run TestGolden -update +// +// and copy testdata/ to the other repo in the same change. A format change also needs a +// new version in the artifact's file name, since readers pick an object by that name +// and an old index stays readable by the reader that was built for it. + +import ( + "flag" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var updateGolden = flag.Bool("update", false, "rewrite the golden index from the fixture mapping") + +const ( + goldenMappingPath = "testdata/golden.mapping.txt" + goldenIndexPath = "testdata/golden.v1.index" +) + +func goldenMapping(t *testing.T) []byte { + t.Helper() + data, err := os.ReadFile(goldenMappingPath) + require.NoError(t, err) + return data +} + +// Both encoders produce the golden bytes: the streaming one the CLI uses, and the +// whole-mapping one the tests here encode fixtures with. +func TestGoldenIndexBytes(t *testing.T) { + mapping := goldenMapping(t) + + streamed, err := EncodeFrom(strings.NewReader(string(mapping))) + require.NoError(t, err) + + if *updateGolden { + require.NoError(t, os.WriteFile(goldenIndexPath, streamed, 0o644)) + t.Log("wrote", goldenIndexPath) + } + + want, err := os.ReadFile(goldenIndexPath) + require.NoError(t, err) + assert.Equal(t, want, streamed, + "the encoder no longer reproduces the golden index. If the format changed on purpose, re-run with -update, copy testdata/ to the other repo, and give the artifact a new version in its file name") + + parsed, err := Encode(Parse(mapping)) + require.NoError(t, err) + assert.Equal(t, want, parsed, "the two encoders must agree, or which one built an index would matter") +} + +// The bytes are only worth pinning if they answer correctly, and these are the answers +// both sides depend on: a line inside a range, a collapsed range, a name-only member, +// an inlined chain, a class with no members, and R8's recorded source file. +func TestGoldenIndexAnswers(t *testing.T) { + raw, err := os.ReadFile(goldenIndexPath) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + frames := ix.Retrace("a.b.c", "a", 2) + require.Len(t, frames, 1) + assert.Equal(t, "com.example.app.UserService", frames[0].Class) + assert.Equal(t, "loadUser", frames[0].Method) + assert.Equal(t, 41, frames[0].Line, "line 2 of range 1:3 is original 40 + 1") + + frames = ix.Retrace("a.b.c", "b", 5) + require.Len(t, frames, 1) + assert.Equal(t, 60, frames[0].Line, "a collapsed range answers one line whatever it is asked") + + frames = ix.Retrace("a.b.c", "d", 99) + require.Len(t, frames, 1) + assert.Equal(t, "helper", frames[0].Method) + assert.Equal(t, 99, frames[0].Line, "a name-only member cannot move a line") + + frames = ix.Retrace("a.b.d", "b", 20) + require.Len(t, frames, 2) + assert.Equal(t, "handleClick", frames[0].Method) + assert.Equal(t, "com.example.app.Analytics", frames[1].Class) + assert.Equal(t, "track", frames[1].Method) + assert.True(t, frames[1].Inlined) + + // One physical frame in a synthetic lambda, three original frames. + frames = ix.Retrace("g5.g", "invoke", 57) + require.Len(t, frames, 3) + assert.Equal(t, "computeTotal", frames[0].Method) + assert.Equal(t, 19, frames[0].Line) + assert.Equal(t, "priceOrder", frames[1].Method) + assert.Equal(t, "startCheckout", frames[2].Method) + + original, ok := ix.RetraceClass("g5.a") + assert.True(t, ok) + assert.Equal(t, "com.example.app.PaymentFailedException", original, + "a class with no members is still a name a thrown type resolves through") + + assert.Equal(t, "UserService.java", ix.SourceFile("com.example.app.UserService")) + assert.Equal(t, "SymbolicationDemo.kt", ix.SourceFile("com.example.app.MainActivity")) + assert.Empty(t, ix.SourceFile("com.example.app.MainActivityKt$$ExternalSyntheticLambda10"), + "R8's synthetic placeholder names no file") +} diff --git a/internal/symbols/r8index/testdata/golden.mapping.txt b/internal/symbols/r8index/testdata/golden.mapping.txt new file mode 100644 index 00000000..5aced0c3 --- /dev/null +++ b/internal/symbols/r8index/testdata/golden.mapping.txt @@ -0,0 +1,22 @@ +# compiler: R8 +# compiler_version: 8.2.47 +# min_api: 24 +com.example.app.UserService -> a.b.c: +# {"id":"sourceFile","fileName":"UserService.java"} + 1:3:com.example.app.User loadUser(java.lang.String):40:42 -> a + 5:5:void validate():60:60 -> b + void helper() -> d +com.example.app.MainActivity -> a.b.d: +# {"id":"sourceFile","fileName":"SymbolicationDemo.kt"} + 10:10:void onCreate(android.os.Bundle):100:100 -> a + 20:20:void handleClick():5:5 -> b + 20:20:void com.example.app.Analytics.track():15:15 -> b +com.example.app.PaymentFailedException -> g5.a: +com.example.app.MainActivityKt$$ExternalSyntheticLambda10 -> g5.g: +# {"id":"sourceFile","fileName":"R8$$SyntheticClass"} +# {"id":"com.android.tools.r8.synthesized"} + 1:1:void g5.MainActivityKt$$ExternalSyntheticLambda10.(g5.MainActivityViewModel,int):0:0 -> + # {"id":"com.android.tools.r8.synthesized"} + 52:64:int com.example.app.CartPricing.computeTotal(java.lang.String):19:19 -> invoke + 52:64:int com.example.app.CartPricing.priceOrder(java.lang.String):16 -> invoke + 52:64:int com.example.app.CheckoutDemo.startCheckout(java.lang.String):12 -> invoke diff --git a/internal/symbols/r8index/testdata/golden.v1.index b/internal/symbols/r8index/testdata/golden.v1.index new file mode 100644 index 0000000000000000000000000000000000000000..a1252d2be962f9cc0a9cc837f7a8046e73b8c729 GIT binary patch literal 730 zcmWFza&}^5U}RurU|>)H;sPL61mcN6?8(Fcl!1U0AmsR{(J?5DNnF9UyiD zVoe~H0Ag97roBK~07%ON>03ZLA4nGiF+;L`qF$0-GL%jM(&?spi4ZCss6@V-1Eh?B z;lIeqi=2lQ1lS(z77K}4Xl>!W@GOJ)U&TY6?VOK(D4xHs{(ahpB{P*eBX9Zj$RArH z`6Vs!f6;w=*0(`&kLK3)WCzYzwDkn5p3f7$6^VC%PWb;nR{cmc$P)-VCV8Gaf7N@f z#~Ixdo>#qfG%kGhJ#(hmNAIN0X`PiR2}wRFDdK{b2P>K;HY(en->G4`QDv%OXJ@Q) zb9Z;6wX&hUs`^z+bz{rfvChtZenNK*j_)}#=hUP#Pm-=pT6E+|jswUe|I=p83I_!i z!rmZnG<&%{)|~M@t6k`Q=8UIr;aU$rU5%4x&L<}@uz(EkH}P=>g)YK?O+IJ_AUphv zUSMG1S`AO#Q{HDd*PYQm>7jel=bPslUoTG+lZ2F{%_q;C*4dc+Bt0o9;ew~RYMg$E zdURS`WL=q-akU%s;#spyb_LH7?)GBpGMa4s`BLSi&&Awc-a*cvyoHKq7)wcrepvZT zOXHM}-&cJvPhF;!Qzsk@m_2dUoVoE)k0#BUeLgy7QbY)oFeBLU(;pmo15OI?m`yot z$aN@yr{SUe`3IIva*hJ#%O5dba&ODHY?D=S(|y)y!EL*v$Nvzr Date: Thu, 30 Jul 2026 23:21:10 -0700 Subject: [PATCH 06/10] test(symbols): cross-check a CLI-built index against its mapping The golden fixture proves the encoder here still produces the bytes both repos agreed on; this proves the artifact a build machine wrote answers like the mapping it came from, which is the guarantee symbolication actually rests on. Opt-in via R8_MAPPING and R8_INDEX, since it needs a real release build. Co-authored-by: Cursor --- internal/symbols/r8index/realindex_test.go | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 internal/symbols/r8index/realindex_test.go diff --git a/internal/symbols/r8index/realindex_test.go b/internal/symbols/r8index/realindex_test.go new file mode 100644 index 00000000..b76ed3f0 --- /dev/null +++ b/internal/symbols/r8index/realindex_test.go @@ -0,0 +1,55 @@ +package r8index + +// An opt-in check that an index a build machine actually produced answers the way this +// reader expects, over a real release mapping rather than a fixture: +// +// R8_MAPPING=.../mapping.txt R8_INDEX=.../mapping.v1.index \ +// go test ./backend/stacktraces/r8index/ -run RealIndex -v +// +// The mapping is the oracle. The golden fixture makes the same guarantee automatic, but +// only over bytes this repo encoded; this is what closes the loop on an artifact that +// crossed the boundary — written by the CLI, read here. + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRealIndexMatchesMapping(t *testing.T) { + indexPath := os.Getenv("R8_INDEX") + if indexPath == "" { + t.Skip("set R8_INDEX to an index built by the CLI, and R8_MAPPING to the mapping it was built from") + } + raw, err := os.ReadFile(indexPath) + require.NoError(t, err) + ix, err := Open(raw) + require.NoError(t, err) + + m := Parse(realMapping(t)) + require.NotZero(t, m.Classes()) + + frames := 0 + for obf, c := range m.classes { + gotName, gotOK := ix.RetraceClass(obf) + wantName, wantOK := m.RetraceClass(obf) + require.Equal(t, wantOK, gotOK, obf) + require.Equal(t, wantName, gotName, obf) + if wantOK { + require.Equal(t, m.SourceFile(wantName), ix.SourceFile(wantName), wantName) + } + + for obfMethod, members := range c.members { + for _, member := range members { + // The first line of each recorded range, which is where a frame that + // arrives from a device lands. + line := member.minStart + require.Equal(t, m.Retrace(obf, obfMethod, line), ix.Retrace(obf, obfMethod, line), + "%s.%s:%d", obf, obfMethod, line) + frames++ + } + } + } + t.Logf("%d classes, %d frames answered identically through %s", m.Classes(), frames, indexPath) +} From 66ba5c742c91c9ca22c92e43c2f25b29dc9aea3b Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Fri, 31 Jul 2026 14:17:11 -0700 Subject: [PATCH 07/10] test(symbols): name the real mapping to check against with a flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keeps the r8index copy in step with the backend's, where reading these paths from R8_MAPPING and R8_INDEX fails a CI check that bans os.Getenv anywhere in the backend. A test flag is what the package already reaches for anyway (-update, for the golden fixture). end-of-file-fixer runs here too, through pre-commit, and it appended a newline to the backend's copy of the golden index — a byte that moves the footer the reader locates the index by, and breaks the byte-for-byte agreement the fixture exists to prove. It cannot tell a binary file from a text one, so *.index is excluded. Co-authored-by: Cursor --- .pre-commit-config.yaml | 3 ++ internal/symbols/r8index/codec_test.go | 34 +++++++++++++++------- internal/symbols/r8index/realindex_test.go | 10 +++---- internal/symbols/r8index/stream_test.go | 8 ++--- 4 files changed, 33 insertions(+), 22 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 58226af2..55acc646 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,3 +12,6 @@ repos: rev: v4.6.0 hooks: - id: end-of-file-fixer + # The golden R8 index is binary, and the fixer cannot tell. A newline + # appended to it moves the footer the reader locates the index by. + exclude: \.index$ diff --git a/internal/symbols/r8index/codec_test.go b/internal/symbols/r8index/codec_test.go index 1300dfa6..23f55e86 100644 --- a/internal/symbols/r8index/codec_test.go +++ b/internal/symbols/r8index/codec_test.go @@ -1,6 +1,7 @@ package r8index import ( + "flag" "fmt" "os" "runtime" @@ -213,16 +214,30 @@ func TestNilIndexAnswersNothing(t *testing.T) { assert.Empty(t, ix.SourceFile("a")) } -// realMapping reads the mapping named by R8_MAPPING, for the opt-in tests below: +// The checks against a real build are opt-in, since they need one: // -// R8_MAPPING=/path/to/mapping.txt go test ./backend/stacktraces/r8index/ -run RealMapping -v -func realMapping(t *testing.T) []byte { +// go test ./backend/stacktraces/r8index/ -run RealMapping -v -mapping /path/to/mapping.txt +// +// Flags rather than environment variables, so that `go test -args -h` lists them, and +// because os.Getenv is banned throughout the backend. +var ( + realMappingPath = flag.String("mapping", "", "path to a release mapping.txt, for the opt-in checks against a real build") + realIndexPath = flag.String("index", "", "path to a mapping.v1.index the CLI built from -mapping") +) + +// realMappingFile names the mapping to check against, skipping when there is none. For +// the tests that read it themselves, either to time the reading or to stream it. +func realMappingFile(t *testing.T) string { t.Helper() - path := os.Getenv("R8_MAPPING") - if path == "" { - t.Skip("set R8_MAPPING to a release mapping.txt") + if *realMappingPath == "" { + t.Skip("pass -mapping to run this") } - data, err := os.ReadFile(path) + return *realMappingPath +} + +func realMapping(t *testing.T) []byte { + t.Helper() + data, err := os.ReadFile(realMappingFile(t)) require.NoError(t, err) return data } @@ -281,10 +296,7 @@ func TestRealMapping(t *testing.T) { // memory to answer lookups for one build. Each half takes its own baseline before it // allocates anything, so neither is measured against the other's leftovers. func TestRealMappingFootprint(t *testing.T) { - path := os.Getenv("R8_MAPPING") - if path == "" { - t.Skip("set R8_MAPPING to a release mapping.txt") - } + path := realMappingFile(t) var withMapping, withIndex float64 diff --git a/internal/symbols/r8index/realindex_test.go b/internal/symbols/r8index/realindex_test.go index b76ed3f0..2d17297b 100644 --- a/internal/symbols/r8index/realindex_test.go +++ b/internal/symbols/r8index/realindex_test.go @@ -3,8 +3,8 @@ package r8index // An opt-in check that an index a build machine actually produced answers the way this // reader expects, over a real release mapping rather than a fixture: // -// R8_MAPPING=.../mapping.txt R8_INDEX=.../mapping.v1.index \ -// go test ./backend/stacktraces/r8index/ -run RealIndex -v +// go test ./backend/stacktraces/r8index/ -run RealIndex -v \ +// -mapping .../mapping.txt -index .../mapping.v1.index // // The mapping is the oracle. The golden fixture makes the same guarantee automatic, but // only over bytes this repo encoded; this is what closes the loop on an artifact that @@ -18,10 +18,10 @@ import ( ) func TestRealIndexMatchesMapping(t *testing.T) { - indexPath := os.Getenv("R8_INDEX") - if indexPath == "" { - t.Skip("set R8_INDEX to an index built by the CLI, and R8_MAPPING to the mapping it was built from") + if *realIndexPath == "" { + t.Skip("pass -index and -mapping ") } + indexPath := *realIndexPath raw, err := os.ReadFile(indexPath) require.NoError(t, err) ix, err := Open(raw) diff --git a/internal/symbols/r8index/stream_test.go b/internal/symbols/r8index/stream_test.go index 47426ed0..fbbe7c91 100644 --- a/internal/symbols/r8index/stream_test.go +++ b/internal/symbols/r8index/stream_test.go @@ -88,7 +88,7 @@ func (failingReader) Read([]byte) (int, error) { return 0, assert.AnError } // — both ways end holding the same index — so this samples the live heap while the // work runs instead of measuring it afterwards. // -// R8_MAPPING=/path/to/mapping.txt go test ./backend/stacktraces/r8index/ -run RealMappingStream -v +// go test ./backend/stacktraces/r8index/ -run RealMappingStream -v -mapping /path/to/mapping.txt func TestRealMappingStreamFootprint(t *testing.T) { data := realMapping(t) @@ -149,11 +149,7 @@ func peakHeapMB(f func()) float64 { } func TestEncodeFromRealMappingIsReadable(t *testing.T) { - path := os.Getenv("R8_MAPPING") - if path == "" { - t.Skip("set R8_MAPPING to a release mapping.txt") - } - file, err := os.Open(path) + file, err := os.Open(realMappingFile(t)) require.NoError(t, err) defer file.Close() From c4a89755867ec35c5585c5fb2ac8332acf581d48 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Fri, 31 Jul 2026 17:00:49 -0700 Subject: [PATCH 08/10] fix(symbols): accept --source-path on `symbols generate` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generating an Android source bundle reads sourcePathFlag, which generate never registered: --source-path was rejected outright, so sources could only be scanned from the working directory. The read answered anyway, because both commands bind the same viper keys and upload's flag was left to supply the default — a value the caller of generate could neither see nor change. Also stop describing --include-sources as Apple-only, since an Android mapping carries sources now too. Co-authored-by: Cursor --- cmd/symbols/generate.go | 5 ++++- cmd/symbols/generate_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 cmd/symbols/generate_test.go diff --git a/cmd/symbols/generate.go b/cmd/symbols/generate.go index 834011f6..64fc64a5 100644 --- a/cmd/symbols/generate.go +++ b/cmd/symbols/generate.go @@ -235,9 +235,12 @@ func initGenerateFlags(cmd *cobra.Command) { cmd.Flags().String(appVersionFlag, "", "The current version of your deploy") _ = viper.BindPFlag(appVersionFlag, cmd.Flags().Lookup(appVersionFlag)) - cmd.Flags().Bool(includeSourcesFlag, false, fmt.Sprintf("Also generate a source bundle from the files referenced by the debug info, for source context on native frames (%s only)", typeAppleDSYM)) + cmd.Flags().Bool(includeSourcesFlag, false, fmt.Sprintf("Also generate a source bundle, for source context around native frames (%s and %s)", typeAppleDSYM, typeAndroid)) _ = viper.BindPFlag(includeSourcesFlag, cmd.Flags().Lookup(includeSourcesFlag)) + cmd.Flags().String(sourcePathFlag, defaultPath, fmt.Sprintf("Directory to scan for .java/.kt sources when using --%s with --type %s", includeSourcesFlag, typeAndroid)) + _ = viper.BindPFlag(sourcePathFlag, cmd.Flags().Lookup(sourcePathFlag)) + cmd.Flags().String(symbolsIdFlag, "", "The symbols id (launchdarkly.symbols_id.htlhash) to key files by (Symbols Id Lane). If omitted, a *.symbolsid sidecar next to the bundle is used when present") _ = viper.BindPFlag(symbolsIdFlag, cmd.Flags().Lookup(symbolsIdFlag)) diff --git a/cmd/symbols/generate_test.go b/cmd/symbols/generate_test.go new file mode 100644 index 00000000..f038dc83 --- /dev/null +++ b/cmd/symbols/generate_test.go @@ -0,0 +1,35 @@ +package symbols + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/launchdarkly/ldcli/internal/analytics" +) + +func TestNewGenerateCmd(t *testing.T) { + cmd := NewGenerateCmd(func(accessToken, baseURI string, analyticsOptOut bool) analytics.Tracker { + return &analytics.MockTracker{} + }) + + assert.Equal(t, "generate", cmd.Use) + + // Every flag the command reads has to be one it accepts. Both commands bind the + // same viper keys, so a flag this one only reads still answers — with the value + // of `upload`'s flag, which the caller here has no way to see or to set. + for _, flag := range []string{ + typeFlag, + pathFlag, + outputFlag, + appVersionFlag, + symbolsIdFlag, + basePathFlag, + includeSourcesFlag, + sourcePathFlag, + } { + assert.NotNil(t, cmd.Flags().Lookup(flag), "--%s", flag) + } + + assert.Equal(t, []string{"true"}, cmd.Flags().Lookup(typeFlag).Annotations["required"]) +} From e782b6a02e092a03fd4385184ee349649e78c8bf Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Fri, 31 Jul 2026 06:07:49 -0700 Subject: [PATCH 09/10] feat(symbols): key an Android upload by the id R8 recorded for the mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Android build only reached the Symbols Id Lane if something had stamped an id into the app for it, which meant a Gradle task in every project that wanted one. R8 has been recording an id for its own mapping all along — "# pg_map_id:" in the header — and from AGP 8.12 it stamps that same id into each class, so the shipped app already reports it on every frame of every crash. Read it from the header when no id was given or found in the packaged app, and key the upload by it. A project that adds nothing to its build now uploads to the lane its crashes arrive on; an id the app stamped still wins, since that is a build saying what it will report, and the only answer for one too old for R8 to stamp anything itself. Co-authored-by: Cursor --- cmd/symbols/android_mapid.go | 58 ++++++++++++++++++++++ cmd/symbols/android_mapid_test.go | 82 +++++++++++++++++++++++++++++++ cmd/symbols/android_upload.go | 20 ++++++-- cmd/symbols/upload.go | 2 +- 4 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 cmd/symbols/android_mapid.go create mode 100644 cmd/symbols/android_mapid_test.go 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/upload.go b/cmd/symbols/upload.go index 157a5457..24bea865 100644 --- a/cmd/symbols/upload.go +++ b/cmd/symbols/upload.go @@ -700,7 +700,7 @@ 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)) From 5e93e9784faa27c41c9f5a5e7cba65f5b50680d1 Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Fri, 31 Jul 2026 06:34:21 -0700 Subject: [PATCH 10/10] feat(symbols): upload the dSYMs an Xcode build just produced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dSYM is best uploaded by the build that made it, which means a Run Script phase — and every project writing one wrote the same two pieces of shell first: a --path pointing at DWARF_DSYM_FOLDER_PATH, and a guard skipping the configurations that make no dSYM, without which a Debug build fails on "no .dSYM bundles found". Both are debugged inside a build phase, where the way you find out is a failed build. Read the folder from the build environment when no --path was given, and treat finding no dSYM there as nothing to do rather than as an error. What a phase has to say is now the project it uploads to. An explicit --path still wins, so uploading a dSYM from an archive or from CI is unchanged. Co-authored-by: Cursor --- cmd/symbols/apple_upload.go | 14 +++++++-- cmd/symbols/apple_xcode.go | 50 ++++++++++++++++++++++++++++++ cmd/symbols/apple_xcode_test.go | 55 +++++++++++++++++++++++++++++++++ cmd/symbols/upload.go | 9 ++++-- 4 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 cmd/symbols/apple_xcode.go create mode 100644 cmd/symbols/apple_xcode_test.go 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 24bea865..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 @@ -703,7 +706,7 @@ func initFlags(cmd *cobra.Command) { 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")