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 issue 2898 - created seprarate resources for build and debug build #2916

Merged
merged 1 commit into from Oct 3, 2018

Conversation

@prateek3255
Copy link
Collaborator

commented Aug 25, 2018

This PR fixes the issue #2898.

I refered these docs and the comment no. 3 of this StackOverflow post.

I found that all the common files such as manifest files, java files, common resources files and one of both the debug and release resources go into the main folder, and the different files for release and debug such as launcher icons in our case go into their respective debug and release source folders.

The resource files are overriden according to build type because the compiler expects them to be at these locations on the basis of build variant

debug
-----
Compile configuration: debugCompile
build.gradle name: android.sourceSets.debug
Java sources: [app\src\debug\java]
Manifest file: app\src\debug\AndroidManifest.xml
Android resources: [app\src\debug\res]
Assets: [app\src\debug\assets]
AIDL sources: [app\src\debug\aidl]
RenderScript sources: [app\src\debug\rs]
JNI sources: [app\src\debug\jni]
JNI libraries: [app\src\debug\jniLibs]
Java-style resources: [app\src\debug\resources]

main
----
Compile configuration: compile
build.gradle name: android.sourceSets.main
Java sources: [app\src\main\java]
Manifest file: app\src\main\AndroidManifest.xml
Android resources: [app\src\main\res]
Assets: [app\src\main\assets]
AIDL sources: [app\src\main\aidl]
RenderScript sources: [app\src\main\rs]
JNI sources: [app\src\main\jni]
JNI libraries: [app\src\main\jniLibs]
Java-style resources: [app\src\main\resources]

release
-------
Compile configuration: releaseCompile
build.gradle name: android.sourceSets.release
Java sources: [app\src\release\java]
Manifest file: app\src\release\AndroidManifest.xml
Android resources: [app\src\release\res]
Assets: [app\src\release\assets]
AIDL sources: [app\src\release\aidl]
RenderScript sources: [app\src\release\rs]
JNI sources: [app\src\release\jni]
JNI libraries: [app\src\release\jniLibs]
Java-style resources: [app\src\release\resources]
@jainkuniya
Copy link
Member

left a comment

@prateek3255 👍
Had a quick look, and left few comments.

@@ -186,6 +186,8 @@ dependencies {

}


This comment has been minimized.

Copy link
@jainkuniya

jainkuniya Aug 31, 2018

Member

I guess this changes are done accidentally.

This comment has been minimized.

Copy link
@prateek3255

prateek3255 Aug 31, 2018

Author Collaborator

Yeah I guess this was made accidentally

This comment has been minimized.

Copy link
@jainkuniya

jainkuniya Sep 10, 2018

Member

Please remove this change. :)

@@ -0,0 +1,13320 @@
{

This comment has been minimized.

Copy link
@jainkuniya

jainkuniya Aug 31, 2018

Member

this too?

This comment has been minimized.

Copy link
@prateek3255

prateek3255 Aug 31, 2018

Author Collaborator

I didn't touch this file

This comment has been minimized.

Copy link
@prateek3255

prateek3255 Aug 31, 2018

Author Collaborator

how do I resolve this file?

This comment has been minimized.

Copy link
@jainkuniya

jainkuniya Sep 10, 2018

Member

Do a -i rebase and select edit for the commit in which this got commited, and do not add this file (add all other files) and then git rebase --continue.

Let me know on chat.zulip.org if you face any problem. :)

@prateek3255 prateek3255 force-pushed the prateek3255:fix/2898 branch from 544c186 to cad9a1b Sep 11, 2018

@jainkuniya
Copy link
Member

left a comment

@prateek thanks 👍
We test it and take it from here and combine it with #2897.

@jainkuniya
Copy link
Member

left a comment

hi @prateek3255 Thanks on working on this. I have taken this commit in #2897.

Thanks once again :)

@gnprice

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Thanks @prateek3255 for digging into this, and @jainkuniya for the previous review!

A couple of comments:

  • This leaves us with two copies of each of these files: one in .../src/main/, and one in .../src/debug/ or .../src/release/. We should have just one copy to avoid confusion.
  • We can leave the "normal" version of each file under src/main/. That way it'll get used in the release build, and overriden in the debug build, for the same effect as in this version; but it feels a bit clearer to me, by explicitly marking which one we think of as "the" logo, the normal one to think of if you're not paying special attention to the release/debug distinction.

Those changes are super easy for me to make, so I'll just do them before merging.

@gnprice

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Oh also:

  • Since we're using this nice clean source-sets mechanism, we don't need the ic_debug_launcher thing anymore :) I see @jainkuniya went and fixed that aspect up in the commit he included in #2897 , so I'll use that fix.
android: Move debug icons to `debug` source set.
Now in APK res will be added on the basis of build type.  So if it is
release build then it will not contain res of debug build and vice
versa.  Will affect the APK in a good manner, i.e reduce a bit.

[Gradle and manifest changes by @jainkuniya.]

Fixes #2898.

@gnprice gnprice force-pushed the prateek3255:fix/2898 branch from cad9a1b to 6e4677f Oct 3, 2018

@gnprice gnprice merged commit 6e4677f into zulip:master Oct 3, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.