Re: [Quilt-dev] diffstat and test suite

[John Vandenberg]

Hi Jean,

On 9/19/06, Jean Delvare <address@hidden> wrote:
> John,
>
> > On 9/19/06, Jean Delvare <address@hidden> wrote:
> > > There is now one test file in the quilt test suite which requires
> > > diffstat (dir-a-b.test). It causes "make check" to freeze when quilt
> > > has been configured --without-diffstat. The reason is that we create a
> > > diffstat wrapper (compat/diffstat) in this case. The wrapper is
> > > supposed to call the real diffstat, and that would "work" (i.e. fail
> > > cleanly) when the wrapper is installed. However it does NOT work when
> > > the wrapper is still in compat, and compat is in the PATH (which is the
> > > case when running the test suite): the wrapper will call itself in
> > > loops forever.
> > >
> > > This raises two questions:
> > > 1* Why do we create a diffstat wrapper when --without-diffstat has been
> > > passed to configure (or configure simply failed to find diffstat)? I
> > > don't see the point. I would understand a wrapper saying "Sorry,
> > > diffstat isn't installed" as we have for sendmail, but a wrapper
> > > calling a binary we know isn't available, I don't get it.
> >
> > We created wrappers so the code base didnt need to be littered with
> > @address@hidden  When diffstat wasnt found, the build system would fall back
> > to a 'compat' program that failed...
> >
> > 
> http://cvs.savannah.nongnu.org/viewcvs/*checkout*/quilt/quilt/compat/diffstat.in?rev=1.2
>
> I assume you meant rev=1.1.

Yes.

> > That has been updated to try to execute the named program in case the
> > program was added afterwards (e.g. via rpm) ...
> >
> > 
> http://cvs.savannah.nongnu.org/viewcvs/*checkout*/quilt/quilt/compat/diffstat.in?rev=1.2
>
> I understand this, but it still doesn't make sense. I can't see any
> difference between having this wrapper and having no wrapper at all. In
> both cases, "quilt refresh --diffstat" will fail if diffstat isn't
> installed, and succeed if it is. You have an additional level of
> indirection with no benefit that I can see. So I'd rather have no
> wrapper at all.

diffstat, sendmail and rpmbuild were shoehorned into
QUILT_COMPAT_PROG_PATH.  The original compat scripts for these worked
well, but the current ones for diffstat and rpmbuild are a no-op.

> > It looks like a packaging issue, so maybe there is a packaging solution to 
> this.
>
> I don't quite get what you mean here, can you be more precise?

Sure; the problem is in compat/diffstat.in v1.2, which appears to have
been added to solve a packaging problem.  We could roll back to
compat/diffstat.in v1.1 if the quilt rpm included diffstat as a
dependency, or if the hard dependency isnt desirable the packaging
script could simply remove the compat/diffstat v1.1 script after make
install has finished.

> > > Here is the patch I have come up with, which I'll apply to CVS soon
> > > unless somebody objects. It reverts the first chunk of:
> > > 
> http://cvs.savannah.nongnu.org/viewcvs/quilt/configure.ac?root=quilt&r1=1.55&r2=1.56
> > > and deletes compat/diffstat.in. It also removes the use of diffstat in
> > > the test suite.
> > >
> > > With this patch, the test suite passes again on a system I have where
> > > diffstat is not installed.
> > >
> > > John, want to comment on this?
> >
> > On inspection, this patch doesnt appear to work correctly; the program
> > supplied to --with-diffstat wont be hardwired into the quilt source.
>
> Ah, yes, good point. I only tested the case where diffstat is found,
> and the case where it is not - not the case where a specific path is
> provided by the user. I need to fix this. Any idea how I can do that?
> Your QUILT_COMPAT_PROG_PATH macro doesn't appear to support optional
> binaries, and I would like to avoid duplicating code if possible.

IMO another aclocal.m4 macro (QUILT_OPT_PROG_PATH?) for optional
binaries would be a good idea, catering for diffstat, sendmail,
rpmbuild, and any others that pop up.

--
John