Set close_fds True in Popen of cloudtail

Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io unlinks opened files.
Let's close fds when invoking cloudtail.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
Committed: https://chromium.googlesource.com/chromium/tools/build/+/2ef451d6c8afe2635e0eeb55038a0e3fb564d3ad

[tikuta, 2016-09-27 08:34:27]

tikuta@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org,
yyanagisawa@chromium.org

[tikuta, 2016-09-27 08:34:58]

Description was changed from

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and prevents unlink
opened files.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

to

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and prevents unlinking
opened files.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

[tikuta, 2016-09-27 08:36:06]

Description was changed from

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and prevents unlinking
opened files.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

to

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io
unlinks opened files.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

[Yoshisato Yanagisawa, 2016-09-27 08:36:20]

lgtm

[tikuta, 2016-09-27 08:37:59]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 08:38:01]

Dry run: 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-09-27 08:41:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 08:41:51]

Dry run: This issue passed the CQ dry run.

[shinyak (Google), 2016-09-27 09:05:17]

shinyak@google.com changed reviewers:
+ shinyak@google.com

[shinyak (Google), 2016-09-27 09:05:18]

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
align indent?

[tikuta, 2016-09-27 09:06:46]

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
On 2016/09/27 09:05:18, shinyak (Google) wrote:
> align indent?

above lines are in array.
it is correct indent.

[shinyak (Google), 2016-09-27 09:10:45]

Description was changed from

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io
unlinks opened files.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

to

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io
unlinks opened files.
Let's close fds when invoking cloudtail.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

[shinyak (Google), 2016-09-27 09:10:54]

lgtm

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
mmm...

[Paweł Hajdan Jr., 2016-09-27 16:29:19]

LGTM

[Vadim Sh., 2016-09-27 17:21:58]

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

[Vadim Sh., 2016-09-27 17:21:58]

If you try to remove a file that cloudtail is tailing, it will still fail with
permission errors on Windows, since cloudtail keeps a handler for that file.

You'll need to make sure cloudtail is stopped before trying to remove the file.

[ukai, 2016-09-28 02:21:24]

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
might be better to set stdout (and maybe stderr) to open(os.devnull, "w") ?

http://stackoverflow.com/questions/11269575/how-to-hide-output-of-subprocess-...

[ukai, 2016-09-28 02:24:04]

On 2016/09/27 17:21:58, Vadim Sh. wrote:
> If you try to remove a file that cloudtail is tailing, it will still fail with
> permission errors on Windows, since cloudtail keeps a handler for that file.
> 
> You'll need to make sure cloudtail is stopped before trying to remove the
file.

it's not file that cloudtail is tailing.  it's a file that happens to be used as
stdout of cloudtail.

cloudtail_utils.py emits cloudtail's pid to stdout, and recipe raw_io will
remove it when cloudtail_utils.py finished
but cloudtail inherits that stdout and keep opening the descriptor, so removing
the stdout file will fail.

[tikuta, 2016-09-28 02:27:15]

Thank you for review, comment and explanation!

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2374543005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
On 2016/09/28 02:21:24, ukai wrote:
> might be better to set stdout (and maybe stderr) to open(os.devnull, "w") ?
> 
>
http://stackoverflow.com/questions/11269575/how-to-hide-output-of-subprocess-...

Done.

[ukai, 2016-09-28 04:34:00]

lgtm

[tikuta, 2016-09-28 05:27:56]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-09-28 05:27:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
yyanagisawa@chromium.org, phajdan.jr@chromium.org, shinyak@google.com
Link to the patchset: https://codereview.chromium.org/2374543005/#ps20001
(title: "use devnull for stdout/stderr of cloudtail")

[commit-bot: I haz the power, 2016-09-28 05:28:05]

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-09-28 05:31:44]

Description was changed from

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io
unlinks opened files.
Let's close fds when invoking cloudtail.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139
==========

to

==========
Set close_fds True in Popen of cloudtail

In windows, Popen inherits file descriptors and Exception occurs when raw_io
unlinks opened files.
Let's close fds when invoking cloudtail.

This will fix the bug on windows buildbot.
https://codereview.chromium.org/2377553002/

BUG=643139

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/2ef451d6c8afe2635e0e...
==========

[commit-bot: I haz the power, 2016-09-28 05:31:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/2ef451d6c8afe2635e0e...