Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MM-62375): OutOfMemoryError issue with better catching #8622

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

rahimrahman
Copy link
Contributor

@rahimrahman rahimrahman commented Feb 20, 2025

Summary

  • fix OOM issue with better catching. See this PR for more explanation
    • now will allocate buffer, but with proper catching instead assuming that the app will run out of memory
    • APNG4Android is npm installable
    • minSdkVersion changed to 21
  • removed APNG4Android postinstall since it is causing issues with developers

On minSdkVersion change to 21

A problem occurred configuring project ':gif'.
> [CXX1110] Platform version 16 is unsupported by this NDK. Please change minSdk to at least 21 to avoid undefined behavior. To suppress this error, add android.ndk.suppressMinSdkVersionError=21 to the project's gradle.properties or set android.experimentalProperties["android.ndk.suppressMinSdkVersionError"]=21 in the Gradle build file.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62375

Related PRs:

mattermost/APNG4Android#4
mattermost/APNG4Android#3

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Screenshots

Release Note


@mm-cloud-bot mm-cloud-bot added kind/bug Categorizes issue or PR as related to a bug. release-note labels Feb 20, 2025
@rahimrahman rahimrahman changed the title Fix/mm 62375 oom on gif 2 fix(MM-62375): OutOfMemoryError issue with better catching Feb 20, 2025
@rahimrahman rahimrahman force-pushed the fix/MM-62375-OOM-on-gif-2 branch from 1373864 to ec00bc1 Compare February 20, 2025 20:23
@rahimrahman rahimrahman added the Build Apps for PR Build the mobile app for iOS and Android to test label Feb 20, 2025
@rahimrahman rahimrahman removed the Build Apps for PR Build the mobile app for iOS and Android to test label Feb 27, 2025
@rahimrahman
Copy link
Contributor Author

@enzowritescode this APNG4Android package is now consumed as a regular npm package. Is there a way to run snyk to see if it will flag as security vulnerability? I want to be 1 step ahead.

Previously, APNG4Android is added after npm install. That works, but is causing some issues among developers.

@rahimrahman rahimrahman requested a review from Copilot February 27, 2025 13:31

Choose a reason for hiding this comment

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

PR Overview

This PR addresses an OutOfMemoryError issue by improving error catching. The changes involve removing the Git clone step for the APNG4Android repository in the GitHub Actions workflow, indicating a shift in dependency management.

Reviewed Changes

File Description
.github/actions/prepare-node-deps/action.yaml Removed the cloning step for APNG4Android to streamline dependency handling

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

.github/actions/prepare-node-deps/action.yaml:52

  • The removal of the 'ci/clone-APNG4Android' step should be reviewed to ensure that the workflow does not rely on this dependency clone, potentially leading to unexpected issues in the build process.
-    - name: ci/clone-APNG4Android
@rahimrahman rahimrahman added the Build Apps for PR Build the mobile app for iOS and Android to test label Feb 27, 2025
@rahimrahman
Copy link
Contributor Author

@enzowritescode I see "security/snyk - 14 security tests have passed", I'm assuming we're actively checking for security vulnerability.

I think I should be good here.

@rahimrahman rahimrahman marked this pull request as ready for review February 27, 2025 14:05
@rahimrahman
Copy link
Contributor Author

Also address issue brought by devs: #8625

@rahimrahman
Copy link
Contributor Author

Tested by @enzowritescode

@rahimrahman rahimrahman merged commit 1ecba21 into main Feb 28, 2025
19 checks passed
@rahimrahman rahimrahman deleted the fix/MM-62375-OOM-on-gif-2 branch February 28, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Apps for PR Build the mobile app for iOS and Android to test kind/bug Categorizes issue or PR as related to a bug. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants