Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14315010:
Add fetch recipe alias targeting Android (Closed)
|
Created:
7 years, 8 months ago by janx Modified:
7 years, 7 months ago CC:
chromium-reviews, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionAdd fetch recipe alias targetting Android
"fetch android" is a basic alias for "fetch chromium --target_os=android",
which adds "target_os=['android']" to the gclient specs as described in
chromium's AndroidBuildInstructions wiki page.
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=197031
Patch Set 1 #Patch Set 2 : Add fetch recipe alias targeting Android #Patch Set 3 : Fix git cl upload #
Total comments: 16
Patch Set 4 : Additional fixes #
Total comments: 3
Patch Set 5 : Fix pylint warning disables and import order #
Messages
Total messages: 23 (0 generated)
Basic "fetch android" recipe to fetch chromium sources targeting Android, as instructed by https://code.google.com/p/chromium/wiki/AndroidBuildInstructions. This CL depends on https://codereview.chromium.org/13878009/, or else the "target_os=['android']" line might end up on top of "solutions=[{...}]" in the specs.
I'm not sure what the best way to handle variants like this is. What if I wanted a version of the Blink recipe for Android? I'll grant that in many ways even Blink is a variant on Chromium, but at least you have to run extra commands, rather than just overriding a variable. What do others think?
When we created the alias mechanism, we decided to explicitly disallow alias chaining -- if one recipe returns an alias, we make sure that the one it aliases to doesn't also return an alias. This was an arbitrary decision to prevent complex and unreadable recipe chaining, and I stand by it. So to get things like "Android with tip-of-tree Blink", we have two options: create a recipe just for that, or recognize that it is such an uncommon use-case that the developers who want that can simply call "fetch android --webkit_rev=ToT". I'd personally prefer the latter, to prevent a combinatorial explosion of recipes, as long as Android+ToT Blink truly is an uncommon use-case.
For that particular use-case I would agree with having to do "fetch android --webkit_rev=ToT", because "fetch blink"'s job is to create a Blink-flavored development, as opposed to "fetch android" whose job it is to create and Android-flavored one. But since "fetch android" is just an alias for "fetch chromium --target_os=android", I can remove the android.py file if you don't like it, and just leave the "target_os" handling code in chromium.py.
On 2013/04/19 13:32:26, janx wrote: > For that particular use-case I would agree with having to do "fetch android > --webkit_rev=ToT", because "fetch blink"'s job is to create a Blink-flavored > development, as opposed to "fetch android" whose job it is to create and > Android-flavored one. > > But since "fetch android" is just an alias for "fetch chromium > --target_os=android", I can remove the android.py file if you don't like it, and > just leave the "target_os" handling code in chromium.py. I think this lgtm. It fits with the current recipe model, is a pretty simple alias, and fetching android with tip-of-tree blink isn't hard. So I'd say go ahead and commit this unless Dirk has a big objection.
No reviewers yet.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/janx@chromium.org/14315010/1
Presubmit check for 14315010-1 failed and returned exit status 1.
INFO:root:Found 2 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/tools/depot_tools/PRESUBMIT.py
Traceback (most recent call last):
File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
sys.exit(presubmit_support.Main(argv))
File "/b/depot_tools/presubmit_support.py", line 1305, in Main
rietveld_obj)
File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
results += executer.ExecPresubmitScript(presubmit_script, filename)
File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
result = eval(function_name + '(*__args)', context)
File "<string>", line 1, in <module>
File "<string>", line 106, in CheckChangeOnCommit
File "<string>", line 26, in CommonChecks
AttributeError: 'InputApi' object has no attribute 'RunTests'
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/janx@chromium.org/14315010/14001
Presubmit check for 14315010-14001 failed and returned exit status 1. INFO:root:Found 2 file(s). INFO:PRESUBMIT:Running pylint on 61 files Running presubmit commit checks ... Running /b/commit-queue/workdir/tools/depot_tools/PRESUBMIT.py Running tests/gclient_utils_test.py Running tests/watchlists_unittest.py Running tests/checkout_test.py Running tests/scm_unittest.py Running tests/gcl_unittest.py Running tests/patch_test.py Running tests/fix_encoding_test.py Running tests/presubmit_unittest.py Running tests/trychange_unittest.py Running tests/rietveld_test.py Running tests/owners_unittest.py Running tests/breakpad_unittest.py Running tests/gclient_test.py Running tests/gclient_smoketest.py Running tests/subprocess2_test.py Running tests/git_cl_test.py Running tests/gclient_scm_test.py INFO:root:/usr/bin/python /b/google_appengine/dev_appserver.py . --port 8080 --storage /tmp/rietveld_testj_jaNI --clear_search_indexes --skip_sdk_update_check Setting up test upstream git repo... Setting up test git repo... TESTING: uploading to bogus server test | 1 + 1 file changed, 1 insertion(+) Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit upload checks ... Presubmit checks passed. Upload server: http://bogus.example.com:80 (change with -s/--server) Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Got exception while uploading -- saving description to /home/chrome-bot/.git_cl_description_backup TESTING: description was backed up PASS Setting up test upstream git repo... Setting up test git repo... TESTING: git-cl upload wants a server TESTING: git-cl status has no issue TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 77 0 0 100 77 0 1639 --:--:-- --:--:-- --:--:-- 1673 TESTING: Base URL contains branch name TESTING: git-cl push ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'foo-quux\n\nReview URL: http://localhost:8080/5629499534213120' Closing issue (you may be prompted for your codereview password)... TESTING: committed code has proper description TESTING: issue no longer has a branch TESTING: upstream repo has our commit PASS Setting up test SVN repo... Setting up test git-svn repo... The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to Branch feature_branch set up to track local ref refs/remotes/trunk. TESTING: Guessing upstream branch for refs/remotes/trunk The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to Branch feature_branch set up to track local ref refs/remotes/some_branch. TESTING: Guessing upstream branch for refs/remotes/some_branch PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl dcommits ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'test\n\nBUG=\n\nReview URL: http://localhost:8080/5910974510923776' Closing issue (you may be prompted for your codereview password)... PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. Switched to branch 'master' Deleted branch abandoned (was ec577de). TESTING: git-cl status dropped abandoned branch PASS Setting up test SVN repo... Setting up test remote git-svn-submodule repo... Switched to branch 'master' TESTING: dcommitted code Switched to branch 'git-cl-cherry-pick' Using 50% similarity for rename/copy detection. Override with --similarity. Description: 'dcommit' TESTING: svn got new revision TESTING: svn diff is correct TESTING: git svn fetch gets new svn revision PASS Setting up test upstream git repo... Setting up test git repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: git-cl upload hook fails Command "git config rietveld.server" failed. Could not find settings file. You must configure your review setup by running "git cl config". TESTING: git-cl dcommit hook fails Command "git config rietveld.server" failed. Could not find settings file. You must configure your review setup by running "git cl config". PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: dcommitted code Using 50% similarity for rename/copy detection. Override with --similarity. Description: 'dcommit' TESTING: post-cl-dcommit hook executed PASS Setting up test upstream git repo... Setting up test git repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: description shouldn't contain unrelated commits PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue TESTING: git cl patch 5981343255101440 PASS Setting up test SVN repo... Setting up test git-svn repo... TESTING: git-cl upload wants a server TESTING: git-cl status has no issue TESTING: upload succeeds (needs a server running on localhost) WARNING: Use -t or --title to set the title of the patchset. In the near future, -m or --message will send a message instead. See http://goo.gl/JGg0Z for details. TESTING: git-cl status now knows the issue % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 77 0 0 100 77 0 1656 --:--:-- --:--:-- --:--:-- 1711 TESTING: git-cl dcommits ok Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies Running presubmit commit checks ... Presubmit checks passed. Description: 'foo-quux\n\nReview URL: http://localhost:8080/5840605766746112' Closing issue (you may be prompted for your codereview password)... TESTING: dcommitted code has proper description TESTING: issue no longer has a branch TESTING: upstream svn has our commit PASS Checking out rietveld... Running save-description-on-failure.sh Running push-basic.sh Running upstream.sh Running submit-from-new-dir.sh Running abandon.sh Running submodule-merge-test.sh Running upload-local-tracking-branch.sh Running hooks.sh Running post-dcommit-hook-test.sh Running upload-stale.sh Running patch.sh Running basic.sh ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: depot_tools/recipes/android.py depot_tools/recipes/chromium.py Presubmit checks took 91.0s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Apparently the commit queue is having some issues with the presubmit checks, because my patch misses approval from an OWNER. Aaron: Maybe you should be added to the OWNERS file? Dirk: Do you have any objections to this patch?
https://codereview.chromium.org/14315010/diff/14001/recipes/android.py File recipes/android.py (right): https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode6 recipes/android.py:6: import recipe_util stdlib import first then local imports https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode8 recipes/android.py:8: 2 lines https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode9 recipes/android.py:9: # pylint: disable=W0232 Can you add a single line comment with what this warning is? https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode18 recipes/android.py:18: 'props': ['--target_os=android'] Append comma https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode19 recipes/android.py:19: } Here too https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode24 recipes/android.py:24: Android().handle_args(argv) return ? Aaron, does handle_args returns an int? https://codereview.chromium.org/14315010/diff/14001/recipes/chromium.py File recipes/chromium.py (right): https://codereview.chromium.org/14315010/diff/14001/recipes/chromium.py#newco... recipes/chromium.py:34: spec['target_os'] = props['target_os'].split(',') Any problem about whitespaces? If so; [i.strip() for i in props['target_os'].split(',') if i.strip()]
Sorry, forgot I'm only an owner in build/, not in depot_tools/. https://chromiumcodereview.appspot.com/14315010/diff/14001/recipes/android.py File recipes/android.py (right): https://chromiumcodereview.appspot.com/14315010/diff/14001/recipes/android.py... recipes/android.py:9: # pylint: disable=W0232 On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > Can you add a single line comment with what this warning is? It's "class has no __init__ method", and it's my bad for not having a comment in the chromium/blink recipes used as a template for this one. Janx, if you'd like to add the comment to all the recipes, that'd be great, or you can just do it here. https://chromiumcodereview.appspot.com/14315010/diff/14001/recipes/android.py... recipes/android.py:24: Android().handle_args(argv) On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > return ? > > Aaron, does handle_args returns an int? Yes, right this should be "return Android..."
Thanks a lot for your suggestions, Marc-Antoine and Aaron. I addressed most of them in this new patch set, and also fixed the other recipe files where possible.
https://codereview.chromium.org/14315010/diff/14001/recipes/android.py File recipes/android.py (right): https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode6 recipes/android.py:6: import recipe_util On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > stdlib import first > > then local imports Done. https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode8 recipes/android.py:8: On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > 2 lines Done. https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode9 recipes/android.py:9: # pylint: disable=W0232 On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > Can you add a single line comment with what this warning is? Done. https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode18 recipes/android.py:18: 'props': ['--target_os=android'] On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > Append comma Done. https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode19 recipes/android.py:19: } On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > Here too Done. https://codereview.chromium.org/14315010/diff/14001/recipes/android.py#newcode24 recipes/android.py:24: Android().handle_args(argv) On 2013/04/24 18:04:09, Aaron Gable wrote: > On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > > return ? > > > > Aaron, does handle_args returns an int? > > Yes, right this should be "return Android..." Done. https://codereview.chromium.org/14315010/diff/14001/recipes/chromium.py File recipes/chromium.py (right): https://codereview.chromium.org/14315010/diff/14001/recipes/chromium.py#newco... recipes/chromium.py:34: spec['target_os'] = props['target_os'].split(',') On 2013/04/24 13:09:14, Marc-Antoine Ruel wrote: > Any problem about whitespaces? If so; > [i.strip() for i in props['target_os'].split(',') if i.strip()] I don't see why we would have whitespace problems. To set those values, the user calls `fetch recipe --target_os=first,second`, and adding whitespaces here would require additional quotes or else fetch will see several arguments and break. Also, `strip()` would not help against errors like `--target_os="first second"`, so I guess we can let gclient complain about any format errors in the resulting spec.
I'm mostly fine with the CL https://codereview.chromium.org/14315010/diff/20001/recipes/android.py File recipes/android.py (right): https://codereview.chromium.org/14315010/diff/20001/recipes/android.py#newcode6 recipes/android.py:6: # pylint: disable=F0401 I prefer: import recipe_util # pylint: disable=F0401 so it doesn't apply to the following lines. https://codereview.chromium.org/14315010/diff/20001/recipes/blink.py File recipes/blink.py (right): https://codereview.chromium.org/14315010/diff/20001/recipes/blink.py#newcode6 recipes/blink.py:6: import json sorted https://codereview.chromium.org/14315010/diff/20001/recipes/blink.py#newcode7 recipes/blink.py:7: # pylint: disable=F0401 same as the other file. And separate stdlib from non-stdlib imports by an empty line to make this clear.
Thanks for the comments Marc-Antoine, I also addressed them in chromium.py.
perfect, lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/janx@chromium.org/14315010/26001
Message was sent while issue was closed.
Change committed as 197031
Message was sent while issue was closed.
Should we update the wiki to tell people to use "fetch android" ?
Message was sent while issue was closed.
I slightly modified https://code.google.com/p/chromium/wiki/UsingGit and https://code.google.com/p/chromium/wiki/AndroidBuildInstructions to talk about `fetch android`.
Message was sent while issue was closed.
Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
