[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:1
+# REQUIRES: x86, shell
+
----------------
jhenderson wrote:
> ruiu wrote:
> > andrewng wrote:
> > > 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`.
> > So basically you want to run this test on non-windows machine right?
> > 
> >   UNSUPPORTED: system-windows
> > 
> > should express the intention more clearly.
> It might be worth a comment too saying WHY it's not supported on Windows, because that restriction isn't obvious at first glance.
I've followed what the other reproduce tests have done. I suspect the reasoning for `shell` is that this can be overridden if your Windows shell, environment and tools can support long paths. By default, `shell` is not enabled on Windows.

I will add a comment.


================
Comment at: lld/test/ELF/reproduce-deplibs.s:12
+# RUN: ld.lld bar.o -o /dev/null --reproduce repro.tar
+# RUN: tar xf repro.tar
+# RUN: cmp foo.a repro/%:t.dir/foo.a
----------------
grimar wrote:
> jhenderson wrote:
> > Idle musing, feel free to ignore: I wonder whether we really need to check that the archive in the tar file is binary identical to the input one? Would just checking that the file is in the package be sufficient?
> I also wonder about it.
> 
> Our `reproduce-windows.s` has:
> ```
> # RUN: tar tf repro.tar | FileCheck %s
> 
> # CHECK: repro/response.txt
> # CHECK: repro/{{.*}}/build/foo.o
> ```
I've basically followed the convention of the other reproduce tests (except the Windows specific ones) that all check the contents too. I'm guessing this is just to be sure that it's also the correct file. I'm happy to change it to only test for its presence in the TAR file if that's the consensus.


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

https://reviews.llvm.org/D77659