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
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: 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: 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-03 09:01 skrev Ralf Wildenhues: >> 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. > > You can assume close to 2K as minimum limit for grep. > (Hmm, wonder why we didn't ever write down the value in autoconf.texi) > > The patch is OK with nits addressed. > >> --- 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" > > Intended typo 'exists'? ;-) Yes :-) > 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. > 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. > 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. 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. 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? > 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? >> +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]) > > 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. >> +# check for no overly long fputs >> +AT_CHECK([grep ' *fputs.\{250\}' $objdir/lt-usea.c], [1]) >> + >> +AT_CLEANUP Cheers, Peter
Re: cwrapper generates long strings.
Hi Peter, * Peter Rosin wrote on Sat, Oct 02, 2010 at 11:33:02PM CEST: > Den 2010-10-02 13:53 skrev Ralf Wildenhues: > > 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. Cool. Thanks. > 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. You can assume close to 2K as minimum limit for grep. (Hmm, wonder why we didn't ever write down the value in autoconf.texi) The patch is OK with nits addressed. > --- 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" Intended typo 'exists'? ;-) Does $LIBTOOL or the autotest machinery ever intentionally try to run commands that won't exist in $PATH within this shell? 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. This length doesn't yet trigger the compiler string literal length limit; not sure whether it should? 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. > +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]) Rather than redirecting stdout, put [ignore] in the third argument. > +# check for no overly long fputs > +AT_CHECK([grep ' *fputs.\{250\}' $objdir/lt-usea.c], [1]) > + > +AT_CLEANUP Cheers, Ralf
Re: cwrapper generates long strings.
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
Re: cwrapper generates long strings.
[ 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.
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.
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? Thanks, Ralf > --- 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. > +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' > cat <<"EOF" > } > EOF
Re: cwrapper generates long strings.
Hi Chuck, Den 2010-10-01 16:03 skrev Charles Wilson: > On 10/1/2010 7:28 AM, Peter Rosin wrote: >> I.e. I have this on line 865 in lt-main.c: >> >> fputs ("relink_command=\"(cd >> /c/cygwin/home/peda/libtool/git/msvc/tests/testsuite.dir/112; >> PATH=\\\"/LOADS:/OF:/ENTRIES\\\"; export PATH; >> PATH=\\\"/LOADS:/OF:/ENTRIES\\\"; export PATH; >> /c/cygwin/home/peda/automake/lib/compile cl -MD -Zi -EHsc -o @OUTPUT@ >> .libs/main-static.obj sub2/.libs/a.lib )\"\n", f); >> >> In my case the string is 3400+ characters which is too much for MSVC 6, >> but this appears to not be really compiler specific, and I can easily >> imagine other compilers with other arbitrary (and possibly standardized) >> limits. > > c99 compilers must support const char* arrays of at least 4096. Most > compilers actually support much larger ones. > > c89 requires only support for up to 509. See: > http://lists.gnu.org/archive/html/libtool-patches/2008-04/msg00161.html > http://lists.gnu.org/archive/html/libtool-patches/2009-01/msg4.html I vaguely remember that, and rereading reveals that noone thought about one individual line being too long (not publicly anyway). >> One thing that could be done is to only have the PATH once, but that is >> not a real fix. > > It's also not general. One is explicitly setting PATH, the other is > setting $shlibpath_var (which happens to be PATH on win32). > >> Should we worry about my insane case? > > I don't think it is a high priority. What we could do is implement a > "line emitter" in ltmain.m4sh, for use by func_emit_cwrapper_exe. Right > now, it takes the output of func_emit_wrapper and puts "fputs(\"" and > "\n\"" around each line. > > Instead, each line could be fed to a line-emitter function that chops > the line into segments of <= 500 chars. > > But...that's a lot of effort for very little gain. Ok, I wouldn't say a lot of effort, and it was a bit of fun. > I think a better approach would be to start asking, why do we still need > the cwrapper to contain and emit the shell wrapper code in the first > place? Maybe instead, we should just remove --lt-dump-script and all > related code. > > It was originally present because the cwrapper would emit the shwrapper, > and then exec it. But now, the cwrapper exec's the real program > directly, so this functionality is no longer needed. It hasn't been > needed for years, but I didn't want to make too many changes at once so > I left it there even after cwrapper was changed to direct exec the real > program. But...I always intended to get rid of it. Perhaps now is the > time. Right, but I had already sent the first version of the line splitter when I read this. Here's an improved version as a proper git patch (why is it so hard to keep your fingers away from small scriptlets?). Now, someone needs to decide which way to go, and provide a --lt-dump-script removal patch if that is decided. Hardish testing outside libtool, and limited testing with the actual patch, but at least stresstest.at is happy with my looong PATH from the original post, and the lt-main.c code looks good too. Cheers, Peter >From 9c30540472d66c0caf56bdc90a7bf7152ad771d4 Mon Sep 17 00:00:00 2001 From: Peter Rosin Date: Fri, 1 Oct 2010 22:55:55 +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. Signed-off-by: Peter Rosin --- ChangeLog |7 +++ libltdl/config/ltmain.m4sh | 11 --- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index a7aa489..515c23a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2010-10-01 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. + 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..6eb13f0 100644 --- 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/ +h +s/\([\\"]\)/\\\1/g +s/$/\\n/ +s/\([^\n]*\).*/ fputs ("\1", f);/p +g +D' cat <<"EOF" } EOF -- 1.7.1