Implement DaemonController::GetVersion() method on Linux

Implement DaemonController::GetVersion() method on Linux

BUG=120950
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155349

[Jamie, 2012-09-06 01:29:50]

lgtm unless you think it's worth adding a pre-processing step to avoid the awk
ugliness, in which case I'll re-review.

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_linux.cc (right):

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:316: std::string output;
TrimWhitespaceASCII states that it's safe to use the same string for input and
output, so perhaps just call this |version| and use it for both?

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:320: if (!result) {
LOG(ERROR) here?

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:324: 
Perhaps add a test for exit_code being zero?

http://codereview.chromium.org/10909090/diff/1/remoting/tools/me2me_virtual_h...
File remoting/tools/me2me_virtual_host.py (right):

http://codereview.chromium.org/10909090/diff/1/remoting/tools/me2me_virtual_h...
remoting/tools/me2me_virtual_host.py:610: "dpkg -s chrome-remote-desktop | awk
'/^Version:/ { print $2; }'") >> 8
Yikes! I wonder if it would be cleaner to use @@VERSION@@ and pre-process this
script as we do with some others?

[Lambros, 2012-09-06 01:44:29]

On 2012/09/06 01:29:50, Jamie wrote:
> lgtm unless you think it's worth adding a pre-processing step to avoid the awk
> ugliness, in which case I'll re-review.
> 
Just a drive-by comment:

I wonder if it's worth compiling the version string into the host binary, and
pulling it out with a '--version' command-line argument?  Might be cleaner than
trying to pull it out of the Debian/RPM package.

Also, in the case of roll-back, the package version may diverge from the true
version of the underlying source-code (since Debian package versions can only be
increased, not decreased).

[Sergey Ulanov, 2012-09-06 02:07:11]

On 2012/09/06 01:44:29, Lambros wrote:
> On 2012/09/06 01:29:50, Jamie wrote:
> > lgtm unless you think it's worth adding a pre-processing step to avoid the
awk
> > ugliness, in which case I'll re-review.
> > 
> Just a drive-by comment:
> 
> I wonder if it's worth compiling the version string into the host binary, and
> pulling it out with a '--version' command-line argument?  Might be cleaner
than
> trying to pull it out of the Debian/RPM package.
> 
> Also, in the case of roll-back, the package version may diverge from the true
> version of the underlying source-code (since Debian package versions can only
be
> increased, not decreased).

I implemented it this way for consistency with Mac where we also check the
version for the packet installed instead of putting the version in a binary or a
script. I also wanted to avoid prepossessing of the daemon script to make it
easier to debug it. 
But the approach you are suggesting here is probably the best one: it allows to
return correct version in every case and avoid preprocessing of the python code.
Should we switch Mac to using this approach too?

[Jamie, 2012-09-06 05:58:39]

On 2012/09/06 02:07:11, sergeyu wrote:
> On 2012/09/06 01:44:29, Lambros wrote:
> > On 2012/09/06 01:29:50, Jamie wrote:
> > > lgtm unless you think it's worth adding a pre-processing step to avoid the
> awk
> > > ugliness, in which case I'll re-review.
> > > 
> > Just a drive-by comment:
> > 
> > I wonder if it's worth compiling the version string into the host binary,
and
> > pulling it out with a '--version' command-line argument?  Might be cleaner
> than
> > trying to pull it out of the Debian/RPM package.
> > 
> > Also, in the case of roll-back, the package version may diverge from the
true
> > version of the underlying source-code (since Debian package versions can
only
> be
> > increased, not decreased).
> 
> I implemented it this way for consistency with Mac where we also check the
> version for the packet installed instead of putting the version in a binary or
a
> script. I also wanted to avoid prepossessing of the daemon script to make it
> easier to debug it. 
> But the approach you are suggesting here is probably the best one: it allows
to
> return correct version in every case and avoid preprocessing of the python
code.
> Should we switch Mac to using this approach too?

I think that makes sense, though it doesn't need to be in this CL.

[Sergey Ulanov, 2012-09-06 19:31:57]

PTAL

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_linux.cc (right):

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:316: std::string output;
On 2012/09/06 01:29:50, Jamie wrote:
> TrimWhitespaceASCII states that it's safe to use the same string for input and
> output, so perhaps just call this |version| and use it for both?

Done.

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:320: if (!result) {
On 2012/09/06 01:29:50, Jamie wrote:
> LOG(ERROR) here?

Done.

http://codereview.chromium.org/10909090/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_linux.cc:324: 
On 2012/09/06 01:29:50, Jamie wrote:
> Perhaps add a test for exit_code being zero?

Done.

http://codereview.chromium.org/10909090/diff/1/remoting/tools/me2me_virtual_h...
File remoting/tools/me2me_virtual_host.py (right):

http://codereview.chromium.org/10909090/diff/1/remoting/tools/me2me_virtual_h...
remoting/tools/me2me_virtual_host.py:610: "dpkg -s chrome-remote-desktop | awk
'/^Version:/ { print $2; }'") >> 8
On 2012/09/06 01:29:50, Jamie wrote:
> Yikes! I wonder if it would be cleaner to use @@VERSION@@ and pre-process this
> script as we do with some others?

Done.

[Jamie, 2012-09-06 20:13:58]

lgtm

[commit-bot: I haz the power, 2012-09-06 20:54:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10909090/5002

[commit-bot: I haz the power, 2012-09-07 07:06:36]

Change committed as 155349