Preparation work to enable isolateserver.py download to use a local cache.

Preparation work to enable isolateserver.py download to use a local cache.

- Move many file functions from run_isolated.py to utils/file_path.py
- Move DiskCache from run_isolated to isolateserver.
- Move relevant tests.

This is in preparation to be able to use a local cache when downloading files.
It's useful for developpers not desiring to use run_isolated at all but just to
use isolateserver as a mean to efficiently transfert isolated trees around.

There's no functional change exception exception type change in link_file(), the
rest is only moving code around. The functional change will be done as a
separate change to make it more readable.

R=pgervais@chromium.org
BUG=

Committed: https://code.google.com/p/swarming/source/detail?repo=client&r=e4ad07ed10d0af2238f19af00d8ad0101a6ff1ed

[M-A, 2014-10-15 19:01:02]

A warmup CL while Vadim is sleeping.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py
File utils/file_path.py (right):

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:575: def hardlink(source, link_name):
Eventually I'll change it to hardlink(out, in) like other calls, but I'm
definitely not changing that in this CL.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:643: raise OSError('%s is missing' % infile)
These used to raise isolated_format.MappingError() but it's not a big deal.
That's the only functional change.

[pgervais, 2014-10-15 21:52:41]

lgtm

I added inline comments, they are all unrelated questions or non-constructive
criticisms :-) Feel free to ignore the latter, but I'm actually interested in
the answers.

https://codereview.appspot.com/156270043/diff/20001/isolateserver.py
File isolateserver.py (right):

https://codereview.appspot.com/156270043/diff/20001/isolateserver.py#newcode1969
isolateserver.py:1969: file_path.rmtree(tempdir)
Here is another functional change ;-)

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py
File tests/isolate_test.py (right):

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py#new...
tests/isolate_test.py:26: import isolated_format
isolate / isolated is unfortunate I think. It's not changeable now but finding
two more separated words would have been valuable.

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py#new...
tests/isolate_test.py:1490: optparse.OptionParser(), cmd + ['--config-variable',
'foo=bar']))
I guess we're still supporting python 2.6?

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py
File utils/file_path.py (right):

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:575: def hardlink(source, link_name):
On 2014/10/15 19:01:01, M-A wrote:
> Eventually I'll change it to hardlink(out, in) like other calls, but I'm
> definitely not changing that in this CL.

I'd rather have (in, out), because it's the way ln and cp work :-)
But consistency matters and there's no reason to change this signature
everywhere in swarming.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:641: raise ValueError('Unknown mapping action %s' % action)
(dont' take any action on this remark) The error message will show a number
after the word 'action', that will not make any sense to the user. If `action`
was a string it would be more useful.

something like:

VALID_ACTIONS = set(('hardlink', 'hardlink_with_fallback', 'symlink', 'copy'))

def link_file(...):
   if action not in VALID_ACTIONS:
       raise ValueError('Unknown mapping action: %s' % action)

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:643: raise OSError('%s is missing' % infile)
On 2014/10/15 19:01:02, M-A wrote:
> These used to raise isolated_format.MappingError() but it's not a big deal.
> That's the only functional change.

Could you add a call to os.path.abspath()? (unless infile is already absolute)

[M-A, 2014-10-15 23:23:04]

https://codereview.appspot.com/156270043/diff/20001/isolateserver.py
File isolateserver.py (right):

https://codereview.appspot.com/156270043/diff/20001/isolateserver.py#newcode1969
isolateserver.py:1969: file_path.rmtree(tempdir)
On 2014/10/15 21:52:41, pgervais wrote:
> Here is another functional change ;-)

Eh, indeed.

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py
File tests/isolate_test.py (right):

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py#new...
tests/isolate_test.py:26: import isolated_format
On 2014/10/15 21:52:41, pgervais wrote:
> isolate / isolated is unfortunate I think. It's not changeable now but finding
> two more separated words would have been valuable.

As I said last week, that's what you get when you ask French Canadians to name
things.

https://codereview.appspot.com/156270043/diff/20001/tests/isolate_test.py#new...
tests/isolate_test.py:1490: optparse.OptionParser(), cmd + ['--config-variable',
'foo=bar']))
On 2014/10/15 21:52:41, pgervais wrote:
> I guess we're still supporting python 2.6?

Yes, but not for long. Then it's sunk cost.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py
File utils/file_path.py (right):

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:575: def hardlink(source, link_name):
On 2014/10/15 21:52:41, pgervais wrote:
> On 2014/10/15 19:01:01, M-A wrote:
> > Eventually I'll change it to hardlink(out, in) like other calls, but I'm
> > definitely not changing that in this CL.
> 
> I'd rather have (in, out), because it's the way ln and cp work :-)
> But consistency matters and there's no reason to change this signature
> everywhere in swarming.

Ah right shutil uses (src, dst). Anyway, that's going to be in a follow up.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:641: raise ValueError('Unknown mapping action %s' % action)
On 2014/10/15 21:52:41, pgervais wrote:
> (dont' take any action on this remark) The error message will show a number
> after the word 'action', that will not make any sense to the user. If `action`
> was a string it would be more useful.
> 
> something like:
> 
> VALID_ACTIONS = set(('hardlink', 'hardlink_with_fallback', 'symlink', 'copy'))
> 
> def link_file(...):
>    if action not in VALID_ACTIONS:
>        raise ValueError('Unknown mapping action: %s' % action)

Call sites use all static values.

https://codereview.appspot.com/156270043/diff/20001/utils/file_path.py#newcod...
utils/file_path.py:643: raise OSError('%s is missing' % infile)
On 2014/10/15 21:52:41, pgervais wrote:
> On 2014/10/15 19:01:02, M-A wrote:
> > These used to raise isolated_format.MappingError() but it's not a big deal.
> > That's the only functional change.
> 
> Could you add a call to os.path.abspath()? (unless infile is already absolute)


I could assert os.path.isabs() instead. That's already an implicit assumption
that pretty much all calls are with absolute paths.

[M-A, 2014-10-16 00:22:35]

Committed patchset #1 (id:20001) manually as
e4ad07ed10d0af2238f19af00d8ad0101a6ff1ed (presubmit successful).