[lit] Ignore src permissions when copying lit_test_times

[anguslees]

cp will attempt to re-produce the source permissions on the
destination file.

When the input files are read-only (0444) - eg when building in a bazel
sandbox - this means the .lit_test_times.txt tmpfile is also 0444.
When TestTimes.py is run a second time, it fails (permission denied)
to write to this read-only file.

Rather than attempt to navigate portable cp command line options to
disable this, just copy using a naive cat src > dst instead.

For posterity, the failure appeared as:

Command Output (stdout):
--
cp Inputs/reorder/lit_test_times Inputs/reorder/.lit_test_times.txt
not env -u FILECHECK_OPTS "/var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/python_3_12_x86_64-unknown-linux-gnu/bin/python3" /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/lit.py -j1 Inputs/reorder > /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/tests/Output/reorder.py.tmp.out
FileCheck --check-prefix=TIMES < Inputs/reorder/.lit_test_times.txt /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/tests/reorder.py
--

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: Angus Lees (anguslees)

Changes

cp will attempt to re-produce the source permissions on the
destination file.

When the input files are read-only (0444) - eg when building in a bazel
sandbox - this means the .lit_test_times.txt tmpfile is also 0444.
When TestTimes.py is run a second time, it fails (permission denied)
to write to this read-only file.

Rather than attempt to navigate portable cp command line options to
disable this, just copy using a naive cat src > dst instead.

For posterity, the failure appeared as:

Command Output (stdout):
--
cp Inputs/reorder/lit_test_times Inputs/reorder/.lit_test_times.txt
not env -u FILECHECK_OPTS "/var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/python_3_12_x86_64-unknown-linux-gnu/bin/python3" /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/lit.py -j1 Inputs/reorder > /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/tests/Output/reorder.py.tmp.out
FileCheck --check-prefix=TIMES < Inputs/reorder/.lit_test_times.txt /var/lib/engflow/worker/work/27/exec/bazel-out/k8-fastbuild/bin/external/llvm-project/llvm/utils/lit/tests/reorder.py.test.runfiles/__main__/external/llvm-project/llvm/utils/lit/tests/reorder.py
--

---

Full diff: https://github.com/llvm/llvm-project/pull/121377.diff

1 Files Affected:

• (modified) llvm/utils/lit/tests/reorder.py (+1-1)

diff --git a/llvm/utils/lit/tests/reorder.py b/llvm/utils/lit/tests/reorder.py
index 8ebeac393d5575..633ed808a2454c 100644
--- a/llvm/utils/lit/tests/reorder.py
+++ b/llvm/utils/lit/tests/reorder.py
@@ -1,6 +1,6 @@
 ## Check that we can reorder test runs.
 
-# RUN: cp %{inputs}/reorder/lit_test_times %{inputs}/reorder/.lit_test_times.txt
+# RUN: cat %{inputs}/reorder/lit_test_times > %{inputs}/reorder/.lit_test_times.txt
 # RUN: not %{lit-no-order-opt} %{inputs}/reorder > %t.out
 # RUN: FileCheck --check-prefix=TIMES < %{inputs}/reorder/.lit_test_times.txt %s
 # RUN: FileCheck < %t.out %s

[anguslees]

Reviewers: I don't have merge permissions - so please mash the merge button yourself when you feel appropriate.

[RoboTux]

I had no idea cp tried to keep the same permission. LGTM. Since this is a workaround I'll give some time for people to object & reply if they have a better (portable) idea.

[jh7370]

I don't know anything about bazel or the proposed setup, but it seems to me like the problem is that .lit_test_times.txt exists in the first place here. As I understand it, this file should be generated on the first run of the test suite and then used/updated on later runs. As it's generated by lit, I wouldn't expect it to ever have read-only permissions. That would suggest that the problem is either in the creation of the sandbox (perhaps a file has been included in the sandbox image or whatever is used to initialise things that shouldn't have been), or possibly in the test stuff itself.

cp will attempt to re-produce the source permissions on the destination file.

Which version of cp is this? The posix standard discusses a -p option to preserve permissions, which would imply that without that option, it shouldn't be preserving the permissions... NB: I haven't actually checked any real cp implementations to see what happens in practice.

[anguslees]

Apologies, I would have included a much longer explanation if I'd realised this change was controversial!

As it's generated by lit, I wouldn't expect it to ever have read-only permissions. That would suggest that the problem is either in the creation of the sandbox (perhaps a file has been included in the sandbox image or whatever is used to initialise things that shouldn't have been), or possibly in the test stuff itself.

Bazel runs each 'action' (usually a single command with explicit input/output files) in a new minimal sandbox, so it can cache/parallelise at the level of individual commands. Bazel is a bit loose with file permissions, and generally only tracks "executable" or "not executable". Different sandbox implementations provide the 'input' files to each action in different ways - eg a hardlink/symlink to a cached files, or an on-demand FUSE filesystem - and some of these protect the input files with read-only file permissions.

We can discuss whether that is "correct" or not, but I don't really have any authority or useful history to provide on this aspect of bazel. 🤷

Also so we don't distracted, I don't think this is unique to bazel. I think any compiler cache or read-only source tree that used a 0444 Inputs/reorder/lit_test_times would encounter this issue.

Which version of cp is this? The posix standard discusses a -p option to preserve permissions, which would imply that without that option, it shouldn't be preserving the permissions... NB: I haven't actually checked any real cp implementations to see what happens in practice.

In my real build environment (where I encountered this issue), I'm using busybox 'cp'. Testing on a mac (using mac's cp) and Linux (coreutils 8.32), I get similar results:

$ cp --version | head -n1
cp (GNU coreutils) 8.32
$ umask 002
$ touch foo
$ ls -l foo
-rw-rw-r-- 1 coder coder 0 Jan 12 22:04 foo
$ chmod a-w foo
$ cp foo bar
$ ls -l foo bar
-r--r--r-- 1 coder coder 0 Jan 12 22:04 bar
-r--r--r-- 1 coder coder 0 Jan 12 22:04 foo

I'm sure I can track down the relevant lines of code in coreutils, if you wish. It's possible that this code is so old that I won't be able to track it back to a commit justification/explanation however.

Is there an actual concern that I need to respond to here?

[jh7370]

Is there an actual concern that I need to respond to here?

Not with cp. I was just confused, having recently been looking at the POSIX spec for different reasons. On further read, it turns out that I misread the spec anyway!

I was concerned that this was some special behaviour needed to cope with bazel that would easily be forgotten in a future change. I've taken a second look at the test and understand more about what is going on, but I'm now confused by why 1dbcb79 isn't sufficient?

[anguslees]

but I'm now confused by why 1dbcb79 isn't sufficient?

This is not about bazel. If %{inputs}/reorder/lit_test_times is 0444 for any reason, then we need this fix.

It just so happens that in my peculiar bazel build environment, input files are presented as 0444. This is not the usual/default bazel setup, and it's unlikely that other bazel users will experience this same issue. But again: this is not about bazel. I can contrive other non-bazel scenarios where input source is read-only, if that will help.

I agree this is challenging to exercise in a test without setting up an entire new build environment. Without that, I agree a future change might (re)introduce a similar issue.

[jh7370]

but I'm now confused by why 1dbcb79 isn't sufficient?

This is not about bazel. If %{inputs}/reorder/lit_test_times is 0444 for any reason, then we need this fix.

Yes, I see your point. Missed that generality.

It just so happens that in my peculiar bazel build environment, input files are presented as 0444. This is not the usual/default bazel setup, and it's unlikely that other bazel users will experience this same issue. But again: this is not about bazel. I can contrive other non-bazel scenarios where input source is read-only, if that will help.

I don't think this issue is even specifically about %{inputs}/reorder/lit_test_times, since there could be other cases of in-place modification after a copy that would be desirable. A slightly contrived example might be having a canned binary used as a test input for an llvm-objcopy test that modifies the binary in place: copying it before modification simply won't work.

That makes me wonder whether the "right" fix is to implement a built-in cp within lit (similar to other existing utilities that we've provided local implementations of), with an option to copy the file but without the original permissions (akin to a cp + chmod 777 sequence or similar). What do you think?

[anguslees]

What do you think?

I think you're overcomplicating this, and I'm not sure why.

We already have a copy operation that ignores the original permissions. "Cat" and shell redirection are already used elsewhere so these are not new dependencies.

This is an uncontroversial one line patch. I agree this can be solved in other ways - and I don't object to any other solution. But what am I missing here?

[jh7370]

This is an uncontroversial one line patch.

If it was uncontroversial, I wouldn't be disagreeing with it...

I agree this can be solved in other ways - and I don't object to any other solution. But what am I missing here?

If I came to this test in the future and saw it written oddly like that, I'd probably go "oh, somebody didn't realise it would just be simpler to use cp, so I'll just fix it while I'm here, since cp more clearly conveys the intended meaning". It's unlikely that I'd bother looking at the blame, because "it's obviously fine".

A big glaring comment saying not to do that in the file itself would probably be sufficient to satisfy me for now though.