Make the auto-update script really smart. It no longer replaces the versione...

Make the auto-update script really smart.  It no longer replaces the versioned
directory corresponding to the version that was just replaced or to any
version that is in use on the system.

BUG=14610
TEST=keystone_install_test.sh, and play with auto-update.  A lot.

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

[Mark Mentovai, 2009-10-16 06:02:39]

http://codereview.chromium.org/285002/diff/6003/6005
File chrome/tools/build/mac/keystone_install.sh (left):

http://codereview.chromium.org/285002/diff/6003/6005#oldcode68
Line 68: # Make sure we have permission to write the destination
I got rid of this because it's totally extraneous.  Let's get on with the update
already.  If it wants to fail because there's no permission, then let it. 
Mostly, I'm not convinced that this will work correctly when access control
regimes other than POSIX mode bits are in effect, namely, ACLs.

http://codereview.chromium.org/285002/diff/6003/6005
File chrome/tools/build/mac/keystone_install.sh (right):

http://codereview.chromium.org/285002/diff/6003/6005#newcode35
Line 35: # Make sure that there's something to copy from, and that it's an
absolute
I got a little bit of paranoia with the way that we throw around arguments. 
They're always safe in that they're quoted, but for sanity's sake, I wanted to
be sure that they don't start with dashes.

http://codereview.chromium.org/285002/diff/6003/6005#newcode50
Line 50: sed -Ene \
You know how much I like replacing "grep | sed" pipelines with stragiht sed,
right?  :)  The new RE here also guarantees an absolute path for DEST.

http://codereview.chromium.org/285002/diff/6003/6004
File chrome/tools/build/mac/keystone_install_test.sh (left):

http://codereview.chromium.org/285002/diff/6003/6004#oldcode26
Line 26: TEMPDIR=$(mktemp -d ${TMPDIR}/$(basename ${0}).XXXXXX)
${TMPDIR} wan't set on one of my Snow Leopard machines.  The new invocation
works properly on Leopard and Snow Leopard.

[TVL, 2009-10-16 13:39:11]

http://codereview.chromium.org/285002/diff/6003/6005
File chrome/tools/build/mac/keystone_install.sh (right):

http://codereview.chromium.org/285002/diff/6003/6005#newcode61
Line 61: KS_VERSION_KEY="KSVersion"
move down a few lines to where it's now used? (since you removed the early use
of it.

http://codereview.chromium.org/285002/diff/6003/6005#newcode82
Line 82: rsync -ac --exclude "*" "${SRC}/Contents/Versions/" \
is it worth putting this behind an if so it only happens if it doesn't exist? 
Or do you always want to to run from the permissions copy/update?

http://codereview.chromium.org/285002/diff/6003/6005#newcode83
Line 83: "${DEST}/Contents/Versions" || exit 5
use use 4 here, is it worth giving these 3 rsyncs distinct values so we can tell
which it was if something does fail?

http://codereview.chromium.org/285002/diff/6003/6005#newcode141
Line 141: # Notify LaunchServices.  This is not considered a critical step.
and we don't want to mix lsregister exit codes with our codes...

http://codereview.chromium.org/285002/diff/6003/6005#newcode187
Line 187: shopt -s nullglob
comment for why?

http://codereview.chromium.org/285002/diff/6003/6005#newcode216
Line 216: rm -rf "${versioned_dir}"
so one thing gets through this...

if someone runs via commandline to add flags to the browser.  they control the
path on the browser, it might not be the full path...

If they have any renderers open, you are safe, we always get those will full
paths, so you catch the helpers...

But if it's running without any windows open, this test is blind to it, and it
will be nuked.

But, anyone doing all that probably falls under your network shares logic, so...

http://codereview.chromium.org/285002/diff/2003/7003#newcode94
Line 94: 
this would be one place to leave off -r and then you don't need the exclude?

[Mark Mentovai, 2009-10-16 14:20:53]

Comments addressed.  'nother look?

http://codereview.chromium.org/285002/diff/2003/7003
File chrome/tools/build/mac/keystone_install.sh (right):

http://codereview.chromium.org/285002/diff/2003/7003#newcode94
Line 94: rsync ${RSYNC_FLAGS} --exclude "*" "${SRC}/Contents/Versions/" \
TVL wrote:
> this would be one place to leave off -r and then you don't need the exclude?

If you don't use -r and you give rsync a directory, you get:

mark@crack bash$ mkdir -p /tmp/a/junk
mark@crack bash$ rsync -clpt /tmp/a/ /tmp/b
skipping directory /tmp/a/.

[TVL, 2009-10-16 14:29:38]

http://codereview.chromium.org/285002/diff/9001/11
File chrome/tools/build/mac/keystone_install.sh (right):

http://codereview.chromium.org/285002/diff/9001/11#newcode130
Line 130: "${SRC}/" "${DEST}" || exit 5
I think you wanted exit 6 here now.

Actually, for the upgrade from non versions dir to versions dir, this will nuke
the framework out from under running copies...

[TVL, 2009-10-16 14:42:23]

lgtm

http://codereview.chromium.org/285002/diff/7006/7008
File chrome/tools/build/mac/keystone_install.sh (right):

http://codereview.chromium.org/285002/diff/7006/7008#newcode256
Line 256: # process.  If the file exists and is open, it will exit zero (true).
nit - you explained the args on ps, not lsof