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

[Gary V. Vaughan]

Hallo Ralf,

On 21 Aug 2005, at 08:21, Ralf Wildenhues wrote:
> When you eventually commit the reorganization, I will definitely want
> this in two steps: 1) move files only, 2) change files.

Okay.  NP.

> Ah, and also libltdl/Makefile.am needs to be removed from CVS if  
> it's to
> be generated.  (It's good to see its diff now, though, made me find  
> some
> bugs in your patch.)

ACK.

> 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.

> standalone.at is missing from the patch -- has it changed since
> the first version?

Nup.  Just forgot cvs add before cvs diff.  Fixed.

> | +if INSTALL_LTDL
> | +include_HEADERS        = libltdl/ltdl.h \
> | +              libltdl/libltdl/lt_system.h \
> | +              libltdl/libltdl/lt_error.h \
> | +              libltdl/libltdl/lt_dlloader.h
> | +lib_LTLIBRARIES        = libltdl/libltdl.la
> | +
> | +ltdlincludedir = $(includedir)/libltdl
> | +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.

> | -install-data-local:
> | +install-data-local: libltdl/Makefile.in
> |  ## Don't install over the top of an old pkgdatadir
> |      -rm -rf $(DESTDIR)$(pkgdatadir)
> |  ## To avoid spurious reconfiguration when the user installs  
> these files
> |  ## with libtoolize, we have to preserve their timestamps carefully:
> | -    $(mkinstalldirs) $(DESTDIR)$(ltdldatadir)
> | -    ( cd $(srcdir)/libltdl && $(AMTAR) chf - $(ltdldatafiles); ) \
> | -      | ( umask 0 && cd $(DESTDIR)$(ltdldatadir) && $(AMTAR) xf  
> -; )
> | +    $(mkinstalldirs) $(DESTDIR)$(pkgdatadir)
> | +    ( cd $(srcdir) && $(AMTAR) chf - $(ltdldatafiles); ) \
> | +      | ( umask 0 && cd $(DESTDIR)$(pkgdatadir) && $(AMTAR) xf -; )
> | +## And similarly copy the config auxilliary files into the  
> master tree
> | +    ( cd $(srcdir)/libltdl && $(AMTAR) chf - $(auxfiles); ) \
> | +      | ( umask 0 && cd $(DESTDIR)$(pkgdatadir) && $(AMTAR) xf -; )
> | +    @for f in : $(auxexefiles); do \
> | +      test "X$$f" = X: && continue; \
> | +      echo "chmod +x '$(DESTDIR)$(pkgdatadir)/$$f'"; \
> | +      chmod +x "$(DESTDIR)$(pkgdatadir)/$$f"; \
> | +    done
> | +## Put a copy of the libtool m4 macros in the aclocal dir
> |      $(mkinstalldirs) $(DESTDIR)$(aclocaldir)
> |      @for p in $(aclocalfiles); do \
> |        f=`echo "$$p" | sed 's|^.*/||'`; \
> |        aclocalfiles="$$aclocalfiles $$f"; \
> |      done; \
> | -    ( cd $(srcdir)/m4 && $(AMTAR) chf - $$aclocalfiles; ) \
> | +    ( cd $(srcdir)/$(m4dir) && $(AMTAR) chf - $$aclocalfiles; ) \
> |        | ( umask 0 && cd $(DESTDIR)$(aclocaldir) && $(AMTAR) xf -; )
> | -    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.

> | -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 ;-)


> |  ## ------------- ##
> | @@ -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.

> |  .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.

> | +* 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.

> |  dnl These are bootstrap requirements, once built, libtool may  
> work with
> |  dnl much older releases of autoconf and automake.  See release  
> notes.
> | -AM_INIT_AUTOMAKE([1.8 gnits dist-bzip2])dnl We use auto- 
> m4_including
> | +AM_INIT_AUTOMAKE([1.8 gnits subdir-objects dist-bzip2])
>
> Not true.  subdir-objects are Automake-1.9 stuff.

Agreed.  Fixed.

> | -libltdl_la_CPPFLAGS    = -DLTDLOPEN=$(LTDLOPEN) $(AM_CPPFLAGS)
> | +
> | +libltdl_la_CPPFLAGS    = -DLTDLOPEN=libltdl $(AM_CPPFLAGS)
>
> Hey, don't break bugs we have fixed already!
> http://lists.gnu.org/archive/html/libtool-patches/2005-07/ 
> msg00107.html

Sorry, dunno why I didn't get a merge conflict there.  Thanks for
catching it!  Fixed.

> | -libltdlc_la_CPPFLAGS    = -DLTDLOPEN=$(LTDLOPEN)c $(AM_CPPFLAGS)
> | +libltdlc_la_CPPFLAGS    = -DLTDLOPEN=libltdlc $(AM_CPPFLAGS)
>
> Ditto.

ACK.

The attached now passes distcheck.

Cheers,
    Gary.
--
Gary V. Vaughan      ())_.  gary@ 
{lilith.warpmail.net,gnu.org},address@hidden
Research Scientist   ( '/   http://www.tkd.kicks-ass.net
GNU Hacker           / )=   http://www.gnu.org/software/{libtool,m4}
Technical Author   `(_~)_   http://sources.redhat.com/autobook

libtool--gary--1.0--patch-23.patch
Description: Binary data

PGP.sig
Description: This is a digitally signed message part