Add cipd selfupdate command.

Add cipd selfupdate command.

This will allow depot_tools (or, really, any installation) to manage its copy of
cipd if there isn't a better mechanism (e.g. puppet) available.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843
Committed: https://github.com/luci/luci-go/commit/14213b9083041668662a7a22d03e28cb11ab5f60

[iannucci, 2016-11-16 19:56:12]

On 2016/11/16 19:55:44, iannucci wrote:

PTAL @ proposed skeleton. Implementation is not complete yet.

[Vadim Sh., 2016-11-16 20:04:58]

Consider using this machinery for file operations:
https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/fs.go#L25

It already knows how to update "locked" executables and how to cleanup trash.
See EnsureFile and CleanupTrash.

https://codereview.chromium.org/2509803002/diff/1/cipd/client/cmd/cipd/main.go
File cipd/client/cmd/cipd/main.go (right):

https://codereview.chromium.org/2509803002/diff/1/cipd/client/cmd/cipd/main.g...
cipd/client/cmd/cipd/main.go:1924: file, err := os.Open(os.Args[0])
https://github.com/luci/luci-go/blob/master/cipd/version/version.go#L137

Though I forgot why I used it...

[iannucci, 2016-11-18 07:05:59]

OK, PTAL. This essentially works now (e.g. I ran it and it correctly
self-updated on OS X). I need to add tests, obviously, but wanted to check to
see if this is the rightish implementation or if I'm doing something terribly
wrong :).

R

[iannucci, 2016-11-18 07:07:21]

On 2016/11/18 07:05:59, iannucci wrote:
> OK, PTAL. This essentially works now (e.g. I ran it and it correctly
> self-updated on OS X). I need to add tests, obviously, but wanted to check to
> see if this is the rightish implementation or if I'm doing something terribly
> wrong :).
> 
> R

(also need to verify that this WAI on windows)

[dnj, 2016-11-18 16:27:12]

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:734: if exeHash != "" && exeHash == currentHash {
Wouldn't "exeHash == currentHash" cover the inequality case? Or is currentHash
allowed to be empty?

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:755: return fs.EnsureFile(ctx, destination, func(of
*os.File) error {
So you're overwriting the running binary in-place? Isn't this expected to not
work on Windows? Or were we wrong in that premise?

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1264: Size     int64  `json:"size,string"`
Let's hope we don't need the string part :)

[Vadim Sh., 2016-11-18 20:14:32]

looks ok

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:722: func (client *clientImpl) MaybeUpdateClient(ctx
context.Context, fs local.FileSystem, targetVersion, currentHash, destination
string) error {
since you are doing multiple resolve ops, add at the beginning

client.BeingBatch(ctx)
defer client.EndBatch()

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:729: cache := client.getTagCache()
this can be nil if cache is disable, this is expected condition and not an
error, treat it as cache miss

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:755: return fs.EnsureFile(ctx, destination, func(of
*os.File) error {
On 2016/11/18 16:27:12, dnj wrote:
> So you're overwriting the running binary in-place? Isn't this expected to not
> work on Windows? Or were we wrong in that premise?

EnsureFile handles this. It first creates the temp file, and then tried to
rename it into the final destination. If it doesn't work, it moves the existing
file aside first.

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:1264: Size     int64  `json:"size,string"`
On 2016/11/18 16:27:12, dnj wrote:
> Let's hope we don't need the string part :)

Cloud Endpoints :-/

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
File cipd/client/cmd/cipd/main.go (right):

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
cipd/client/cmd/cipd/main.go:1928: defer fs.CleanupTrash(ctx)
this will leave the file hanging on Windows in same directory as exe. It may be
confusing. Maybe instead of "" use ".cipd_selfupdate" for trash dir?

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
cipd/client/cmd/cipd/main.go:1957: s.ClientOptions.cacheDir =
filepath.Join(filepath.Dir(exePath), ".client_cache")
actually, let's put both trash dir and this special cache under some single
directory (".cipd_client" or something).

[iannucci, 2016-11-19 01:30:53]

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:722: func (client *clientImpl) MaybeUpdateClient(ctx
context.Context, fs local.FileSystem, targetVersion, currentHash, destination
string) error {
On 2016/11/18 20:14:32, Vadim Sh. wrote:
> since you are doing multiple resolve ops, add at the beginning
> 
> client.BeingBatch(ctx)
> defer client.EndBatch()

Done.

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:729: cache := client.getTagCache()
On 2016/11/18 20:14:32, Vadim Sh. wrote:
> this can be nil if cache is disable, this is expected condition and not an
> error, treat it as cache miss

Done.

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cipd/client...
cipd/client/cipd/client.go:734: if exeHash != "" && exeHash == currentHash {
On 2016/11/18 16:27:12, dnj wrote:
> Wouldn't "exeHash == currentHash" cover the inequality case? Or is currentHash
> allowed to be empty?

added validation for currentHash so this is true now.

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
File cipd/client/cmd/cipd/main.go (right):

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
cipd/client/cmd/cipd/main.go:1928: defer fs.CleanupTrash(ctx)
On 2016/11/18 20:14:32, Vadim Sh. wrote:
> this will leave the file hanging on Windows in same directory as exe. It may
be
> confusing. Maybe instead of "" use ".cipd_selfupdate" for trash dir?

Done.

https://codereview.chromium.org/2509803002/diff/20001/cipd/client/cmd/cipd/ma...
cipd/client/cmd/cipd/main.go:1957: s.ClientOptions.cacheDir =
filepath.Join(filepath.Dir(exePath), ".client_cache")
On 2016/11/18 20:14:32, Vadim Sh. wrote:
> actually, let's put both trash dir and this special cache under some single
> directory (".cipd_client" or something).

Done.

[Vadim Sh., 2016-11-19 01:55:32]

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:730: logging.Infof(ctx, "cipd: maybe updating client
%q to version %q in package %q", destination, targetVersion, clientPackage)
nit: "checkin client updates, version %q"

(destination is redundant, package name is easily deduciable)

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:766: return fs.EnsureFile(ctx, destination, func(of
*os.File) error {
skip updating if info.clientBinary.SHA1 == currentHash?

[iannucci, 2016-11-19 02:54:52]

OK, PTAL. I'm verifying that this WAI on winders and will post back. I added
some tests... I'm not sure what else would really be good to test on
MaybeUpdateClient where the tests will be valuable though.

[iannucci, 2016-11-19 03:07:03]

On 2016/11/19 02:54:52, iannucci wrote:
> OK, PTAL. I'm verifying that this WAI on winders and will post back. I added
> some tests... I'm not sure what else would really be good to test on
> MaybeUpdateClient where the tests will be valuable though.

Works on windows now

[Vadim Sh., 2016-11-21 19:35:30]

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:730: logging.Infof(ctx, "cipd: maybe updating client
%q to version %q in package %q", destination, targetVersion, clientPackage)
On 2016/11/19 01:55:32, Vadim Sh. wrote:
> nit: "checkin client updates, version %q"
> 
> (destination is redundant, package name is easily deduciable)

Ping.

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:766: return fs.EnsureFile(ctx, destination, func(of
*os.File) error {
On 2016/11/19 01:55:32, Vadim Sh. wrote:
> skip updating if info.clientBinary.SHA1 == currentHash?

Ping.

[iannucci, 2016-11-21 20:16:24]

PTAL

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:730: logging.Infof(ctx, "cipd: maybe updating client
%q to version %q in package %q", destination, targetVersion, clientPackage)
On 2016/11/21 19:35:30, Vadim Sh. wrote:
> On 2016/11/19 01:55:32, Vadim Sh. wrote:
> > nit: "checkin client updates, version %q"
> > 
> > (destination is redundant, package name is easily deduciable)
> 
> Ping.

I had package name in there because it had the result of some auto-detection
(platform/arch), but I took it out.

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:743: if exeHash == currentHash {
I already skip everything up here.

[Vadim Sh., 2016-11-21 20:19:04]

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
File cipd/client/cipd/client.go (right):

https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
cipd/client/cipd/client.go:743: if exeHash == currentHash {
On 2016/11/21 20:16:24, iannucci wrote:
> I already skip everything up here.

It's not everything.

If the cache is missing, exeHash here is "". But later fetchClientBinaryInfo can
return a hash that already matches the binary we are runing. In fact, it will
always happen on first run after bootstrap.

[iannucci, 2016-11-21 20:48:42]

On 2016/11/21 20:19:04, Vadim Sh. wrote:
>
https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
> File cipd/client/cipd/client.go (right):
> 
>
https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
> cipd/client/cipd/client.go:743: if exeHash == currentHash {
> On 2016/11/21 20:16:24, iannucci wrote:
> > I already skip everything up here.
> 
> It's not everything.
> 
> If the cache is missing, exeHash here is "". But later fetchClientBinaryInfo
can
> return a hash that already matches the binary we are runing. In fact, it will
> always happen on first run after bootstrap.

Oh! Yeah ok, that's a good point.

[iannucci, 2016-11-21 20:49:58]

On 2016/11/21 20:48:42, iannucci wrote:
> On 2016/11/21 20:19:04, Vadim Sh. wrote:
> >
>
https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client.go
> > File cipd/client/cipd/client.go (right):
> > 
> >
>
https://codereview.chromium.org/2509803002/diff/40001/cipd/client/cipd/client...
> > cipd/client/cipd/client.go:743: if exeHash == currentHash {
> > On 2016/11/21 20:16:24, iannucci wrote:
> > > I already skip everything up here.
> > 
> > It's not everything.
> > 
> > If the cache is missing, exeHash here is "". But later fetchClientBinaryInfo
> can
> > return a hash that already matches the binary we are runing. In fact, it
will
> > always happen on first run after bootstrap.
> 
> Oh! Yeah ok, that's a good point.

Done

[Vadim Sh., 2016-11-21 21:09:35]

lgtm

[iannucci, 2016-11-21 21:16:39]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2016-11-21 21:16:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 21:22:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479762999872440,
"parent_rev": "ca8863e086c09405bd27517cd0c80a56a36054c3", "commit_rev":
"14213b9083041668662a7a22d03e28cb11ab5f60"}

[commit-bot: I haz the power, 2016-11-21 21:22:46]

Description was changed from

==========
Add cipd selfupdate command.

This will allow depot_tools (or, really, any installation) to manage its copy of
cipd if there isn't a better mechanism (e.g. puppet) available.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843
==========

to

==========
Add cipd selfupdate command.

This will allow depot_tools (or, really, any installation) to manage its copy of
cipd if there isn't a better mechanism (e.g. puppet) available.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=663843

Committed:
https://github.com/luci/luci-go/commit/14213b9083041668662a7a22d03e28cb11ab5f60
==========

[commit-bot: I haz the power, 2016-11-21 21:22:47]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/14213b9083041668662a7a22d03e28cb11ab5f60