Re: [PATCH v2 10/10] t/unit-tests: adapt lib-reftable{c,h} helper functions to clar

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 26, 2025 at 10:04:43AM +0100, Seyi Chamber wrote:
> On Mon, 5 May 2025 at 10:52, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, May 05, 2025 at 08:27:16AM +0100, Seyi Chamber wrote:
> > > On Fri, 2 May 2025 at 10:58, Patrick Steinhardt <ps@pks.im> wrote:
> > > > On Tue, Apr 29, 2025 at 06:53:02PM +0100, Seyi Kuforiji wrote:
> > > > > diff --git a/t/unit-tests/lib-reftable.h b/t/unit-tests/lib-reftable.h
> > > > > index e4c360fa7e..2958db5dc0 100644
> > > > > --- a/t/unit-tests/lib-reftable.h
> > > > > +++ b/t/unit-tests/lib-reftable.h
> > > > > @@ -6,12 +6,12 @@
> > > > >
> > > > >  struct reftable_buf;
> > > > >
> > > > > -void t_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);
> > > > > +void cl_reftable_set_hash(uint8_t *p, int i, enum reftable_hash id);
> > > > >
> > > > > -struct reftable_writer *t_reftable_strbuf_writer(struct reftable_buf *buf,
> > > > > +struct reftable_writer *cl_reftable_strbuf_writer(struct reftable_buf *buf,
> > > > >                                                struct reftable_write_options *opts);
> > > > >
> > > > > -void t_reftable_write_to_buf(struct reftable_buf *buf,
> > > > > +void cl_reftable_write_to_buf(struct reftable_buf *buf,
> > > > >                            struct reftable_ref_record *refs,
> > > > >                            size_t nrecords,
> > > > >                            struct reftable_log_record *logs,
> > > >
> > > > It is quite weird that we declare the replacement functions in
> > > > "unit-test.h" in the first commit only to remove them at a later point.
> > > > It would make way more sense if we introduced the functions in
> > > > "t/unit/lib-reftable.{c,h}" right from the start and then only remove
> > > > the unused functions in the last step.
> > > >
> > > > Patrick
> > >
> > > If I get it correctly, you're suggesting I have both the original
> > > functions and the clar-based variant in `t/unit/lib-reftable.{c,h}`
> >
> > Yup, exactly.
> >
> > Patrick
> 
> Hi Patrick,
> 
> Thanks for the suggestion to move both the original t-helpers and the
> Clar-based cl_ versions into `t/unit/lib-reftable.{c,h}`.
> 
> I’ve tried doing that but ran into some build issues. The Clar-based
> functions use clar_assert and clar_assert_equal, which aren’t
> available to non-Clar tests. Since both sets of helpers would live in
> the same object file, this causes linker errors for binaries that
> don’t link against Clar.
> 
> I also hit Makefile warnings like “target 'lib-reftable.o' given more
> than once,” due to the object being included in both Clar and non-Clar
> test builds. And with all declarations in one header, non-Clar tests
> see prototypes for Clar-only functions they can't link.
> 
> Also facing errors like these:
> `LINK t/unit-tests/bin/t-reftable-basics
> /usr/bin/ld: t/unit-tests/lib-reftable.o: in function
> `cl_reftable_strbuf_writer':
> /home/seyik/git/t/unit-tests/lib-reftable.c:113:(.text+0x520):
> undefined reference to `clar__assert'`
> 
> What would you recommend I do to fix this?

Hm. You could also have a separate "lib-reftable-clar.{c,h}" code unit
that contains the new definitions that you introduce early in your
series. At the end of the series you could then delete the old
"lib-reftable.{c,h}" and rename "lib-reftable-clar.{c,h}" accordingly to
have that name. It's requires a bit more shuffling, but given that the
final commit would then be a plain rename it shouldn't be too bad, I
guess.

Patrick