Re: cwrapper generates long strings.

[Peter Rosin]

Den 2010-10-02 08:32 skrev Ralf Wildenhues:
> Hi Peter,
> 
> * Peter Rosin wrote on Fri, Oct 01, 2010 at 11:22:54PM CEST:
>> 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.
> 
> OK with me with nits addressed.  I see this as a fairly straightforward
> way to work around the issue; we can still think about following Chuck's
> proposal in due course even with this in place.
> 
> Nit 1: testsuite exposure.  Should be fairly straightforward to set
>   PATH=$PATH$PATH_SEPARATOR$PATH
> 
> a couple of times until long enough (but not too long so that
> environment plus command line length already go over the limit), then
> build a library and a program linked against it, covering the path that
> failed for you, no?

Should do it, but I can't fix that this weekend.  I guess I'll get to it
sometime next week.

>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh
>> @@ -4268,9 +4268,14 @@ void lt_dump_script (FILE* f)
>>  {
>>  EOF
>>  func_emit_wrapper yes |
>> -  $SED -e 's/\([\\"]\)/\\\1/g' \
>> -   -e 's/^/  fputs ("/' -e 's/$/\\n", f);/'
>> -
>> +  $SED -n -e '
>> +s/^\(.\{79\}\)\(..*\)/\1\n\2/
> 
> \n is portable only in the regex part, but not in the replacement part.
> For that you need backslash then literal newline.

Ok, so replacing with

s/^\(.\{79\}\)\(..*\)/\1\
\2/

is portable?  Easy enough, I'll fold it in with nit 1 and repost
before I push (if someone beats me to it and writes the test, feel
free to snarf the sed program and push if you do).

>> +h
>> +s/\([\\"]\)/\\\1/g
>> +s/$/\\n/
>> +s/\([^\n]*\).*/  fputs ("\1", f);/p
> 
> Why not above, right after h, do s/\n.*// and then simplify this s
> command?
> 
>> +g
>> +D'

Because then we no longer know if the C-string "\n" at the end of the
line (added by the 's/$/\\n/' statement) should be inserted or not.
The trick is to add it and then cut it out if the string was too long
(contains a literal newline from the s command).  We can only have
the "\n" for the final chunk, otherwise the output will be riddled
with too many newlines.

Cheers,
Peter

Re: cwrapper generates long strings.

[Ralf Wildenhues]

[ dropped bug-libtool ]

* Peter Rosin wrote on Sat, Oct 02, 2010 at 01:42:02PM CEST:
> Den 2010-10-02 08:32 skrev Ralf Wildenhues:
> >> +$SED -n -e '
> >> +s/^\(.\{79\}\)\(..*\)/\1\n\2/
> > 
> > \n is portable only in the regex part, but not in the replacement part.
> > For that you need backslash then literal newline.
> 
> Ok, so replacing with
> 
> s/^\(.\{79\}\)\(..*\)/\1\
> \2/
> 
> is portable?

Yes.  Well, we might get the odd report about the Cygwin non-binmode
mount where the CR NL messes up things, but otherwise, it should work.

> >> +h
> >> +s/\([\\"]\)/\\\1/g
> >> +s/$/\\n/
> >> +s/\([^\n]*\).*/  fputs ("\1", f);/p
> > 
> > Why not above, right after h, do s/\n.*// and then simplify this s
> > command?
> > 
> >> +g
> >> +D'
> 
> Because then we no longer know if the C-string "\n" at the end of the
> line (added by the 's/$/\\n/' statement) should be inserted or not.
> The trick is to add it and then cut it out if the string was too long
> (contains a literal newline from the s command).  We can only have
> the "\n" for the final chunk, otherwise the output will be riddled
> with too many newlines.

Ah yes, thinko of mine, thanks for explaining.

Cheers,
Ralf

Re: cwrapper generates long strings.

[Peter Rosin]

Den 2010-10-02 13:53 skrev Ralf Wildenhues:
> * Peter Rosin wrote on Sat, Oct 02, 2010 at 01:42:02PM CEST:
>> Den 2010-10-02 08:32 skrev Ralf Wildenhues:
 +$SED -n -e '
 +s/^\(.\{79\}\)\(..*\)/\1\n\2/
>>>
>>> \n is portable only in the regex part, but not in the replacement part.
>>> For that you need backslash then literal newline.
>>
>> Ok, so replacing with
>>
>> s/^\(.\{79\}\)\(..*\)/\1\
>> \2/
>>
>> is portable?
> 
> Yes.  Well, we might get the odd report about the Cygwin non-binmode
> mount where the CR NL messes up things, but otherwise, it should work.

If you are on a text mount, it should fixup CR NL issues.  If you have
CR NL files on a binary mount you deserve to suffer.  So, a non-issue.

I found unexpectedly found time early, so here's the updated patch
with a test case.

I used 250 at the limit in the test as the 79 characters from the string
splitter in ltmain might be doubled due to C string escapes and then I
added some extra margin for quotes and the ", f);" trailer.  Still below
255 which might be an arbitrary limit in some grep implementations.

Ok to push?

Cheers,
Peter

>From 5e1b9944049b3956841f2af7e473f3e2504205d1 Mon Sep 17 00:00:00 2001
From: Peter Rosin 
Date: Sat, 2 Oct 2010 23:19:42 +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  |   50 
 3 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a7aa489..db3585a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2010-10-02  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/config/ltmain.m4sh
@@ -4268,9 +4268,15 @@ void lt_dump_script (FILE* f)
 {
 EOF
func_emit_wrapper yes |
-  $SED -e 's/\([\\"]\)/\\\1/g' \
-  -e 's/^/  fputs ("/' -e 's/$/\\n", f);/'
-
+ $SED -n -e '
+s/^\(.\{79\}\)\(..*\)/\1\
+\2/
+h
+s/\([\\"]\)/\\\1/g
+s/$/\\n/
+s/\([^\n]*\).*/  fputs ("\1", f);/p
+g
+D'
 cat <<"EOF"
 }
 EOF
diff --git a/tests/cwrapper.at b/tests/cwrapper.at
index 248c0c0..3c1b054 100644
--- a/tests/cwrapper.at
+++ b/tests/cwrapper.at
@@ -134,3 +134,53 @@ done
 
 AT_CLEANUP
 
+
+AT_SETUP([cwrapper string length])
+
+eval "`$LIBTOOL --config | $EGREP '^(objdir)='`"
+
+AT_DATA([liba.c],
+[[int liba_func1 (int arg)
+{
+  return arg + 1;
+}
+]])
+AT_DATA([usea.c],
+[[extern int liba_func1 (int arg);
+int main (void)
+{
+  int a = 2;
+  int b = liba_func1 (a);
+  if (b == 3) return 0;
+  return 1;
+}
+]])
+
+AT_CHECK([$LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c liba.c],
+[], [ignore], [ignore])
+AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -no-undefined ]dnl
+[-o liba.la -rpath /foo liba.lo],
+[], [ignore], [ignore])
+AT_CHECK([$CC $CPPFLAGS $CFLAGS -c usea.c],
+[], [ignore], [ignore])
+
+# make sure PATH is at least 250 chars, should force line breaks in lt-usea.c
+for i in 25 50 75 100 125 150 175 200 225 250
+do
+  PATH="$PATH$PATH_SEPARATOR/somewhere-that-eksists-not"
+done
+export PATH
+
+AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -no-fast-install ]dnl
+[-o usea$EXEEXT usea.$OBJEXT liba.la],
+[], [ignore], [ignore])
+
+# skip if no cwrapper is generated
+AT_CHECK([test -f $objdir/lt-usea.c || exit 77])
+
+# try to make sure the test is relevant
+AT_CHECK([grep ' *fputs' $objdir/lt-usea.c > /dev/null])
+# check for no overly long fputs
+AT_CHECK([grep ' *fputs.\{250\}' $objdir/lt-usea.c], [1])
+
+AT_CLEANUP
-- 
1.7.1