Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691353005:
Generate custom manifests when letting gradle process resources. (Closed)
|
Created:
3 years, 10 months ago by sakal-chromium Modified:
3 years, 10 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGenerate custom manifests when letting gradle process resources.
Gradle uses package id from manifest when generating R.class. So, we
need to generate a custom manifest if we let gradle process resources.
BUG=682846, webrtc:6328
Review-Url: https://codereview.chromium.org/2691353005
Cr-Commit-Position: refs/heads/master@{#451029}
Committed: https://chromium.googlesource.com/chromium/src/+/cab87bffc69256f873c19f5a977d35e290a180ee
Patch Set 1 : Generate custom manifests when letting gradle process resources. #
Total comments: 4
Patch Set 2 : Changes according to Peter's comments. #1 #
Total comments: 6
Patch Set 3 : Changes according to agrieve's comments. #1 #
Messages
Total messages: 21 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
sakal@chromium.org changed reviewers: + agrieve@chromium.org, wnwen@chromium.org
PTAL
Description was changed from ========== Generate custom manifests when letting gradle process resources. BUG=682846 ========== to ========== Generate custom manifests when letting gradle process resources. Gradle uses package id from manifest when generating R.class. So, we need to generate a custom manifest if we let gradle process resources. BUG=682846 ==========
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Generate custom manifests when letting gradle process resources. Gradle uses package id from manifest when generating R.class. So, we need to generate a custom manifest if we let gradle process resources. BUG=682846 ========== to ========== Generate custom manifests when letting gradle process resources. Gradle uses package id from manifest when generating R.class. So, we need to generate a custom manifest if we let gradle process resources. BUG=682846, webrtc:6328 ==========
https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:274: def Generate(self, entry, jinja_processor): Rather than passing this in, can you pull it out to initialize the _ProjectContextGenerator with the jinja processor? https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:281: android_manifest = entry.Gradle().get('android_manifest', None) None is default, no need to specify it
https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:274: def Generate(self, entry, jinja_processor): On 2017/02/16 14:21:15, Peter Wen wrote: > Rather than passing this in, can you pull it out to initialize the > _ProjectContextGenerator with the jinja processor? Done. https://codereview.chromium.org/2691353005/diff/60001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:281: android_manifest = entry.Gradle().get('android_manifest', None) On 2017/02/16 14:21:15, Peter Wen wrote: > None is default, no need to specify it Done.
Curious if you tried setting this via build.gradle rather than a manifest ia:
android {
defaultConfig {
applicationId "com.example.myapp"
}
}
On 2017/02/16 15:11:30, agrieve wrote:
> Curious if you tried setting this via build.gradle rather than a manifest ia:
>
> android {
> defaultConfig {
> applicationId "com.example.myapp"
> }
> }
I did try this. Unfortunately, this only works for apk targets. It is not
allowed for library targets.
lgtm, thanks!
lgtm. Great stuff! https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:286: # need to generate a custom manifest if we let gradle process resources. nit: can you add a note that applicationId doesn't work for library targets. https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:287: if self.use_gradle_process_resources and variables['res_dirs']: nit: I think we'll want to do this regardless of use_gradle_process_resources https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ma... File build/android/gradle/manifest.jinja (right): https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ma... build/android/gradle/manifest.jinja:9: <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="24" /> nit: make the targetSdkVersion be {{compile_sdk_version}}
Note: I tried this out locally for the main chrome targets and verified it still syncs fine and shows no console warnings when generating.
https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:286: # need to generate a custom manifest if we let gradle process resources. On 2017/02/16 15:28:01, agrieve wrote: > nit: can you add a note that applicationId doesn't work for library targets. Done. https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:287: if self.use_gradle_process_resources and variables['res_dirs']: On 2017/02/16 15:28:01, agrieve wrote: > nit: I think we'll want to do this regardless of use_gradle_process_resources Done. https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ma... File build/android/gradle/manifest.jinja (right): https://codereview.chromium.org/2691353005/diff/80001/build/android/gradle/ma... build/android/gradle/manifest.jinja:9: <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="24" /> On 2017/02/16 15:28:01, agrieve wrote: > nit: make the targetSdkVersion be {{compile_sdk_version}} Done.
The CQ bit was checked by sakal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2691353005/#ps100001 (title: "Changes according to agrieve's comments. #1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487260766656490,
"parent_rev": "9d7cb47d75dd40e0f819191c637720fd06538829", "commit_rev":
"cab87bffc69256f873c19f5a977d35e290a180ee"}
Message was sent while issue was closed.
Description was changed from ========== Generate custom manifests when letting gradle process resources. Gradle uses package id from manifest when generating R.class. So, we need to generate a custom manifest if we let gradle process resources. BUG=682846, webrtc:6328 ========== to ========== Generate custom manifests when letting gradle process resources. Gradle uses package id from manifest when generating R.class. So, we need to generate a custom manifest if we let gradle process resources. BUG=682846, webrtc:6328 Review-Url: https://codereview.chromium.org/2691353005 Cr-Commit-Position: refs/heads/master@{#451029} Committed: https://chromium.googlesource.com/chromium/src/+/cab87bffc69256f873c19f5a977d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/cab87bffc69256f873c19f5a977d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
