Skip to content

Add multiple architectures Android insight#474

Merged
rbro112 merged 4 commits intomainfrom
ryan/add_multiple_architectures_android_insight
Nov 14, 2025
Merged

Add multiple architectures Android insight#474
rbro112 merged 4 commits intomainfrom
ryan/add_multiple_architectures_android_insight

Conversation

@rbro112
Copy link
Member

@rbro112 rbro112 commented Nov 14, 2025

Adds multiple native library insight as a carryover of https://docs.emergetools.com/docs/multiple-native-library-architectures-android

Will implement frontend handling for this over in sentry repo as follow up to resolve EME-582

Copy link
Member Author

rbro112 commented Nov 14, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@linear
Copy link

linear bot commented Nov 14, 2025

Comment on lines +42 to +46
path_parts = Path(file_info.path).parts
if len(path_parts) >= 3 and path_parts[0] == "lib":
arch = path_parts[1]
if arch in self.ARCHITECTURE_MAPPING:
arch_to_files[arch].append((file_info.path, file_info.size))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: On Windows, Path().parts fails to parse POSIX paths, causing the insight to be non-functional.
Severity: CRITICAL | Confidence: 0.95

🔍 Detailed Analysis

On Windows systems, Path(file_info.path).parts incorrectly parses POSIX-formatted paths (using forward slashes) because Path creates a WindowsPath object expecting backslashes. This causes the path_parts condition if len(path_parts) >= 3 and path_parts[0] == "lib" to fail, leading the insight to always return None and rendering the feature non-functional on Windows.

💡 Suggested Fix

Replace Path(file_info.path).parts with PurePosixPath(file_info.path).parts after importing PurePosixPath from pathlib.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/launchpad/size/insights/android/multiple_native_library_arch.py#L42-L46

Potential issue: On Windows systems, `Path(file_info.path).parts` incorrectly parses
POSIX-formatted paths (using forward slashes) because `Path` creates a `WindowsPath`
object expecting backslashes. This causes the `path_parts` condition `if len(path_parts)
>= 3 and path_parts[0] == "lib"` to fail, leading the insight to always return `None`
and rendering the feature non-functional on Windows.

Did we get this right? 👍 / 👎 to inform future reviews.

Reference_id: 2694231

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we're not running this on windows so....

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

❌ Patch coverage is 97.79412% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.53%. Comparing base (47b5c92) to head (3d7e128).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...e/insights/android/multiple_native_library_arch.py 91.66% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #474      +/-   ##
==========================================
+ Coverage   80.35%   80.53%   +0.17%     
==========================================
  Files         159      161       +2     
  Lines       13465    13601     +136     
  Branches     1424     1435      +11     
==========================================
+ Hits        10820    10953     +133     
- Misses       2109     2110       +1     
- Partials      536      538       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

"""

# Architectures that can most likely be removed (keeping only arm64-v8a)
REMOVABLE_ARCHITECTURES = {"x86", "x86_64", "armeabi-v7a"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it more that only one architecture should be shipped in an APK to the app store, rather than non-arm64-v8a archs? Just wondering if generalizing this makes sense, like perhaps a different region like Asia is still on some older architectures.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only ever seen arm64-v8a in the wild. x86_* are for emulators, and armeabi-v7a is the only remaining. A quick search indicates that this arch is quite outdated at this point, so I think it's a pretty safe bet to keep this as-is

@rbro112 rbro112 force-pushed the ryan/add_multiple_architectures_android_insight branch from c59828e to 3d7e128 Compare November 14, 2025 19:37
@rbro112 rbro112 merged commit 30b5a96 into main Nov 14, 2025
22 checks passed
@rbro112 rbro112 deleted the ryan/add_multiple_architectures_android_insight branch November 14, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants