Added TempDir

Added TempDir

Context-manager for temporary directory handling, similar to
tempfile.TemporaryDirectory on Python 3.

BUG=405428
R=dnj@chromium.org, tandrii@chromium.org

Committed: https://chromium.googlesource.com/infra/testing/expect_tests/+/1ca63fb18b9e896e98ccc7c8db53a7893ed4f74e

[pgervais, 2014-09-11 00:45:54]

pgervais@chromium.org changed reviewers:
+ agable@chromium.org, iannucci@chromium.org

[pgervais, 2014-09-11 00:45:55]

I made temporary directory handling re-entrant.

[pgervais, 2014-09-11 19:47:25]

pgervais@chromium.org changed reviewers:
+ dnj@chromium.org, sergeyberezin@chromium.org

[pgervais, 2014-09-11 19:47:26]

+Sergey, +Dan

[tandrii(chromium), 2014-09-12 16:18:11]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2014-09-12 16:18:11]

Not reviewer, but did a review.
LGTM with nitpicks.

https://codereview.chromium.org/556313004/diff/1/expect_tests/tempdir.py
File expect_tests/tempdir.py (right):

https://codereview.chromium.org/556313004/diff/1/expect_tests/tempdir.py#newc...
expect_tests/tempdir.py:39: # Clean up obsolete directories
nit: period at the end of comment.

https://codereview.chromium.org/556313004/diff/1/expect_tests/tempdir.py#newc...
expect_tests/tempdir.py:55: # Create current PID file
nit: period at the end of comment.

https://codereview.chromium.org/556313004/diff/1/expect_tests/tempdir.py#newc...
expect_tests/tempdir.py:62: # Try to nuke the pidfile first
nit: period at the end of comment.

[dnj, 2014-09-12 17:31:57]

https://codereview.chromium.org/556313004/diff/20001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:231: multiprocessing.Process(
Consider giving the process a name for debugging/tracking purposes. 'TestJob-%d'
% (j,) for j in xrange(opts.jobs)?

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py
File expect_tests/tempdir.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:31: def set_temp_dir(suffix=TEMP_SUFFIX):
This function seems to be mixing application-level things (PIDfile, cleanup from
previous state) with basic directory creation. I think you should split the two
up. Make the PIDfile, and previous application/directory cleanup something
that's handled explicitly at the application level during startup (before this
is even invoked), not implicitly during a "set_temp_dir" call.

This also means you don't have to exempt the newly-created directory from the
cleanup list.

This will simplify things both for testing and also conceptually.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:46: with open(pfile, 'rb') as f:
I don't see any reason to use the 'b' flag here.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:48: os.kill(int(f.read()), 0)
What's kill(..., 0) do? I'm used to killing w/ a signal. This also won't work on
Windows, will it?

I'm not sure I like this previous cleanup thing at all. Is it really
unreasonable to allow multiple 'expect_tests' to run at the same time? If not,
maybe just let them clean themselves up and leave it to the user to cleanup any
unlikely leakage, like other UNIX programs tend to do.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:56: with open(os.path.join(temp_dir, TEMP_PID_FILE),
'wb') as f:
I don't see any reason to use the 'b' flag here.

[pgervais, 2014-09-12 18:16:02]

https://codereview.chromium.org/556313004/diff/20001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:231: multiprocessing.Process(
On 2014/09/12 17:31:56, dnj wrote:
> Consider giving the process a name for debugging/tracking purposes.
'TestJob-%d'
> % (j,) for j in xrange(opts.jobs)?

Done.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py
File expect_tests/tempdir.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:31: def set_temp_dir(suffix=TEMP_SUFFIX):
On 2014/09/12 17:31:56, dnj wrote:
> This function seems to be mixing application-level things (PIDfile, cleanup
from
> previous state) with basic directory creation. I think you should split the
two
> up. Make the PIDfile, and previous application/directory cleanup something
> that's handled explicitly at the application level during startup (before this
> is even invoked), not implicitly during a "set_temp_dir" call.
> 
> This also means you don't have to exempt the newly-created directory from the
> cleanup list.
> 
> This will simplify things both for testing and also conceptually.

I haven't written this code, so I'm just guessing the design decisions here: 

- This is all intermingled because there is no way to get the name of the
temporary directory other than what is done line 40:  os.path.dirname(temp_dir).
It means /creating/ a temp file is required to know the location where it's been
created.

- Cleaning up the temporary directory automatically will prove valuable on bots.
I agree we can do that in an external process but the fewer leftovers we have,
the better.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:46: with open(pfile, 'rb') as f:
On 2014/09/12 17:31:57, dnj wrote:
> I don't see any reason to use the 'b' flag here.

Well, it doesn't hurt either.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:48: os.kill(int(f.read()), 0)
On 2014/09/12 17:31:57, dnj wrote:
> What's kill(..., 0) do? I'm used to killing w/ a signal. This also won't work
on
> Windows, will it?
> 
> I'm not sure I like this previous cleanup thing at all. Is it really
> unreasonable to allow multiple 'expect_tests' to run at the same time? If not,
> maybe just let them clean themselves up and leave it to the user to cleanup
any
> unlikely leakage, like other UNIX programs tend to do.

Sure. Having multiple expect_tests running at the same time is useful. I guess
it's here for the same reason as the cleanup: removing stuck processes.

[pgervais, 2014-09-12 18:19:17]

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py
File expect_tests/tempdir.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:48: os.kill(int(f.read()), 0)
On 2014/09/12 18:16:02, pgervais wrote:
> On 2014/09/12 17:31:57, dnj wrote:
> > What's kill(..., 0) do? I'm used to killing w/ a signal. This also won't
work
> on
> > Windows, will it?
> > 
> > I'm not sure I like this previous cleanup thing at all. Is it really
> > unreasonable to allow multiple 'expect_tests' to run at the same time? If
not,
> > maybe just let them clean themselves up and leave it to the user to cleanup
> any
> > unlikely leakage, like other UNIX programs tend to do.
> 
> Sure. Having multiple expect_tests running at the same time is useful. I guess
> it's here for the same reason as the cleanup: removing stuck processes.

Hm. os.kill(pid, 0) does nothing to the process, but raises an exception if it
doesn't exists. So it's just checking if the process is running.

[dnj, 2014-09-12 23:22:11]

lgtm

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py
File expect_tests/tempdir.py (right):

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:31: def set_temp_dir(suffix=TEMP_SUFFIX):
On 2014/09/12 18:16:02, pgervais wrote:
> On 2014/09/12 17:31:56, dnj wrote:
> > This function seems to be mixing application-level things (PIDfile, cleanup
> from
> > previous state) with basic directory creation. I think you should split the
> two
> > up. Make the PIDfile, and previous application/directory cleanup something
> > that's handled explicitly at the application level during startup (before
this
> > is even invoked), not implicitly during a "set_temp_dir" call.
> > 
> > This also means you don't have to exempt the newly-created directory from
the
> > cleanup list.
> > 
> > This will simplify things both for testing and also conceptually.
> 
> I haven't written this code, so I'm just guessing the design decisions here: 
> 
> - This is all intermingled because there is no way to get the name of the
> temporary directory other than what is done line 40: 
os.path.dirname(temp_dir).
> It means /creating/ a temp file is required to know the location where it's
been
> created.
> 
> - Cleaning up the temporary directory automatically will prove valuable on
bots.
> I agree we can do that in an external process but the fewer leftovers we have,
> the better.
> 

Acknowledged.

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:46: with open(pfile, 'rb') as f:
On 2014/09/12 18:16:02, pgervais wrote:
> On 2014/09/12 17:31:57, dnj wrote:
> > I don't see any reason to use the 'b' flag here.
> 
> Well, it doesn't hurt either.

It sort of implies that binary-level correctness is important *shrug*

https://codereview.chromium.org/556313004/diff/20001/expect_tests/tempdir.py#...
expect_tests/tempdir.py:48: os.kill(int(f.read()), 0)
On 2014/09/12 18:19:17, pgervais wrote:
> On 2014/09/12 18:16:02, pgervais wrote:
> > On 2014/09/12 17:31:57, dnj wrote:
> > > What's kill(..., 0) do? I'm used to killing w/ a signal. This also won't
> work
> > on
> > > Windows, will it?
> > > 
> > > I'm not sure I like this previous cleanup thing at all. Is it really
> > > unreasonable to allow multiple 'expect_tests' to run at the same time? If
> not,
> > > maybe just let them clean themselves up and leave it to the user to
cleanup
> > any
> > > unlikely leakage, like other UNIX programs tend to do.
> > 
> > Sure. Having multiple expect_tests running at the same time is useful. I
guess
> > it's here for the same reason as the cleanup: removing stuck processes.
> 
> Hm. os.kill(pid, 0) does nothing to the process, but raises an exception if it
> doesn't exists. So it's just checking if the process is running.

Ah neat trick.

[pgervais, 2014-09-12 23:24:20]

Committed patchset #4 (id:60001) manually as 1ca63fb (presubmit successful).