client/isolate: expand deps into individual files

client/isolate: expand deps into individual files

This adds the start of the logic for exp archive. We walk the provided
deps to expand directories into individual files. These files are then
sorted into three categories: symlinks, large files (to be isolated
individually) and small files (to be isolated in archives).

Issue luci/luci-go#9

BUG=59899
R=mcgreevy@chromium.org, tansell@chromium.org

Committed: https://github.com/luci/luci-go/commit/c6ecfb224e3065dd78fcf84b38c26844dee59a53

[mcgreevy, 2016-11-22 23:23:47]

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
File client/cmd/isolate/exp_archive.go (right):

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:23: // archiveThreshold is the size (in bytes)
used to determine whether to add
I've added some nitpicky comments about the wording of this comment, but I
actually think you'd be better off just doing:

func shouldArchive(item Item) bool {
  return item.Size < 100 * 1024
}

I think that this function will be simpler to document.  Then, on line 81, do:

var links, archiveFiles, individualFiles []*Item

etc.

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:24: // files to an tar archive before
uploading. Files smaller than this size will
s/an tar/a tar/

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:25: // be combined into archives before
s/before uploaded directly to/before being uploaded to/

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:27: archiveThreshold = 100e3 // 100kB
I'd expect this to be 100 * 1024 ...

[djd-OOO-Apr2017, 2016-11-23 00:36:34]

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
File client/cmd/isolate/exp_archive.go (right):

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:23: // archiveThreshold is the size (in bytes)
used to determine whether to add
On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> I've added some nitpicky comments about the wording of this comment, but I
actually think you'd be better off just doing:
> 
> func shouldArchive(item Item) bool {
>   return item.Size < 100 * 1024
> }
> 
> I think that this function will be simpler to document.  Then, on line 81, do:
> 
> var links, archiveFiles, individualFiles []*Item
> 
> etc.

In future stages of this change we have more thresholds that need to be
documented (like such as the total size to which we let archives grow). I'd like
to have a const() stanza at the start of the file which lets us clearly document
what these magic thresholds are (and easily switch to a flag-controlled
threshold instead).

So if I introduced a func, I'd still like to have the const. At that point a
func/method doesn't seem to hold its weight. wdyt?

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:24: // files to an tar archive before
uploading. Files smaller than this size will
On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> s/an tar/a tar/

Done.

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:25: // be combined into archives before
On 2016/11/22 at 23:23:46, mcgreevy2 wrote:
> s/before uploaded directly to/before being uploaded to/

Done.

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:27: archiveThreshold = 100e3 // 100kB
On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> I'd expect this to be 100 * 1024 ...

Without invoking religious wars (https://en.wikipedia.org/wiki/Kibibyte), I
thought 100e3 was easier to read (in the context of also having a maximum
archive size 10e6). I don't think it matter much either way. Happy to be swayed.

[mcgreevy, 2016-11-23 00:44:13]

LGTM

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
File client/cmd/isolate/exp_archive.go (right):

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:23: // archiveThreshold is the size (in bytes)
used to determine whether to add
On 2016/11/23 00:36:34, djd wrote:
> On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> > I've added some nitpicky comments about the wording of this comment, but I
> actually think you'd be better off just doing:
> > 
> > func shouldArchive(item Item) bool {
> >   return item.Size < 100 * 1024
> > }
> > 
> > I think that this function will be simpler to document.  Then, on line 81,
do:
> > 
> > var links, archiveFiles, individualFiles []*Item
> > 
> > etc.
> 
> In future stages of this change we have more thresholds that need to be
> documented (like such as the total size to which we let archives grow). I'd
like
> to have a const() stanza at the start of the file which lets us clearly
document
> what these magic thresholds are (and easily switch to a flag-controlled
> threshold instead).
> 
> So if I introduced a func, I'd still like to have the const. At that point a
> func/method doesn't seem to hold its weight. wdyt?

OK, let's stick with the consts, no func, and see how we go (you could still
change the name changes on line 81 though).

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:27: archiveThreshold = 100e3 // 100kB
On 2016/11/23 00:36:34, djd wrote:
> On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> > I'd expect this to be 100 * 1024 ...
> 
> Without invoking religious wars (https://en.wikipedia.org/wiki/Kibibyte), I
> thought 100e3 was easier to read (in the context of also having a maximum
> archive size 10e6). I don't think it matter much either way. Happy to be
swayed.

I don't care that much.

[djd-OOO-Apr2017, 2016-11-23 00:46:29]

Thanks!

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
File client/cmd/isolate/exp_archive.go (right):

https://codereview.chromium.org/2522803002/diff/1/client/cmd/isolate/exp_arch...
client/cmd/isolate/exp_archive.go:23: // archiveThreshold is the size (in bytes)
used to determine whether to add
On 2016/11/23 at 00:44:13, mcgreevy2 wrote:
> On 2016/11/23 00:36:34, djd wrote:
> > On 2016/11/22 at 23:23:47, mcgreevy2 wrote:
> > > I've added some nitpicky comments about the wording of this comment, but I
> > actually think you'd be better off just doing:
> > > 
> > > func shouldArchive(item Item) bool {
> > >   return item.Size < 100 * 1024
> > > }
> > > 
> > > I think that this function will be simpler to document.  Then, on line 81,
do:
> > > 
> > > var links, archiveFiles, individualFiles []*Item
> > > 
> > > etc.
> > 
> > In future stages of this change we have more thresholds that need to be
> > documented (like such as the total size to which we let archives grow). I'd
like
> > to have a const() stanza at the start of the file which lets us clearly
document
> > what these magic thresholds are (and easily switch to a flag-controlled
> > threshold instead).
> > 
> > So if I introduced a func, I'd still like to have the const. At that point a
> > func/method doesn't seem to hold its weight. wdyt?
> 
> OK, let's stick with the consts, no func, and see how we go (you could still
change the name changes on line 81 though).

(I had done – see ps2)

[djd-OOO-Apr2017, 2016-11-23 00:46:41]

The CQ bit was checked by djd@chromium.org

[commit-bot: I haz the power, 2016-11-23 00:46:49]

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-23 00:52:24]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479862001965960,
"parent_rev": "74934dfa148729458ce3adf87904733c6e46cfe5", "commit_rev":
"c6ecfb224e3065dd78fcf84b38c26844dee59a53"}

[commit-bot: I haz the power, 2016-11-23 00:52:26]

Description was changed from

==========
client/isolate: expand deps into individual files

This adds the start of the logic for exp archive. We walk the provided
deps to expand directories into individual files. These files are then
sorted into three categories: symlinks, large files (to be isolated
individually) and small files (to be isolated in archives).

Issue luci/luci-go#9

BUG=59899
R=mcgreevy@chromium.org, tansell@chromium.org
==========

to

==========
client/isolate: expand deps into individual files

This adds the start of the logic for exp archive. We walk the provided
deps to expand directories into individual files. These files are then
sorted into three categories: symlinks, large files (to be isolated
individually) and small files (to be isolated in archives).

Issue luci/luci-go#9

BUG=59899
R=mcgreevy@chromium.org, tansell@chromium.org

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

[commit-bot: I haz the power, 2016-11-23 00:52:27]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/c6ecfb224e3065dd78fcf84b38c26844dee59a53