remoting: fix out-of-tree build

remoting: fix out-of-tree build

See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvOfX-NQAJ for context.

BUG=609422
Committed: https://crrev.com/ba1f0bf22c49b8e9963f90c929b440382bbb8561
Cr-Commit-Position: refs/heads/master@{#392017}

[Paweł Hajdan Jr., 2016-05-04 13:35:01]

Description was changed from

==========
remoting: fix out-of-tree build

BUG=596887
==========

to

==========
remoting: fix out-of-tree build

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvO...
for context.

BUG=596887
==========

[Paweł Hajdan Jr., 2016-05-04 14:21:00]

phajdan.jr@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org, garykac@chromium.org,
lambroslambrou@chromium.org

[brettw, 2016-05-04 16:41:39]

https://codereview.chromium.org/1947923002/diff/1/remoting/host/BUILD.gn
File remoting/host/BUILD.gn (right):

https://codereview.chromium.org/1947923002/diff/1/remoting/host/BUILD.gn#newc...
remoting/host/BUILD.gn:737: script =
rebase_path("//remoting/tools/build/remoting_localize.py")
I don't understand this change. "script" is a built-in variable that GN
interprets to be relative to the current directory. Why does making this be
system-absolute fix anything?

https://codereview.chromium.org/1947923002/diff/1/remoting/remoting_options.gni
File remoting/remoting_options.gni (right):

https://codereview.chromium.org/1947923002/diff/1/remoting/remoting_options.g...
remoting/remoting_options.gni:32: branding_path =
rebase_path("//remoting/branding_Chrome")
For this kind of thing, we should define the globals to be the correct thing in
every context, which to me is just:
  branding_path = "//remoting/branding_Chrome"
and then have users of that variable rebase it to what they need at the time.
Different callers can easily have different expectations of what they want it to
be relative to, and making assumptions that a certain global path is
system-absolute in random BUILD files will be fragile.

It looks like there are only a couple of places that use this variable.

[Dirk Pranke, 2016-05-05 02:19:08]

I defer to Brett and the others here; I didn't notice anything else in the
reviews.

[Paweł Hajdan Jr., 2016-05-05 09:56:49]

Description was changed from

==========
remoting: fix out-of-tree build

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvO...
for context.

BUG=596887
==========

to

==========
remoting: fix out-of-tree build

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvO...
for context.

BUG=609422
==========

[Paweł Hajdan Jr., 2016-05-05 13:22:56]

Thanks for the feedback! PTAL.

https://codereview.chromium.org/1947923002/diff/1/remoting/host/BUILD.gn
File remoting/host/BUILD.gn (right):

https://codereview.chromium.org/1947923002/diff/1/remoting/host/BUILD.gn#newc...
remoting/host/BUILD.gn:737: script =
rebase_path("//remoting/tools/build/remoting_localize.py")
On 2016/05/04 at 16:41:39, brettw wrote:
> I don't understand this change. "script" is a built-in variable that GN
interprets to be relative to the current directory. Why does making this be
system-absolute fix anything?

It wasn't needed. Removed these changes. I didn't realize the relative path is
preferred in this case.

https://codereview.chromium.org/1947923002/diff/1/remoting/remoting_options.gni
File remoting/remoting_options.gni (right):

https://codereview.chromium.org/1947923002/diff/1/remoting/remoting_options.g...
remoting/remoting_options.gni:32: branding_path =
rebase_path("//remoting/branding_Chrome")
On 2016/05/04 at 16:41:39, brettw wrote:
> For this kind of thing, we should define the globals to be the correct thing
in every context, which to me is just:
>   branding_path = "//remoting/branding_Chrome"
> and then have users of that variable rebase it to what they need at the time.
Different callers can easily have different expectations of what they want it to
be relative to, and making assumptions that a certain global path is
system-absolute in random BUILD files will be fragile.
> 
> It looks like there are only a couple of places that use this variable.

Done.

[Lambros, 2016-05-05 17:20:40]

lgtm

[Paweł Hajdan Jr., 2016-05-06 06:35:28]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2016-05-06 06:35:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1947923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1947923002/40001

[commit-bot: I haz the power, 2016-05-06 06:39:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-06 06:41:42]

Description was changed from

==========
remoting: fix out-of-tree build

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvO...
for context.

BUG=609422
==========

to

==========
remoting: fix out-of-tree build

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3NAXifb84dk/CXSvO...
for context.

BUG=609422

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

[commit-bot: I haz the power, 2016-05-06 06:41:43]

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