Copy Linux me2me script to output dir for GN builds

[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.

BUG=602690
Committed: https://crrev.com/2dfafe066c1f16fde58ac93a63c8e6218101e3b2
Cr-Commit-Position: refs/heads/master@{#388376}

[Lambros, 2016-04-12 18:20:40]

Description was changed from

==========
Copy Linux me2me script to output dir for GN builds

BUG=602690
==========

to

==========
[remoting host] Copy Linux Me2Me script for GN builds

This modifies the remoting_me2me_host target to copy the
linux_me2me_host.py script to the GN output directory, so that it can be run
from that directory, and will work for arbitrary choices of directory name.

BUG=602690
==========

[Lambros, 2016-04-12 18:20:55]

lambroslambrou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Lambros, 2016-04-12 18:21:27]

Description was changed from

==========
[remoting host] Copy Linux Me2Me script for GN builds

This modifies the remoting_me2me_host target to copy the
linux_me2me_host.py script to the GN output directory, so that it can be run
from that directory, and will work for arbitrary choices of directory name.

BUG=602690
==========

to

==========
[remoting host] Copy Linux Me2Me script for GN builds

This modifies the remoting_me2me_host target to copy the
linux_me2me_host.py script to the GN output directory, so
that it can be run from that directory, and will work for
arbitrary choices of directory name.

BUG=602690
==========

[Sergey Ulanov, 2016-04-12 23:53:20]

https://codereview.chromium.org/1885483003/diff/1/remoting/host/linux/linux_m...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/1/remoting/host/linux/linux_m...
remoting/host/linux/linux_me2me_host.py:639: paths_to_try = map(lambda p:
os.path.join(SCRIPT_DIR, p),
If we expect this script to be in the same directory with the host binary, then
this code can be cleaned up

[Lambros, 2016-04-14 18:12:58]

ptal

https://codereview.chromium.org/1885483003/diff/1/remoting/host/linux/linux_m...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/1/remoting/host/linux/linux_m...
remoting/host/linux/linux_me2me_host.py:639: paths_to_try = map(lambda p:
os.path.join(SCRIPT_DIR, p),
On 2016/04/12 23:53:20, Sergey Ulanov wrote:
> If we expect this script to be in the same directory with the host binary,
then
> this code can be cleaned up

Done. Since this removes out/{Debug,Release}, I added a GYP 'copies' rule for
convenience.

[Sergey Ulanov, 2016-04-14 23:49:06]

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

https://codereview.chromium.org/1885483003/diff/20001/remoting/host/BUILD.gn#...
remoting/host/BUILD.gn:858: "$root_build_dir/run_me2me_host",
It's confusing to use 3 different name for the same script depending on where
it's placed.

Also for people who don't work on remoting it will be confusing to see this file
in the shared output directory, as they have not idea what is "me2me_host". We
should either put our stuff under out/<..>/remoting or have remoting_ prefix for
all files. In this case I think we want to put it under a separate directory (as
we don't to rename the file). But then the host binary has to be moved as well.
How about we just use the same name that we use in
/opt/google/chrome-remote-desktop/, i.e.:

out/<..>/remoting/chrome-remote-desktop (the python script)
out/<..>/remoting/chrome-remote-desktop-host (the host binary)

That way we won't need anything special in the python script to handle the dev
mode. WDYT?

[Lambros, 2016-04-16 01:54:54]

On 2016/04/14 23:49:06, Sergey Ulanov wrote:
> https://codereview.chromium.org/1885483003/diff/20001/remoting/host/BUILD.gn
> File remoting/host/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1885483003/diff/20001/remoting/host/BUILD.gn#...
> remoting/host/BUILD.gn:858: "$root_build_dir/run_me2me_host",
> It's confusing to use 3 different name for the same script depending on where
> it's placed.
> 
> Also for people who don't work on remoting it will be confusing to see this
file
> in the shared output directory, as they have not idea what is "me2me_host". We
> should either put our stuff under out/<..>/remoting or have remoting_ prefix
for
> all files. In this case I think we want to put it under a separate directory
(as
> we don't to rename the file). But then the host binary has to be moved as
well.
> How about we just use the same name that we use in
> /opt/google/chrome-remote-desktop/, i.e.:
> 
> out/<..>/remoting/chrome-remote-desktop (the python script)
> out/<..>/remoting/chrome-remote-desktop-host (the host binary)
> 
> That way we won't need anything special in the python script to handle the dev
> mode. WDYT?

I like the idea in principle. Unfortunately, GN won't let you produce the
"remoting_me2me_host" executable target in a remoting/ subdirectory. I think it
will let you change the filename, but not the directory. And then there will
no longer be a "remoting_me2me_host" binary in the expected place, so we'd
need to update things like the Debian packager, unittests and so on.

Alternatively, we could try to copy the script and the host binary into
remoting/... But (for the binary at least) we have to define a separate
target to do the copy, and make it depend on remoting_me2me_host.
Or re-jiggle things so that remoting_me2me_host does the copying, and
depends on another target to actually build the binary first.
Or we define an "action" in the remoting_me2me_host target, which then needs
to run a script to do the copying.

WDYT?

[Sergey Ulanov, 2016-04-18 20:44:08]

On 2016/04/16 01:54:54, Lambros wrote:
> On 2016/04/14 23:49:06, Sergey Ulanov wrote:
> > https://codereview.chromium.org/1885483003/diff/20001/remoting/host/BUILD.gn
> > File remoting/host/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1885483003/diff/20001/remoting/host/BUILD.gn#...
> > remoting/host/BUILD.gn:858: "$root_build_dir/run_me2me_host",
> > It's confusing to use 3 different name for the same script depending on
where
> > it's placed.
> > 
> > Also for people who don't work on remoting it will be confusing to see this
> file
> > in the shared output directory, as they have not idea what is "me2me_host".
We
> > should either put our stuff under out/<..>/remoting or have remoting_ prefix
> for
> > all files. In this case I think we want to put it under a separate directory
> (as
> > we don't to rename the file). But then the host binary has to be moved as
> well.
> > How about we just use the same name that we use in
> > /opt/google/chrome-remote-desktop/, i.e.:
> > 
> > out/<..>/remoting/chrome-remote-desktop (the python script)
> > out/<..>/remoting/chrome-remote-desktop-host (the host binary)
> > 
> > That way we won't need anything special in the python script to handle the
dev
> > mode. WDYT?
> 
> I like the idea in principle. Unfortunately, GN won't let you produce the
> "remoting_me2me_host" executable target in a remoting/ subdirectory. I think
it
> will let you change the filename, but not the directory. And then there will
> no longer be a "remoting_me2me_host" binary in the expected place, so we'd
> need to update things like the Debian packager, unittests and so on.

Yes, we just need to copy remoting_me2me_host binary. I don't think we want to
change the default output location.

> 
> Alternatively, we could try to copy the script and the host binary into
> remoting/... But (for the binary at least) we have to define a separate
> target to do the copy, and make it depend on remoting_me2me_host.
> Or re-jiggle things so that remoting_me2me_host does the copying, and
> depends on another target to actually build the binary first.
> Or we define an "action" in the remoting_me2me_host target, which then needs
> to run a script to do the copying.

I think it's better to copy it outside of remoting_me2me_host target in a target
that depends on remoting_me2me_host


> 
> WDYT?

[Sergey Ulanov, 2016-04-18 20:50:32]

One issue here is that we copy or move the binary it will not work for component
builds, so maybe it's not the right solution here. How about just creating
out/<..>/remoting/chrome-remote-desktop-host as a shell script that runs
out/<..>/remoting_me2me_host?

[Lambros, 2016-04-18 22:29:20]

Description was changed from

==========
[remoting host] Copy Linux Me2Me script for GN builds

This modifies the remoting_me2me_host target to copy the
linux_me2me_host.py script to the GN output directory, so
that it can be run from that directory, and will work for
arbitrary choices of directory name.

BUG=602690
==========

to

==========
[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.


BUG=602690
==========

[Lambros, 2016-04-18 22:32:33]

ptal at patch #3

This undoes the GYP changes (I don't think we care about GYP
at this point?)

It creates a new GN target (remoting_dev_me2me_host) to copy
the script and a host wrapper script. I've updated the CL
description to reflect these changes.

[Sergey Ulanov, 2016-04-18 22:48:01]

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:104: if not os.path.exists(xvfb):
merge this check with the one in locate_executable()? Then you wouldn't need
try-except in this function.

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:627: def locate_executable(exe_name):
I don't think we need this function anymore. Particularly it makes little sense
to check that path exists before running it. It's better to just try running the
binary and handle the error if exec() or system() fails.

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/rem...
File remoting/host/linux/remoting_me2me_host_wrapper.sh (right):

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/rem...
remoting/host/linux/remoting_me2me_host_wrapper.sh:16: # cwd cannot be used
since it is set to $HOME when chrome-remote-desktop
This is not the only reason to avoid CWD. Scripts should never rely on CWD to
locate relative paths to other files. Running ./dir/foo.sh should have the same
effect as ./foo.sh . Just remove this comment?

[Lambros, 2016-04-18 23:45:53]

ptal

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:104: if not os.path.exists(xvfb):
On 2016/04/18 22:48:01, Sergey Ulanov wrote:
> merge this check with the one in locate_executable()? Then you wouldn't need
> try-except in this function.

Re-written this.

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:627: def locate_executable(exe_name):
On 2016/04/18 22:48:01, Sergey Ulanov wrote:
> I don't think we need this function anymore. Particularly it makes little
sense
> to check that path exists before running it. It's better to just try running
the
> binary and handle the error if exec() or system() fails.

Removed. I slightly tweaked the logging in one of the places where it was
called, so that we see the path logged before subprocess.Popen throws.

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/rem...
File remoting/host/linux/remoting_me2me_host_wrapper.sh (right):

https://codereview.chromium.org/1885483003/diff/40001/remoting/host/linux/rem...
remoting/host/linux/remoting_me2me_host_wrapper.sh:16: # cwd cannot be used
since it is set to $HOME when chrome-remote-desktop
On 2016/04/18 22:48:01, Sergey Ulanov wrote:
> This is not the only reason to avoid CWD. Scripts should never rely on CWD to
> locate relative paths to other files. Running ./dir/foo.sh should have the
same
> effect as ./foo.sh . Just remove this comment?

Yep, removed.

[Sergey Ulanov, 2016-04-18 23:58:37]

lgtm after my comments are addressed.

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:110: 
add return None here. get() function without return at the end looks strange.

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:1170: if options.host_version:
I don't think we need this flag anymore (it was useful with NPAPI plugin, but
not with native messaging). Maybe just remove it?

BTW, it's also broken in current builds 

$ /opt/google/chrome-remote-desktop/chrome-remote-desktop-host --version
VERSION
$ /opt/google/chrome-remote-desktop/chrome-remote-desktop --host-version
VERSION

:-(

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:1172: host_path =
os.path.join(SCRIPT_DIR, HOST_BINARY_NAME)
If we still need the flag, then this expression is duplicated in two places. Add
HOST_BINARY_PATH after HOST_BINARY_NAME?

[Lambros, 2016-04-19 01:51:29]

Latest patch adds a missing dependency, so the new target triggers re-building
the host binary if needed.

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:110: 
On 2016/04/18 23:58:36, Sergey Ulanov wrote:
> add return None here. get() function without return at the end looks strange.

Done.

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:1170: if options.host_version:
On 2016/04/18 23:58:36, Sergey Ulanov wrote:
> I don't think we need this flag anymore (it was useful with NPAPI plugin, but
> not with native messaging). Maybe just remove it?
> 
> BTW, it's also broken in current builds 
> 
> $ /opt/google/chrome-remote-desktop/chrome-remote-desktop-host --version
> VERSION
> $ /opt/google/chrome-remote-desktop/chrome-remote-desktop --host-version
> VERSION
> 
> :-(

Flag removed in https://codereview.chromium.org/1899093002/

https://codereview.chromium.org/1885483003/diff/60001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:1172: host_path =
os.path.join(SCRIPT_DIR, HOST_BINARY_NAME)
On 2016/04/18 23:58:36, Sergey Ulanov wrote:
> If we still need the flag, then this expression is duplicated in two places.
Add
> HOST_BINARY_PATH after HOST_BINARY_NAME?

Acknowledged.

[Lambros, 2016-04-19 22:59:42]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2016-04-19 22:59:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1885483003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-19 23:00:15]

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

[commit-bot: I haz the power, 2016-04-20 00:27:30]

Description was changed from

==========
[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.


BUG=602690
==========

to

==========
[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.


BUG=602690
==========

[commit-bot: I haz the power, 2016-04-20 00:27:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:18:51]

Description was changed from

==========
[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.


BUG=602690
==========

to

==========
[remoting host] Copy Linux Me2Me script for GN builds

On Linux, this adds a new build target which copies the
linux_me2me_host.py script to a remoting/ subdirectory
of the GN output directory. It also creates a wrapper
script in the same place to run the remoting_me2me_host
binary.

This enables a developer to do:
ninja -C out/foo remoting_dev_me2me_host
and then:
out/foo/remoting/chrome-remote-desktop-host --start
to run the newly-built host.

BUG=602690

Committed: https://crrev.com/2dfafe066c1f16fde58ac93a63c8e6218101e3b2
Cr-Commit-Position: refs/heads/master@{#388376}
==========

[commit-bot: I haz the power, 2016-04-22 19:18:53]

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