Re: cwrapper generates long strings.
* Peter Rosin wrote on Sun, Oct 03, 2010 at 08:28:47PM CEST:
> Den 2010-10-03 09:01 skrev Ralf Wildenhues:
> >> +for i in 25 50 75 100 125 150 175 200 225 250
> >> +do
> >> + PATH="$PATH$PATH_SEPARATOR/somewhere-that-eksists-not"
> > Does $LIBTOOL or the autotest machinery ever intentionally try to run
> > commands that won't exist in $PATH within this shell?
>
> I don't think so, and it is a really hard question to answer too.
Indeed.
I'm kinda wondering whether we should at least delimit our use of
nonexistent files and directories to a common subtree, like below
/nonexistent or so. I realize we're getting near bike shedding
issues though, so how about we cross that bridge when we get to it,
and leave your patch as is for now.
> > If so, then we
> > might get the odd bug report from security-hardened distributions that
> > complain about read or execute accessses to non-allowed parts of the
> > file system.
>
> Using "$PATH$PATH_SEPARATOR$PATH" seemed like a much quicker way
> to make the length explode so I didn't like that.
OK.
> > This length doesn't yet trigger the compiler string literal length
> > limit; not sure whether it should?
>
> That's not reliable anyway as most compilers support so long strings
> that it's hard to tickle it.
FWIW, that is not necessary. It would be sufficient if it were tickled
with the one compiler in question.
> On a tangent, another bug is that linking
> doesn't fail (libtool returns zero) when building the cwrapper fails.
> I think that's a separate issue from this one, which is why I haven't
> mixed them up previously.
OK, good.
> Another nit in that area is that there are
> multiple levels of "$opt_dry_run || {" which seems superfluous, but
> that might be intentional in order to keep the code copy-paste-safe?
Not sure. I don't have much sympathy for copy-paste-safety, because
I dislike the kludge that copy-paste is. Functions should be used
instead. So yes, I guess a cleanup is a good idea, after ensuring that
the code really works fine with the outer opt_dry_run enclosing.
> > Do we have to cater to the case where the environment is very large
> > already? I'm not sure, we might want to deal with it when crossing
> > that bridge only, if it's hard to know off-hand.
>
> Are your three above paragraphs nits and part of what I need to
> address, or just notes for the future?
They started out as nits, but I guess they've become notes by now.
So go ahead with your patch, please.
> >> +# try to make sure the test is relevant
> >> +AT_CHECK([grep ' *fputs' $objdir/lt-usea.c > /dev/null])
> >
> > Rather than redirecting stdout, put [ignore] in the third argument.
>
> Certainly possible, but I didn't think anyone would be interested in a
> couple of hundred lines of boilerplate in the log. I don't really care
> though, so if you still think [ignore] is a good idea, then ok.
Ah yes. Autoconf 2.64 provides stdout-nolog for this, but I guess you
can keep the code the way it is, for compatibility.
Thanks,
Ralf
Re: cwrapper generates long strings.
Den 2010-10-04 07:00 skrev Ralf Wildenhues:
> * Peter Rosin wrote on Sun, Oct 03, 2010 at 08:28:47PM CEST:
>> Den 2010-10-03 09:01 skrev Ralf Wildenhues:
+for i in 25 50 75 100 125 150 175 200 225 250
+do
+ PATH="$PATH$PATH_SEPARATOR/somewhere-that-eksists-not"
>
>>> Does $LIBTOOL or the autotest machinery ever intentionally try to run
>>> commands that won't exist in $PATH within this shell?
>>
>> I don't think so, and it is a really hard question to answer too.
>
> Indeed.
>
> I'm kinda wondering whether we should at least delimit our use of
> nonexistent files and directories to a common subtree, like below
> /nonexistent or so. I realize we're getting near bike shedding
> issues though, so how about we cross that bridge when we get to it,
> and leave your patch as is for now.
>
>>> If so, then we
>>> might get the odd bug report from security-hardened distributions that
>>> complain about read or execute accessses to non-allowed parts of the
>>> file system.
>>
>> Using "$PATH$PATH_SEPARATOR$PATH" seemed like a much quicker way
>> to make the length explode so I didn't like that.
>
> OK.
I wrote a loop appending the first PATH component until the length
exceeds the limit. The longest possible PATH should be 499 characters
this way, which seems OK to me, and it should have no "wild" components.
>>> This length doesn't yet trigger the compiler string literal length
>>> limit; not sure whether it should?
>>
>> That's not reliable anyway as most compilers support so long strings
>> that it's hard to tickle it.
>
> FWIW, that is not necessary. It would be sufficient if it were tickled
> with the one compiler in question.
Since the compiler limit in this case is as large as 2048 (whoooa), which
is about the same as you quoted for grep, I decided to not do that.
>> On a tangent, another bug is that linking
>> doesn't fail (libtool returns zero) when building the cwrapper fails.
>> I think that's a separate issue from this one, which is why I haven't
>> mixed them up previously.
>
> OK, good.
>
>> Another nit in that area is that there are
>> multiple levels of "$opt_dry_run || {" which seems superfluous, but
>> that might be intentional in order to keep the code copy-paste-safe?
>
> Not sure. I don't have much sympathy for copy-paste-safety, because
> I dislike the kludge that copy-paste is. Functions should be used
> instead. So yes, I guess a cleanup is a good idea, after ensuring that
> the code really works fine with the outer opt_dry_run enclosing.
Ok.
>>> Do we have to cater to the case where the environment is very large
>>> already? I'm not sure, we might want to deal with it when crossing
>>> that bridge only, if it's hard to know off-hand.
>>
>> Are your three above paragraphs nits and part of what I need to
>> address, or just notes for the future?
>
> They started out as nits, but I guess they've become notes by now.
> So go ahead with your patch, please.
Holding it up for one more iteration.
+# try to make sure the test is relevant
+AT_CHECK([grep ' *fputs' $objdir/lt-usea.c > /dev/null])
>>>
>>> Rather than redirecting stdout, put [ignore] in the third argument.
>>
>> Certainly possible, but I didn't think anyone would be interested in a
>> couple of hundred lines of boilerplate in the log. I don't really care
>> though, so if you still think [ignore] is a good idea, then ok.
>
> Ah yes. Autoconf 2.64 provides stdout-nolog for this, but I guess you
> can keep the code the way it is, for compatibility.
Ok.
Cheers,
Peter
>From 0cdd5a00c698cc47e4c7d1af377b7fb5090a417b Mon Sep 17 00:00:00 2001
From: Peter Rosin
Date: Mon, 4 Oct 2010 09:40:45 +0200
Subject: [PATCH] cwrapper: split long lines when dumping the wrapper script.
* libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src): If
the wrapper script contains long lines, split them for
readability and to conform with C standards.
* tests/cwrapper.at (cwrapper string length): New test, making
sure we don't regress.
Signed-off-by: Peter Rosin
---
ChangeLog |9 ++
libltdl/config/ltmain.m4sh | 12 ++--
tests/cwrapper.at | 61
3 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a7aa489..e45bfe8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2010-10-04 Peter Rosin
+
+ cwrapper: split long lines when dumping the wrapper script.
+ * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src): If
+ the wrapper script contains long lines, split them for
+ readability and to conform with C standards.
+ * tests/cwrapper.at (cwrapper string length): New test, making
+ sure we don't regress.
+
2010-09-27 Peter Rosin
tests: check if sys_lib_search_path_spec works on MSVC.
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index 0418007..1078e75 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/conf
Re: [PATCH] msvc: handle symbols from different files independently.
Den 2010-10-04 13:25 skrev Peter Rosin: > +AT_CHECK([eval "cat dumpbin-output | $global_symbol_pipe"], [], [stdout]) Oops, forgot one nit, I'm going with +AT_CHECK([< dumpbin-output eval "$global_symbol_pipe"], [], [stdout]) instead. Cheers, Peter
Re: [PATCH] msvc: handle symbols from different files independently.
Den 2010-10-02 08:40 skrev Ralf Wildenhues:
> Hi Peter,
>
> * Peter Rosin wrote on Fri, Oct 01, 2010 at 01:38:42PM CEST:
>> Anyway, is this test case good enough? Should I find a better
>> way to skip on non-dumpbin runs? How?
>
> Skip if $NM != $DUMPBIN? But then you'd need to ensure DUMPBIN is set.
It's a pest that you can have:
DUMPBIN="link -dump"
NM="dumpbin -symbols"
or
DUMPBIN=dumpbin
NM="link -dump -symbols"
I.e. link -dump is a synonym for dumpbin.
> How for a slight improvement at least something that's bound to remain
> even if the symbol pipe is rewritten in sed or another language, e.g.,
> looking whether ' UNDEF ' or 'Section length' is present?
I redid it in the same manner the configure test is doing it instead.
> Please consider moving testsuite additions which are clearly system-
> specific to other tests which are clearly system-specific, and grouping
> those testing the same systems, or family of systems (such as w32).
> And then using one AT_BANNER for a set of test groups.
I moved it before deplibs-mingw.at and changed the banner to "Windows
tests.", quietly fixing the cosmetic bug that deplibs-mingw.at isn't a
darwin test to begin with.
> OK with nits below.
One more iteration since I'm not sure if redoing the configure test
is ok.
Cheers,
Peter
>From 2975fc3e74374fcf1c949766f65db5670d8c61a9 Mon Sep 17 00:00:00 2001
From: Peter Rosin
Date: Mon, 4 Oct 2010 13:13:01 +0200
Subject: [PATCH] msvc: handle symbols from different files independently.
* libltdl/m4/libtool.m4 (_LT_CMD_GLOBAL_SYMBOLS)
: Make all sections
viable for symbol extraction again when the symbols from a new
file starts. Fixes tests/tagdemo-make.test for MSVC 10.
* tests/dumpbin-symbols.at: New test, making sure we don't
regress.
* Makefile.am (TESTSUITE_AT): Update.
Signed-off-by: Peter Rosin
---
ChangeLog| 11 +
Makefile.am |1 +
libltdl/m4/libtool.m4|1 +
tests/dumpbin-symbols.at | 111 ++
4 files changed, 124 insertions(+), 0 deletions(-)
create mode 100644 tests/dumpbin-symbols.at
diff --git a/ChangeLog b/ChangeLog
index a7aa489..4477414 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2010-10-04 Peter Rosin
+
+ msvc: handle symbols from different files independently.
+ * libltdl/m4/libtool.m4 (_LT_CMD_GLOBAL_SYMBOLS)
+ : Make all sections
+ viable for symbol extraction again when the symbols from a new
+ file starts. Fixes tests/tagdemo-make.test for MSVC 10.
+ * tests/dumpbin-symbols.at: New test, making sure we don't
+ regress.
+ * Makefile.am (TESTSUITE_AT): Update.
+
2010-09-27 Peter Rosin
tests: check if sys_lib_search_path_spec works on MSVC.
diff --git a/Makefile.am b/Makefile.am
index 6e29a29..6c23a5e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -503,6 +503,7 @@ TESTSUITE_AT= tests/testsuite.at \
tests/cmdline_wrap.at \
tests/pic_flag.at \
tests/darwin.at \
+ tests/dumpbin-symbols.at \
tests/deplibs-mingw.at \
tests/sysroot.at
diff --git a/libltdl/m4/libtool.m4 b/libltdl/m4/libtool.m4
index fd732d0..967dd38 100644
--- a/libltdl/m4/libtool.m4
+++ b/libltdl/m4/libtool.m4
@@ -3645,6 +3645,7 @@ for ac_symprfx in "" "_"; do
# which start with @ or ?.
lt_cv_sys_global_symbol_pipe="$AWK ['"\
" {last_section=section; section=\$ 3};"\
+" /^COFF SYMBOL TABLE/{for(i in hide) delete hide[i]};"\
" /Section length .*#relocs.*(pick any)/{hide[last_section]=1};"\
" \$ 0!~/External *\|/{next};"\
" / 0+ UNDEF /{next}; / UNDEF \([^|]\)*()/{next};"\
diff --git a/tests/dumpbin-symbols.at b/tests/dumpbin-symbols.at
new file mode 100644
index 000..e184306
--- /dev/null
+++ b/tests/dumpbin-symbols.at
@@ -0,0 +1,111 @@
+# dumpbin-symbols.at -- libtool "dumpbin -symbols" support-*- Autotest -*-
+
+# Copyright (C) 2010 Free Software Foundation, Inc.
+#
+# This file is part of GNU Libtool.
+#
+# GNU Libtool 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 of
+# the License, or (at your option) any later version.
+#
+# GNU Libtool 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 GNU Libtool; see the file COPYING. If not, a copy
+# can be downloaded from http://www.gnu.org/licenses/gpl.html,
+# or obtained by writing to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+
+AT_BANNER([Windows tests.])
+AT_SETUP([dumpbin -symbols section hiding]
Re: [PATCH] Add test case for 69e77671 (cwrapper PATH manipulation order)
Hi Charles,
* libt...@cwilson.fastmail.fm wrote on Sun, Oct 03, 2010 at 11:15:17PM CEST:
> * tests/cwrapper.at: Add new test 'cwrapper and installed shared
> libraries.'
OK with nits addressed. You may want to use a ChangeLog and/or --author
entry that suitably documents the main author of the patch.
Thanks,
Ralf
> --- a/tests/cwrapper.at
> +++ b/tests/cwrapper.at
> @@ -134,3 +134,73 @@ done
>
> AT_CLEANUP
>
> +
> +AT_SETUP([cwrapper and installed shared libraries])
> +AT_KEYWORDS([libtool])
> +
> +# make sure existing libtool is configured for shared libraries
> +AT_CHECK([$LIBTOOL --features | grep 'disable shared libraries' && exit 77],
> + [1], [ignore])
Let's be positive:
AT_CHECK([$LIBTOOL --features | grep 'enable shared libraries' || exit 77],
[], [ignore])
> +# FIXME with shared_fails for this test on AIX
> +# copy from link-order2.at:
> +eval `$LIBTOOL --config | $EGREP '^(shlibpath_var|allow_undefined_flag)='`
> +
> +undefined_setting=-no-undefined
> +shared_fails=no
> +case $host_os,$LDFLAGS,$allow_undefined_flag in
> +aix*,*-brtl*,*) ;;
> +aix*) shared_fails=yes ;;
> +darwin*,*,*-flat_namespace*) undefined_setting= ;;
> +darwin*,*,*) shared_fails=yes ;;
> +esac
> +# end of copy from link-order2.at
> +
> +LDFLAGS="$LDFLAGS $undefined_setting"
Let's replace these 15 lines with
LDFLAGS="$LDFLAGS -no-undefined"
because I don't see how this test should need to depend on them at all.
Since the test is explicitly about the cwrapper, I'd probably prefer to
skip it on systems that have a problem with the test but don't use the
cwrapper anyway. If you agree then I can just test this later and we
keep the simple code for now.
> +inst=`pwd`/inst
> +libdir=$inst/lib
> +bindir=$inst/bin
> +mkdir $inst $libdir $bindir
> +
> +# we must build foo library in a separate directory to avoid on some
> +# platforms shared library to be loaded from "current" directory
I have trouble parsing this sentence, and it is lacking punctuation and
capitalization. How about this?
# Build the library in a separate directory to avoid the special case
# of loading from the current directory.
> +mkdir foo
> +cd foo
> +# build and install "old" library version
> +AT_DATA([a.c], [[
> +int liba_ver (void) { return(1); }
Please no parens for argument to return, that is not a function.
Three instances.
> +]])
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c a.c
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 -o liba.la
> -rpath $libdir a.lo
> +$LIBTOOL --mode=install $lt_INSTALL liba.la $libdir
Is there any, however unlikely, chance that these $LIBTOOL commands fail
on some system or setup? Then they should be wrapped in
AT_CHECK([...], [], [ignore], [ignore])
> +# build a new library version
> +AT_DATA([a.c], [[
> +int liba_ver (void) { return(2); }
> +]])
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c a.c
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 -o liba.la
> -rpath $libdir a.lo
Ditto.
> +cd ..
> +
> +# build and run test application
> +AT_DATA([m.c], [[
> +extern int liba_ver (void);
> +int main (void)
> +{
> + int r = (liba_ver () == 2) ? 0 : 1;
> + return(r);
> +}
> +]])
> +
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c m.c
> +
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT m.$OBJEXT foo/liba.la
> +LT_AT_EXEC_CHECK([./m1], [0], [ignore], [ignore], [])
> +
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m2$EXEEXT m.$OBJEXT foo/liba.la
> -L$inst/lib
> +LT_AT_EXEC_CHECK([./m2], [0], [ignore], [ignore], [])
> +
> +AT_CLEANUP
Re: cwrapper generates long strings.
* Peter Rosin wrote on Mon, Oct 04, 2010 at 10:02:15AM CEST: > > I wrote a loop appending the first PATH component until the length > exceeds the limit. The longest possible PATH should be 499 characters > this way, which seems OK to me, and it should have no "wild" components. Cool. > Den 2010-10-04 07:00 skrev Ralf Wildenhues: > > * Peter Rosin wrote on Sun, Oct 03, 2010 at 08:28:47PM CEST: > >>> This length doesn't yet trigger the compiler string literal length > >>> limit; not sure whether it should? > >> > >> That's not reliable anyway as most compilers support so long strings > >> that it's hard to tickle it. > > > > FWIW, that is not necessary. It would be sufficient if it were tickled > > with the one compiler in question. > > Since the compiler limit in this case is as large as 2048 (whoooa), which > is about the same as you quoted for grep, I decided to not do that. Good point. The updated patch is fine. Thanks, Ralf > Subject: [PATCH] cwrapper: split long lines when dumping the wrapper script. > > * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src): If > the wrapper script contains long lines, split them for > readability and to conform with C standards. > * tests/cwrapper.at (cwrapper string length): New test, making > sure we don't regress.
Re: [PATCH] msvc: handle symbols from different files independently.
Hi Peter, * Peter Rosin wrote on Mon, Oct 04, 2010 at 01:25:42PM CEST: > Den 2010-10-02 08:40 skrev Ralf Wildenhues: > > Skip if $NM != $DUMPBIN? But then you'd need to ensure DUMPBIN is set. > > It's a pest that you can have: > > DUMPBIN="link -dump" > NM="dumpbin -symbols" > > or > > DUMPBIN=dumpbin > NM="link -dump -symbols" > > I.e. link -dump is a synonym for dumpbin. > > > How for a slight improvement at least something that's bound to remain > > even if the symbol pipe is rewritten in sed or another language, e.g., > > looking whether ' UNDEF ' or 'Section length' is present? > > I redid it in the same manner the configure test is doing it instead. Definitely better, also than my suggestion above; thanks. > > Please consider moving testsuite additions which are clearly system- > > specific to other tests which are clearly system-specific, and grouping > > those testing the same systems, or family of systems (such as w32). > > And then using one AT_BANNER for a set of test groups. > > I moved it before deplibs-mingw.at and changed the banner to "Windows > tests.", quietly fixing the cosmetic bug that deplibs-mingw.at isn't a > darwin test to begin with. Ah, good. > > OK with nits below. > > One more iteration since I'm not sure if redoing the configure test > is ok. Go! Thanks, Ralf > Subject: [PATCH] msvc: handle symbols from different files independently. > > * libltdl/m4/libtool.m4 (_LT_CMD_GLOBAL_SYMBOLS) > : Make all sections > viable for symbol extraction again when the symbols from a new > file starts. Fixes tests/tagdemo-make.test for MSVC 10. > * tests/dumpbin-symbols.at: New test, making sure we don't > regress. > * Makefile.am (TESTSUITE_AT): Update.
Re: [PATCH] msvc: handle symbols from different files independently.
Den 2010-10-04 20:07 skrev Ralf Wildenhues: > Go! Pushed, thanks for reviewing! Cheers, Peter
Re: cwrapper generates long strings.
Den 2010-10-04 19:58 skrev Ralf Wildenhues: > The updated patch is fine. Pushed, and again thanks for reviewing! Cheers, Peter
