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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.

LGTM



================
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
----------------
andrewng wrote:
> grimar wrote:
> > jhenderson wrote:
> > > andrewng wrote:
> > > > 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.
> > > I don't really have an opinion either way. Happy to defer to others on this one.
> > I'd slightly prefer to avoid checking the content because LLD tests are already take some time to pass and would be good to reduce this time. I do not mind to leave it for a follow-up, but see my question above.
> I think that if these comparisons are a concern, then we should address all the reproduce tests accordingly in a follow up.
My main concern now it that this and seems few other tests about `--reproduce` are disabled for no reason it seems.
Addressing the reproduce tests accordingly  would solve both problems at once.


================
Comment at: lld/test/ELF/reproduce-deplibs.s:5
+## Windows as the extraction of the tar archive can cause problems related to
+## path length limits.
+
----------------
andrewng wrote:
> grimar wrote:
> > Does it mean that using `tar tf` like `reproduce-windows.s` do would allow this to be enabled on windows?
> Yes, `tar tf` only lists the contents of the TAR archive. That's why that is used in the Windows tests.
Then it is better to do in this way as disabling this test is unnecessary. If your intention is to improve testing coverage then enabling it to run on windows bots looks like a good thing to me.


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

https://reviews.llvm.org/D77659