[PATCH] D77659: [ELF][test] Add reproduce test for dependent libraries

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked 2 inline comments as done.
andrewng added inline comments.


================
Comment at: lld/test/ELF/reproduce-deplibs.s:13
+# RUN: tar xf repro.tar
+# RUN: diff foo.a repro/%:t.dir/foo.a
+
----------------
MaskRay wrote:
> andrewng wrote:
> > MaskRay wrote:
> > > `cmp` may be better for comparison between binary files.
> > The other reproduce tests make use of `diff`, so I've just followed their lead. However, `cmp` probably is more appropriate for the binary files. Perhaps we should change them all to use `cmp` instead of `diff` where applicable?
> Yeah, I've noticed that. `diff -> cmp` should be fine.
> 
> Out of curiosity, how did you notice that reproduce-deplibs.s should be tested as well? 
I will change to `cmp`, although `diff` is a lit built-in command, so that might explain its usage elsewhere.

We have some legacy downstream regression tests which we are gradually migrating to lit. That's why we noticed that this coverage was missing.


================
Comment at: lld/test/ELF/reproduce-deplibs.s:1
+# REQUIRES: x86, shell
+
----------------
MaskRay wrote:
> Can you also delete `shell` and check whether harbomaster (pre-merge testing) complains?
I can try deleting `shell` but will need to add it back to prevent potential testing issues on Windows related to paths being too long when the reproduce TAR file is extracted. That's the reason why all the reproduce tests that extract the TAR file include `shell`.


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

https://reviews.llvm.org/D77659