Re: [PATCH v3] Work around a bug in Solaris make's file-inclusion mechanism.

[Stefano Lattarini]

On Sunday 26 September 2010, Ralf Wildenhues wrote:
> Hello Stefano,
> 
> * Stefano Lattarini wrote on Thu, Sep 23, 2010 at 12:18:07AM CEST:
> > Subject: [PATCH] Work around a bug in file-inclusion mechanism of
> > Solaris make.
> > 
> > * automake.in (handle_single_transform): In the name of the
> > dependency file: collapse multiple slash characters into a single
> > one.
> > * tests/subobj11a.test: New test.
> > * tests/subobj11b.test: Likewise.
> > * tests/subobj11c.test: Likewise.
> > * tests/depcomp8a.test: Likewise.
> > * tests/depcomp8b.test: Likewise.
> > * tests/Makefile.am (TESTS): Updated.
> > * NEWS: Updated.
> > Report by Stefano Lattarini, quick fix by Ralf Wildenhues, final
> > patch and tests by Stefano Lattarini.
> > 
> > 
> > --- a/NEWS
> > +++ b/NEWS
> > +
> > +  - The code for automatic dependency tracking works around a Solaris
> > +    make bug triggered by sources containg repeated slashes when the
> containing
Fixed, thanks.

> > 
> > --- a/automake.in
> > +++ b/automake.in
> > @@ -2108,14 +2108,24 @@ sub handle_single_transform ($$$$$%)
> > 
> >     # Transform .o or $o file into .P file (for automatic
> >     # dependency code).
> > 
> > -   if ($lang && $lang->autodep ne 'no')
> > -   {
> > -       my $depfile = $object;
> > -       $depfile =~ s/\.([^.]*)$/.P$1/;
> > -       $depfile =~ s/\$\(OBJEXT\)$/o/;
> > -       $dep_files{dirname ($depfile) . '/$(DEPDIR)/'
> > -                    . basename ($depfile)} = 1;
> > -   }
> > +        # Properly flatten multiple adjacent slashes, as Solaris 10 make
> > +        # might fail over them in an include statement.
> > +        # Leading double slashes may be special, as per Posix, so deal
> > +        # with them carefully.
> > +        if ($lang && $lang->autodep ne 'no')
> > +        {
> > +            my $depfile = $object;
> > +            $depfile =~ s/\.([^.]*)$/.P$1/;
> > +            $depfile =~ s/\$\(OBJEXT\)$/o/;
> > 
> > +            my $maybe_extra_leading_slash = '';
> > +            $maybe_extra_leading_slash = '/' if $depfile =~ m,^//[^/],;
> > +            $depfile =~ s,/+,/,g;
> > +            my $basename = basename ($depfile);
> > +            # This might make $dirname empty, but we account for that 
> > below.
> > +            (my $dirname = dirname ($depfile)) =~ s/\/*$//;
> > +            $dirname = $maybe_extra_leading_slash . $dirname;
> > +            $dep_files{$dirname . '/$(DEPDIR)/' . $basename} = 1;
> 
> I guess I don't get why this code is so complex.  The comment seems
> wrong: the result of dirname is never empty,
It could be: if $object = "/foo.o", then $dirname = "/", and s/\/*$// makes
$dirname empty.
> nor should it end in a slash.
See above.
> The  s,/+,/,g  regex matches more often than necessary,
> causing unneeded copies; this can be fixed by adding another /.
You're saying I should use `//+' instead, for optimization reasons, right?
If yes, I agree.
 
> Why not something like this instead of the above eight lines
> (untested)?
> 
>   my ($depname = dirname ($depfile) . '/$(DEPDIR)/' . basename ($depfile)) =~ 
> s{([^/])//+}{$1/}g;
>       $dep_files{$depname} = 1;
This is wrong if $object is, say, "/foo.o":
 $depfile = /foo.Po
 dirname($depfile) = /
 basename($depfile) = foo.Po
 dirname($depfile) . '/$(DEPDIR)/' . basename($depfile) = //$(DEPDIR)/foo.Po
And even after the "s{([^/])//+}{$1/}g":
 $depname = //$(DEPDIR)/foo.Po
which is wrong: should be "/$(DEPDIR)/foo.Po".

Plus, my code is more cumbersome, true, but also easier to follow IMVHO.

> or maybe adding this as second command
>                $depname =~ s{/./}{/}g; 
> if you want to avoid unneeded in-string './' instances?
I wouldn't care about this, unless someday we find a make implementation
that chokes on it (and let's hope we'll never find one!)
 
> I'm not actually sure whether we want to avoid leading './'
> instances. GNU make will always start searching for include files
> in the current directory, but I haven't checked other makes yet. 
> Hmm, but since we don't explicitly add leading './', I guess doing
> without ought to be ok.
Adding `./' might be for another patch anyway.  The current one should
just fix the bug under analysis IMHO.

> > --- /dev/null
> > +++ b/tests/depcomp8a.test
> > 
> > +# Test for regressions in computation of names of .Po files for
> > +# automatic dependency tracking.
> 
> What was the regression, actually?
My second attempt at the patch (v2) broke when libtool objects were
involved; this test case is here for completeness, to check that there
is not a similar breakage when libtool objects are *not* involved.
Call it "defensive testing".

> > +# Try again with subdir-objects option.
> > +
> > +echo AM_PROG_CC_C_O >> configure.in
> > +echo AUTOMAKE_OPTIONS = subdir-objects >> Makefile.am
> > +
> > +$ACLOCAL
> > +$AUTOMAKE -a
> > +grep include Makefile.in # for debugging
> > +grep 'include.*\./\$(DEPDIR)/foo\.P' Makefile.in
> > +grep 'include.*[^a-zA-Z0-9_/]sub/\$(DEPDIR)/bar\.P' Makefile.in
> > +$EGREP 'include.*/(\.|sub)/\$(DEPDIR)' Makefile.in && Exit 1
> 
> This regex is not right.  (DEPDIR) should be \(DEPDIR\).
*blush*  Fixed, thanks.

> I don't quite understand why you want to forbid this regex, it
> doesn't seem to have anything to do with the Solaris make bug?
It doesn't; it is meant to guard against the regression I introduced
in my previous patch attempt.  You can try to apply that patch on a
temporary branch, if you are interested in seeing what this regression
was exactly.
 
> Both of these comments apply to the other tests as well, I guess.

> --- /dev/null
> +++ b/tests/depcomp8b.test
> @@ -0,0 +1,73 @@
> > +# Test for regressions in computation of names of .Plo files for
> > +# automatic dependency tracking.
> > +# Keep this in sync with sister test `depcomp8a.test', which checks the
> > +# same thing for non-libtool objects.
> > +
> > +required='libtool libtoolize'
>
> libtool is not required for this, only libtoolize.
I'll fix it then.  Thanks.

> > diff --git a/tests/subobj11a.test b/tests/subobj11a.test
> > new file mode 100755
> > index 0000000..752d492
> > --- /dev/null
> > +++ b/tests/subobj11a.test
> >
> > [[CUT]]
> > +
> > +if test -d src/.deps; then
> > +  depdir=src/.deps
> > +elif test -d src/_deps; then
> > +  depdir=src/_deps
> 
> How about
>   depdir=`sed -n 's/^DEPDIR = //p' Makefile`
>   if test -z "$depdir"; then
Fine with me; I'll do the change.
> > +else
> > +  echo "$me: depdir not found in src/" >&2
> > +  Exit 1
> > +fi
> > +
> > [[CUT]]

> > --- /dev/null
> > +++ b/tests/subobj11c.test
> > 
> > +# Automatic dependency tracking with subdir-objects option
> > active: +# check for a patologic case of slash-collapsing in the
> > name of
> 
> pathologic
Oops.  I thought I had corrected it already.  I'll fix it this time,
sorry for the sloppiness.

> > +:
> Both subobj11b and subobj11c would better be unit tests for the
> to-be factored-out helper function normalize_file_name.  Oh well.
I heartily agree w.r.t. subobj11c, not completely w.r.t. subobj11b.
Anyway, we'll look at them again once the to-be factored-out helper
function is ready and unit-testable.

Regards,
   Stefano