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