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

[Jez Ng via Phabricator via llvm-commits]

int3 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:
> > 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 :)


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

https://reviews.llvm.org/D144596