exceptions during test running actions lead to non-executable testlogs

[benjaminp]

If a Java exception occurs during a test runner action, the test logs will not be marked executable because the outputs of failed actions aren't "checked" by ActionMetadataHandler. This is particularly noticeable with --notest_keep_going because a exception is used to stop the build after the first test failure.

Example:

$ cat always-fail.sh
#/bin/sh
exit 1
$ cat BUILD
sh_test(
    name = 'always-fail',
    srcs = ['always-fail.sh'],
)
$ bazel test :always-fail
-r-xr-xr-x 1 benjamin benjamin 206 Nov  9 02:11 bazel-testlogs/always-fail/test.log
$ bazel test --notest_keep_going :always-fail
$ ls -lh bazel-testlogs/always-fail/test.log 
-rw-rw-r-- 1 benjamin benjamin 206 Nov  9 02:11 bazel-testlogs/always-fail/test.log

[jin]

cc @jmmv please triage if this issue is still relevant.

[jmmv]

Confirmed that this is still reproducible as reported. I think we should fix it because the inconsistent behavior doesn't look right.

But I'm curious: why does it matter if logs are executable or not?

[benjaminp]

Because test logs display themselves in a pager if you execute them.

…

On Mon, May 11, 2020, at 14:05, Julio Merino wrote: Confirmed that this is still reproducible as reported. I think we should fix it because the inconsistent behavior doesn't look right. But I'm curious: why does it matter if logs are executable or not? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4059 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABVSTUMIFLVAI45IO2DMTTRRBD6PANCNFSM4EC5KZAQ>.

[jmmv]

I knew that and found out by chance years ago... but that's... not really a "feature" I would have expected anyone to notice. The reason logs are executable is due to some super-obscure reason inside Google from many, many years ago. I see that, in Bazel, they are prefixed with:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1

but, internally, that line comes with a bug reference that justifies why. I could look and see if the details can be shared if you are curious, but I don't remember them right now.

Given this, I'd have hoped we'd make logs not executable.

[ulfjack]

The reason is that Google's remote execution system marks all files as executable in order to avoid having to have multiple copies of the same file available locally (multiple hard-links to the same file have the same mod bits). At my new company, we use a similar approach for the same reasons. Since these text files are executable, it made sense to make them do something sensible rather than output weird error messages when executed.

It's probably easy to fix, but I'm currently overloaded. Want to send a patch?

[benjaminp]

While I enjoy viewing test logs through execution, I'm not terribly invested in testlogs being executable or not; their mode just shouldn't depend on --[no]test_keep_going.

Broadly, I agree only supporting executable output files is a nice simplification for implementing a CAS. The general public is understandably less than pleased to find existing-but-nonfunctional apis for changing the mode of outputs (#2888). Failure to consider the mode of source files is simply a correctness bug (#3400).

[sgowroji]

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen (or ping me to reopen) if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.