Re: Fix makedepend depmode to cope with VPATH builds

[Stefano Lattarini]

On Friday 08 April 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Fri, Apr 08, 2011 at 01:05:53PM CEST:
> > Hello Ralf, sorry for the delay.
> 
> I don't think you need to apologize for any delays on your behalf,
> for at least a few months.  ;-)
> 
> > On Wednesday 06 April 2011, Ralf Wildenhues wrote:
> > > Ugh, a looong-standing depmode=makedepend bug that silently breaks
> > > rebuilds in VPATH trees.  It has now hit me in the wild.  :-(
> > >
> > Just out of curiosity: with which compiler?
> 
> IBM XL on BlueGene.
> 
> > > --- a/lib/depcomp
> > > +++ b/lib/depcomp
> 
> > > @@ -557,7 +557,9 @@ makedepend)
> > >    touch "$tmpdepfile"
> > >    ${MAKEDEPEND-makedepend} -o"$obj_suffix" -f"$tmpdepfile" "$@"
> > >    rm -f "$depfile"
> > > -  cat < "$tmpdepfile" > "$depfile"
> > > +  # makedepend may prepend the VPATH from the source file name to the 
> > > object.
> > > +  sed_object_re=`echo $object | sed 's/[].[^$\\*|]/\\\\&/g'`
> > >
> > A couple of questions/observations about the sed command:
> >  1. I wasn't sure whether `.', `$', `^' and `*' are to be escaped when used 
> > in
> >     a bracket expression; however, the POSIX standard at:
> >       <http://pubs.opengroup.org/onlinepubs/007908799/xbd/re.html>
> >     says they shouldn't be.  Also, I couldn't find any reference in the
> >     Autoconf manual pointing to a sed implementation that doesn't respect
> >     this part of the standard.
> 
> Ah, cool.  Then I recon we don't need this escaping step at all.
>
Why not?  The sed command below:
  sed "s|^.*\($sed_object_re *:\)|\1|" "$tmpdepfile" > "$depfile"
doesn't use any bracket, does it?

> The only character I was worried about was $, as it can occur in
> object names now and then (ok, mostly java, and that doesn't use
> depcomp).  Any others in the list are pretty much excluded anyway,
> because they will evoke shell expansion errors when used in a
> makefile unquoted.  Except for '.', but I'm not afraid of matching
> a bit too broadly here, the final 'o' will take care that we don't
> match any header files.
>
OK, this makes sense (even if I'd err on the safe side w.r.t. the `.'
character), but I don't see how it's related to my observation above.
What am I missing?

> >  2. I guess the doubled `\\' in the sed command  is there to account for the
> >     extra stripping performed by the backtick `` command substitution, 
> > right?
> >     But is that truly portable, considering that the doubled `\\' is also
> >     into a single-quotes string?  Or would it be better to play safe and use
> >     an indirection like:
> >       sed_quoting_cmd='s/[].[^$\\*|]/\\&/g'
> >       sed_object_re=`echo $object | sed -e "$sed_quoting_cmd"`
> >     WDYT?
> 
> This is all moot given above.
> 
> > > +  sed "s|^.*\($sed_object_re *:\)|\1|" "$tmpdepfile" > "$depfile"
> > >    sed '1,2d' "$tmpdepfile" | tr ' ' '
> > >  ' | \
> > >  ## Some versions of the HPUX 10.20 sed can't process this invocation
> 
> > > --- /dev/null
> > > +++ b/tests/depcomp9.test
> 
> > > +cd build
> > > +../configure am_cv_CC_dependencies_compiler_type=makedepend
> > > +$MAKE || Exit 77
> > >
> > Why skip the test here?
> 
> Well, makedepend mode might not actually work for some reason.
> It's pretty contrived, granted, but let's say the makedepend script
> is so broken that _AM_DEPENDENCIES wouldn't have chosen it because
> it would fail one of the tests in m4/depend.m4.  Then I don't really
> want to see a test failure here.
>
OK.

> > Wouldn't be better to just let the test fail if $MAKE fails the first
> > time?
> 
> See above.
> 
> > (Which means the first $MAKE
> > call and the subsequent "$MAKE clean" are just to be removed as
> > redundant).
> 
> Ah, no.  This sequence is very important.  Sorry for not being clear
> enough.  The error I'm chasing does not happen upon the first make,
> only the second one: the first time, the .Po files contain only the
> dummy text.  During the first build, makedepend fills them, but without
> the depcomp fix, it would create dependencies like this:
> 
>   ../../src/foo.o: ../../src/foo.c ../../src/foo.h
> 
> Then, the second time, make sees that foo.o is a prerequisite of foo,
> sees a '.c.o' inference rule, and the above.  GNU make then infers that
> 'foo.o' should actually be '../../src/foo.o', so the command to compile
> foo.c gets to be
>   source=../../src/foo.c' object='../../src/foo.o' ... depcomp $(CC) ...
> 
> instead of the correct
>   source=../../src/foo.c' object='foo.o' ... depcomp $(CC) ...
> 
> and that messes up the depcomp script work, causing the error messages
> quoted in my patch.
>
Ah, I hadn't thought in detail about this.  Sorry for the sloppiness.

> I'm adding a comment now:
> 
>   # Do not error out with the first make, as the forced 'makedepend'
>   # depmode might not actually work, but we have overridden the
>   # _AM_DEPENDENCIES tests.  The actual error only happens the second time
>   # the objects are built, because 'makedepend' has silently messed up the
>   # .Po files the first time.
> 
> Hope that makes it clearer.
>
It does for me.  Thanks.

> > > +$MAKE clean
> > > +
> > > +$MAKE >out 2>&1 || { cat out; Exit 1; }
> > > +cat out
> > > +$FGREP 'src/.deps' out && Exit 1
> > > +
> > This is probably paranoid but ... what if $(DEPDIR) is != `.deps'?
> > (e.g., it is `_deps').
> 
> Thanks, fixed now.
> 
> Below's what I'm pushing to maint.
> 
> Thanks for the review,
> Ralf
> 
>     Fix makedepend depmode for VPATH builds.
>     
>     * lib/depcomp [makedepend]: Remove any VPATH prefix from the
>     object file name, so a rebuild doesn't attempt to update the
>     .Po files in the source tree.
>     * tests/depcomp9.test: New test.
>     * tests/Makefile.am (TESTS): Update.
>     * NEWS: Update.
> 
> diff --git a/NEWS b/NEWS
> index 3132b16..6bd67f6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -56,6 +56,8 @@ Bugs fixed in 1.11.0a:
>  
>    - The parallel-tests driver now does not produce erroneous results
>      with Tru64/OSF 5.1 sh upon unreadable log files any more.
> +
> +  - The makedepend depmode now works better with VPATH builds.
>  
>  New in 1.11:
>  
> diff --git a/lib/depcomp b/lib/depcomp
> index df8eea7..ce9419c 100755
> --- a/lib/depcomp
> +++ b/lib/depcomp
> @@ -1,10 +1,10 @@
>  #! /bin/sh
>  # depcomp - compile a program generating dependencies as side-effects
>  
> -scriptversion=2009-04-28.21; # UTC
> +scriptversion=2011-04-08-18; # UTC
>  
> -# Copyright (C) 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009 Free
> -# Software Foundation, Inc.
> +# Copyright (C) 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009, 2011,
> +# Free Software Foundation, Inc.
>  
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -503,7 +503,8 @@ makedepend)
>    touch "$tmpdepfile"
>    ${MAKEDEPEND-makedepend} -o"$obj_suffix" -f"$tmpdepfile" "$@"
>    rm -f "$depfile"
> -  cat < "$tmpdepfile" > "$depfile"
> +  # makedepend may prepend the VPATH from the source file name to the object.
>
I'd add a comment here telling why there's no need to escape $object for the
sed commmand just below.

> +  sed "s|^.*\($object *:\)|\1|" "$tmpdepfile" > "$depfile"
>    sed '1,2d' "$tmpdepfile" | tr ' ' '
>  ' | \
>  ## Some versions of the HPUX 10.20 sed can't process this invocation
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index d4d9474..c894864 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -277,6 +277,7 @@ depcomp6.test \
>  depcomp7.test \
>  depcomp8a.test \
>  depcomp8b.test \
> +depcomp9.test \
>  depdist.test \
>  depend.test \
>  depend2.test \
> diff --git a/tests/depcomp9.test b/tests/depcomp9.test
> new file mode 100755
> index 0000000..d137fad
> --- /dev/null
> +++ b/tests/depcomp9.test
> @@ -0,0 +1,90 @@
> +#! /bin/sh
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# makedepend should work in VPATH mode.
> +
> +# Here's the bug: makedepend will prefix VPATH to the object file name,
> +# thus the second make will invoke depcomp with object='../../src/foo.o',
> +# causing errors such as:
> +# touch: cannot touch `../../src/.deps/foo.TPo': No such file or directory
> +# makedepend: error:  cannot open "../../src/.deps/foo.TPo"
> +# ../../depcomp: line 560: ../../src/.deps/foo.TPo: No such file or directory
> +
> +# We include subfoo only to be sure that we don't remove too much
> +# from the object file name.
> +
> +required='makedepend'
> +. ./defs || Exit 1
> +
> +mkdir src src/sub build
> +
> +cat >> configure.in << 'END'
> +AC_PROG_CC
> +AM_PROG_CC_C_O
> +AC_CONFIG_FILES([src/Makefile])
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +SUBDIRS = src
> +END
> +
> +cat > src/Makefile.am << 'END'
> +AUTOMAKE_OPTIONS = subdir-objects
> +bin_PROGRAMS = foo
> +foo_SOURCES = foo.c foo.h sub/subfoo.c
> +END
> +
> +cat > src/foo.h <<EOF
> +extern int subfoo (void);
> +EOF
> +
> +cat >src/foo.c <<EOF
> +#include "foo.h"
> +int main (void)
> +{
> +  return subfoo ();
> +}
> +EOF
> +
> +cat >src/sub/subfoo.c <<EOF
> +#include "foo.h"
> +int subfoo (void)
> +{
> +  return 0;
> +}
> +EOF
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE -a
> +
> +cd build
> +../configure am_cv_CC_dependencies_compiler_type=makedepend
> +
> +# Do not error out with the first make, as the forced 'makedepend'
> +# depmode might not actually work, but we have overridden the
> +# _AM_DEPENDENCIES tests.  The actual error only happens the second time
> +# the objects are built, because 'makedepend' has silently messed up the
> +# .Po files the first time.
> +$MAKE || Exit 77
> +$MAKE clean
> +
> +$MAKE >out 2>&1 || { cat out; Exit 1; }
> +cat out
> +grep 'src/[._]deps' out && Exit 1
> +
> +:
>