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

Avoid referencing resource names within Resource Loader #5388

Merged
merged 1 commit into from Apr 29, 2024

Conversation

benjaminRomano
Copy link
Contributor

@benjaminRomano benjaminRomano commented Mar 22, 2024

Description

We are working on enabling the collapse names optimization of aapt2 that removes the resource names from the Resource Table as an app size optimization. However, once enabled, we ran into issues loading resources.

One requirement of removing resource names from the Resource Table is ensuring that resources are not referenced by resource name when loaded. If they are referenced by resource name, they must be explicitly kept with keep rules. (ref).

Motivation and Context

ResourceLoader is using the android.resource://<package_name>/<resource_type>/<resource_name> content URI to resolve Drawable resources, which is incompatible with collapse names optimization.

Fortunately, the ContentResolver for resources has another URI scheme that enables loading resources without referencing the name: android.resource://<package_name>/<resource_id>, which can be used instead (ref).

Test Plan

Pre-merge passes (TODO: still building locally and verifying)

@benjaminRomano benjaminRomano changed the title Update ResourceLoader.java Avoid referencing resource names within Resource Loader Mar 22, 2024
@falhassen
Copy link
Collaborator

@benjaminRomano thanks for the pull request and sorry for the delay in reviewing it.

Do you have an update on your test Plan, ("TODO: still building locally and verifying")? Can you update your commit message with the answer?

Copy link
Collaborator

@falhassen falhassen left a comment

Choose a reason for hiding this comment

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

Can you squash this commit with your other commits?

Copy link
Collaborator

@falhassen falhassen left a comment

Choose a reason for hiding this comment

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

To be extra safe, would it be possible for you to flag guard this change, roll it out in your binary, verify there are no issues, and then clean-up the flag?

See recent pull requests where this was done:

  1. Update TransformationUtils#rotateImageExif to preserve gainmap and co… #5334 Clean up preserveGainmapAndColorSpaceForTransformations flag #5366

  2. Add workaround for AOSP bug with decoding single channel hardware gai… #5357 / Clean up setEnableHardwareGainmapFixOnU flag #5386

@benjaminRomano
Copy link
Contributor Author

benjaminRomano commented Apr 24, 2024

I tried to build locally, but I'm hitting the following error when running ./gradlew jar:

* What went wrong:
A problem occurred configuring root project 'glide-parent'.
> Could not resolve all files for configuration ':classpath'.
   > Could not resolve androidx.benchmark:benchmark-gradle-plugin:1.2.0-beta05.
     Required by:
         project :
      > No matching variant of androidx.benchmark:benchmark-gradle-plugin:1.2.0-beta05 was found. The consumer was configured to find a library for use during runtime, compatible with Java 11, packaged as a jar, and its dependencies declared externally, as well as attribute 'org.gradle.plugin.api-version' with value '8.3' but:
          - Variant 'apiElements' capability androidx.benchmark:benchmark-gradle-plugin:1.2.0-beta05 declares a library, packaged as a jar, and its dependencies declared externally:
              - Incompatible because this component declares a component for use during compile-time, compatible with Java 17 and the consumer needed a component for use during runtime, compatible with Java 11
              - Other compatible attribute:
                  - Doesn't say anything about org.gradle.plugin.api-version (required '8.3')
          - Variant 'runtimeElements' capability androidx.benchmark:benchmark-gradle-plugin:1.2.0-beta05 declares a library for use during runtime, packaged as a jar, and its dependencies declared externally:
              - Incompatible because this component declares a component, compatible with Java 17 and the consumer needed a component, compatible with Java 11
              - Other compatible attribute:
                  - Doesn't say anything about org.gradle.plugin.api-version (required '8.3')
          - Variant 'sourcesElements' capability androidx.benchmark:benchmark-gradle-plugin:1.2.0-beta05 declares a component for use during runtime, and its dependencies declared externally:
              - Incompatible because this component declares documentation and the consumer needed a library
              - Other compatible attributes:
                  - Doesn't say anything about its target Java version (required compatibility with Java 11)
                  - Doesn't say anything about its elements (required them packaged as a jar)
                  - Doesn't say anything about org.gradle.plugin.api-version (required '8.3')

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.

To be extra safe, would it be possible for you to flag guard this change, roll it out in your binary, verify there are no issues, and then clean-up the flag?

I'm OK with wiring an experiment if need be, but it's likely unnecessary precautions.

The content URI format being used has been around since Cupcake (ref): https://developer.android.com/reference/android/content/ContentResolver#the-android.resource-scheme_android_resource-scheme

The equivalent code in AOSP can be found here: https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/content/ContentResolver.java;l=1499?q=SCHEME_ANDROID_RESOURCE

Tracing the AOSP source code, it can be verified that migrating the the new content URI scheme is safe. Effectively, ContentResolver is undoing the conversion from resourceID to resourceName that glide is doing today here. My change removes this unnecessary conversion and resource access through reflection (getIdentifier).

About a month or two ago, we migrated all our content URIs to the new format for Snapchat and did not encounter any issues as expected.

Can you squash this commit with your other commits?

I can do this once I've addressed all comments, but you'll also be able to squash merge my commits and re-write the commit messages when you merge changes through Github.

@falhassen
Copy link
Collaborator

falhassen commented Apr 26, 2024

Sorry for the late reply.

I use JDK 17 to build. On my macOS device, I run

./gradlew -Dorg.gradle.java.home=/Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home build

to build Glide.

Thanks for addressing my other comments. Unfortunately, this repo is not configured to allow squashing and merging (and I don't have permission to change that). Can you take care of the squashing?

@benjaminRomano
Copy link
Contributor Author

Squash merged my commits. it should be good to go now.

Unfortunately, I still can't get the dependency error. If I find some time, I'll dig into it further. Potentially this is an issue with some kind of global gradle config being mis-configured

@falhassen falhassen merged commit a7351b0 into bumptech:master Apr 29, 2024
4 checks passed
@benjaminRomano
Copy link
Contributor Author

@falhassen when should I expect new version of Glide to be released?

@falhassen
Copy link
Collaborator

@benjaminRomano unfortunately getting a new version of Glide may take some time.

However, you can immediately deploy the new changes in your app by depending on a Glide snapshot, following Glide's instructions: https://bumptech.github.io/glide/dev/snapshots.html#about-snapshots.

I believe 5.0.0-SNAPSHOT/ should have your commit.

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.

None yet

2 participants