Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
286423002:
Make adb_install_apk.py saner for humans. (Closed)
|
Created:
6 years, 7 months ago by mlamouri (slow - plz ping) Modified:
5 years, 5 months ago CC:
bulach+watch_chromium.org, chromium-reviews, ilevy-cc_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@android_build_cleanup Visibility:
Public. |
DescriptionMake adb_install_apk.py saner for humans.
Scripts like that usually do not require a --option if this is a
mandatory singleton argument. Also, forgetting to pass the extension
sounds like something that the script could simply handle.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275755
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : review comments #Patch Set 4 : rebase #
Total comments: 2
Messages
Total messages: 20 (1 generated)
nice!! please wait for the other folks. it'd be great to fix up the usage in build/android/buildbot/bb_device_steps.py and build/android/provision_devices.py as part of this patch? https://codereview.chromium.org/286423002/diff/1/build/android/adb_install_ap... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:61: if not options.apk.endswith(".apk"): nit: here and below, s/"/'/
I've changed the files in build/ using adb_install_apk. I guess tools/build/ changes could be made in another CL. Hopefully, we could remove --apk altogether at some point after that. https://codereview.chromium.org/286423002/diff/1/build/android/adb_install_ap... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:61: if not options.apk.endswith(".apk"): On 2014/05/19 10:22:36, bulach wrote: > nit: here and below, s/"/'/ Done.
https://codereview.chromium.org/286423002/diff/20001/build/android/adb_instal... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/20001/build/android/adb_instal... build/android/adb_install_apk.py:51: return nit: OptionParser.error exits, so the return is unnecessary
PTAL https://codereview.chromium.org/286423002/diff/20001/build/android/adb_instal... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/20001/build/android/adb_instal... build/android/adb_install_apk.py:51: return On 2014/05/19 15:13:42, jbudorick wrote: > nit: OptionParser.error exits, so the return is unnecessary Oups, forgot about that one. Fixed.
(non-binding) lgtm, +craigdh
lgtm, thanks! please let armand RS the buildbot parts..
Armand, this CL is blocked on you. I would be grateful if you could have a quick look. Thanks :)
On 2014/05/23 09:46:11, Mounir Lamouri wrote: > Armand, this CL is blocked on you. I would be grateful if you could have a quick > look. Thanks :) ping review
On 2014/05/29 16:25:33, Mounir Lamouri wrote: > On 2014/05/23 09:46:11, Mounir Lamouri wrote: > > Armand, this CL is blocked on you. I would be grateful if you could have a > quick > > look. Thanks :) > > ping review ping review. It's been more than two weeks :(
lgtm
lgtm
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlamouri@chromium.org/286423002/60001
Message was sent while issue was closed.
Change committed as 275755
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... build/android/adb_install_apk.py:22: help=('DEPRECATED The name of the apk containing the' I happened to look at this file today and see this DEPRECATED. Putting DEPRECATED in help strings is not an effective deprecation strategy: I never saw this, and I've been using this flag for a while now – it's what's recommended on https://www.chromium.org/developers/how-tos/android-build-instructions etc. Even now that I happened to see this, I don't know what to do instead so I can't change the docs. Since the flag works fine and is the documented way to go abou tings, maybe we can just remove the DEPRECATED here? What's the motivation? (If it's to get rid of the implicit dependency of "out", that's a good motivation, in that case there could be a "use X instead" note here)
Message was sent while issue was closed.
https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... build/android/adb_install_apk.py:22: help=('DEPRECATED The name of the apk containing the' On 2015/07/23 at 17:14:15, Nico (hiding) wrote: > I happened to look at this file today and see this DEPRECATED. Putting DEPRECATED in help strings is not an effective deprecation strategy: I never saw this, and I've been using this flag for a while now – it's what's recommended on https://www.chromium.org/developers/how-tos/android-build-instructions etc. Even now that I happened to see this, I don't know what to do instead so I can't change the docs. > > Since the flag works fine and is the documented way to go abou tings, maybe we can just remove the DEPRECATED here? What's the motivation? (If it's to get rid of the implicit dependency of "out", that's a good motivation, in that case there could be a "use X instead" note here) I believe there are multiple reasons for doing this: - removing the flag itself for a required single argument, as mentioned in the description - more braodly, moving people and bots from the crazy automagical mode: # what will this install? who knows? ./build/android/adb_install_apk.py --apk ChromePublic to the more explicit, less magical mode: # much more clear ./build/android/adb_install_apk.py out/Release/apks/ChromePublic.apk I would prefer updating the docs to calling this option un-deprecated, and will do so.
Message was sent while issue was closed.
On 2015/07/23 17:28:49, jbudorick wrote: > https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... > File build/android/adb_install_apk.py (right): > > https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... > build/android/adb_install_apk.py:22: help=('DEPRECATED The name of the apk > containing the' > On 2015/07/23 at 17:14:15, Nico (hiding) wrote: > > I happened to look at this file today and see this DEPRECATED. Putting > DEPRECATED in help strings is not an effective deprecation strategy: I never saw > this, and I've been using this flag for a while now – it's what's recommended on > https://www.chromium.org/developers/how-tos/android-build-instructions etc. Even > now that I happened to see this, I don't know what to do instead so I can't > change the docs. > > > > Since the flag works fine and is the documented way to go abou tings, maybe we > can just remove the DEPRECATED here? What's the motivation? (If it's to get rid > of the implicit dependency of "out", that's a good motivation, in that case > there could be a "use X instead" note here) > > I believe there are multiple reasons for doing this: > - removing the flag itself for a required single argument, as mentioned in the > description > - more braodly, moving people and bots from the crazy automagical mode: > > # what will this install? who knows? > ./build/android/adb_install_apk.py --apk ChromePublic > > to the more explicit, less magical mode: > > # much more clear > ./build/android/adb_install_apk.py out/Release/apks/ChromePublic.apk > > I would prefer updating the docs to calling this option un-deprecated, and will > do so. Thanks! I guess my high-order bit was "never say just 'deprecated', always say 'deprecated, use X instead'"
Message was sent while issue was closed.
On 2015/07/23 at 17:29:56, thakis wrote: > On 2015/07/23 17:28:49, jbudorick wrote: > > https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... > > File build/android/adb_install_apk.py (right): > > > > https://codereview.chromium.org/286423002/diff/60001/build/android/adb_instal... > > build/android/adb_install_apk.py:22: help=('DEPRECATED The name of the apk > > containing the' > > On 2015/07/23 at 17:14:15, Nico (hiding) wrote: > > > I happened to look at this file today and see this DEPRECATED. Putting > > DEPRECATED in help strings is not an effective deprecation strategy: I never saw > > this, and I've been using this flag for a while now – it's what's recommended on > > https://www.chromium.org/developers/how-tos/android-build-instructions etc. Even > > now that I happened to see this, I don't know what to do instead so I can't > > change the docs. > > > > > > Since the flag works fine and is the documented way to go abou tings, maybe we > > can just remove the DEPRECATED here? What's the motivation? (If it's to get rid > > of the implicit dependency of "out", that's a good motivation, in that case > > there could be a "use X instead" note here) > > > > I believe there are multiple reasons for doing this: > > - removing the flag itself for a required single argument, as mentioned in the > > description > > - more braodly, moving people and bots from the crazy automagical mode: > > > > # what will this install? who knows? > > ./build/android/adb_install_apk.py --apk ChromePublic > > > > to the more explicit, less magical mode: > > > > # much more clear > > ./build/android/adb_install_apk.py out/Release/apks/ChromePublic.apk > > > > I would prefer updating the docs to calling this option un-deprecated, and will > > do so. > updated: - http://www.chromium.org/developers/how-tos/android-build-instructions - http://www.chromium.org/developers/testing/webkit-layout-tests/using-breakpad... > Thanks! > > I guess my high-order bit was "never say just 'deprecated', always say 'deprecated, use X instead'" Ah, ok. This may not be the only script of ours that has a deprecated option without a suggestion; I'll go through and clean those up. |
