Use a qualified path to blink resources, blink part.

Use a qualified path to blink resources, blink part.

This is technically a prerequisite to https://codereview.chromium.org/430083002
(In practice, content/ probably already has an include path for
SHARED_INTERMEDIATE_DIR.)

The gn file didn't set the output directory correctly, so that part
is a hard prerequisite.

BUG=400860
TBR=eseidel

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179670

[tfarina, 2014-07-31 18:29:36]

https://codereview.chromium.org/431883002/diff/80001/public/BUILD.gn
File public/BUILD.gn (right):

https://codereview.chromium.org/431883002/diff/80001/public/BUILD.gn#newcode81
public/BUILD.gn:81: output_dir = "$root_gen_dir/blink/public/resources"
that is right, otherwise it will expand to
gen/third_party/WebKit/public/grit/blink_resources.h

[brettw, 2014-07-31 20:00:23]

This Chrome patch seems to fix it:

diff --git a/build/secondary/tools/grit/grit_rule.gni
b/build/secondary/tools/grit/grit_rule.gni
index 3abbdb8..64d542a 100644
--- a/build/secondary/tools/grit/grit_rule.gni
+++ b/build/secondary/tools/grit/grit_rule.gni
@@ -256,7 +256,7 @@ template("grit") {
     script = "//tools/grit/grit.py"
     inputs = grit_inputs
     outputs = grit_outputs
-    depfile = "$target_out_dir/${target_name}.d"
+    depfile = "$output_dir/${target_name}.d"
 
     args = [
       "-i", source_path, "build",

However, the location of the depfile _shouldn't_ affect its contents. I think
there must be a bug in the grit code that writes these files.

[Nico, 2014-07-31 23:35:04]

On Thu, Jul 31, 2014 at 1:00 PM, <brettw@chromium.org> wrote:

> This Chrome patch seems to fix it:
>
> diff --git a/build/secondary/tools/grit/grit_rule.gni
> b/build/secondary/tools/grit/grit_rule.gni
> index 3abbdb8..64d542a 100644
> --- a/build/secondary/tools/grit/grit_rule.gni
> +++ b/build/secondary/tools/grit/grit_rule.gni
> @@ -256,7 +256,7 @@ template("grit") {
>      script = "//tools/grit/grit.py"
>      inputs = grit_inputs
>      outputs = grit_outputs
> -    depfile = "$target_out_dir/${target_name}.d"
> +    depfile = "$output_dir/${target_name}.d"
>
>      args = [
>        "-i", source_path, "build",
>
> However, the location of the depfile _shouldn't_ affect its contents. I
> think
> there must be a bug in the grit code that writes these files.
>

So I guess I should just wait a week?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[brettw, 2014-08-06 20:58:43]

On Thu, Jul 31, 2014 at 4:35 PM, Nico Weber <thakis@chromium.org> wrote:
> On Thu, Jul 31, 2014 at 1:00 PM, <brettw@chromium.org> wrote:
>>
>> This Chrome patch seems to fix it:
>>
>> diff --git a/build/secondary/tools/grit/grit_rule.gni
>> b/build/secondary/tools/grit/grit_rule.gni
>> index 3abbdb8..64d542a 100644
>> --- a/build/secondary/tools/grit/grit_rule.gni
>> +++ b/build/secondary/tools/grit/grit_rule.gni
>> @@ -256,7 +256,7 @@ template("grit") {
>>      script = "//tools/grit/grit.py"
>>      inputs = grit_inputs
>>      outputs = grit_outputs
>> -    depfile = "$target_out_dir/${target_name}.d"
>> +    depfile = "$output_dir/${target_name}.d"
>>
>>      args = [
>>        "-i", source_path, "build",
>>
>> However, the location of the depfile _shouldn't_ affect its contents. I
>> think
>> there must be a bug in the grit code that writes these files.
>
> So I guess I should just wait a week?

Actually, I looked at this some more and this patch is the correct
solution. The issue is that you moved the output but the .d file
didn't change. Now Ninja sees a different target declaration but the
old .d file referencing the old location and gets confused. Your patch
works properly if you delete the old .d file. I'll send a patch for
this.

Brett

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nico, 2014-08-06 21:20:33]

On 2014/08/06 20:58:43, brettw wrote:
> On Thu, Jul 31, 2014 at 4:35 PM, Nico Weber <mailto:thakis@chromium.org>
wrote:
> > On Thu, Jul 31, 2014 at 1:00 PM, <mailto:brettw@chromium.org> wrote:
> >>
> >> This Chrome patch seems to fix it:
> >>
> >> diff --git a/build/secondary/tools/grit/grit_rule.gni
> >> b/build/secondary/tools/grit/grit_rule.gni
> >> index 3abbdb8..64d542a 100644
> >> --- a/build/secondary/tools/grit/grit_rule.gni
> >> +++ b/build/secondary/tools/grit/grit_rule.gni
> >> @@ -256,7 +256,7 @@ template("grit") {
> >>      script = "//tools/grit/grit.py"
> >>      inputs = grit_inputs
> >>      outputs = grit_outputs
> >> -    depfile = "$target_out_dir/${target_name}.d"
> >> +    depfile = "$output_dir/${target_name}.d"
> >>
> >>      args = [
> >>        "-i", source_path, "build",
> >>
> >> However, the location of the depfile _shouldn't_ affect its contents. I
> >> think
> >> there must be a bug in the grit code that writes these files.
> >
> > So I guess I should just wait a week?
> 
> Actually, I looked at this some more and this patch is the correct
> solution. The issue is that you moved the output but the .d file
> didn't change. Now Ninja sees a different target declaration but the
> old .d file referencing the old location and gets confused. Your patch
> works properly if you delete the old .d file. I'll send a patch for
> this.

Oh, is the .d file not next to the output? It probably should be? (Maybe that's
the fix you're planning to write?)

[brettw, 2014-08-06 21:36:06]

On 2014/08/06 21:20:33, Nico (very away) wrote:
> On 2014/08/06 20:58:43, brettw wrote:
> > On Thu, Jul 31, 2014 at 4:35 PM, Nico Weber <mailto:thakis@chromium.org>
> wrote:
> > > On Thu, Jul 31, 2014 at 1:00 PM, <mailto:brettw@chromium.org> wrote:
> > >>
> > >> This Chrome patch seems to fix it:
> > >>
> > >> diff --git a/build/secondary/tools/grit/grit_rule.gni
> > >> b/build/secondary/tools/grit/grit_rule.gni
> > >> index 3abbdb8..64d542a 100644
> > >> --- a/build/secondary/tools/grit/grit_rule.gni
> > >> +++ b/build/secondary/tools/grit/grit_rule.gni
> > >> @@ -256,7 +256,7 @@ template("grit") {
> > >>      script = "//tools/grit/grit.py"
> > >>      inputs = grit_inputs
> > >>      outputs = grit_outputs
> > >> -    depfile = "$target_out_dir/${target_name}.d"
> > >> +    depfile = "$output_dir/${target_name}.d"
> > >>
> > >>      args = [
> > >>        "-i", source_path, "build",
> > >>
> > >> However, the location of the depfile _shouldn't_ affect its contents. I
> > >> think
> > >> there must be a bug in the grit code that writes these files.
> > >
> > > So I guess I should just wait a week?
> > 
> > Actually, I looked at this some more and this patch is the correct
> > solution. The issue is that you moved the output but the .d file
> > didn't change. Now Ninja sees a different target declaration but the
> > old .d file referencing the old location and gets confused. Your patch
> > works properly if you delete the old .d file. I'll send a patch for
> > this.
> 
> Oh, is the .d file not next to the output? It probably should be? (Maybe
that's
> the fix you're planning to write?)

Correct, this patch should fix it: https://codereview.chromium.org/450483004/

[eseidel, 2014-08-06 23:21:19]

Nico is very-away.  Landing this will hopefully fix the marauding bot failures
from blink_resources.grd changes.  So I've queued a try-job for gn and plan to
tick the CQ box shortly.

[eseidel, 2014-08-06 23:27:21]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-08-06 23:28:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thakis@chromium.org/431883002/80001

[commit-bot: I haz the power, 2014-08-06 23:28:30]

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

[commit-bot: I haz the power, 2014-08-06 23:28:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Nico, 2014-08-07 00:43:02]

eseidel: You need to say "LGTM" before this can land :-)

[Nico, 2014-08-07 00:46:06]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:46:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thakis@chromium.org/431883002/80001

[eseidel, 2014-08-07 01:25:44]

lgtm

[commit-bot: I haz the power, 2014-08-07 01:28:57]

Change committed as 179670