[PATCH] D144596: Add extra parameter to thinlto-prefix-replace

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/test/MachO/thinlto-index-file-object-prefix-replace.ll:3
+; RUN: rm -rf %t ; split-file %s %t && cd %t
+; RUN: mkdir -p oldpath oldpath/prefix_replace
+
----------------
int3 wrote:
> MaskRay wrote:
> > int3 wrote:
> > > itf wrote:
> > > > oontvoo wrote:
> > > > > This needs to go under `%t` (otherwise the tests may fail in our integrates because only stuff under `%t` is writeable)
> > > > is `cd %t` in the previous line not enough to keep the following commands under `%t` ?
> > > It is. However, none of the other tests do this, so I would prefer we avoid it unless it's required by the nature of this test
> > I support the use of `cd %t` as in `rm -rf %t && split-file %s %t && cd %t`, especially if there are many temporary/output files or complex file hierarchies. It's difficult to ensure a test doesn't write to other directories without `%t`.
> > 
> > (As mentioned elsewhere, Google has a simplified lit runner where CWD is the source directory instead of some directive relative to `%t`. And writing to the source directory will cause an error due to Bazel's sandbox.)
> aside from consistency, I like explicit paths in order that I can copy and paste individual lines from the verbose test output for debugging purposes. (I prefer to avoid environment variables in test for the same reasons.)
> 
> using a sandbox does seem like a better way of ensuring a test doesn't write outside of the test dir
> 
> but ok, I won't insist :)
OK:) I use my own simplified lit runner: https://github.com/MaskRay/Config/blob/master/home/bin/ff

```
% cd lld/test/ELF/lto 
% ff thinlto-index-file.ll
0  rm -rf a && mkdir a && cd a
1  opt -module-summary ../thinlto-index-file.ll -o 1.o
2  opt -module-summary $HOME/llvm/lld/test/ELF/lto/Inputs/thinlto.ll -o 2.o
3  opt -module-summary $HOME/llvm/lld/test/ELF/lto/Inputs/thinlto_empty.ll -o 3.o
4  ld.lld --plugin-opt=thinlto-index-only=1.txt -shared 1.o 2.o 3.o -o /dev/null
5  FileCheck ../thinlto-index-file.ll < 1.txt
6  ld.lld --thinlto-index-only=2.txt -shared 1.o 2.o 3.o -o /dev/null
7  diff 1.txt 2.txt
```

When I want to inspect something, I just `cd a`. So `cd %t` in the test file doesn't bother me that much :)

Another advantage is that some features depend on whether a stale output file exists. `rm -rf %t` makes the test immune to such bugs. With a lot of `%t1 %t2` it's tedious to do `rm -f %t1 %t2`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144596/new/

https://reviews.llvm.org/D144596