Assert when source is an absoluth path

Assert when source is an absolute path

If the path passed to GypPathToUniqueOutput() is an absolute path,
the output path is absolute too, instead of one starting from obj
directory. Any intermediate file not residing in obj directory would
cause problem and this CL adds an assertion.

BUG=462153

[Yang Gu, 2015-02-27 01:26:10]

yang.gu@intel.com changed reviewers:
+ evan@chromium.org, fdegans@chromium.org, thakis@chromium.org

[Yang Gu, 2015-02-27 01:26:11]

This bug is introduced when moving ndk.gyp and cpufeatures.gypi from
third_party/android_tools (https://codereview.chromium.org/864323002). Please
review, thanks!

[Nico, 2015-02-27 01:57:59]

It'd be nice if there was a way to do this without requiring absolute path
support for sources.

(In any case, this needs a test if it's really needed:
https://code.google.com/p/gyp/wiki/GypHacking)

[Yang Gu, 2015-02-27 02:46:24]

On 2015/02/27 01:57:59, Nico wrote:
> It'd be nice if there was a way to do this without requiring absolute path
> support for sources.
> 
> (In any case, this needs a test if it's really needed:
> https://code.google.com/p/gyp/wiki/GypHacking)

Thanks for pointing this to me. I will follow the wiki to add test case. But
first, I need your insight to see if this is the best fix we can get. 
To me, we can't restrict the usage of absolute path in gyp files, so I still
think the better way to handle this is in ninja.py.
Then in ninja.py, WriteSourcesForArch() will go through each source path, and
invoke GypPathToUniqueOutput() to generate its output path. 
For this case, the source path is
'/xxx/src/third_party/android_tools/ndk/sources/android/cpufeatures/cpu-features.c'
(absolute path). And gyp file is at build/android/ndk.gyp, which is totally
unrelated to the source path. For simplicity, my fix just follows the path of
gyp and omits the absolute path of source totally. So the guideline here is
simple, if the source path is not something relative to gyp path, we just ignore
it.
Any better idea?

[Nico, 2015-02-27 03:57:47]

On 2015/02/27 02:46:24, Yang Gu wrote:
> On 2015/02/27 01:57:59, Nico wrote:
> > It'd be nice if there was a way to do this without requiring absolute path
> > support for sources.
> > 
> > (In any case, this needs a test if it's really needed:
> > https://code.google.com/p/gyp/wiki/GypHacking)
> 
> Thanks for pointing this to me. I will follow the wiki to add test case. But
> first, I need your insight to see if this is the best fix we can get. 
> To me, we can't restrict the usage of absolute path in gyp files,

Well, so far we've been able to. It'd be nice if we could continue to only
support relative paths for sources.

> so I still
> think the better way to handle this is in ninja.py.
> Then in ninja.py, WriteSourcesForArch() will go through each source path, and
> invoke GypPathToUniqueOutput() to generate its output path. 
> For this case, the source path is
>
'/xxx/src/third_party/android_tools/ndk/sources/android/cpufeatures/cpu-features.c'
> (absolute path). And gyp file is at build/android/ndk.gyp, which is totally
> unrelated to the source path. For simplicity, my fix just follows the path of
> gyp and omits the absolute path of source totally. So the guideline here is
> simple, if the source path is not something relative to gyp path, we just
ignore
> it.
> Any better idea?

[Yang Gu, 2015-02-27 04:56:19]

I updated the patch to use relative path for sources. Is this what you'd like to
see so far?
Note that if someone doesn't realize this implicit restriction and write
absolute path in gyp file in future, we may still hit the issue. May I add a
warning or even error (I think error is better) in ninja.py?

[Yang Gu, 2015-02-27 04:57:29]

By the way, if patch set 2 is better, I'll change commit message accordingly.

[Yang Gu, 2015-02-28 01:35:13]

> That's better, but it now repeats what's in build/common.gypi for
android_ndk_path.
> The comment in build/common.gypi says something about ant. We use ant a lot
less than we used to – what breaks if you just make android_ndk_path relative?

It's strange I got your email on comment, but I couldn't see them here. So I
just copied them above. 
I also noticed the comment in common.gypi, so I renamed the variable a bit
different to distinguish. The reason is I don't have idea how ant is used. I
just had some worry it may break some of your server scripts. If you think using
relative path for android_ndk_root wouldn't break anything, or we can fix
simply, I can revise the patch.

[Fabrice (no longer in Chrome), 2015-03-02 13:54:23]

On 2015/02/28 01:35:13, Yang Gu wrote:
> > That's better, but it now repeats what's in build/common.gypi for
> android_ndk_path.
> > The comment in build/common.gypi says something about ant. We use ant a lot
> less than we used to – what breaks if you just make android_ndk_path relative?
> 
> It's strange I got your email on comment, but I couldn't see them here. So I
> just copied them above. 
> I also noticed the comment in common.gypi, so I renamed the variable a bit
> different to distinguish. The reason is I don't have idea how ant is used. I
> just had some worry it may break some of your server scripts. If you think
using
> relative path for android_ndk_root wouldn't break anything, or we can fix
> simply, I can revise the patch.

First, apologies for breaking things.
Unfortunately, changing android_ndk_root to be relative will not work because
compilers and standard libraries are here and they need to be absolute path or
the build breaks. GN does not have the same problem because it can use a
"secondary root". Using a relative path in this specific case is our best
option. I am not sure if using a new variable is necessary though.

[Nico, 2015-03-02 21:10:59]

On 2015/03/02 13:54:23, Fabrice wrote:
> On 2015/02/28 01:35:13, Yang Gu wrote:
> > > That's better, but it now repeats what's in build/common.gypi for
> > android_ndk_path.
> > > The comment in build/common.gypi says something about ant. We use ant a
lot
> > less than we used to – what breaks if you just make android_ndk_path
relative?
> > 
> > It's strange I got your email on comment, but I couldn't see them here. So I
> > just copied them above. 
> > I also noticed the comment in common.gypi, so I renamed the variable a bit
> > different to distinguish. The reason is I don't have idea how ant is used. I
> > just had some worry it may break some of your server scripts. If you think
> using
> > relative path for android_ndk_root wouldn't break anything, or we can fix
> > simply, I can revise the patch.
> 
> First, apologies for breaking things.
> Unfortunately, changing android_ndk_root to be relative will not work because
> compilers and standard libraries are here and they need to be absolute path or
> the build breaks.

In what way?

> GN does not have the same problem because it can use a
> "secondary root". Using a relative path in this specific case is our best
> option. I am not sure if using a new variable is necessary though.

[Fabrice (no longer in Chrome), 2015-03-03 16:41:21]

On 2015/03/02 21:10:59, Nico wrote:
> On 2015/03/02 13:54:23, Fabrice wrote:
> > First, apologies for breaking things.
> > Unfortunately, changing android_ndk_root to be relative will not work
because
> > compilers and standard libraries are here and they need to be absolute path
or
> > the build breaks.
> 
> In what way?

I tried to dig a bit further, the issue lies with the compiler path.
Setting android_ndk_root to "<(DEPTH)/third_party/android_tools/etc" causes
issues with make_global_settings because it needs to be the same for all targets
and not all targets are at the same depth. For instance, some targets will set
CC to "../../third_party/android_tools/etc/" and others will set it to
"../third_party/android_tools/etc/".
Setting it to a relative path like "third_party/android_tools/etc" or
"../../third_party/android_tools/etc" causes issues because we need to call gcc
from common.gypi to get the libgcc file to link with, since the GYP files that
include common.gypi are not necessarily 2 levels in.

However, the standard libraries can (and should) use a relative path. I am
writing a patch to introduce a new variable (android_ndk_absolute_root) that
will only be used to reference the paths to gdbserver and gcc.

[Yang Gu, 2015-03-04 02:12:38]

@Fabrice, thanks for your patch, which is very nice for me.
I think this issue has been fixed. However, someone may still break the rule and
use absolute path for sources. May I add an assertion in ninja.py for this? I
uploaded patch set 3 for this (description was also revised).

[Nico, 2015-03-04 05:41:34]

lgtm

I suppose this is better than silently doing the wrong thing.

[Fabrice (no longer in Chrome), 2015-03-04 12:46:34]

lgtm, that sounds reasonable. Thanks!

Just a small nit in the issue description.
Could you fix the grammar in the description? "instead of someone started from
obj directory" I'm not sure that is correct.

[Yang Gu, 2015-03-05 06:42:43]

On 2015/03/04 12:46:34, Fabrice wrote:
> lgtm, that sounds reasonable. Thanks!
> 
> Just a small nit in the issue description.
> Could you fix the grammar in the description? "instead of someone started from
> obj directory" I'm not sure that is correct.

Thanks, and revise the description.

[Yang Gu, 2015-03-05 06:43:14]

The CQ bit was checked by yang.gu@intel.com

[commit-bot: I haz the power, 2015-03-05 06:46:10]

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

[commit-bot: I haz the power, 2015-03-05 06:46:11]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Yang Gu, 2015-03-05 14:34:00]

The CQ bit was checked by yang.gu@intel.com

[Yang Gu, 2015-03-05 14:34:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
fdegans@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/967443002/#ps60001 (title:
"Resubmit using gyp repo")

[commit-bot: I haz the power, 2015-03-05 14:38:34]

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

[commit-bot: I haz the power, 2015-03-05 14:38:36]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Yang Gu, 2015-03-05 14:38:48]

On 2015/03/05 06:46:11, I haz the power (commit-bot) wrote:
> Commit queue rejected this change because it did not recognize the base URL.
> Please commit your change manually.

The patch was sent from Chromium. Not sure if this is the reason of rejection. I
just resubmitted using gyp repo.
Maybe I need commit right for svn server.

[Yang Gu, 2015-03-05 14:39:13]

The CQ bit was checked by yang.gu@intel.com

[commit-bot: I haz the power, 2015-03-05 14:43:39]

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

[commit-bot: I haz the power, 2015-03-05 14:43:40]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Fabrice (no longer in Chrome), 2015-03-05 15:50:18]

On 2015/03/05 14:38:48, Yang Gu wrote:
> On 2015/03/05 06:46:11, I haz the power (commit-bot) wrote:
> > Commit queue rejected this change because it did not recognize the base URL.
> > Please commit your change manually.
> 
> The patch was sent from Chromium. Not sure if this is the reason of rejection.
I
> just resubmitted using gyp repo.
> Maybe I need commit right for svn server.

This repository has no CQ afaik. You need a gyp committer to land it for you.

[Yang Gu, 2015-03-06 00:43:52]

On 2015/03/05 15:50:18, Fabrice wrote:
> On 2015/03/05 14:38:48, Yang Gu wrote:
> > On 2015/03/05 06:46:11, I haz the power (commit-bot) wrote:
> > > Commit queue rejected this change because it did not recognize the base
URL.
> > > Please commit your change manually.
> > 
> > The patch was sent from Chromium. Not sure if this is the reason of
rejection.
> I
> > just resubmitted using gyp repo.
> > Maybe I need commit right for svn server.
> 
> This repository has no CQ afaik. You need a gyp committer to land it for you.

Got it.
@Nico, I see lots of commits from you. Are you a committer on gyp repo? Could
you help me land the patch?

[Yang Gu, 2015-03-12 01:31:26]

yang.gu@intel.com changed reviewers:
+ bradnelson@chromium.org

[Yang Gu, 2015-03-12 01:35:53]

bradnelson@, in patch set 4, I want to add an assertion in gyp repo to ensure
any intermediate file is generated in obj directory. I already got LGTM from
Nico and Fabrice. However, it seems I couldn't commit the patch as I'm not a
committer to gyp repo. Could you please review this and help me land the patch?
Thanks!