[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] Work around a bug in Solaris make's file-inclusion mechan
From: |
Stefano Lattarini |
Subject: |
Re: [PATCH v3] Work around a bug in Solaris make's file-inclusion mechanism. |
Date: |
Sun, 26 Sep 2010 20:39:31 +0200 |
User-agent: |
KMail/1.13.3 (Linux/2.6.30-2-686; KDE/4.4.4; i686; ; ) |
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
- Re: [PATCH] Work around a bug in Solaris make's file-inclusion mechanism., Ralf Wildenhues, 2010/09/13
- Re: [PATCH] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/13
- [PATCH v2] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/15
- Re: [PATCH v2] Work around a bug in Solaris make's file-inclusion mechanism., Ralf Wildenhues, 2010/09/15
- Re: [PATCH v2] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/15
- Re: [PATCH v2] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/22
- [PATCH v3] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/22
- Re: [PATCH v3] Work around a bug in Solaris make's file-inclusion mechanism., Ralf Wildenhues, 2010/09/26
- Re: [PATCH v3] Work around a bug in Solaris make's file-inclusion mechanism.,
Stefano Lattarini <=
- [PATCH v3 2/?] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/24
- Re: [PATCH v3 2/?] Work around a bug in Solaris make's file-inclusion mechanism., Ralf Wildenhues, 2010/09/26
- Re: [PATCH v3 2/?] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/26
- Re: [PATCH v3 2/?] Work around a bug in Solaris make's file-inclusion mechanism., Ralf Wildenhues, 2010/09/26
- Re: [PATCH v3 2/?] Work around a bug in Solaris make's file-inclusion mechanism., Stefano Lattarini, 2010/09/26