Log file names generated by gtest-parallel are too long

[volo-zyko]

I've been hit by this at least 2 times.

1. Evidently, on Windows, there is a restriction of MAX_PATH (260 characters) FS path length.

2. On encrypted partition in Ubuntu (using eCryptfs). See this comment. Since eCryptfs protects not only file's content but also file names then there is an approximate restriction of 140 characters file name length.

There probably other cases when gtest-parallel can fail writing log file due to file/path length restrictions.

Now I'm considering options for fixing these issues all at once:

1. Make writing log files configurable and if it's disabled then write everything to console. Drawback: this will clutter the output.

2. Restrict file/path length. It seems we discussed this previously. For example, use hash of test name as log file name. Drawback: difficult to find log files for each specific test.

3. Try to use system extensions for writing longer file names. While this can be done on Windows it will not work for eCryptfs or any other FS without such extensions.

4. Propose yours.

I'm in favor of option 1. Which option is acceptable for gtest-parallel?

[pbos]

Do you think this could be solved alongside #54? The proposed solution there is that we have --output_dir be None by default (and that just eats the passing logs, not spitting them out like 1. proposed here). When the output_dir is None, maybe we can use shorter tmpfile names and clear the temporary directory on exit?

Importantly: We need to save the log files to disk even when output_dir is unset (or go to /dev/null directly). Piping stdin/stdout to Python using the subprocess commands have been slow/buffering/unpredictable and causing hangs on WebRTC's build bots where a command can stall for multiple seconds. This was fixed in commit 34ae4d7.

[pbos]

.. and I think the output can't go to /dev/null directly, because then we can't get logs from failing processes. But we could definitely have shorter names for the interrim log files, maybe generated by mktemp, (as users don't have to get to them), and we shouldn't have to move the files on completion either (to passed/, failed/ etc.).

[volo-zyko]

Do you think this could be solved alongside #54?

yes, this one is closely related.

Importantly: We need to save the log files to disk even when output_dir is unset

I don't understand this. What I was thinking about when output_dir is None in Task.run() instead of this code:

    with open(self.log_file, 'w') as log:
      task = subprocess.Popen(self.test_command, stdout=log, stderr=log)

log could be in-memory file stream, say, StringIO. Is this what you tried to warn against when stating "Piping stdin/stdout to Python using the subprocess commands have been slow..."?

[pbos]

I think it would be fine if log was an in-ram file handle on the OS level, what we had performance issues with was: stdout = subprocess.PIPE, stderr = subprocess.STDOUT in the Popen call. I'm not sure what in the implementation make it so (that makes me wary of python-implemented StringIO etc).

There's a possibly-related post here: https://stackoverflow.com/a/10889177 but in our use we found delays / buffering / whatever that would not only delay the runtime with a constant factor but also see occasional bumps resulting both in timeouts (looking like flaky tests) and runtimes not representative of the underlying test runtime. We should add as little as possible to test runtimes or we're essentially lying. :)

[pbos]

Perhaps something like tempfile.mkstemp or even tempfile.TemporaryFile could be used. They seem more like wrappers around real OS functions. A big upshot for not using in-memory backing is that I've seen some huge tests that essentially spit out raw video frames to stdout. This would kill having a memory-backed file (and even if it's stupid I don't think we should call it unsupported).

[volo-zyko]

So, to summarize. If output_dir is None we save to temporary file created with tempfile.mkstemp. If a test fails we dump log file's content to stdout (like it's done right now) and also, in this case, we can safely remove the temporary file (as no one need it in a temporary directory). tempfile.TemporaryFile is not a fit as the file is removed once it is closed.

Does this look reasonable?

[pbos]

Yep, sounds good. TemporaryFile could work if we keep an open handle to it for reading later, but whatever's easiest tbh.

Yes, once the test has finished running we should remove the file, regardless of if we print the contents before or not. If there are any finally: or destructor things that we could run those could ensure that the file is deleted even if someone Ctrl+C's the commands (but I don't know python well enough to know if that's easily applicable).

If you implement this change I can take care of documentation updates if you'd prefer.

[volo-zyko]

See pull request #59.

And yes please update the documentation. You'll definitely come up with better wording.