Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
140503007:
Scripts to generate Android.mk for framework Skia. (Closed)
|
Created:
6 years, 11 months ago by scroggo Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionScripts to generate Android.mk for framework Skia.
In order to create Android.mk, run
>> python platform_tools/android/bin/gyp_to_android.py
For the change in the Android.mk file, see
https://googleplex-android-review.git.corp.google.com/#/c/408170/
(SkipBuildbotRuns)
BUG=skia:1975
Committed: http://code.google.com/p/skia/source/detail?r=13344
Patch Set 1 #Patch Set 2 : Small fixes from comments in Android CL #Patch Set 3 : Update copyrights, remove unnecessary code. #
Total comments: 15
Patch Set 4 : Defer x86 opts until we figure out how to include them. #Patch Set 5 : Respond to comments in patch set 3. #
Total comments: 14
Patch Set 6 : Respond to comments in patch set 5. #
Total comments: 40
Patch Set 7 : Respond to Derek's comments on patch set 6. #Patch Set 8 : Respond to/acknowledge Elliot's changes in patch set 6. #Patch Set 9 : Merge in self_tests from https://codereview.chromium.org/142173002 #Patch Set 10 : Use lower case function names. #Patch Set 11 : Split up gyp_to_android.py #Patch Set 12 : Small changes to get the split files working again. #Patch Set 13 : VarsDict = namedtuple of OrderedSets #
Total comments: 4
Patch Set 14 : Small fixes. #Patch Set 15 : Override __getitem__ to allow dictionary lookup. #Patch Set 16 : Fix lint errors. #
Total comments: 23
Patch Set 17 : Respond to Elliot's comments in patch set 16. #
Total comments: 2
Patch Set 18 : Remove gyp changes, which have already been committed. #Patch Set 19 : Move tests (split, but unchanged) into platform_tools. #Patch Set 20 : Update tests. #
Total comments: 4
Patch Set 21 : Respond to Elliot's comments in patch set 20. #Messages
Total messages: 24 (0 generated)
Latest patch on creating Android.mk from gyp, ready for review. (Slight side note: It worked on ToT Android, but once Android catches up, we will likely need to edit this some more to account for things currently in ToT Skia.)
https://codereview.chromium.org/140503007/diff/100001/gyp/android_deps.gyp File gyp/android_deps.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/android_deps.gyp#ne... gyp/android_deps.gyp:45: '$(TOPDIR)external/giflib', do you need topdir here? https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.g... gyp/common_conditions.gypi:207: 'cflags!': [ how do we determine what goes in this list? https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.g... gyp/common_conditions.gypi:245: 'include_dirs!': [ who generates these? https://codereview.chromium.org/140503007/diff/100001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/core.gyp#newcode79 gyp/core.gyp:79: [ 'skia_os == "android" and skia_android_framework==0', { I don't think you need to modify this file. Just update android_deps.gyp to have an entry for cpu_features when built for the framework. https://codereview.chromium.org/140503007/diff/100001/gyp/freetype.gyp File gyp/freetype.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/freetype.gyp#newcode1 gyp/freetype.gyp:1: # Target for building freetype. I would remove this as it isn't related to the primary purpose of this CL. https://codereview.chromium.org/140503007/diff/100001/gyp/gm.gyp File gyp/gm.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/gm.gyp#newcode116 gyp/gm.gyp:116: ['skia_android_framework', { if we aren't building gm yet, do we need this? https://codereview.chromium.org/140503007/diff/100001/gyp/zlib.gyp File gyp/zlib.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/zlib.gyp#newcode34 gyp/zlib.gyp:34: 'conditions': [ is this condition necessary?
https://codereview.chromium.org/140503007/diff/100001/gyp/android_deps.gyp File gyp/android_deps.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/android_deps.gyp#ne... gyp/android_deps.gyp:45: '$(TOPDIR)external/giflib', On 2014/01/21 17:47:37, djsollen wrote: > do you need topdir here? This was required when I was attempting to use the Android generators. No longer needed. https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.g... gyp/common_conditions.gypi:207: 'cflags!': [ On 2014/01/21 17:47:37, djsollen wrote: > how do we determine what goes in this list? Mostly these are flags that ended up in the final generated Android.mk (i.e. our conditions resulted in them being set*) which weren't there in the hand edited one. The reason I removed them ALL was because with all of them set I was able to build but the final result failed to boot on my Android device. Instead of figuring out one by one which were okay, I used my cheat sheet (the hand edited Android.mk). Should some of these flags be set (or are some indifferent)? * Another way of doing this would be to prevent each of these flags from being set if skia_android_framework == 1. I started off using that method, but switched to turning them off here because I wasn't sure which ones HAD to be turned off. https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.g... gyp/common_conditions.gypi:215: '-g', This flag came along with using the default configuration, which was SK_DEBUG. I think the way the current Android.mk is written makes sense: -g is NOT defined, and if someone wants to debug they can define it (maybe it would be better to use a release build?). This may bring up a larger issue: as far as I can tell, our gyp files currently do not compile different files (only sections of code within files are selected/deselected). This works with the current model where someone compiling Android can modify the makefile slightly in order to get the desired debug/release/developer build (this also means it makes little difference which build flavor I request from gyp). If we were to make a change to Skia that requires different files for different build flavors, it would require some more complexity for generating Android.mk https://codereview.chromium.org/140503007/diff/100001/gyp/common_conditions.g... gyp/common_conditions.gypi:245: 'include_dirs!': [ On 2014/01/21 17:47:37, djsollen wrote: > who generates these? core.gyp. They've been there since the start (see https://codereview.appspot.com/4282056/). I'm not sure what they refer to, since we don't have toplevel folders named config or ext. https://codereview.chromium.org/140503007/diff/100001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/core.gyp#newcode79 gyp/core.gyp:79: [ 'skia_os == "android" and skia_android_framework==0', { On 2014/01/21 17:47:37, djsollen wrote: > I don't think you need to modify this file. Just update android_deps.gyp to have > an entry for cpu_features when built for the framework. Done. https://codereview.chromium.org/140503007/diff/100001/gyp/freetype.gyp File gyp/freetype.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/freetype.gyp#newcode1 gyp/freetype.gyp:1: # Target for building freetype. On 2014/01/21 17:47:37, djsollen wrote: > I would remove this as it isn't related to the primary purpose of this CL. Done. https://codereview.chromium.org/140503007/diff/100001/gyp/gm.gyp File gyp/gm.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/gm.gyp#newcode116 gyp/gm.gyp:116: ['skia_android_framework', { On 2014/01/21 17:47:37, djsollen wrote: > if we aren't building gm yet, do we need this? Removed. https://codereview.chromium.org/140503007/diff/100001/gyp/zlib.gyp File gyp/zlib.gyp (right): https://codereview.chromium.org/140503007/diff/100001/gyp/zlib.gyp#newcode34 gyp/zlib.gyp:34: 'conditions': [ On 2014/01/21 17:47:37, djsollen wrote: > is this condition necessary? Yes. When I attempt to build the Android framework, it cannot find zlib.h - unless I need to add its include directory?
https://codereview.chromium.org/140503007/diff/270001/gyp/android_framework_l... File gyp/android_framework_lib.gyp (right): https://codereview.chromium.org/140503007/diff/270001/gyp/android_framework_l... gyp/android_framework_lib.gyp:22: 'android_unmangled_name': 1, not sure if you need that if you are parsing your own gypd https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:207: 'cflags!': [ If these are in our gyp files then I would prefer that we either include them in our Android.mk or explicitly disallow them at the source instead of doing this catch all removal here. https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:242: 'libraries!': [ same here https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:246: 'include_dirs!': [ and here https://codereview.chromium.org/140503007/diff/270001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/140503007/diff/270001/gyp/ports.gyp#newcode72 gyp/ports.gyp:72: and skia_android_framework == 0', { not sure I understand what is going on here. Is there a bug in gyp? https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... File platform_tools/android/bin/android_framework_gyp.py (right): https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:1: #!/usr/bin/python not sure I'm the best person to review python. Can you add eric, ravi, joe, or elliot to take a look at this as well? https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:97: ############################################################################### we should also include now that this file is auto-generated so that people don't make edits to it that we will erase.
Elliot, can you review the python code? (Sorry for adding you in the middle - some of your complaints will have already been addressed in https://codereview.chromium.org/142173002/. Let me know if it will make it easier to review if I merge that change into this one.) https://codereview.chromium.org/140503007/diff/270001/gyp/android_framework_l... File gyp/android_framework_lib.gyp (right): https://codereview.chromium.org/140503007/diff/270001/gyp/android_framework_l... gyp/android_framework_lib.gyp:22: 'android_unmangled_name': 1, On 2014/01/22 21:02:44, djsollen wrote: > not sure if you need that if you are parsing your own gypd Done. https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:207: 'cflags!': [ On 2014/01/22 21:02:44, djsollen wrote: > If these are in our gyp files then I would prefer that we either include them in > our Android.mk or explicitly disallow them at the source instead of doing this > catch all removal here. Done. https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:242: 'libraries!': [ On 2014/01/22 21:02:44, djsollen wrote: > same here Done. https://codereview.chromium.org/140503007/diff/270001/gyp/common_conditions.g... gyp/common_conditions.gypi:246: 'include_dirs!': [ On 2014/01/22 21:02:44, djsollen wrote: > and here I think these are actually unneeded in general, so I've uploaded https://codereview.chromium.org/145203003/ If it turns out they ARE needed for some reason, I'll disallow them at the source with skia_android_framework, but I suspect that is not the case, so removing entirely. https://codereview.chromium.org/140503007/diff/270001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/140503007/diff/270001/gyp/ports.gyp#newcode72 gyp/ports.gyp:72: and skia_android_framework == 0', { On 2014/01/22 21:02:44, djsollen wrote: > not sure I understand what is going on here. Is there a bug in gyp? Hmm, I thought so, but it must have been user error, since I'm no longer running into the problem (or perhaps it was when running using the real Android gyp generator). I've removed this complication. https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... File platform_tools/android/bin/android_framework_gyp.py (right): https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:1: #!/usr/bin/python On 2014/01/22 21:02:44, djsollen wrote: > not sure I'm the best person to review python. Can you add eric, ravi, joe, or > elliot to take a look at this as well? Adding elliot, who has been looking at https://codereview.chromium.org/142173002/, which tests this change. https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/270001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:97: ############################################################################### On 2014/01/22 21:02:44, djsollen wrote: > we should also include now that this file is auto-generated so that people don't > make edits to it that we will erase. Done.
2 nits, but other than getting someone to look at the python code it 1gtm. https://codereview.chromium.org/140503007/diff/430001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/140503007/diff/430001/gyp/core.gyp#newcode86 gyp/core.gyp:86: '-lcutils', can you document that this is for SkAtomics_android.h https://codereview.chromium.org/140503007/diff/430001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/140503007/diff/430001/gyp/ports.gyp#newcode173 gyp/ports.gyp:173: [ 'skia_android_framework', { would it make more sense to do this work in the freetype.gyp file instead of here. That way if any of our other gyps depended on that target we wouldn't have to dup this code.
looking now
Reviewed the .py files, and ignored the rest. Let's discuss live... plenty to chew on... https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... File platform_tools/android/bin/android_framework_gyp.py (right): https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:17: # Unlike gyp_skia, this file is nested deep inside Skia. Move to Skia trunk. I'm not sure what you mean by "Move to Skia trunk". Unless you mean "TODO(scroggo): Move this file to Skia's trunk dir", maybe "Find Skia's trunk dir" would be clearer? https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:43: gyp_defines = gyp_defines + 'arm_thumb=1 arm_version=7 ' You can use x += y instead of x = x + y https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:9: Script for generating the Android framework's verision of Skia from gyp No need to indent these lines. verision -> version https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:18: # FIXME: Currently the x86 specific files are not included. Include them. I think we more commonly use TODO(username) https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:21: def WriteGroup(f, name, items, append): Throughout: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming calls for lower_with_under() naming for functions and methods. Except when we are trying to maintain consistency within existing code, I think we should follow the style guide. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:29: if len(items) == 0: mild preference for "if not items:", which will handle either a 0-length list or None https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:36: f.write('\t%s \\\n' % item) There's probably some easy way to make python do this for you, like f.write(' '.join(items)) or something like that. If you want to format them in a nice human-readable way, this would work: f.write('%s ' % name) if append: f.write('+=') else: f.write(':=') f.write(' \\\n\t') f.write(' \\\n\t'.join(items)) f.write('\n') Edit: having typed all that out, I think yours is clearer anyway. But food for thought. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:42: Custom dict of lists for each makefile variable. Does not allow insertion of I think the Python-y way of doing this is http://docs.python.org/2/library/collections.html#collections.namedtuple , as in: MakefileVars = namedtuple('MakefileVars', ['cflags', 'cppflags', 'defines', ... 'known_targets']) Then, you can access the fields by name: WriteGroup(f, 'LOCAL_CFLAGS', makefile_vars.cflags, True) WriteGroup(f, 'LOCAL_CPPFLAGS', makefile_vars.cppflags, append) ... Edit: Based on usage of VarsDict throughout the rest of the module, I'm not sure how the data structure is best arranged. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:49: self['cppflags'] = [] Are these truly lists (ordered, allowing multiple occurrences of the same value) or sets (unordered, no more than one occurrence of each value)? See http://docs.python.org/2/tutorial/datastructures.html#sets , which includes this tidbit that may be helpful to you: "Set objects also support mathematical operations like union, intersection, difference, and symmetric difference." https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:67: Add each string in 'defines' to 'cflags', prepended with '-D'. Can only Alternatively, I guess you could add a GetCflags() method that would return something based on cflags and defines fields. I don't know which is more appropriate for your use. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:73: define = '-D' + define I think modifying the iterator is dangerous/confusing. Instead, maybe: for define in self['defines']: self['cflags'].append('-D' + define) https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:86: WriteGroup(f, 'LOCAL_CFLAGS', var_dict['cflags'], True) Maybe we should just use 'LOCAL_CFLAGS' as the key into var_dict, instead of 'cflags'? Then your could set all the members like this: for key in ['LOCAL_SRC_FILES', 'LOCAL_SHARED_LIBRARIES', ...]: WriteGroup(f, key, var_dict[key], append) https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:202: Add item to li, but only if it is not already in li. sounds like a set, not a list https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:207: def GetMembers(d, name): I think this function can be replaced by calls to d.get(name, []) See http://www.tutorialspoint.com/python/dictionary_get.htm https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:323: def Intersect(var_dict_list): I think the VarsDict class, and functions that operate on VarsDict objects, should be extracted into a separate module and unittested. There are plenty of ways to get these implementations wrong, and they will cause mysterious failures.
<To reduce the churn for you guys, since this isn't quite ready, I'm unchecking "Send mail" from "Publish + Mail Draft Comments" and just publishing them.> I've responded to your comments inline. Next I'll merge in https://codereview.chromium.org/142173002/ (which Elliot already approved) as a single patch set. Then I'll start moving the code around. https://codereview.chromium.org/140503007/diff/430001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/140503007/diff/430001/gyp/core.gyp#newcode86 gyp/core.gyp:86: '-lcutils', On 2014/01/23 15:01:33, djsollen wrote: > can you document that this is for SkAtomics_android.h Done. https://codereview.chromium.org/140503007/diff/430001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/140503007/diff/430001/gyp/ports.gyp#newcode173 gyp/ports.gyp:173: [ 'skia_android_framework', { On 2014/01/23 15:01:33, djsollen wrote: > would it make more sense to do this work in the freetype.gyp file instead of > here. That way if any of our other gyps depended on that target we wouldn't > have to dup this code. Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... File platform_tools/android/bin/android_framework_gyp.py (right): https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:17: # Unlike gyp_skia, this file is nested deep inside Skia. Move to Skia trunk. On 2014/01/23 18:57:58, epoger wrote: > I'm not sure what you mean by "Move to Skia trunk". > > Unless you mean "TODO(scroggo): Move this file to Skia's trunk dir", maybe "Find > Skia's trunk dir" would be clearer? Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/android_framework_gyp.py:43: gyp_defines = gyp_defines + 'arm_thumb=1 arm_version=7 ' On 2014/01/23 18:57:58, epoger wrote: > You can use > x += y > instead of > x = x + y Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:9: Script for generating the Android framework's verision of Skia from gyp On 2014/01/23 18:57:58, epoger wrote: > No need to indent these lines. > > verision -> version Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:18: # FIXME: Currently the x86 specific files are not included. Include them. On 2014/01/23 18:57:58, epoger wrote: > I think we more commonly use TODO(username) Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:21: def WriteGroup(f, name, items, append): On 2014/01/23 18:57:58, epoger wrote: > Throughout: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming calls for > lower_with_under() naming for functions and methods. > > Except when we are trying to maintain consistency within existing code, I think > we should follow the style guide. I would swear I found some style guide that told me to use the format I used, but now I can't find it. Will fix after I merge in the tests. Added a TODO for now. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:29: if len(items) == 0: On 2014/01/23 18:57:58, epoger wrote: > mild preference for "if not items:", which will handle either a 0-length list or > None That is clearer, I think. Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:36: f.write('\t%s \\\n' % item) On 2014/01/23 18:57:58, epoger wrote: > There's probably some easy way to make python do this for you, like > > f.write(' '.join(items)) > > or something like that. > If you want to format them in a nice human-readable way, this would work: > > f.write('%s ' % name) > if append: > f.write('+=') > else: > f.write(':=') > f.write(' \\\n\t') > f.write(' \\\n\t'.join(items)) > f.write('\n') > > Edit: having typed all that out, I think yours is clearer anyway. But food for > thought. I think the choice between '+' and ':' is clearer the way I wrote it. I hear there's a sneaky way to do <cond> ? <true branch> : <false branch> in Python using 'and' and 'or', but it is prone to error so I think I'll save myself the trouble.... As for the rest, I'm not being very Pythonic by using a for statement. I've taken your suggestion a step further - if I create a new list with the first entry being the name (and '+' or ':'), and then use ' \\\n\t'.join() for the whole list, I think that ends up being cleaner. It also means my last line doesn't end in a useless '\'. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:42: Custom dict of lists for each makefile variable. Does not allow insertion of On 2014/01/23 18:57:58, epoger wrote: > I think the Python-y way of doing this is > http://docs.python.org/2/library/collections.html#collections.namedtuple , as > in: > MakefileVars = namedtuple('MakefileVars', ['cflags', 'cppflags', 'defines', ... > 'known_targets']) > > Then, you can access the fields by name: > WriteGroup(f, 'LOCAL_CFLAGS', makefile_vars.cflags, True) > WriteGroup(f, 'LOCAL_CPPFLAGS', makefile_vars.cppflags, append) > ... > > Edit: Based on usage of VarsDict throughout the rest of the module, I'm not sure > how the data structure is best arranged. Added a TODO. Will think on it. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:49: self['cppflags'] = [] On 2014/01/23 18:57:58, epoger wrote: > Are these truly lists (ordered, allowing multiple occurrences of the same value) > or sets (unordered, no more than one occurrence of each value)? > > See http://docs.python.org/2/tutorial/datastructures.html#sets , which includes > this tidbit that may be helpful to you: > "Set objects also support mathematical operations like union, intersection, > difference, and symmetric difference." Thanks for the tip. They're actually neither - they are ordered lists, with only one occurrence of each value. They are ordered for two reasons: 1. It will be easier to track changes in the resulting makefile. 2. For SkImageDecoders, we do care about the order (since the last one compiled will be the first format checked for). We could change things so this does not depend on the compile order (in a future change), which would eliminate this case. The intersect/difference operators would be useful though! I could write an ordered set, but I think this would just be more complicated... https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:67: Add each string in 'defines' to 'cflags', prepended with '-D'. Can only On 2014/01/23 18:57:58, epoger wrote: > Alternatively, I guess you could add a GetCflags() method that would return > something based on cflags and defines fields. I don't know which is more > appropriate for your use. Good thinking. I could also add defines directly to the cflags. Will think on it. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:73: define = '-D' + define On 2014/01/23 18:57:58, epoger wrote: > I think modifying the iterator is dangerous/confusing. Instead, maybe: > > for define in self['defines']: > self['cflags'].append('-D' + define) Good point. I'll add that, and just know that ConvertDefines may go away entirely anyway. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:86: WriteGroup(f, 'LOCAL_CFLAGS', var_dict['cflags'], True) On 2014/01/23 18:57:58, epoger wrote: > Maybe we should just use 'LOCAL_CFLAGS' as the key into var_dict, instead of > 'cflags'? Then your could set all the members like this: > > for key in ['LOCAL_SRC_FILES', 'LOCAL_SHARED_LIBRARIES', ...]: > WriteGroup(f, key, var_dict[key], append) I could potentially take that further and pass the dictionary itself. Added a TODO for now, and will think on it. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:202: Add item to li, but only if it is not already in li. On 2014/01/23 18:57:58, epoger wrote: > sounds like a set, not a list As stated above, I do care about order. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:207: def GetMembers(d, name): On 2014/01/23 18:57:58, epoger wrote: > I think this function can be replaced by calls to > > d.get(name, []) > > See http://www.tutorialspoint.com/python/dictionary_get.htm Done. Thanks! https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:323: def Intersect(var_dict_list): On 2014/01/23 18:57:58, epoger wrote: > I think the VarsDict class, and functions that operate on VarsDict objects, > should be extracted into a separate module and unittested. > > There are plenty of ways to get these implementations wrong, and they will cause > mysterious failures. Added a TODO. Will do in a followup patch set.
On 2014/01/24 20:09:02, scroggo wrote: > <To reduce the churn for you guys, since this isn't quite ready, I'm unchecking > "Send mail" from "Publish + Mail Draft Comments" and just publishing them.> Thanks for keeping reviewer sanity in mind. I'll wait for your next emailed update on this bug, and re-review.
https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:21: def WriteGroup(f, name, items, append): On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > Throughout: > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming calls > for > > lower_with_under() naming for functions and methods. > > > > Except when we are trying to maintain consistency within existing code, I > think > > we should follow the style guide. > > I would swear I found some style guide that told me to use the format I used, > but now I can't find it. Will fix after I merge in the tests. > > Added a TODO for now. And thanks to Joe I now see where I found that: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head Now we just need a guide to tell us which style guide to use... https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:42: Custom dict of lists for each makefile variable. Does not allow insertion of On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > I think the Python-y way of doing this is > > http://docs.python.org/2/library/collections.html#collections.namedtuple , as > > in: > > MakefileVars = namedtuple('MakefileVars', ['cflags', 'cppflags', 'defines', > ... > > 'known_targets']) > > > > Then, you can access the fields by name: > > WriteGroup(f, 'LOCAL_CFLAGS', makefile_vars.cflags, True) > > WriteGroup(f, 'LOCAL_CPPFLAGS', makefile_vars.cppflags, append) > > ... > > > > Edit: Based on usage of VarsDict throughout the rest of the module, I'm not > sure > > how the data structure is best arranged. > > Added a TODO. Will think on it. Patch set 13 uses a namedtuple. It has some downsides (documented elsewhere), but I think overall it yields much clearer code. Thanks for the suggestion! https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:49: self['cppflags'] = [] On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > Are these truly lists (ordered, allowing multiple occurrences of the same > value) > > or sets (unordered, no more than one occurrence of each value)? > > > > See http://docs.python.org/2/tutorial/datastructures.html#sets , which > includes > > this tidbit that may be helpful to you: > > "Set objects also support mathematical operations like union, intersection, > > difference, and symmetric difference." > > Thanks for the tip. They're actually neither - they are ordered lists, with only > one occurrence of each value. > > They are ordered for two reasons: > 1. It will be easier to track changes in the resulting makefile. > 2. For SkImageDecoders, we do care about the order (since the last one compiled > will be the first format checked for). We could change things so this does not > depend on the compile order (in a future change), which would eliminate this > case. > > The intersect/difference operators would be useful though! I could write an > ordered set, but I think this would just be more complicated... Now they are OrderedSets. I don't think it was so complicated after all. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:73: define = '-D' + define On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > I think modifying the iterator is dangerous/confusing. Instead, maybe: > > > > for define in self['defines']: > > self['cflags'].append('-D' + define) > > Good point. I'll add that, and just know that ConvertDefines may go away > entirely anyway. As hinted at previously, patch set 13 eliminates ConvertDefines entirely. Instead, defines are added (prepended with '-D') to LOCAL_CFLAGS to begin with. The only thing gained by ConvertDefines was it grouped the defines at the end. I'm not sure that's necessary (though it could also be done by sorting) and the current way of doing things requires some extra smarts to make sure it's only done once. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:86: WriteGroup(f, 'LOCAL_CFLAGS', var_dict['cflags'], True) On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > Maybe we should just use 'LOCAL_CFLAGS' as the key into var_dict, instead of > > 'cflags'? Then your could set all the members like this: > > > > for key in ['LOCAL_SRC_FILES', 'LOCAL_SHARED_LIBRARIES', ...]: > > WriteGroup(f, key, var_dict[key], append) > > I could potentially take that further and pass the dictionary itself. Added a > TODO for now, and will think on it. Done. https://codereview.chromium.org/140503007/diff/430001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:323: def Intersect(var_dict_list): On 2014/01/24 20:09:02, scroggo wrote: > On 2014/01/23 18:57:58, epoger wrote: > > I think the VarsDict class, and functions that operate on VarsDict objects, > > should be extracted into a separate module and unittested. > > > > There are plenty of ways to get these implementations wrong, and they will > cause > > mysterious failures. > > Added a TODO. Will do in a followup patch set. Done. https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... File platform_tools/android/gyp_gen/makefile_writer.py (right): https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... platform_tools/android/gyp_gen/makefile_writer.py:58: write_group(f, field, eval('var_dict.%s' % field), _append) This is the only drawback (in my mind) of switching to the namedtuple. I need a clear way to programmatically inspect all the names and their corresponding tuple-value, and I don't know what it is. I tried defining __getitem__() to return this, and sneakily make it act like a dictionary (then I could say the following(roughly): for field in var_dict._fields: write_group(f, field, var_dict[field], append) ) I was foiled though, because __getitem__() is ALSO used by tuples, so redefining it broke the namedtuple use. Alternatively to this approach, I could take the same (also ugly, IMHO) approach I do in intersect(). https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... platform_tools/android/gyp_gen/vars_dict_lib.py:90: for i in range(len(var_dictA)): As in write_local_vars, I essentially want a way to say: for key in var_dictA.keys(): # Which should be var_dict_a... aList = list(var_dictA[key]) # a_list... # etc. As far as I can tell, I've lost this ability by changing to a namedtuple. That said, I think it's probably worth it given the improved readability of gypd_parser.py. Note that here I could have done the eval('var_dictA.%s' % key) trick I do in write_local_vars. Or I could do this there. Or I could write a new function (getField?) to hide that. What do you think?
Elliot, This is ready for your input at patch set 14. The patch sets in between show the progression, in case it helps to follow along. Each step is intended to be self contained so meaningful changes didn't get lost along the way. In patch set 13 I have a couple of questions; otherwise my comments are just responses to your earlier comments. Patch set 14 still isn't ready for submission; I need to update the tests (I wanted to get your feedback on my current approach before I write tests that may change depending on my approach) and probably change some names (I like your suggestion MakefileVars, which may be the final name of VarsDict). Thanks again for your input!
https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... File platform_tools/android/gyp_gen/makefile_writer.py (right): https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... platform_tools/android/gyp_gen/makefile_writer.py:58: write_group(f, field, eval('var_dict.%s' % field), _append) On 2014/01/31 22:16:24, scroggo wrote: > This is the only drawback (in my mind) of switching to the namedtuple. I need a > clear way to programmatically inspect all the names and their corresponding > tuple-value, and I don't know what it is. > > I tried defining __getitem__() to return this, and sneakily make it act like a > dictionary (then I could say the following(roughly): > > for field in var_dict._fields: > write_group(f, field, var_dict[field], append) > > ) > > I was foiled though, because __getitem__() is ALSO used by tuples, so redefining > it broke the namedtuple use. > > Alternatively to this approach, I could take the same (also ugly, IMHO) approach > I do in intersect(). Figured this out. Next patch will address this. https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... platform_tools/android/gyp_gen/vars_dict_lib.py:90: for i in range(len(var_dictA)): On 2014/01/31 22:16:24, scroggo wrote: > As in write_local_vars, I essentially want a way to say: > > for key in var_dictA.keys(): # Which should be var_dict_a... > aList = list(var_dictA[key]) # a_list... > # etc. > > As far as I can tell, I've lost this ability by changing to a namedtuple. That > said, I think it's probably worth it given the improved readability of > gypd_parser.py. > > Note that here I could have done the eval('var_dictA.%s' % key) trick I do in > write_local_vars. Or I could do this there. Or I could write a new function > (getField?) to hide that. What do you think? Just figured this out. I'll override __getitem__ to keep its current behavior if the parameter is an int, and to treat it like a dictionary lookup if the parameter is a string. Will put up a patch fixing this on Monday.
On 2014/02/01 16:24:01, scroggo wrote: > https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... > File platform_tools/android/gyp_gen/makefile_writer.py (right): > > https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... > platform_tools/android/gyp_gen/makefile_writer.py:58: write_group(f, field, > eval('var_dict.%s' % field), _append) > On 2014/01/31 22:16:24, scroggo wrote: > > This is the only drawback (in my mind) of switching to the namedtuple. I need > a > > clear way to programmatically inspect all the names and their corresponding > > tuple-value, and I don't know what it is. > > > > I tried defining __getitem__() to return this, and sneakily make it act like a > > dictionary (then I could say the following(roughly): > > > > for field in var_dict._fields: > > write_group(f, field, var_dict[field], append) > > > > ) > > > > I was foiled though, because __getitem__() is ALSO used by tuples, so > redefining > > it broke the namedtuple use. > > > > Alternatively to this approach, I could take the same (also ugly, IMHO) > approach > > I do in intersect(). > > Figured this out. Next patch will address this. > > https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... > File platform_tools/android/gyp_gen/vars_dict_lib.py (right): > > https://codereview.chromium.org/140503007/diff/820001/platform_tools/android/... > platform_tools/android/gyp_gen/vars_dict_lib.py:90: for i in > range(len(var_dictA)): > On 2014/01/31 22:16:24, scroggo wrote: > > As in write_local_vars, I essentially want a way to say: > > > > for key in var_dictA.keys(): # Which should be var_dict_a... > > aList = list(var_dictA[key]) # a_list... > > # etc. > > > > As far as I can tell, I've lost this ability by changing to a namedtuple. That > > said, I think it's probably worth it given the improved readability of > > gypd_parser.py. > > > > Note that here I could have done the eval('var_dictA.%s' % key) trick I do in > > write_local_vars. Or I could do this there. Or I could write a new function > > (getField?) to hide that. What do you think? > > Just figured this out. I'll override __getitem__ to keep its current behavior if > the parameter is an int, and to treat it like a dictionary lookup if the > parameter is a string. > > Will put up a patch fixing this on Monday. Patch set 16 is up and ready for review.
On 2014/01/31 22:22:54, scroggo wrote: > Elliot, > > This is ready for your input at patch set 14. Sorry for the delay. I will re-review today, for sure.
Python files look almost ready at patchset 16. (I did not look at the gyp files.) Thanks for all the cleanup! https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:70: # Call the script to generate gypd files, first using a non-existant If the following writeup is accurate, I think it might be helpful to paste into the code at this point: # Generate a separate VarsDict for each architecture type. For each archtype: # 1. call android_framework_gyp.main() to generate gypd files # 2. call parse_gypd to read those gypd files into the VarsDict # 3. delete the gypd files # # Once we have the VarsDict for each architecture type, we combine them all into # a single Android.mk file, which can build targets of any architecture type. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:72: android_framework_gyp.main(tmp_folder, main_gyp_file, 'other', False) If my writeup above is true, maybe you could reduce the copy-paste here as follows: default_var_dict = generate_var_dict('other', False) arm_var_dict = generate_var_dict('arm', False) arm_neon_var_dict = generate_var_dict('arm', True) ... and hide the per-architecture repetitive details within generate_var_dict() https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:117: arm_var_dict = vars_dict_lib.intersect([arm_var_dict, arm_neon_var_dict]) So, after this call, does arm_neon_var_dict somehow know that it depends on arm_var_dict? (i.e. that it contains just the variances from arm_var_dict, like how x86_var_dict and arm_var_dict contain just the variances from common) Edit: Looking at write_android_mk(), I see that this is an inherent distinction for the armNeon parameter. I'm OK with that, but it's somewhat confusing that all the VarsDicts passed into write_android_mk() are *almost* but not quite equivalent (arm, x86, and default are variations from common; armNeon is variations from arm). I think write_android_mk()'s interface would be more elegant if it took a variable-length list of VarsDicts, all "peers" at the same level of abstraction, each of which was paired with the 'ifeq' blocking information that should be used around it. On the other hand, I guess that would make it harder to see just the neon-vs-noneon distinctions within the Android.mk file All in all, I think the way you have it now is fine, but when we add more archtypes we may want to re-evaluate. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:119: makefile_writer.write_android_mk(target_dir, common, arm_var_dict, please use named parameters makefile_writer.write_android_mk(target_dir=target_dir, common=common, arm=arm_var_dict, ...) Given that many of the parameters are the same type but correspond to different platforms, I fear it would be too easy for them to get out of order and we wouldn't discover it for a while. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/gyp_gen/gypd_parser.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:17: @param var_dict VarsDict object for storing the results of the parsing. I think it's unusual for Python functions to take an "out" variable like this, instead of just returning a VarsDict. But it should be fine. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:25: if source.endswith('.h'): Do we need to worry about capitalization? (Is it possible that there might be header files ending with '.H', and similar for .gypi files?) https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:34: source = source.replace('../src', 'src', 1) Does this modify the dictionary in place? If so, I think you should warn callers in the function docstring. Or even better, stop doing so. :-) Edit: Looking at http://docs.python.org/2/library/string.html#string.replace , I guess not. replace() creates a copy of the original "source" string, and unless you call d.set(), I guess d remains unchanged regardless of your reassignation of "source" to point at a new string. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:78: # The input path will be relative to gyp/, but Android wants relative to I worry that some of these adjustments will prove to be fragile over time, but I guess we'll cross that bridge if/when we come to it. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:106: # named 'targets' And if it doesn't have a 'targets' entry, shouldn't you allow that to raise an Exception rather than returning quietly? If so, you could replace lines 107-110 with: for target in d['targets']: https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/vars_dict_lib.py:11: class OrderedSet(object): Cool beans. Seems like it may be generally useful... perhaps someday we will pull it out into its own module within a utility directory. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/vars_dict_lib.py:101: @param var_dict_list list of VarsDicts. WARNING: each VarsDict will be modified in place, to remove the common elements! https://codereview.chromium.org/140503007/diff/990001/tools/tests/gyp_to_andr... File tools/tests/gyp_to_android_tests.py (right): https://codereview.chromium.org/140503007/diff/990001/tools/tests/gyp_to_andr... tools/tests/gyp_to_android_tests.py:9: Test gyp_to_android.py Thanks for the unittests!
https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/bin/gyp_to_android.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:70: # Call the script to generate gypd files, first using a non-existant On 2014/02/03 17:26:32, epoger wrote: > If the following writeup is accurate, I think it might be helpful to paste into > the code at this point: > > # Generate a separate VarsDict for each architecture type. For each archtype: > # 1. call android_framework_gyp.main() to generate gypd files > # 2. call parse_gypd to read those gypd files into the VarsDict > # 3. delete the gypd files > # > # Once we have the VarsDict for each architecture type, we combine them all into > # a single Android.mk file, which can build targets of any architecture type. > Done. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:72: android_framework_gyp.main(tmp_folder, main_gyp_file, 'other', False) On 2014/02/03 17:26:32, epoger wrote: > If my writeup above is true, maybe you could reduce the copy-paste here as > follows: > > default_var_dict = generate_var_dict('other', False) > arm_var_dict = generate_var_dict('arm', False) > arm_neon_var_dict = generate_var_dict('arm', True) > > ... and hide the per-architecture repetitive details within generate_var_dict() Done. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:117: arm_var_dict = vars_dict_lib.intersect([arm_var_dict, arm_neon_var_dict]) On 2014/02/03 17:26:32, epoger wrote: > So, after this call, does arm_neon_var_dict somehow know that it depends on > arm_var_dict? (i.e. that it contains just the variances from arm_var_dict, like > how x86_var_dict and arm_var_dict contain just the variances from common) > > Edit: Looking at write_android_mk(), I see that this is an inherent distinction > for the armNeon parameter. I'm OK with that, but it's somewhat confusing that > all the VarsDicts passed into write_android_mk() are *almost* but not quite > equivalent (arm, x86, and default are variations from common; armNeon is > variations from arm). > > I think write_android_mk()'s interface would be more elegant if it took a > variable-length list of VarsDicts, all "peers" at the same level of abstraction, > each of which was paired with the 'ifeq' blocking information that should be > used around it. > > On the other hand, I guess that would make it harder to see just the > neon-vs-noneon distinctions within the Android.mk file > > All in all, I think the way you have it now is fine, but when we add more > archtypes we may want to re-evaluate. Leaving as is, and adding a TODO in makefile_writer. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/bin/gyp_to_android.py:119: makefile_writer.write_android_mk(target_dir, common, arm_var_dict, On 2014/02/03 17:26:32, epoger wrote: > please use named parameters > > makefile_writer.write_android_mk(target_dir=target_dir, common=common, > arm=arm_var_dict, ...) > > Given that many of the parameters are the same type but correspond to different > platforms, I fear it would be too easy for them to get out of order and we > wouldn't discover it for a while. Done. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/gyp_gen/gypd_parser.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:17: @param var_dict VarsDict object for storing the results of the parsing. On 2014/02/03 17:26:32, epoger wrote: > I think it's unusual for Python functions to take an "out" variable like this, > instead of just returning a VarsDict. But it should be fine. Hmm. The reason I do it this way is because this function recurses, and I want to keep adding entries to the same VarsDict, rather than creating a new one. Is there another way you'd recommend? https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:25: if source.endswith('.h'): On 2014/02/03 17:26:32, epoger wrote: > Do we need to worry about capitalization? (Is it possible that there might be > header files ending with '.H', and similar for .gypi files?) Well, someone could easily rewrite our gyp files to use capital letters. It's easy enough to compare against a lowercase version of source. Done. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:34: source = source.replace('../src', 'src', 1) On 2014/02/03 17:26:32, epoger wrote: > Does this modify the dictionary in place? If so, I think you should warn > callers in the function docstring. Or even better, stop doing so. :-) > > Edit: Looking at http://docs.python.org/2/library/string.html#string.replace , I > guess not. replace() creates a copy of the original "source" string, and unless > you call d.set(), I guess d remains unchanged regardless of your reassignation > of "source" to point at a new string. That's correct. And I do not call d.set :) https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:106: # named 'targets' On 2014/02/03 17:26:32, epoger wrote: > And if it doesn't have a 'targets' entry, shouldn't you allow that to raise an > Exception rather than returning quietly? > > If so, you could replace lines 107-110 with: > > for target in d['targets']: Done. https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/vars_dict_lib.py:101: @param var_dict_list list of VarsDicts. On 2014/02/03 17:26:32, epoger wrote: > WARNING: each VarsDict will be modified in place, to remove the common elements! Done. https://codereview.chromium.org/140503007/diff/990001/tools/tests/gyp_to_andr... File tools/tests/gyp_to_android_tests.py (right): https://codereview.chromium.org/140503007/diff/990001/tools/tests/gyp_to_andr... tools/tests/gyp_to_android_tests.py:9: Test gyp_to_android.py On 2014/02/03 17:26:32, epoger wrote: > Thanks for the unittests! Haha, sure thing! ... they're out of date though. I figured that I still might change names, move things etc, so these are still roughly in the state they were in in https://codereview.chromium.org/142173002 https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... platform_tools/android/gyp_gen/vars_dict_lib.py:69: __slots__ = () pylint has a couple of complaints on this file: 1) E1001: 63,0:VarsDict: Use of __slots__ on an old style class I don't know much about __slots__, but put it in here as the page you sent me to about namedtuple (http://docs.python.org/2/library/collections.html#collections.namedtuple) suggested I use it. Any idea what should be done to fix it? 2) W0232: 63,0:VarsDict: Class has no __init__ method It seems redundant to have both a __new__ and an __init__ method, but perhaps that's just my lack of Python knowledge talking?
LGTM! https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... File platform_tools/android/gyp_gen/gypd_parser.py (right): https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... platform_tools/android/gyp_gen/gypd_parser.py:17: @param var_dict VarsDict object for storing the results of the parsing. On 2014/02/03 23:22:31, scroggo wrote: > On 2014/02/03 17:26:32, epoger wrote: > > I think it's unusual for Python functions to take an "out" variable like this, > > instead of just returning a VarsDict. But it should be fine. > > Hmm. The reason I do it this way is because this function recurses, and I want > to keep adding entries to the same VarsDict, rather than creating a new one. Is > there another way you'd recommend? I guess the alternative would be to collect multiple VarsDicts and combine them. But I think it's fine to do it this way, just a little unusual in Python (in my huuuuge Python experience) https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... File platform_tools/android/gyp_gen/vars_dict_lib.py (right): https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... platform_tools/android/gyp_gen/vars_dict_lib.py:69: __slots__ = () On 2014/02/03 23:22:32, scroggo wrote: > pylint has a couple of complaints on this file: > > 1) E1001: 63,0:VarsDict: Use of __slots__ on an old style class > > I don't know much about __slots__, but put it in here as the page you sent me to > about namedtuple > (http://docs.python.org/2/library/collections.html#collections.namedtuple) > suggested I use it. Any idea what should be done to fix it? > > 2) W0232: 63,0:VarsDict: Class has no __init__ method > It seems redundant to have both a __new__ and an __init__ method, but perhaps > that's just my lack of Python knowledge talking? I'm sorry to report I don't know anything about either one. :-)
On 2014/02/04 15:00:12, epoger wrote: > LGTM! > > https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... > File platform_tools/android/gyp_gen/gypd_parser.py (right): > > https://codereview.chromium.org/140503007/diff/990001/platform_tools/android/... > platform_tools/android/gyp_gen/gypd_parser.py:17: @param var_dict VarsDict > object for storing the results of the parsing. > On 2014/02/03 23:22:31, scroggo wrote: > > On 2014/02/03 17:26:32, epoger wrote: > > > I think it's unusual for Python functions to take an "out" variable like > this, > > > instead of just returning a VarsDict. But it should be fine. > > > > Hmm. The reason I do it this way is because this function recurses, and I want > > to keep adding entries to the same VarsDict, rather than creating a new one. > Is > > there another way you'd recommend? > > I guess the alternative would be to collect multiple VarsDicts and combine them. > But I think it's fine to do it this way, just a little unusual in Python (in my > huuuuge Python experience) > > https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... > File platform_tools/android/gyp_gen/vars_dict_lib.py (right): > > https://codereview.chromium.org/140503007/diff/1090001/platform_tools/android... > platform_tools/android/gyp_gen/vars_dict_lib.py:69: __slots__ = () > On 2014/02/03 23:22:32, scroggo wrote: > > pylint has a couple of complaints on this file: > > > > 1) E1001: 63,0:VarsDict: Use of __slots__ on an old style class > > > > I don't know much about __slots__, but put it in here as the page you sent me > to > > about namedtuple > > (http://docs.python.org/2/library/collections.html#collections.namedtuple) > > suggested I use it. Any idea what should be done to fix it? > > > > 2) W0232: 63,0:VarsDict: Class has no __init__ method > > It seems redundant to have both a __new__ and an __init__ method, but perhaps > > that's just my lack of Python knowledge talking? > > I'm sorry to report I don't know anything about either one. :-) Patch set 18 is basically a rebase (removes the gyp changes, which were submitted separately). Patch set 19 moves the tests, and 20 updates them. Please take another look.
LGTM, with or without suggestions, at patchset 20 https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... File platform_tools/android/tests/helpers.py (right): https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... platform_tools/android/tests/helpers.py:9: Common code for tests. optional: seems like this module is really more a collection of variables than code. variables.py might be a better name. https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... File platform_tools/android/tests/run_all.py (right): https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... platform_tools/android/tests/run_all.py:18: gyp_to_android_tests.main() Alternatively, you can call unittest.TestLoader().discover() to automatically discover all tests within the directory. See https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py
https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... File platform_tools/android/tests/helpers.py (right): https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... platform_tools/android/tests/helpers.py:9: Common code for tests. On 2014/02/06 14:35:17, epoger wrote: > optional: seems like this module is really more a collection of variables than > code. variables.py might be a better name. Done. I think I expected to pull more code in here, but variables is a better name for what it is right now, like you say. I ended up naming it test_variables, in order to not conflict with my other variables.py https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... File platform_tools/android/tests/run_all.py (right): https://codereview.chromium.org/140503007/diff/1240001/platform_tools/android... platform_tools/android/tests/run_all.py:18: gyp_to_android_tests.main() On 2014/02/06 14:35:17, epoger wrote: > Alternatively, you can call unittest.TestLoader().discover() to automatically > discover all tests within the directory. > > See > https://github.com/google/skia/blob/master/gm/rebaseline_server/test_all.py Done.
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/scroggo@google.com/140503007/1330001
Message was sent while issue was closed.
Change committed as 13344 |
