[Chromoting] Sign, brand and version the me2me_host

[Chromoting] Script to sign the me2me_host binary and build the installer.

This script lives in that me2me_host archive file and is called by the
signing bots.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133994

[garykac, 2012-04-24 08:04:03]

+mark for sign_and_build.sh

[alexeypa (please no reviews), 2012-04-24 16:06:19]

Please see my comments. I didn't look at sign_and_build.sh.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
File remoting/host/installer/build-installer-archive.py (right):

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:96:
copyFileWithDefs(src_file, full_dst_file, defs)
This means that copyFileWithDefs could be used for non-text files. I guess if
should be called for a subset of known text file extensions instead.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:107: out_dir: Target
directory where files are copied.
nit: This function does not have |out_dir| and |files_root| parameters.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:121: #shutil.copy2(src_file,
dst_file)
nit: Remove the commented out code.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:136: # file permissions
correctly. We use the 'unzip' command manually.
This does not seem to be cross-platform. I believe unzip is not available on
Windows by default (unless it comes with python).

This may be not a problem since it looks like we are not going to use this
script on Windows anyway. But then we should not build the target invoking the
script on Windows too.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:173: if ext == '.zip':
nit: The logic here can be:

if .zip: copyZipIntoArchive
elif (text file): copyFileIntoArchive
else: copyFileWithDefs

This way there will be a single point to make a decision based on the file
extension. The prototype of copyFileWithDefs will have to be different then.

http://codereview.chromium.org/10197005/diff/7002/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10197005/diff/7002/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1169: ['branding == "Chrome"', {
nit: we have way to many way to do branding. Windows port is using
../chrome/tools/build/version.py to do string substituting in files. Let's talk
offline about it.

[garykac, 2012-04-24 19:48:21]

PTAL:
Review comments addressed + I renamed the signing script to 'do_signing.sh' so
that we can migrate the Mac Chrome signing code to use a generic script that
calls into archives.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
File remoting/host/installer/build-installer-archive.py (right):

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:96:
copyFileWithDefs(src_file, full_dst_file, defs)
On 2012/04/24 16:06:19, alexeypa wrote:
> This means that copyFileWithDefs could be used for non-text files. I guess if
> should be called for a subset of known text file extensions instead.

Done.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:107: out_dir: Target
directory where files are copied.
On 2012/04/24 16:06:19, alexeypa wrote:
> nit: This function does not have |out_dir| and |files_root| parameters.

Done.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:121: #shutil.copy2(src_file,
dst_file)
On 2012/04/24 16:06:19, alexeypa wrote:
> nit: Remove the commented out code.

Done.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:136: # file permissions
correctly. We use the 'unzip' command manually.
On 2012/04/24 16:06:19, alexeypa wrote:
> This does not seem to be cross-platform. I believe unzip is not available on
> Windows by default (unless it comes with python).
> 
> This may be not a problem since it looks like we are not going to use this
> script on Windows anyway. But then we should not build the target invoking the
> script on Windows too.

Per our discussion, I'll followup this cl with one to make this target
Mac-specific.

http://codereview.chromium.org/10197005/diff/7002/remoting/host/installer/bui...
remoting/host/installer/build-installer-archive.py:173: if ext == '.zip':
On 2012/04/24 16:06:19, alexeypa wrote:
> nit: The logic here can be:
> 
> if .zip: copyZipIntoArchive
> elif (text file): copyFileIntoArchive
> else: copyFileWithDefs
> 
> This way there will be a single point to make a decision based on the file
> extension. The prototype of copyFileWithDefs will have to be different then.

I like that, but I'll do that in a follow-up cl since this one's big enough as
it is.

http://codereview.chromium.org/10197005/diff/7002/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10197005/diff/7002/remoting/remoting.gyp#newco...
remoting/remoting.gyp:1169: ['branding == "Chrome"', {
On 2012/04/24 16:06:19, alexeypa wrote:
> nit: we have way to many way to do branding. Windows port is using
> ../chrome/tools/build/version.py to do string substituting in files. Let's
talk
> offline about it.

Agreed. I considered using version.py here, but that would have required a
separate action or target to pre-process the files before sending them to the
archive script for more processing. It seemed like overkill for this case, but
we should definitely unify our branding, copyrights and versioning.

[alexeypa (please no reviews), 2012-04-24 20:01:53]

LGTM (except do_signing.sh which should be reviewed by Mark).

[Mark Mentovai, 2012-04-24 20:13:11]

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
File remoting/host/installer/mac/do_signing.sh (right):

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:6: 
I like to run scripts under |set -e| to make them fail (exit nonzero) as soon as
any command in them fails.

-u is also nice because it prevents you from referencing undefined variables.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:90: if [[ ${?} != 0 ]]; then
If you were using |set -e|, you could get rid of these. The nature of the
failure would be obvious from the echos and the output from codesign.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:121: # Create the .dmg.
It’s advisable to use pkg-dmg instead of this (as is done for Chrome).

pkg-dmg creates optimal disk images. What you’ve gotten here isn’t optimal.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:153: build_packages "${input_dir}"
You’re not signing the package?

Or does that just not happen inside this script?

[garykac, 2012-04-24 22:31:12]

PTAL

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
File remoting/host/installer/mac/do_signing.sh (right):

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:6: 
On 2012/04/24 20:13:11, Mark Mentovai wrote:
> I like to run scripts under |set -e| to make them fail (exit nonzero) as soon
as
> any command in them fails.
> 
> -u is also nice because it prevents you from referencing undefined variables.

Done.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:90: if [[ ${?} != 0 ]]; then
On 2012/04/24 20:13:11, Mark Mentovai wrote:
> If you were using |set -e|, you could get rid of these. The nature of the
> failure would be obvious from the echos and the output from codesign.

Done.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:121: # Create the .dmg.
On 2012/04/24 20:13:11, Mark Mentovai wrote:
> It’s advisable to use pkg-dmg instead of this (as is done for Chrome).
> 
> pkg-dmg creates optimal disk images. What you’ve gotten here isn’t optimal.

OK, I'll make a follow-up cl and send it to you for review.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:153: build_packages "${input_dir}"
On 2012/04/24 20:13:11, Mark Mentovai wrote:
> You’re not signing the package?
> 
> Or does that just not happen inside this script?

First step is to get the binaries signed since they're causing the firewall
warnings.

I'll be following up with the code to sign the .mpkg. My first attempt using
|codesign| didn't work - I think I need to use |packagemaker --sign|, but I
haven't played around with it.

As far as I know, I don't need to sign the .dmg (but let me know if you feel
otherwise).

[Mark Mentovai, 2012-04-24 22:58:19]

LGTM

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
File remoting/host/installer/mac/do_signing.sh (right):

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:121: # Create the .dmg.
garykac wrote:
> On 2012/04/24 20:13:11, Mark Mentovai wrote:
> > It’s advisable to use pkg-dmg instead of this (as is done for Chrome).
> > 
> > pkg-dmg creates optimal disk images. What you’ve gotten here isn’t optimal.
> 
> OK, I'll make a follow-up cl and send it to you for review.

OK to do this in a follow-up.

http://codereview.chromium.org/10197005/diff/14001/remoting/host/installer/ma...
remoting/host/installer/mac/do_signing.sh:153: build_packages "${input_dir}"
garykac wrote:
> On 2012/04/24 20:13:11, Mark Mentovai wrote:
> > You’re not signing the package?
> > 
> > Or does that just not happen inside this script?
> 
> First step is to get the binaries signed since they're causing the firewall
> warnings.
> 
> I'll be following up with the code to sign the .mpkg. My first attempt using
> |codesign| didn't work - I think I need to use |packagemaker --sign|, but I
> haven't played around with it.
> 
> As far as I know, I don't need to sign the .dmg (but let me know if you feel
> otherwise).

Correct, .dmg files don’t need to be signed.

[commit-bot: I haz the power, 2012-04-25 17:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/garykac@chromium.org/10197005/21001

[commit-bot: I haz the power, 2012-04-25 17:48:33]

Can't process patch for file
remoting/host/installer/mac/ChromotingHostService.packproj.
Unsupported svn property format.