Re: support standalone libltdl [libtool--gary--1.0--patch-23]

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Mon, Aug 22, 2005 at 09:28:59AM CEST:
> On 21 Aug 2005, at 08:21, Ralf Wildenhues wrote:
> 
> >No need to start working on that yet,  this patch has several issues
> >that make it unsuitable for application.
> 
> Okay, addressed below and attached new version of the full patch.

You forgot to
- kill all use of `#' except within comments from Makefile.am
  (from your patch)
- change "sed '...{ };d'" to "sed -n '...{ }'" in all cases

Both are regressions to what we have now (the use of `#' that is in
CVS HEAD now is harmless because it applies to tests only).  More below.

> >| +install-data-hook:
> >| +    $(mkinstalldirs) $(DESTDIR)$(ltdlincludedir)
> >| +    @for f in lt_system.h lt_error.h lt_dlloader.h; do \
> >| +      test -f $(DESTDIR)$(includedir)/$$f || continue; \
> >| +      echo "mv $(DESTDIR)$(includedir)/$$f $(DESTDIR)$ 
> >(ltdlincludedir)"; \
> >| +      mv $(DESTDIR)$(includedir)/$$f $(DESTDIR)$(ltdlincludedir); \
> >| +    done
> >| +endif
> >
> >This is wrong, don't do it, please.  I am not certain I understand
> >what you are trying to work around here, but it seems to me you should
> >just call these ltdlinclude_HEADERS, and kill the install/uninstall
> >hooks.  Maybe you need to adjust the sed script which creates
> >libltdl/Makefile.am, but ATM I don't see why that should be.
> 
> Long story, but good catch.  Thanks.  Fixed.

OK.

> >| -install-data-local:
> >| +install-data-local: libltdl/Makefile.in
> >|  ## Don't install over the top of an old pkgdatadir
> >|      -rm -rf $(DESTDIR)$(pkgdatadir)
*snip*
> >| -    chown -R root $(DESTDIR)$(ltdldatadir) && \
> >| -    chgrp -R root $(DESTDIR)$(ltdldatadir) || :
> >
> >This install-data-local rule needs to be killed.  I mean it.  Read my
> >proposed patch to that extent
> >http://lists.gnu.org/archive/html/libtool/2005-08/msg00084.html
> >and the thread that led to it, no need to iterate the arguments yet
> >again.
> >
> >The only reason I have not applied my proposed patch yet is that I  
> >know it conflicts with your patch.
> 
> Agreed.  But that is orthogonal.  If you want to commit yours first,  
> I'll merge before my commit... and vice versa.

OK.  Since mine works better when libltdl/configure is actually created,
I'll leave you to commit first, when your patch is ready.  I definitely
want this story over as soon as possible.

> >| -install-data-hook:
> >| -    chmod +x $(DESTDIR)$(pkgdatadir)/config/config.guess
> >| -    chmod +x $(DESTDIR)$(pkgdatadir)/config/config.sub
> >| -    chmod +x $(DESTDIR)$(pkgdatadir)/config/install-sh
> >| +    chown -R root $(DESTDIR)$(pkgdatadir) && \
> >| +    chgrp -R root $(DESTDIR)$(pkgdatadir) || :
> >
> >Kill these chown/chgrp, too.  You might need to resurrect the chmod.
> 
> Orthogonal to this patch again.  All I've done is change from $ 
> (DESTDIR)$(ltdldatadir) to $(DESTDIR)(pkgdatadir).  Second committer  
> gets to merge ;-)

ACK.

> >|  ## ------------- ##
> >| @@ -276,11 +437,24 @@ uninstall-hook:
> >|        f=`echo "$$p" | sed 's|^.*/||'`; \
> >|        echo " rm -rf '$(DESTDIR)$(aclocaldir)/$$f'"; \
> >|        rm -f "$(DESTDIR)$(aclocaldir)/$$f"; \
> >| -    done;
> >| +    done
> >| +## Uninstall files from install-data-hook rule from INSTALL_LTDL  
> >condition:
> >| +    @for f in $(include_HEADERS); do \
> >| +      f=`echo $$f | sed 's,^libltdl/,,'`; \
> >| +      test -f $(DESTDIR)$(includedir)/$$f || continue; \
> >| +      echo "rm -f $(DESTDIR)$(includedir)/$$f"; \
> >| +      rm -f $(DESTDIR)$(includedir)/$$f; \
> >| +    done
> >| +## Uninstall files from install-data-local rule above:
> >| +    @for f in $(ltdldatafiles) $(auxfiles); do \
> >| +      test -f $(DESTDIR)$(pkgdatadir)/$$f || continue; \
> >| +      echo "rm -f $(DESTDIR)$(pkgdatadir)/$$f"; \
> >| +      rm -f $(DESTDIR)$(pkgdatadir)/$$f; \
> >| +    done
> >
> >Kill these two rules.
> 
> Agreed for first new block, but the last part is still needed, otherwise
> the installed $(auxfiles) are left behind by make uninstall.

No.  All you need to do is specify all files in the correct order,
and then have them installed and uninstalled by Automaken rules
(something like `aclocal_DATA = $(aclocalfiles)' should suffice).
We might have to forbid parallel "make install".  I don't mind.

Oh, well.  If you don't fix this, then I'll just have to put this in
my patch as well, I guess.

> >|  .PHONY: web-manual
> >|  web-manual:
> >| -    @$(LN_S) $(top_srcdir)/doc/libtool.texi doc/manual.texi
> >| +    @$(LN_S) $(srcdir)/doc/libtool.texi doc/manual.texi
> >
> >Now that I see it: This rule is broken, the target may not have a
> >directory component.  Do like this:
> >+    @cd doc && $(LN_S) $(srcdir)/doc/libtool.texi manual.texi
> 
> Why?  As long as the first argument is an absolute path it works fine.
> I don't know of any host that doesn't support this use.

The first argument is _not_ an absolute path in general.  And now that
I see it, my replacement is broken as well, of course.  This really is
orthogonal to your patch, though, so you might leave that as-is.

> >| +* Support for libltdl in non-autotooled projects.
> >
> >Erm, this used to work with branch-1-5.
> >So, it's not a new feature, it's fixing a regression.
> 
> Ah yes, good call :-)  Removed.

Oh, it'd still be good news to a lot of people that HEAD is finally a
bit less broken, so you might want to mention _that_.   :))

> The attached now passes distcheck.

Does it pass `make install'?  Can you libtoolize and libtoolize --ltdl
a client package from the installed?

More:

| Index: libltdl/Makefile.am
| ===================================================================
| RCS file: /cvsroot/libtool/libtool/libltdl/Makefile.am,v
| retrieving revision 1.84
| diff -u -p -r1.84 Makefile.am
| --- libltdl/Makefile.am       21 Aug 2005 18:53:15 -0000      1.84
| +++ libltdl/Makefile.am       22 Aug 2005 07:27:36 -0000
| @@ -17,25 +21,28 @@
|  ## along with this program; see the file COPYING.  If not, write to
|  ## the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
|  ## Boston, MA 02110-1301, USA.
| -
| -BUILT_SOURCES                =
| -MOSTLYCLEANFILES     =
| -EXTRA_DIST           = configure.ac
| -
| -AUTOMAKE_OPTIONS     = foreign
| -ACLOCAL_AMFLAGS              = -I m4
| +ACLOCAL_AMFLAGS = -I m4
| +AUTOMAKE_OPTIONS = foreign
| +BUILT_SOURCES =
| +EXTRA_DIST =
| +CLEANFILES =
| +MOSTLYCLEANFILES =
| +# ### BEGIN Makefile.am
|  
|  DEFS                 = -DHAVE_CONFIG_H="<$(CONFIG_H)>" -DLTDL
| -AM_CPPFLAGS          = -I. -I$(srcdir) -I$(srcdir)/libltdl
| +AM_CPPFLAGS          = -I. -Ilibltdl -I$(srcdir)/libltdl \
| +                       -I$(srcdir)/libltdl

You won't believe it, but I put that -I$(srcdir) in there for a reason:
The libltdl user might build libltdl with a sub-Automake but not as a
subpackage and use the nostdinc Automake option.
 
another small nit: This
| +$(srcdir)/libltdl/Makefile.in: libltdl/Makefile.am
| +       @echo ' cd $(srcdir)/libltdl && $(AUTOMAKE) Makefile'; \
| +       cd $(srcdir)/libltdl && $(AUTOMAKE) Makefile

is shorter written like this:
+$(srcdir)/libltdl/Makefile.in: libltdl/Makefile.am
+       cd $(srcdir)/libltdl && $(AUTOMAKE) Makefile


My idea of reviewing is: if I don't allow through a patch with known
regressions, things will eventually have to get better.  So I don't
approve of this patch, because it introduces regressions.

Cheers,
Ralf