Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209233002:
▶ Update the Play services to 9.8 and load from AARs (Closed)
|
Created:
4 years, 4 months ago by dgn Modified:
4 years, 1 month ago Reviewers:
agrieve CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[gmscore] Update the Play services to 9.8 and load from AARs
- Update the build to load the play services library from split
AARs.
- Update the upload/download script to handle the new library
distribution format
- Roll the dependencies to 9.8
BUG=624324, 659366
Committed: https://crrev.com/220f57b3c7299133c089ce3c4ede7861cb8bb1e8
Cr-Commit-Position: refs/heads/master@{#428333}
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : address comments #Patch Set 4 : rebase #Patch Set 5 : rebase, fix script test #Patch Set 6 : rebase #Patch Set 7 : Update to 9.8.0 #Patch Set 8 : mention the configuration file in the upload command line help #
Messages
Total messages: 49 (28 generated)
Description was changed from ========== [gmscore] Update the Play services to 9.4 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.4 BUG=624324 ========== to ========== [gmscore] Update the Play services to 9.4 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.4 BUG=624324 ==========
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dgn@chromium.org changed reviewers: + agrieve@chromium.org
PTAL. I uploaded the new SDK to the cloud storage already. https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... File build/android/play_services/update.py (left): https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... build/android/play_services/update.py:273: new_version_number = utils.GetVersionNumberFromLibraryResources( We don't when we update it we get all the existing versions of the library anyway and we use the config file as input anyway, so I'm not checking anymore the version here. This was fairly useless anyway, if we do redundant updates, the code review for the update should pick it up. https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... build/android/play_services/update.py:307: config.UpdateVersionNumber(new_version_number) We now use the config file as input rather than output (get the list of clients, get the version), so I'm not updating it anymore at the end.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... File build/secondary/third_party/android_tools/BUILD.gn (right): https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:113: java_group("google_play_services_default_resources") { This is a noop target because targets out of the main repo have it as a dependency and I don't want to break them now. I probably can't remove it until downstream also removes the equivalent target anyway.
This is *so much nicer* to have them split out! https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... File build/android/play_services/update.py (right): https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... build/android/play_services/update.py:443: zipf = zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) nit: nicer to use with: for this. with zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) as zipf: https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... File build/secondary/third_party/android_tools/BUILD.gn (left): https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:126: input_jars_paths = [ "$android_sdk/optional/org.apache.http.legacy.jar" ] I think this is the line needed to fix the bots. However, I don't think it's currently a property that you can attach to a library and have it get passed along to the apk target. I really needs to be though. I'll try and work on a patch for this today. https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... File build/secondary/third_party/android_tools/BUILD.gn (right): https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:152: lib_name = "play-services-basement" nit: lib_name -> _lib_name https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:202: # android_aar_prebuilt("google_play_services_cast_framework_java") { Remove?
On 2016/08/05 18:59:28, agrieve wrote: > This is *so much nicer* to have them split out! > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > File build/android/play_services/update.py (right): > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > build/android/play_services/update.py:443: zipf = zipfile.ZipFile(zip_name, 'w', > zipfile.ZIP_DEFLATED) > nit: nicer to use with: for this. > > with zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) as zipf: > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > File build/secondary/third_party/android_tools/BUILD.gn (left): > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > build/secondary/third_party/android_tools/BUILD.gn:126: input_jars_paths = [ > "$android_sdk/optional/org.apache.http.legacy.jar" ] > I think this is the line needed to fix the bots. However, I don't think it's > currently a property that you can attach to a library and have it get passed > along to the apk target. I really needs to be though. I'll try and work on a > patch for this today. > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > File build/secondary/third_party/android_tools/BUILD.gn (right): > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > build/secondary/third_party/android_tools/BUILD.gn:152: lib_name = > "play-services-basement" > nit: lib_name -> _lib_name > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > build/secondary/third_party/android_tools/BUILD.gn:202: # > android_aar_prebuilt("google_play_services_cast_framework_java") { > Remove? Actually - another question: I think right now we're purposefully omitting play services resources. Does this change cause unused play services resources to be included?
On 2016/08/07 20:55:08, agrieve wrote: > On 2016/08/05 18:59:28, agrieve wrote: > > This is *so much nicer* to have them split out! > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > > File build/android/play_services/update.py (right): > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > > build/android/play_services/update.py:443: zipf = zipfile.ZipFile(zip_name, > 'w', > > zipfile.ZIP_DEFLATED) > > nit: nicer to use with: for this. > > > > with zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) as zipf: > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > File build/secondary/third_party/android_tools/BUILD.gn (left): > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > build/secondary/third_party/android_tools/BUILD.gn:126: input_jars_paths = [ > > "$android_sdk/optional/org.apache.http.legacy.jar" ] > > I think this is the line needed to fix the bots. However, I don't think it's > > currently a property that you can attach to a library and have it get passed > > along to the apk target. I really needs to be though. I'll try and work on a > > patch for this today. > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > File build/secondary/third_party/android_tools/BUILD.gn (right): > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > build/secondary/third_party/android_tools/BUILD.gn:152: lib_name = > > "play-services-basement" > > nit: lib_name -> _lib_name > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > build/secondary/third_party/android_tools/BUILD.gn:202: # > > android_aar_prebuilt("google_play_services_cast_framework_java") { > > Remove? > > Actually - another question: > > I think right now we're purposefully omitting play services resources. Does this > change cause unused play services resources to be included? also also - Maybe you're ontop of it already, but when I patch this locally the .aar files don't exist (no google/m2repository in android_tools)
On 2016/08/07 21:14:54, agrieve wrote: > On 2016/08/07 20:55:08, agrieve wrote: > > On 2016/08/05 18:59:28, agrieve wrote: > > > This is *so much nicer* to have them split out! > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > > > File build/android/play_services/update.py (right): > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... > > > build/android/play_services/update.py:443: zipf = zipfile.ZipFile(zip_name, > > 'w', > > > zipfile.ZIP_DEFLATED) > > > nit: nicer to use with: for this. > > > > > > with zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) as zipf: > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > > File build/secondary/third_party/android_tools/BUILD.gn (left): > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > > build/secondary/third_party/android_tools/BUILD.gn:126: input_jars_paths = [ > > > "$android_sdk/optional/org.apache.http.legacy.jar" ] > > > I think this is the line needed to fix the bots. However, I don't think it's > > > currently a property that you can attach to a library and have it get passed > > > along to the apk target. I really needs to be though. I'll try and work on a > > > patch for this today. > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > > File build/secondary/third_party/android_tools/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > > build/secondary/third_party/android_tools/BUILD.gn:152: lib_name = > > > "play-services-basement" > > > nit: lib_name -> _lib_name > > > > > > > > > https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... > > > build/secondary/third_party/android_tools/BUILD.gn:202: # > > > android_aar_prebuilt("google_play_services_cast_framework_java") { > > > Remove? > > > > Actually - another question: > > > > I think right now we're purposefully omitting play services resources. Does > this > > change cause unused play services resources to be included? > > also also - Maybe you're ontop of it already, but when I patch this locally the > .aar files don't exist (no google/m2repository in android_tools) Patch to allow for input_jars_paths: https://codereview.chromium.org/2227523002/ Although... not sure which of the sub-libraries is the one that relies on apache libs...
> > > I think right now we're purposefully omitting play services resources. Does > > this > > > change cause unused play services resources to be included? Yes, but it's only on the ChromePublic so it's OK I guess. Downstream we use some sort of white+blacklist IIRC. But we should have something that analyses the build and removes unused resources, like Gradle builds can do. > > also also - Maybe you're ontop of it already, but when I patch this locally > the > > .aar files don't exist (no google/m2repository in android_tools) You need to run `update_gcore.py download` to get the AARs :) > Patch to allow for input_jars_paths: https://codereview.chromium.org/2227523002/ > Although... not sure which of the sub-libraries is the one that relies on apache > libs... Thanks! That being said, the dependency on apache.http is not mentioned anywhere in the pom files... How do gradle builds get the library?
> > > I think right now we're purposefully omitting play services resources. Does > > this > > > change cause unused play services resources to be included? Yes, but it's only on the ChromePublic so it's OK I guess. Downstream we use some sort of white+blacklist IIRC. But we should have something that analyses the build and removes unused resources, like Gradle builds can do. > > also also - Maybe you're ontop of it already, but when I patch this locally > the > > .aar files don't exist (no google/m2repository in android_tools) You need to run `update_gcore.py download` to get the AARs :) > Patch to allow for input_jars_paths: https://codereview.chromium.org/2227523002/ > Although... not sure which of the sub-libraries is the one that relies on apache > libs... Thanks! That being said, the dependency on apache.http is not mentioned anywhere in the pom files... How do gradle builds get the library?
> > > also also - Maybe you're ontop of it already, but when I patch this locally > > the > > > .aar files don't exist (no google/m2repository in android_tools) > > You need to run `update_gcore.py download` to get the AARs :) I mean build/android/play_services/update.py download
> > > also also - Maybe you're ontop of it already, but when I patch this locally > > the > > > .aar files don't exist (no google/m2repository in android_tools) > > You need to run `update_gcore.py download` to get the AARs :) I mean build/android/play_services/update.py download
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... File build/android/play_services/update.py (right): https://codereview.chromium.org/2209233002/diff/20001/build/android/play_serv... build/android/play_services/update.py:443: zipf = zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) On 2016/08/05 18:59:28, agrieve wrote: > nit: nicer to use with: for this. > > with zipfile.ZipFile(zip_name, 'w', zipfile.ZIP_DEFLATED) as zipf: Done. https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... File build/secondary/third_party/android_tools/BUILD.gn (left): https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:126: input_jars_paths = [ "$android_sdk/optional/org.apache.http.legacy.jar" ] On 2016/08/05 18:59:28, agrieve wrote: > I think this is the line needed to fix the bots. However, I don't think it's > currently a property that you can attach to a library and have it get passed > along to the apk target. I really needs to be though. I'll try and work on a > patch for this today. Thanks! Release builds work locally with your patch. https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... File build/secondary/third_party/android_tools/BUILD.gn (right): https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:152: lib_name = "play-services-basement" On 2016/08/05 18:59:28, agrieve wrote: > nit: lib_name -> _lib_name Done. https://codereview.chromium.org/2209233002/diff/20001/build/secondary/third_p... build/secondary/third_party/android_tools/BUILD.gn:202: # android_aar_prebuilt("google_play_services_cast_framework_java") { On 2016/08/05 18:59:28, agrieve wrote: > Remove? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
FYI: The resource filtering happens in _ProcessResources in build/android/play_services/preprocess.py. "Drawable" resources are removed and there's a language whitelist for "value" resources.
On 2016/08/08 17:35:46, paulmiller wrote: > FYI: The resource filtering happens in _ProcessResources in > build/android/play_services/preprocess.py. "Drawable" resources are removed and > there's a language whitelist for "value" resources. I think this looks good, but I'd like to fix the resources issue before we go ahead with it. I've filed a bug for resource shrinking: https://bugs.chromium.org/p/chromium/issues/detail?id=636448
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It seems like this is going to be on hold for a while: b/30869015 and crbug.com/639494. We might have to wait until 9.8 to update the library
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [gmscore] Update the Play services to 9.4 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.4 BUG=624324 ========== to ========== [gmscore] Update the Play services to 9.8 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.8 BUG=624324,659366 ==========
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
PTAL
On 2016/10/27 14:45:58, dgn wrote: > PTAL lgtm. Fingers crossed!
Looks like the swarming bot is super flaky in the sandboxed webview test: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... Retrying...
Looks like the swarming bot is super flaky in the sandboxed webview test: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... Retrying...
The CQ bit was checked by dgn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [gmscore] Update the Play services to 9.8 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.8 BUG=624324,659366 ========== to ========== [gmscore] Update the Play services to 9.8 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.8 BUG=624324,659366 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [gmscore] Update the Play services to 9.8 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.8 BUG=624324,659366 ========== to ========== [gmscore] Update the Play services to 9.8 and load from AARs - Update the build to load the play services library from split AARs. - Update the upload/download script to handle the new library distribution format - Roll the dependencies to 9.8 BUG=624324,659366 Committed: https://crrev.com/220f57b3c7299133c089ce3c4ede7861cb8bb1e8 Cr-Commit-Position: refs/heads/master@{#428333} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/220f57b3c7299133c089ce3c4ede7861cb8bb1e8 Cr-Commit-Position: refs/heads/master@{#428333} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
