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

[Build] Enable Non-Transitive Resources #18707

Merged
merged 36 commits into from
Jul 4, 2023

Conversation

ParaskP7
Copy link
Contributor

This PR is a prerequisite for the Gradle 8.1.1 & AGP 8.0.2 Upgrade for WPAndroid, WCAndroid & Related Libs project.

Platform Request: pdnsEh-13V-p2
Project Thread: paaHJt-57Z-p2


This PR enables non-transitive resources (android.nonTransitiveRClass) for the project.

FYI: This behavior becomes the default in AGP 8.0 and higher. As such, this becomes a prerequisite for the AGP 8.0.2 upgrade, that is of course, unless android.nonTransitiveRClass is explicitly set to false.

PS: I recommend reviewing this PR commit-by-commit, and more so, to focus on the Build related commits. Avoid spending too much time on reviewing the Refactor related commits as those was purely done for codebase alignment purposes(*).


PS: @AjeshRPai I added you as the main reviewer, randomly so, since I just wanted someone from the WordPress team to be aware of and sign-off on that change for WPAndroid. I also added the @wordpress-mobile/apps-infrastructure team, but this in done only for monitoring purposes, as such, I am not expecting any active review from that team. Thus, feel free to merge this PR if you deem so.


(*) The convention I followed in order to align the R related usage in the codebase is the following:

  • To start with, import org.wordpress.android.R is the main import of choice.
  • Then, and for Kotlin files only, such imports have now the below aliases:
    • LoginR for org.wordpress.android.login.R
    • StoriesR for com.wordpress.stories.R
    • EditorR for org.wordpress.android.editor.R
    • ZendeskR for com.zendesk.sdk.R
    • ComposeR for androidx.compose.ui.R
    • CoordinatorLayoutR for androidx.coordinatorlayout.R
    • MaterialR for import com.google.android.material.R
    • AndroidR for import android.R

To test:

  1. Verify that all the CI checks are successful.
  2. Thoroughly smoke test both the WordPress and Jetpack apps and check if everything is working as expected.

Regression Notes

  1. Potential unintended areas of impact

    • Potential breakage or misbehaviour of screens as these R related changes apply to the whole project, including its module related libraries (like libs:image-editor and libs:editor).
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • See To test section above.
  3. What automated tests I added (or what prevented me from doing so)

    • N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

ParaskP7 added 30 commits June 27, 2023 12:27
Sources:
- Production:  54 imports
  - WordPress:  2 imports
  - Main:      52 imports
- Test:        18 imports
Sources:
- Production:  15 imports
  - WordPress:  2 imports
  - Main:      13 imports
- Test:         0 imports
Sources:
- Production:  11 imports
  - WordPress:  0 imports
  - Main:      11 imports
- Test:         0 imports
Sources:
- Production:   9 imports
  - WordPress:  3 imports
  - Main:       6 imports
- Test:         1 imports
Sources:
- Production:   9 imports
  - WordPress:  2 imports
  - Main:       7 imports
- Test:         2 imports
Sources:
- Production:   6 imports
  - WordPress:  0 imports
  - Main:       6 imports
- Test:         0 imports
Sources:
- Production:   4 imports
  - WordPress:  0 imports
  - Main:       4 imports
- Test:         0 imports
This 'other' keyword includes the following categories:
- other         6 import
  - style       1 import (Main)
  - styleable   1 import (Main)
  - anim        1 import (Main)
  - font        1 import (WordPress)
  - integer     1 import (Main)
  - array       1 import (Main)

Sources:
- Production:   6 imports
  - WordPress:  1 imports
  - Main:       5 imports
- Test:         0 imports
Sources:
- Production:   6 imports
  - WordPress:  0 imports
  - Main:       6 imports
- Test:         0 imports
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
This optimization only applies to Kotlin files since import aliases is
not supported on Java files
This optimization only applies to Kotlin files since import aliases is
not supported on Java files.
ParaskP7 added 5 commits June 27, 2023 16:47
These 'R.drawable.ic_close_24px' resources come from the
'mobile-gutenberg', and more specifically the
':react-native-gutenberg-bridge' project dependency. As such these
resources cannot be used in a non-transitive way.

Replacing the 'R.drawable.ic_close_24px' resources with its identical
'org.wordpress.android.R' such 'R.drawable.ic_close_white_24dp'
resources resolves these non-transitive related issues.
This 'R.drawable.ic_arrow_back_white_24dp' resources come from the
'tenor-android-core-jetified' dependency. As such this resource cannot
be used in a non-transitive way.

Replacing the 'R.drawable.ic_arrow_back_white_24dp' resources with its
identical 'org.wordpress.android.R' such
'R.drawable.ic_arrow_left_white_24dp' resource resolves this
non-transitive related issue.
This commit updates the encrypted 'gradle.properties.enc' file to point
to that pinned hash which has the 'android.nonTransitiveRClass=true'
property added into the 'gradle.properties' file for the WPAndroid
project.

FYI: This is done in order for the CI to pick-up this specific change
and download the correct version of 'gradle.properties' to then build
the project with non transitive resources enabled.
@ParaskP7 ParaskP7 added this to the Future milestone Jun 27, 2023
@ParaskP7 ParaskP7 requested review from AjeshRPai and a team June 27, 2023 16:28
@ParaskP7 ParaskP7 self-assigned this Jun 27, 2023
@ParaskP7 ParaskP7 requested a review from a team as a code owner June 27, 2023 16:28
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 27, 2023

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr18707-ed4c57e
Commited4c57e
Direct Downloadwordpress-prototype-build-pr18707-ed4c57e.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 27, 2023

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr18707-ed4c57e
Commited4c57e
Direct Downloadjetpack-prototype-build-pr18707-ed4c57e.apk
Note: Google Login is not supported on these builds.

Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

I did the smoke testing of the Jetpack and wordpress app, everything looks good to me 👍🏼 ✅

Scenario's i covered in my smoke testing

  • login
  • publish a story post
  • publish a blog post with images and video
  • follow a site
  • check the stats
  • read an article

@AjeshRPai AjeshRPai merged commit 8c60d49 into trunk Jul 4, 2023
@AjeshRPai AjeshRPai deleted the build/enable-non-transitive-resources branch July 4, 2023 04:46
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Jul 4, 2023

Awesome @AjeshRPai , thank you so much for reviewing, testing and merging this, you rock! 🙇❤️🚀

@ParaskP7 ParaskP7 mentioned this pull request Jul 4, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants