Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(619)

Issue 6601051: Make it possible to generate the ios xcode proj by specifying only skia_os="ios" in GYP_DEFINES. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 8 months ago by bsalomon
Modified:
11 years, 8 months ago
Reviewers:
epoger, EricB, caryclark1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Make 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -3 lines) Patch
M gyp/common_variables.gypi View 1 3 chunks +9 lines, -3 lines 2 comments Download

Messages

Total messages: 11
bsalomon
11 years, 8 months ago (2012-10-03 19:41:44 UTC) #1
caryclark1
lgtm
11 years, 8 months ago (2012-10-03 19:52:24 UTC) #2
bsalomon
Adding Eric. Eric will this adversely affect Android? If the scripts that process our gyp ...
11 years, 8 months ago (2012-10-03 19:54:55 UTC) #3
EricB
This shouldn't break Android. We set these variables in our build scripts. We should probably ...
11 years, 8 months ago (2012-10-03 20:00:51 UTC) #4
bsalomon
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#newcode50 gyp/common_variables.gypi:50: }, On 2012/10/03 20:00:51, EricB wrote: > Do we ...
11 years, 8 months ago (2012-10-03 20:04:37 UTC) #5
EricB
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#newcode50 gyp/common_variables.gypi:50: }, On 2012/10/03 20:04:37, bsalomon wrote: > On 2012/10/03 ...
11 years, 8 months ago (2012-10-03 20:08:59 UTC) #6
bsalomon
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#newcode50 gyp/common_variables.gypi:50: }, On 2012/10/03 20:08:59, EricB wrote: > On 2012/10/03 ...
11 years, 8 months ago (2012-10-03 20:16:01 UTC) #7
bsalomon
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#newcode106 gyp/common_variables.gypi:106: 'armv7%': ...
11 years, 8 months ago (2012-10-03 20:28:27 UTC) #8
EricB
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#newcode106 gyp/common_variables.gypi:106: 'armv7%': 1, On 2012/10/03 20:28:28, bsalomon wrote: > Because ...
11 years, 8 months ago (2012-10-03 20:33:22 UTC) #9
bsalomon
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#newcode106 > ...
11 years, 8 months ago (2012-10-03 20:56:08 UTC) #10
EricB
11 years, 8 months ago (2012-10-03 20:57:18 UTC) #11
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b