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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo 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
+
----------------
MaskRay wrote:
> 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`
+1 - also prefer the explicit paths taht I can copy without havign to look up where the current directory is


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

https://reviews.llvm.org/D144596