Issue 6601051: Make it possible to generate the ios xcode proj by specifying only skia_os="ios" in GYP_DEFINES. (Closed)
|
Created:
12 years, 1 month ago by bsalomon Modified:
12 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionMake it possible to generate the ios xcode proj by specifying only skia_os="ios" in GYP_DEFINES.
R=caryclark@google.com,epoger@google.com
Committed: https://code.google.com/p/skia/source/detail?r=5796
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
MessagesTotal messages: 11
lgtm
Sign in to reply to this message.
Adding Eric. Eric will this adversely affect Android? If the scripts that process our gyp files for android have already specified armv7 and arm_neon then I think we're OK since they are specified here with the % modifier.
Sign in to reply to this message.
This shouldn't break Android. We set these variables in our build scripts. We should probably move that logic into gyp though, since it's a bit confusing to have it in two places. https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi#newco... gyp/common_variables.gypi:50: }, Do we really need this fourth level?
Sign in to reply to this message.
https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi#newco... gyp/common_variables.gypi:50: }, On 2012/10/03 20:00:51, EricB wrote: > Do we really need this fourth level? Yeah, I want skia_arch_type to depend on skia_os. Both were previously defined at level 3. So I had to make skia_os be defined at level 4. Isn't gyp lovely?
Sign in to reply to this message.
https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi#newco... gyp/common_variables.gypi:50: }, On 2012/10/03 20:04:37, bsalomon wrote: > On 2012/10/03 20:00:51, EricB wrote: > > Do we really need this fourth level? > > Yeah, I want skia_arch_type to depend on skia_os. Both were previously defined > at level 3. So I had to make skia_os be defined at level 4. Isn't gyp lovely? But do we use it at level 3? It seems like it could be at level 2, right where skia_arch_width has its os-dependent switch.
Sign in to reply to this message.
https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/1/gyp/common_variables.gypi#newco... gyp/common_variables.gypi:50: }, On 2012/10/03 20:08:59, EricB wrote: > On 2012/10/03 20:04:37, bsalomon wrote: > > On 2012/10/03 20:00:51, EricB wrote: > > > Do we really need this fourth level? > > > > Yeah, I want skia_arch_type to depend on skia_os. Both were previously defined > > at level 3. So I had to make skia_os be defined at level 4. Isn't gyp lovely? > > But do we use it at level 3? It seems like it could be at level 2, right where > skia_arch_width has its os-dependent switch. Ah-ha. I think you're right. It appears that in the current file skia_arch_type is unnecessarily defined in the 3rd circle of variable hell and could have been defined at 2 instead or even 1 since no other var depends on it. I'll post a revised change.
Sign in to reply to this message.
The latest patch moves skia_arch_type to level 1. https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi#ne... gyp/common_variables.gypi:106: 'armv7%': 1, Because armv7 and arm_neon are now at level 1 and don't need to be rippled up the variables dicts it isn't required for them to defined in the else case.
Sign in to reply to this message.
https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi File gyp/common_variables.gypi (right): https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi#ne... gyp/common_variables.gypi:106: 'armv7%': 1, On 2012/10/03 20:28:28, bsalomon wrote: > Because armv7 and arm_neon are now at level 1 and don't need to be rippled up > the variables dicts it isn't required for them to defined in the else case. Sorry to nag, but are we sure that this compiles? It looks like skia_arch_type is used in level 2 dicts in common_conditions.gypi, although maybe I'm misunderstanding what the levels mean.
Sign in to reply to this message.
On 2012/10/03 20:33:22, EricB wrote: > https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi > File gyp/common_variables.gypi (right): > > https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi#ne... > gyp/common_variables.gypi:106: 'armv7%': 1, > On 2012/10/03 20:28:28, bsalomon wrote: > > Because armv7 and arm_neon are now at level 1 and don't need to be rippled up > > the variables dicts it isn't required for them to defined in the else case. > > Sorry to nag, but are we sure that this compiles? It looks like skia_arch_type > is used in level 2 dicts in common_conditions.gypi, although maybe I'm > misunderstanding what the levels mean. No prob... this stuff is confusing as all hell. I believe that it is OK because the usage in common_conditions is within a conditions dictionary not a variables dictionary. I was able to build Mac and iOS SampleApp with the change.
Sign in to reply to this message.
On 2012/10/03 20:56:08, bsalomon wrote: > On 2012/10/03 20:33:22, EricB wrote: > > https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi > > File gyp/common_variables.gypi (right): > > > > > https://codereview.appspot.com/6601051/diff/5003/gyp/common_variables.gypi#ne... > > gyp/common_variables.gypi:106: 'armv7%': 1, > > On 2012/10/03 20:28:28, bsalomon wrote: > > > Because armv7 and arm_neon are now at level 1 and don't need to be rippled > up > > > the variables dicts it isn't required for them to defined in the else case. > > > > Sorry to nag, but are we sure that this compiles? It looks like > skia_arch_type > > is used in level 2 dicts in common_conditions.gypi, although maybe I'm > > misunderstanding what the levels mean. > > No prob... this stuff is confusing as all hell. I believe that it is OK because > the usage in common_conditions is within a conditions dictionary not a variables > dictionary. I was able to build Mac and iOS SampleApp with the change. LGTM
Sign in to reply to this message.
|
