Re: [patch] allow --with-pic to accept package names

[Ollie Wild]

On Wed, Nov 10, 2010 at 3:41 PM, Ralf Wildenhues  wrote:
>
> Thanks.  The patch looks good to me, with only trivial nits:
> the testsuite addition should be sorted under the 'libtool script
> generation' banner rather than with the sysroot tests, and I don't
> think that it is necessary to skip the test if static libraries are
> disabled: your --disable-shared will (should!) lead to static libs
> being enabled everywhere.
>
> I'm pushing the patch with the diff below squashed in, hope you
> don't mind.

Sounds good to me.  Thanks, Ralf.

Ollie

Re: Fix cwrapper test failure with --disable-static.

[Ralf Wildenhues]

* Charles Wilson wrote on Wed, Nov 10, 2010 at 11:23:52PM CET:
> On 11/10/2010 4:07 PM, Ralf Wildenhues wrote:
> > * Charles Wilson wrote on Wed, Nov 10, 2010 at 09:46:54PM CET:
> >> Wouldn't a better fix be to change the link command to reference m.lo
> >> instead of m.$OBJEXT ?
> > 
> > That would be an alternative, but it would mean that we (needlessly) use
> > PIC code on systems where non-PIC is turned off by default (or
> > --disable-static was used).
> 
> I thought that in those cases, the .lo file would redirect to the
> regular, non-pic .o but I guess what actually happens is that you get
> neither the .lo NOR the .libs/.o pic file, and you see only the non-pic .o.

I'm not sure I follow.

If you pass --disable-static, then a command to build an .lo file will
build that, plus the .libs/.o PIC file.  There just will not be an .o
file outside of the .libs directory.

We could of course do 'test -f m.$OBJEXT || o=$objdir/m.$OBJEXT'
or something like that, but I didn't really want to make the code
more complicated than necessary.

> > automake-generated code also compiles
> > program sources without libtool, so the change was, to me, the canonical
> > one.
> 
> Meh...only if the target is explicitly *.o   If it's .lo, then
> $(LTCOMPILE) is used, and then libtool generates either or both of the
> .o's itself, as determined by the system defaults and/or
> --enable-{shared,static}.

Sure.  But automake will let the target explicitly be *.$(OBJEXT) for
objects to be linked into programs.

> > Is there a portability issue associated with it?
> 
> I don't think so. It was simply a stylistic question: we're testing
> libtool, so...use libtool. :-)

Sure.  But we are allowed to test it in the same way that it is commonly
used.  ;-)

Cheers,
Ralf

Re: Fix cwrapper test failure with --disable-static.

[Charles Wilson]

On 11/10/2010 4:07 PM, Ralf Wildenhues wrote:
> * Charles Wilson wrote on Wed, Nov 10, 2010 at 09:46:54PM CET:
>> Wouldn't a better fix be to change the link command to reference m.lo
>> instead of m.$OBJEXT ?
> 
> That would be an alternative, but it would mean that we (needlessly) use
> PIC code on systems where non-PIC is turned off by default (or
> --disable-static was used).

I thought that in those cases, the .lo file would redirect to the
regular, non-pic .o but I guess what actually happens is that you get
neither the .lo NOR the .libs/.o pic file, and you see only the non-pic .o.

> automake-generated code also compiles
> program sources without libtool, so the change was, to me, the canonical
> one.

Meh...only if the target is explicitly *.o   If it's .lo, then
$(LTCOMPILE) is used, and then libtool generates either or both of the
.o's itself, as determined by the system defaults and/or
--enable-{shared,static}.

> Is there a portability issue associated with it?

I don't think so. It was simply a stylistic question: we're testing
libtool, so...use libtool. :-)

--
Chuck

Re: [patch] allow --with-pic to accept package names

[Ralf Wildenhues]

Hi Ollie,

* Ollie Wild wrote on Mon, Nov 08, 2010 at 05:09:44AM CET:
> On Sat, Oct 23, 2010 at 2:32 AM, Ralf Wildenhues wrote:
> >
> > This patch looks ok but it uses $pkg and $p which are not in Libtool's
> > name space, and it lacks updates to NEWS, libtool.texi, and the test
> > suite.  Oh yes, the --enable-shared code has similar problems, but a
> > patch shouldn't be held hostage for drive-by bugs.  ;-)
> 
> Here's an updated patch which includes a new test, documentation, and
> fixed variable names.  Please take another look.

> 2010-11-07  Ollie Wild  
> 
>   Modify --with-pic to support per-package configurations.
>   * libltdl/m4/libtool.m4:  Modify --with-pic to accept a list of
>   package names.  Modelled off --enable-shared.
>   * tests/with-pic.at: New test.
>   * Makefile.am (TESTSUITE_AT): Add tests/with-pic.at.
>   * doc/libtool.texi (LT_INIT): Enhance documentation of
>   --with-pic configure flag.
>   * NEWS (New features): Mention that --with-pic now accepts a
>   comma-separated list of package names.

Thanks.  The patch looks good to me, with only trivial nits:
the testsuite addition should be sorted under the 'libtool script
generation' banner rather than with the sysroot tests, and I don't
think that it is necessary to skip the test if static libraries are
disabled: your --disable-shared will (should!) lead to static libs
being enabled everywhere.

I'm pushing the patch with the diff below squashed in, hope you
don't mind.

Cheers,
Ralf

diff --git a/Makefile.am b/Makefile.am
index 53c8e8b..66f38b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -502,6 +502,7 @@ TESTSUITE_AT= tests/testsuite.at \
  tests/ctor.at \
  tests/exceptions.at \
  tests/early-libtool.at \
+ tests/with-pic.at \
  tests/no-executables.at \
  tests/deplibs-ident.at \
  tests/configure-iface.at \
@@ -511,8 +512,7 @@ TESTSUITE_AT= tests/testsuite.at \
  tests/darwin.at \
  tests/dumpbin-symbols.at \
  tests/deplibs-mingw.at \
- tests/sysroot.at \
- tests/with-pic.at
+ tests/sysroot.at
 
 EXTRA_DIST += $(srcdir)/$(TESTSUITE) $(TESTSUITE_AT) 
$(srcdir)/tests/package.m4
 
diff --git a/tests/with-pic.at b/tests/with-pic.at
index a80535b..c01e5d7 100644
--- a/tests/with-pic.at
+++ b/tests/with-pic.at
@@ -26,8 +26,6 @@ eval `$LIBTOOL --config | $EGREP '^(pic_flag|FGREP)='`
 
 AT_CHECK([test "z$pic_flag" != "z" || exit 77])
 AT_CHECK([test "$at_srcdir" != . || exit 77])
-AT_CHECK([$LIBTOOL --features | $FGREP 'enable static libraries' || exit 77],
-[], [ignore], [ignore])
 
 CONFIGURE=$abs_top_srcdir/tests/demo/configure
 : ${MAKE=make}

Re: Fix cwrapper test failure with --disable-static.

[Ralf Wildenhues]

Hi Charles,

* Charles Wilson wrote on Wed, Nov 10, 2010 at 09:46:54PM CET:
> On 11/10/2010 1:29 PM, Ralf Wildenhues wrote:
> >  
> > -AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c m.c],
> > - [], [ignore], [ignore])
> > +AT_CHECK([$CC $CPPFLAGS $CFLAGS -c m.c], [], [ignore], [ignore])
> >  
> >  AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT m.$OBJEXT 
> > foo/liba.la],
> >   [], [ignore], [ignore])
> 
> Wouldn't a better fix be to change the link command to reference m.lo
> instead of m.$OBJEXT ?

That would be an alternative, but it would mean that we (needlessly) use
PIC code on systems where non-PIC is turned off by default (or
--disable-static was used).  automake-generated code also compiles
program sources without libtool, so the change was, to me, the canonical
one.

Is there a portability issue associated with it?

Thanks,
Ralf

Re: Fix cwrapper test failure with --disable-static.

[Charles Wilson]

On 11/10/2010 1:29 PM, Ralf Wildenhues wrote:
>  
> -AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c m.c],
> - [], [ignore], [ignore])
> +AT_CHECK([$CC $CPPFLAGS $CFLAGS -c m.c], [], [ignore], [ignore])
>  
>  AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT m.$OBJEXT 
> foo/liba.la],
>   [], [ignore], [ignore])

Wouldn't a better fix be to change the link command to reference m.lo
instead of m.$OBJEXT ?

--
Chuck

Fix cwrapper test failure with --disable-static.

[Ralf Wildenhues]

shows among others (some of which are NixOS and not Libtool issues)
the following failure:

libtool: compile:  gcc -g -O2 -c m.c  -fPIC -DPIC -o .libs/m.o
./cwrapper.at:255: $LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT 
m.$OBJEXT foo/liba.la
stderr:
gcc: m.o: No such file or directory
stdout:
libtool: link: gcc -g -O2 -o .libs/m1 m.o  foo/.libs/liba.so -Wl,-rpath 
-Wl,/tmp/nix-build-j7dzmn77cw5yzw9l6d48fdqv4q18mjy0-libtool-2.4.1a.drv-0/libtool-2.4.1a/tests/testsuite.dir/057/inst/lib
./cwrapper.at:255: exit code was 1, expected 0
57. cwrapper.at:201: 57. cwrapper and installed shared libraries 
(cwrapper.at:201): FAILED (cwrapper.at:255)


which should be fixed by the patch below, which I'm pushing as obvious.

Thanks,
Ralf

Fix cwrapper test failure with --disable-static.

* tests/cwrapper.at (cwrapper and installed shared libraries):
Compile program source without libtool, so we can be sure a
non-PIC object will be created.

diff --git a/tests/cwrapper.at b/tests/cwrapper.at
index 6e8cf3c..0e5ecb7 100644
--- a/tests/cwrapper.at
+++ b/tests/cwrapper.at
@@ -249,8 +249,7 @@ int main (void)
 }
 ]])
 
-AT_CHECK([$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c m.c],
- [], [ignore], [ignore])
+AT_CHECK([$CC $CPPFLAGS $CFLAGS -c m.c], [], [ignore], [ignore])
 
 AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT m.$OBJEXT 
foo/liba.la],
  [], [ignore], [ignore])