Android - Fix Drawable v4 paths in Android Artifact Resources (Libraries) #13128

jpshelley · 2017-03-24T05:37:29Z

Quick apologies for the lengthiness of this description. Want to make sure I'm clear and it is understood what is being altered.

Thanks for submitting a PR! Please read these instructions carefully:

Explain the motivation for making this change.
Provide a test plan demonstrating that the code is solid.
Match the code formatting of the rest of the codebase.
Target the master branch, NOT a "stable" branch.

Motivation (required)

#5787

Unknown source file : /home/tom/projects/blueprint-native/android/app/build/intermediates/res/merged/release/drawable-mdpi-v4/images_google.png: error: Duplicate file.

Unknown source file : /home/tom/projects/blueprint-native/android/app/build/intermediates/res/merged/release/drawable-mdpi/images_google.png: Original is here. The version qualifier may be implied.

At Hudl, we've been attempting to package our React Native code into Library Dependencies (Cocoapods / Android Artifact Resource (aar)). Recently in React Native 0.42.0, there was an upgrade to the Android Project's gradle plugin from 1.3.1 to 2.2.3. This update drastically effected the outcome of drawable resources in Android without anyone noticing.

There are 4 outcomes to consider with this change:

You are developing in an Android Application using Gradle 1.3.1 or lower
You are developing in an Android Application using Gradle 2.2.3 or higher
You are developing in an Android Library Module using Gradle 1.3.1 or lower
You are developing in an Android Library Module using Gradle 2.2.3 or higher

With the upgrade to 2.2.3, Android changed the way aapt builds its resources. Any Library created with 2.2.3, has its resources ending with a v4 suffix. The reasoning behind this I'm not sure of but assume it deals with Vector support that was added around that time.

The change I've added checks if React Native is being ran in an Android Library vs an Application, and appends the v4 suffix to the merged asset folders.

Test Plan (required)

Multiple test were performed.

I first started out validating my assumption about the asset merger by creating a new Android Project to verify my assumptions above.
1. Application + >= 2.2.3 -- hdpi contains my drawable. hdpi-v4 contains dependency's drawables.
2. Application + <= 1.3.1 -- Same as above (I expect because deps are compiled against gradle 2.2.3+ themselves.
3. Library + >= 2.2.3 -- Only -v4 folders found! Resources from the library are packages in the app's build output in similar -v4 folder too.
4. Library + <= 1.3.1 -- Same as ii. & iii. -v4 contains other resources, but my resources are located in non -v4 folder.
I then wanted to validate against React Native itself. So I updated my react native code using this PR/Branch, and tested against my project locally. Unfortunately I cannot share that code as it is private, but before this change I was getting the same error as mentioned in gradlew assembleRelease failing at processReleaseResources [0.19] #5787 and now my build runs as intended with the assets being placed where they should be.

Additional resources:

Please let me know if more information is needed, further test plans, etc. With this change we should be able to upgrade to Gradle 2.3.0 as well to support the latest version of Android Studio.

This reverts commit 9ab29b8.

* master: (161 commits) Use the absolute path to the sourcemap in the CS bundle packager: index files by dir for fast matching Revert D4840716: Refactor interfaces packager: GlobalTransformCache: make key globalized Check that RCTDidInitializeModuleNotification is being called with the correct bridge Refactor interfaces Report JS errors in debug console Apple TV documentation Move away from rnplay to snack, with embedded examples! use performanceNow instead of Date.now() in ScrollResponder Deploy v0.43.0 Removed some no-longer-necessary Flow ignores. Update Keyboard.js configurable devEnabled in React.gradle Use tag ids for testID Update IntegrationWithExistingApps.md Update docs to clarify `RCT_REMAP_METHOD` usage. Enable flow in react-native-implementation Improve z-index implementation on Android Remove the dependency of I18nManagerModule on ReactApplicationContext ...

jpshelley · 2017-04-07T18:44:14Z

@vonovak @AndrewJack Would you be able to review? I noticed both of you as the most recent to edit/review this file in git.

vonovak · 2017-04-10T13:12:48Z

@jpshelley I don't understand drawables, but just wondering - why you use moveFunc.curry("ldpi").call() instead of just moveFunc("ldpi")?

AndrewJack · 2017-04-10T13:23:49Z

Does this only occur when applying the script to a library project?

This script was made to support Applications (com.android.application). Not sure if we want to add this to the gradle script?

Do you have a sample project that reproduces this issue?

jpshelley · 2017-04-10T14:14:38Z

@AndrewJack From what I've gathered, it is only happening on Library Projects, that is correct. At Hudl we are taking a similar approach as Artsy and compiling our React Native code into a Library Project that can be imported into our main native apps, in order to minimize friction.

In the Test Plan I have already created 4 separate projects that show the Android build outputs for drawables to back up my claims. I can create a sample react native project that demonstrates this later today and will comment back when I have them done.

jpshelley · 2017-04-13T03:21:07Z

Sorry for the delay @AndrewJack here are the following:

A fresh project ran using react-native init DemoAndroidLibrary: jpshelley/DemoAndroidLibrary@8ca63b8
A migration of React Native to an Android Library without any Images yet: jpshelley/DemoAndroidLibrary@c374a54
Adding Drawables to the project: jpshelley/DemoAndroidLibrary@45e4611
Upgrading Gradle and running the react native bundle command: jpshelley/DemoAndroidLibrary@6cf4eb4

Project 4 is the only one that doesn't work. Its due to the bundler (which can be ran on any commit to verify) and the upgrade of Gradle which mismatches Android Library's declared drawable folder name.

grahammcculloch · 2017-04-27T16:23:09Z

@AndrewJack keen to see this PR merged - will make my dev much less painful!

AndrewJack · 2017-04-27T22:09:19Z

Why not just copy react.gradle in your own project and edit what you need? It's purposely a gradle script for this reason.

Your use case seems fairly uncommon so I'd hesitate including these changes.

jpshelley · 2017-04-27T23:15:08Z

@AndrewJack We could do that, but then if react.gradle were to change in the future we would be out of the loop on any important alterations.

jpshelley · 2017-05-31T14:04:58Z

Its been over a month, is there any remaining issues with this PR? Can it be merged?

TBouder · 2017-07-28T07:55:11Z

+1

facebook-github-bot · 2017-08-27T17:02:24Z

@jpshelley I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

facebook-github-bot · 2017-09-27T17:03:14Z

@jpshelley I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale · 2017-11-26T17:23:41Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

pull-bot · 2017-11-28T21:00:36Z

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

facebook-github-bot · 2017-12-29T18:04:46Z

@jpshelley I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

pull-bot · 2018-01-04T02:27:41Z

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

facebook-github-bot · 2018-02-03T18:04:39Z

@jpshelley I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

jpshelley · 2018-02-06T20:22:15Z

@philikon @satya164 you both seem to edit this file the most. Thoughts on getting a review to merge?

getaaron · 2018-03-29T21:13:24Z

Could we please merge this? 🙃

People are manually hacking the react.gradle file to work around this
It's a pretty common issue

@philikon @satya164 @AndrewJack

getaaron · 2018-03-29T21:13:50Z

(@jpshelley There's a merge conflict now)

facebook-github-bot · 2018-04-29T17:05:01Z

@jpshelley I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

satya164 · 2018-05-01T10:21:17Z

Thanks for the PR!

@facebook-github-bot shipit

facebook-github-bot

@satya164 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…ies) Summary: _Quick apologies for the lengthiness of this description. Want to make sure I'm clear and it is understood what is being altered._ Thanks for submitting a PR! Please read these instructions carefully: - [x] Explain the **motivation** for making this change. - [x] Provide a **test plan** demonstrating that the code is solid. - [x] Match the **code formatting** of the rest of the codebase. - [x] Target the `master` branch, NOT a "stable" branch. facebook#5787 ``` Unknown source file : /home/tom/projects/blueprint-native/android/app/build/intermediates/res/merged/release/drawable-mdpi-v4/images_google.png: error: Duplicate file. Unknown source file : /home/tom/projects/blueprint-native/android/app/build/intermediates/res/merged/release/drawable-mdpi/images_google.png: Original is here. The version qualifier may be implied. ``` At Hudl, we've been attempting to package our React Native code into Library Dependencies _(Cocoapods / Android Artifact Resource (aar))_. Recently in React Native 0.42.0, there was an upgrade to the Android Project's gradle plugin from 1.3.1 to 2.2.3. This update drastically effected the outcome of drawable resources in Android without anyone noticing. **There are 4 outcomes to consider with this change:** 1. You are developing in an Android Application using Gradle 1.3.1 or lower 2. You are developing in an Android Application using Gradle 2.2.3 or higher 3. You are developing in an Android Library Module using Gradle 1.3.1 or lower 4. You are developing in an Android Library Module using Gradle 2.2.3 or higher With the upgrade to 2.2.3, Android changed the way aapt builds its resources. Any Library created with 2.2.3, has its resources ending with a `v4` suffix. The reasoning behind this I'm not sure of but assume it deals with Vector support that was added around that time. The change I've added checks if React Native is being ran in an Android Library vs an Application, and appends the v4 suffix to the merged asset folders. Multiple test were performed. 1. I first started out validating my assumption about the asset merger by creating a new Android Project to verify my assumptions above. 1. [Application + >= 2.2.3](https://github.com/jpshelley/TestAndroidLibraryDrawables/tree/master/app/build/intermediates/res/merged/debug) -- `hdpi` contains my drawable. `hdpi-v4` contains dependency's drawables. 2. [Application + <= 1.3.1](https://github.com/jpshelley/TestAndroidLibraryDrawables/tree/Android-LegacyVersion/app/build/intermediates/res/merged/debug) -- Same as above (I expect because deps are compiled against gradle 2.2.3+ themselves. 3. [Library + >= 2.2.3](https://github.com/jpshelley/TestAndroidLibraryDrawables/tree/Android-UsingAndroidLibrary/library/build/intermediates/res/merged/debug) -- Only `-v4` folders found! Resources from the library are packages in the app's build output in similar `-v4` folder too. 4. [Library + <= 1.3.1](https://github.com/jpshelley/TestAndroidLibraryDrawables/tree/Android-UsingAndroidLibraryLegacyVersion/library/build/intermediates/res/merged/debug) -- Same as ii. & iii. `-v4` contains other resources, but my resources are located in non -v4 folder. 2. I then wanted to validate against React Native itself. So I updated my react native code using this PR/Branch, and tested against my project locally. Unfortunately I cannot share that code as it is private, but before this change I was getting the same error as mentioned in facebook#5787 and now my build runs as intended with the assets being placed where they should be. Additional resources: * facebook#5787 * http://stackoverflow.com/questions/35700272/android-build-tool-adds-v4-qualifier-to-drawable-folders-by-default-in-generated * facebook#12710 Please let me know if more information is needed, further test plans, etc. With this change we should be able to upgrade to Gradle 2.3.0 as well to support the latest version of Android Studio. Closes facebook#13128 Differential Revision: D7828618 Pulled By: hramos fbshipit-source-id: a7ad7b63b1b51cbfd2ea7656e4d77321306ce33a

jpshelley added 3 commits March 23, 2017 14:58

Update to v4 suffix drawables

9ab29b8

Revert "Update to v4 suffix drawables"

767b41d

This reverts commit 9ab29b8.

Add v4 suffix for Library Projects

1b9806f

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Mar 24, 2017

jpshelley added 2 commits April 12, 2017 17:21

Merge branch 'master' into Android-FixDrawableV4Paths

918acea

Merge branch 'master' into Android-FixDrawableV4Paths

1961735

shergin added Android labels Jun 2, 2017

Merge branch 'master' into Android-FixDrawableV4Paths

3d911a5

Merge branch 'master' into Android-FixDrawableV4Paths

be68074

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Nov 26, 2017

Merge branch 'master' into Android-FixDrawableV4Paths

03eb403

stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Nov 28, 2017

Merge branch 'master' into Android-FixDrawableV4Paths

112336f

hramos removed the GH Review: review-needed label Mar 8, 2018

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. Android labels Mar 16, 2018

janicduplessis removed the Android label Mar 18, 2018

react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. labels Mar 18, 2018

janicduplessis removed the Android 🤖 label Mar 19, 2018

Merge branch 'master' into Android-FixDrawableV4Paths

c712fb8

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label May 1, 2018

facebook-github-bot approved these changes May 1, 2018

View reviewed changes

facebook-github-bot closed this in 2d9e2f3 May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android - Fix Drawable v4 paths in Android Artifact Resources (Libraries) #13128

Android - Fix Drawable v4 paths in Android Artifact Resources (Libraries) #13128

jpshelley commented Mar 24, 2017 •

edited

Loading

jpshelley commented Apr 7, 2017

vonovak commented Apr 10, 2017

AndrewJack commented Apr 10, 2017 •

edited

Loading

jpshelley commented Apr 10, 2017

jpshelley commented Apr 13, 2017

grahammcculloch commented Apr 27, 2017

AndrewJack commented Apr 27, 2017

jpshelley commented Apr 27, 2017

jpshelley commented May 31, 2017

TBouder commented Jul 28, 2017

facebook-github-bot commented Aug 27, 2017

facebook-github-bot commented Sep 27, 2017

stale bot commented Nov 26, 2017

pull-bot commented Nov 28, 2017

facebook-github-bot commented Dec 29, 2017

pull-bot commented Jan 4, 2018 •

edited

Loading

facebook-github-bot commented Feb 3, 2018

jpshelley commented Feb 6, 2018

getaaron commented Mar 29, 2018

getaaron commented Mar 29, 2018

facebook-github-bot commented Apr 29, 2018

satya164 commented May 1, 2018

facebook-github-bot left a comment

Android - Fix Drawable v4 paths in Android Artifact Resources (Libraries) #13128

Android - Fix Drawable v4 paths in Android Artifact Resources (Libraries) #13128

Conversation

jpshelley commented Mar 24, 2017 • edited Loading

Motivation (required)

Test Plan (required)

jpshelley commented Apr 7, 2017

vonovak commented Apr 10, 2017

AndrewJack commented Apr 10, 2017 • edited Loading

jpshelley commented Apr 10, 2017

jpshelley commented Apr 13, 2017

grahammcculloch commented Apr 27, 2017

AndrewJack commented Apr 27, 2017

jpshelley commented Apr 27, 2017

jpshelley commented May 31, 2017

TBouder commented Jul 28, 2017

facebook-github-bot commented Aug 27, 2017

facebook-github-bot commented Sep 27, 2017

stale bot commented Nov 26, 2017

pull-bot commented Nov 28, 2017

facebook-github-bot commented Dec 29, 2017

pull-bot commented Jan 4, 2018 • edited Loading

facebook-github-bot commented Feb 3, 2018

jpshelley commented Feb 6, 2018

getaaron commented Mar 29, 2018

Could we please merge this? 🙃

getaaron commented Mar 29, 2018

facebook-github-bot commented Apr 29, 2018

satya164 commented May 1, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

jpshelley commented Mar 24, 2017 •

edited

Loading

AndrewJack commented Apr 10, 2017 •

edited

Loading

pull-bot commented Jan 4, 2018 •

edited

Loading