Using relative path for GN for iOS.

Using relative path for GN for iOS.

BUG=653594
R=tkchin@webrtc.org

Committed: https://crrev.com/c6ca544295c9012d86d6957edac3283f58fcc99d
Cr-Commit-Position: refs/heads/master@{#14605}

[nicholss, 2016-10-07 20:06:42]

PTAL, Thanks!

[tkchin_webrtc, 2016-10-07 20:07:59]

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
why is this needed?

[nicholss, 2016-10-07 20:42:29]

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
On 2016/10/07 20:07:58, tkchin_webrtc wrote:
> why is this needed? 

In GN, if is_ios us true, then is_mac is also true. So what this build file is
doing is setting the target called "rtc_sdk_framework_objc" under this is_mac
block and then overwriting it again in the is_ios block. 

I think it is good practice to be explicate about is_mac and is_ios so you do
not have unintentional side effects down the road. 

An alternative choice could be that we leave it in is_mac, delete the is_ios
target block and then make the minor modifications needed for ios inside the
target with additional is_ios checks, combining the two targets into one.

[tkchin_webrtc, 2016-10-07 20:48:42]

lgtm

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
File webrtc/sdk/BUILD.gn (right):

https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
On 2016/10/07 20:42:29, nicholss wrote:
> On 2016/10/07 20:07:58, tkchin_webrtc wrote:
> > why is this needed? 
> 
> In GN, if is_ios us true, then is_mac is also true. So what this build file is
> doing is setting the target called "rtc_sdk_framework_objc" under this is_mac
> block and then overwriting it again in the is_ios block. 
> 
> I think it is good practice to be explicate about is_mac and is_ios so you do
> not have unintentional side effects down the road. 
> 
> An alternative choice could be that we leave it in is_mac, delete the is_ios
> target block and then make the minor modifications needed for ios inside the
> target with additional is_ios checks, combining the two targets into one.

Is there a history behind that? It's odd that when target_os=ios mac is enabled
as well.

[nicholss, 2016-10-07 20:52:42]

On 2016/10/07 20:48:42, tkchin_webrtc wrote:
> lgtm
> 
> https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
> File webrtc/sdk/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
> webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
> On 2016/10/07 20:42:29, nicholss wrote:
> > On 2016/10/07 20:07:58, tkchin_webrtc wrote:
> > > why is this needed? 
> > 
> > In GN, if is_ios us true, then is_mac is also true. So what this build file
is
> > doing is setting the target called "rtc_sdk_framework_objc" under this
is_mac
> > block and then overwriting it again in the is_ios block. 
> > 
> > I think it is good practice to be explicate about is_mac and is_ios so you
do
> > not have unintentional side effects down the road. 
> > 
> > An alternative choice could be that we leave it in is_mac, delete the is_ios
> > target block and then make the minor modifications needed for ios inside the
> > target with additional is_ios checks, combining the two targets into one.
> 
> Is there a history behind that? It's odd that when target_os=ios mac is
enabled
> as well.

As I understand it, is_mac is true because you are indeed on a mac and building
for an apple target. mac is a superset of ios to gyp and gn. It leads to all
sorts of 
funny bugs on the ios side because of all the special cases you need. For
example
when including libraries not available to iOS. 

For simple applications there is no difference between the GN setup for mac vs
ios.

[tkchin_webrtc, 2016-10-07 21:45:03]

On 2016/10/07 20:52:42, nicholss wrote:
> On 2016/10/07 20:48:42, tkchin_webrtc wrote:
> > lgtm
> > 
> > https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
> > File webrtc/sdk/BUILD.gn (right):
> > 
> >
https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
> > webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
> > On 2016/10/07 20:42:29, nicholss wrote:
> > > On 2016/10/07 20:07:58, tkchin_webrtc wrote:
> > > > why is this needed? 
> > > 
> > > In GN, if is_ios us true, then is_mac is also true. So what this build
file
> is
> > > doing is setting the target called "rtc_sdk_framework_objc" under this
> is_mac
> > > block and then overwriting it again in the is_ios block. 
> > > 
> > > I think it is good practice to be explicate about is_mac and is_ios so you
> do
> > > not have unintentional side effects down the road. 
> > > 
> > > An alternative choice could be that we leave it in is_mac, delete the
is_ios
> > > target block and then make the minor modifications needed for ios inside
the
> > > target with additional is_ios checks, combining the two targets into one.
> > 
> > Is there a history behind that? It's odd that when target_os=ios mac is
> enabled
> > as well.
> 
> As I understand it, is_mac is true because you are indeed on a mac and
building
> for an apple target. mac is a superset of ios to gyp and gn. It leads to all
> sorts of 
> funny bugs on the ios side because of all the special cases you need. For
> example
> when including libraries not available to iOS. 
> 
> For simple applications there is no difference between the GN setup for mac vs
> ios.

Would be nice to address that in GN esp. if it leads to bugs on iOS. Who can we
follow up with on that?

[nicholss, 2016-10-11 17:29:27]

On 2016/10/07 21:45:03, tkchin_webrtc wrote:
> On 2016/10/07 20:52:42, nicholss wrote:
> > On 2016/10/07 20:48:42, tkchin_webrtc wrote:
> > > lgtm
> > > 
> > > https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn
> > > File webrtc/sdk/BUILD.gn (right):
> > > 
> > >
> https://codereview.webrtc.org/2393133007/diff/1/webrtc/sdk/BUILD.gn#newcode256
> > > webrtc/sdk/BUILD.gn:256: if (is_mac && !is_ios) {
> > > On 2016/10/07 20:42:29, nicholss wrote:
> > > > On 2016/10/07 20:07:58, tkchin_webrtc wrote:
> > > > > why is this needed? 
> > > > 
> > > > In GN, if is_ios us true, then is_mac is also true. So what this build
> file
> > is
> > > > doing is setting the target called "rtc_sdk_framework_objc" under this
> > is_mac
> > > > block and then overwriting it again in the is_ios block. 
> > > > 
> > > > I think it is good practice to be explicate about is_mac and is_ios so
you
> > do
> > > > not have unintentional side effects down the road. 
> > > > 
> > > > An alternative choice could be that we leave it in is_mac, delete the
> is_ios
> > > > target block and then make the minor modifications needed for ios inside
> the
> > > > target with additional is_ios checks, combining the two targets into
one.
> > > 
> > > Is there a history behind that? It's odd that when target_os=ios mac is
> > enabled
> > > as well.
> > 
> > As I understand it, is_mac is true because you are indeed on a mac and
> building
> > for an apple target. mac is a superset of ios to gyp and gn. It leads to all
> > sorts of 
> > funny bugs on the ios side because of all the special cases you need. For
> > example
> > when including libraries not available to iOS. 
> > 
> > For simple applications there is no difference between the GN setup for mac
vs
> > ios.
> 
> Would be nice to address that in GN esp. if it leads to bugs on iOS. Who can
we
> follow up with on that?

Started a thread with you on it in gn-dev, committing this CL.

[nicholss, 2016-10-11 17:29:38]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2016-10-11 17:29:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nicholss, 2016-10-11 17:39:56]

The CQ bit was unchecked by nicholss@chromium.org

[nicholss, 2016-10-11 18:03:49]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2016-10-11 18:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2393133007/#ps20001 (title:
"Remove the && \!is_ios check that was added, that is not needed.")

[commit-bot: I haz the power, 2016-10-11 18:04:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 18:21:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 18:21:50]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9125)

[nicholss, 2016-10-11 18:24:54]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2016-10-11 18:25:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 18:39:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 18:39:58]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9127)

[nicholss, 2016-10-11 19:43:45]

On 2016/10/11 18:39:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9127)

The error this is running into is:

======================================================================
ERROR: testParseDepsDict (__main__.TestRollChromiumRevision)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/roll_chromium_revision_test.py",
line 59, in testParseDepsDict
    local_scope = ParseDepsDict(deps_contents)
  File
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/../roll_chromium_revision.py",
line 59, in ParseDepsDict
    'File': GClientKeywords.FileImpl,
AttributeError: type object 'GClientKeywords' has no attribute 'FileImpl'


I was hoping it was a flake but it seems to keep failing in this way. Anyone
know what this error is?

[tkchin_webrtc, 2016-10-11 19:57:16]

Description was changed from

==========
Using relitive path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

to

==========
Using relitive path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

[tkchin_webrtc, 2016-10-11 19:57:16]

tkchin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[tkchin_webrtc, 2016-10-11 19:57:28]

On 2016/10/11 19:43:45, nicholss wrote:
> On 2016/10/11 18:39:58, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   presubmit on master.tryserver.webrtc (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9127)
> 
> The error this is running into is:
> 
> ======================================================================
> ERROR: testParseDepsDict (__main__.TestRollChromiumRevision)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
>
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/roll_chromium_revision_test.py",
> line 59, in testParseDepsDict
>     local_scope = ParseDepsDict(deps_contents)
>   File
>
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/../roll_chromium_revision.py",
> line 59, in ParseDepsDict
>     'File': GClientKeywords.FileImpl,
> AttributeError: type object 'GClientKeywords' has no attribute 'FileImpl'
> 
> 
> I was hoping it was a flake but it seems to keep failing in this way. Anyone
> know what this error is?

@kjellander may know

[kjellander_webrtc, 2016-10-11 22:58:08]

On 2016/10/11 19:57:28, tkchin_webrtc wrote:
> On 2016/10/11 19:43:45, nicholss wrote:
> > On 2016/10/11 18:39:58, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   presubmit on master.tryserver.webrtc (JOB_FAILED,
> > >
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9127)
> > 
> > The error this is running into is:
> > 
> > ======================================================================
> > ERROR: testParseDepsDict (__main__.TestRollChromiumRevision)
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> >   File
> >
>
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/roll_chromium_revision_test.py",
> > line 59, in testParseDepsDict
> >     local_scope = ParseDepsDict(deps_contents)
> >   File
> >
>
"/b/build/slave/linux64/build/src/tools/autoroller/unittests/../roll_chromium_revision.py",
> > line 59, in ParseDepsDict
> >     'File': GClientKeywords.FileImpl,
> > AttributeError: type object 'GClientKeywords' has no attribute 'FileImpl'
> > 
> > 
> > I was hoping it was a flake but it seems to keep failing in this way. Anyone
> > know what this error is?
> 
> @kjellander may know

Yeah, it was a bad CL in depot_tools that caused this. It is now reverted
(https://chromium.googlesource.com/chromium/tools/depot_tools/+/398a46ee099e74...)

[kjellander_webrtc, 2016-10-11 23:02:34]

Description was changed from

==========
Using relitive path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

to

==========
Using relative path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

[kjellander_webrtc, 2016-10-11 23:02:47]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2016-10-11 23:02:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 23:12:46]

Description was changed from

==========
Using relative path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

to

==========
Using relative path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

[commit-bot: I haz the power, 2016-10-11 23:12:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-11 23:12:51]

Description was changed from

==========
Using relative path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org
==========

to

==========
Using relative path for GN for iOS.

BUG=653594

R=tkchin@webrtc.org

Committed: https://crrev.com/c6ca544295c9012d86d6957edac3283f58fcc99d
Cr-Commit-Position: refs/heads/master@{#14605}
==========

[commit-bot: I haz the power, 2016-10-11 23:12:52]

Patchset 2 (id:??) landed as
https://crrev.com/c6ca544295c9012d86d6957edac3283f58fcc99d
Cr-Commit-Position: refs/heads/master@{#14605}