Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-21 Thread Ralf Wildenhues 
* Charles Wilson wrote on Sun, Feb 21, 2010 at 06:34:02AM CET:
 So, open issues, to be addressed if necessary in additional patches:
 1) (func) ... --- '(%s) ..., __func__' ?
 2) chmod in cwrapper.at?

Naah, ignore both.  Thanks.

strerror(errno) can return NULL for unknown errors on some systems.
Not sure this can happen in practice here, wrapping in nonnull () should
be a safe stopgap however.  Patch to this end preapproved.

Thanks,
Ralf

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-21 Thread Charles Wilson 
Ralf Wildenhues wrote:
 * Charles Wilson wrote on Sun, Feb 21, 2010 at 06:34:02AM CET:
 So, open issues, to be addressed if necessary in additional patches:
 1) (func) ... --- '(%s) ..., __func__' ?
 2) chmod in cwrapper.at?
 
 Naah, ignore both.  Thanks.
 
 strerror(errno) can return NULL for unknown errors on some systems.
 Not sure this can happen in practice here, wrapping in nonnull () should
 be a safe stopgap however.  Patch to this end preapproved.

Pushed.

2010-02-21  Charles Wilson  ...

Guard against strerror()==NULL
* libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src:main):
Check return value of strerror() using nonnull().
(func_emit_cwrapperexe_src:find_executable): Ditto.
(func_emit_cwrapperexe_src:chase_symlinks): Ditto.

--
Chuck
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index 043d980..56b7497 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -3192,7 +3192,7 @@ EOF
   /* failed to start process */
   lt_debugprintf (__FILE__, __LINE__,
 		  (main) failed to launch target \%s\: %s\n,
-		  lt_argv_zero, strerror (errno));
+		  lt_argv_zero, nonnull (strerror (errno)));
   return 127;
 }
   return rval;
@@ -3348,7 +3348,7 @@ find_executable (const char *wrapper)
 		  /* empty path: current directory */
 		  if (getcwd (tmp, LT_PATHMAX) == NULL)
 		lt_fatal (__FILE__, __LINE__, getcwd failed: %s,
-  strerror (errno));
+  nonnull (strerror (errno)));
 		  tmp_len = strlen (tmp);
 		  concat_name =
 		XMALLOC (char, tmp_len + 1 + strlen (wrapper) + 1);
@@ -3373,7 +3373,8 @@ find_executable (const char *wrapper)
 }
   /* Relative path | not found in path: prepend cwd */
   if (getcwd (tmp, LT_PATHMAX) == NULL)
-lt_fatal (__FILE__, __LINE__, getcwd failed: %s, strerror (errno));
+lt_fatal (__FILE__, __LINE__, getcwd failed: %s,
+  nonnull (strerror (errno)));
   tmp_len = strlen (tmp);
   concat_name = XMALLOC (char, tmp_len + 1 + strlen (wrapper) + 1);
   memcpy (concat_name, tmp, tmp_len);
@@ -3425,7 +3426,7 @@ chase_symlinks (const char *pathspec)
 	{
 	  lt_fatal (__FILE__, __LINE__,
 		error accessing file \%s\: %s,
-		tmp_pathspec, strerror (errno));
+		tmp_pathspec, nonnull (strerror (errno)));
 	}
 }
   XFREE (tmp_pathspec);

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-20 Thread Ralf Wildenhues 
Hi Charles,

* Charles Wilson wrote on Fri, Feb 19, 2010 at 03:48:40AM CET:
   Enable runtime cwrapper debugging; add tests
   * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src):
   Update comments. Initialize program_name. Eliminate _LENGTH
   variables for string constants. In debug mode, print a
   banner with known content before any other output. Remove
   LTWRAPPER_DEBUGPRINTF macro. Add constants and variables
   to support new --lt-debug option.
   (func_emit_cwrapperexe_src:ltwrapper_debugprintf): Renamed to...
   (func_emit_cwrapperexe_src:lt_debugprintf): this. Only print
   messages if lt_debug != 0. Ensure appearance of messages
   conforms to GCS.
   (func_emit_cwrapperexe_src:lt_fatal): Ditto.
   (func_emit_cwrapperexe_src:lt_error_core): Ditto.
   (func_emit_cwrapperexe_src): Update all callers to lt_fatal.
   Update all users of LTWRAPPER_DEBUGPRINTF (()) to call
   lt_debugprintf () directly.
   (func_emit_cwrapperexe_src:main): Consolidate option parsing.
   Ensure first use of lt_debugprintf occurs after option parsing.
   Add stanza to parse for --lt-debug and set lt_debug variable.
   Use strcmp rather than strncmp, where safe.
   * tests/cwrapper.at: Add new tests for --lt-debug and
   -DLT_DEBUGWRAPPER.

This patch is ok to commit with nits below addressed as you see fit;
no need for further review.

Thanks!
Ralf

 --- a/libltdl/config/ltmain.m4sh
 +++ b/libltdl/config/ltmain.m4sh

 @@ -2855,22 +2851,13 @@ int setenv (const char *, const char *, int);
if (stale) { free ((void *) stale); stale = 0; } \
  } while (0)
  
 -#undef LTWRAPPER_DEBUGPRINTF
 -#if defined LT_DEBUGWRAPPER
 -# define LTWRAPPER_DEBUGPRINTF(args) ltwrapper_debugprintf args
 -static void
 -ltwrapper_debugprintf (const char *fmt, ...)
 -{
 -va_list args;
 -va_start (args, fmt);
 -(void) vfprintf (stderr, fmt, args);
 -va_end (args);
 -}
 +#if defined(LT_DEBUGWRAPPER)
 +static int lt_debug = 1;
  #else
 -# define LTWRAPPER_DEBUGPRINTF(args)
 +static int lt_debug = 0;
  #endif
  
 -const char *program_name = NULL;
 +const char *program_name = wrapper; /* in case xstrdup fails */

libtool wrapper might be clearer.  You decide.

 @@ -2880,7 +2867,8 @@ char *chase_symlinks (const char *pathspec);
  int make_executable (const char *path);
  int check_executable (const char *path);
  char *strendzap (char *str, const char *pat);
 -void lt_fatal (const char *message, ...);
 +void lt_debugprintf (const char *file, int line, const char *fmt, ...);
 +void lt_fatal (const char *file, int line, const char *message, ...);

It'd be nicer to wrap these two in a macro that expands __FILE__ and
__LINE__ automatically, but varargs macros are not yet portable, and
multiple defines are somewhat ugly too,

  #define lt_fatal0 (message) lt_fatal_in (__FILE__, __LINE__, message)
  #define lt_fatal1 (message, arg1) \
  lt_fatal_in (__FILE__, __LINE__, message, arg1)
  #define lt_fatal2 (message, arg1, arg2) \
  lt_fatal_in (__FILE__, __LINE__, message, arg1, arg2)
  ...

so maybe the current code is a good compromise.

 +  if (strcmp (argv[i], ltwrapper_option_prefix) == 0)
 +{
 +  /* however, if there is an option in the LTWRAPPER_OPTION_PREFIX
 + namespace, but it is not one of the ones we know about and
 + have already dealt with, above (inluding dump-script), then
 + report an error. Otherwise, targets might begin to believe
 + they are allowed to use options in the LTWRAPPER_OPTION_PREFIX
 + namespace. The first time any user complains about this, we'll
 + need to make LTWRAPPER_OPTION_PREFIX a configure-time option
 + or a configure.ac-settable value.
 +   */

The problem being that we cannot please this user without providing an
upgrade, or letting her change libtool code.  Maybe warn instead of fail
hard?  I'm really torn here; we suddenly grab in-band namespace without
an easy way to avoid it through some option.

This issue should not be addressed in this patch, it is not new here,
and we first should get consensus on the right way to address it.
(Have we done that elsewhere already?)

 +  lt_fatal (__FILE__, __LINE__,
 + Unrecognized %s option: '%s',
 +ltwrapper_option_prefix, argv[i]);

 + cat EOF
 +  /* first use of lt_debugprintf AFTER parsing options */

Not sure what this comment aims at.  Is this a statement of a nontrivial
fact?

 +  lt_debugprintf (__FILE__, __LINE__, libtool wrapper (GNU 
 $PACKAGE$TIMESTAMP) $VERSION\n);
 +EOF
 + cat EOF
 +  lt_debugprintf (__FILE__, __LINE__, (main) argv[0]: %s\n, argv[0]);
 +  lt_debugprintf (__FILE__, __LINE__, (main) program_name: %s\n, 
 program_name);

actual_cwrapper_path = chase_symlinks (tmp_pathspec);
 -  LTWRAPPER_DEBUGPRINTF (((main) found exe (after symlink chase) at :

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-20 Thread Charles Wilson 
Ralf Wildenhues wrote:
 This patch is ok to commit with nits below addressed as you see fit;
 no need for further review.

Thanks for the review.

 
 --- a/libltdl/config/ltmain.m4sh
 +++ b/libltdl/config/ltmain.m4sh

  
 -const char *program_name = NULL;
 +const char *program_name = wrapper; /* in case xstrdup fails */
 
 libtool wrapper might be clearer.  You decide.

Fixed.

 
 @@ -2880,7 +2867,8 @@ char *chase_symlinks (const char *pathspec);
  int make_executable (const char *path);
  int check_executable (const char *path);
  char *strendzap (char *str, const char *pat);
 -void lt_fatal (const char *message, ...);
 +void lt_debugprintf (const char *file, int line, const char *fmt, ...);
 +void lt_fatal (const char *file, int line, const char *message, ...);
 
 It'd be nicer to wrap these two in a macro that expands __FILE__ and
 __LINE__ automatically, but varargs macros are not yet portable, and
 multiple defines are somewhat ugly too,
 
   #define lt_fatal0 (message) lt_fatal_in (__FILE__, __LINE__, message)
   #define lt_fatal1 (message, arg1) \
   lt_fatal_in (__FILE__, __LINE__, message, arg1)
   #define lt_fatal2 (message, arg1, arg2) \
   lt_fatal_in (__FILE__, __LINE__, message, arg1, arg2)
   ...
 
 so maybe the current code is a good compromise.

Yeah, the original LTWRAPPER_DEBUGPRINTF was also a workaround for the
lack of vararg macros, but it forced you to use (()) which I hated.  I
really didn't want to create N different macros for *both*
lt_debugprintf and lt_fatal (especially lt_debugprintf, which
coincidentally also needs N=2 support macros of this type).  Of course,
N=2 /now/ -- and we could just go with use these macros, and if you
every need N=3... add the macros then.

But it's so UGLY.

So, yeah, the current code is an explicit compromise to work around the
lack of vararg macro support.  I think explicitly invoking __FILE__ and
__LINE__ is marginally less ugly than lt_foo_N() macros...

 +  if (strcmp (argv[i], ltwrapper_option_prefix) == 0)
 +{

[THIS]:

 +  /* however, if there is an option in the LTWRAPPER_OPTION_PREFIX
 + namespace, but it is not one of the ones we know about and
 + have already dealt with, above (inluding dump-script), then
 + report an error. Otherwise, targets might begin to believe
 + they are allowed to use options in the LTWRAPPER_OPTION_PREFIX
 + namespace. The first time any user complains about this, we'll
 + need to make LTWRAPPER_OPTION_PREFIX a configure-time option
 + or a configure.ac-settable value.
 +   */
 
 The problem being that we cannot please this user without providing an
 upgrade, or letting her change libtool code.  Maybe warn instead of fail
 hard?  I'm really torn here; we suddenly grab in-band namespace without
 an easy way to avoid it through some option.

Well, this code has been in the cygwin libtool, in one form or another,
for almost two years.  It's been used to build all of KDE and about 1400
total open source packages (cygwinports project) as well about 2GB
(compressed) source packages in the main cygwin distribution.

If none of THOSE used --lt-* I think we're pretty safe in 'grabbing' it.

 This issue should not be addressed in this patch, it is not new here,
 and we first should get consensus on the right way to address it.
 (Have we done that elsewhere already?)

I believe the consensus was, yeah...the RTTD is to add an ugly configure
option to ltoptions like --libtool-wrapper-option-prefix which defaults
to --lt- (*), and use that value when emitting the cwrapper source and
shwrapper.

http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00066.html
But Eric said
http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00067.html
I don't want to spend any effort coding around it unless someone (and
it won't be me) demonstrates a real need for the extra flexability.

FWIW, that big comment block ([THIS], above) is really just a
placeholder/reminder that we can/should implement some sort of 'easy way
to avoid it' mechanism...

(*) Specifying the value this way requires the '=' form on the configure
line;  --libtool-wrapper-option-prefix=--lt-, but if you 'assume' the
double-dash, then you might be able to get away with
--libtool-wrapper-option-prefix lt-
and no '='

 +  lt_fatal (__FILE__, __LINE__,
 +Unrecognized %s option: '%s',
 +ltwrapper_option_prefix, argv[i]);
 
 +cat EOF
 +  /* first use of lt_debugprintf AFTER parsing options */
 
 Not sure what this comment aims at.  Is this a statement of a nontrivial
 fact?

The important thing is that the very first output, when in debug mode
and no errors have occured (eg. bad arguments, etc), be the banner so
that cwrapper.at is happy.  I'll reword the comment.

/* The GNU banner must be the first non-error debug message */


 +  lt_debugprintf (__FILE__, __LINE__, libtool wrapper

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-18 Thread Charles Wilson 
Squashed together and reposted from:

http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html
and followon:
http://lists.gnu.org/archive/html/libtool-patches/2010-01/msg00047.html

This (combination) was the first of the two patches whose /first/ 72hour
rule invocation was here:
http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg3.html

Starting a new clock.  Test suite history including with the other
patch, since they were tested together.  No regressions attributable to
these changes, on cygwin or mingw.  No test failures at all on linux.

Consolidated changelog:

2010-02-18  Charles Wilson  ...

Enable runtime cwrapper debugging; add tests
* libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src):
Update comments. Initialize program_name. Eliminate _LENGTH
variables for string constants. In debug mode, print a
banner with known content before any other output. Remove
LTWRAPPER_DEBUGPRINTF macro. Add constants and variables
to support new --lt-debug option.
(func_emit_cwrapperexe_src:ltwrapper_debugprintf): Renamed to...
(func_emit_cwrapperexe_src:lt_debugprintf): this. Only print
messages if lt_debug != 0. Ensure appearance of messages
conforms to GCS.
(func_emit_cwrapperexe_src:lt_fatal): Ditto.
(func_emit_cwrapperexe_src:lt_error_core): Ditto.
(func_emit_cwrapperexe_src): Update all callers to lt_fatal.
Update all users of LTWRAPPER_DEBUGPRINTF (()) to call
lt_debugprintf () directly.
(func_emit_cwrapperexe_src:main): Consolidate option parsing.
Ensure first use of lt_debugprintf occurs after option parsing.
Add stanza to parse for --lt-debug and set lt_debug variable.
Use strcmp rather than strncmp, where safe.
* tests/cwrapper.at: Add new tests for --lt-debug and
-DLT_DEBUGWRAPPER.

--
Chuck
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index 80a1ff3..4549023 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -2727,10 +2727,6 @@ func_emit_cwrapperexe_src ()
 
This wrapper executable should never be moved out of the build directory.
If it is, it will not operate correctly.
-
-   Currently, it simply execs the wrapper *script* $SHELL $output,
-   but could eventually absorb all of the scripts functionality and
-   exec $objdir/$outputname directly.
 */
 EOF
 	cat EOF
@@ -2855,22 +2851,13 @@ int setenv (const char *, const char *, int);
   if (stale) { free ((void *) stale); stale = 0; } \
 } while (0)
 
-#undef LTWRAPPER_DEBUGPRINTF
-#if defined LT_DEBUGWRAPPER
-# define LTWRAPPER_DEBUGPRINTF(args) ltwrapper_debugprintf args
-static void
-ltwrapper_debugprintf (const char *fmt, ...)
-{
-va_list args;
-va_start (args, fmt);
-(void) vfprintf (stderr, fmt, args);
-va_end (args);
-}
+#if defined(LT_DEBUGWRAPPER)
+static int lt_debug = 1;
 #else
-# define LTWRAPPER_DEBUGPRINTF(args)
+static int lt_debug = 0;
 #endif
 
-const char *program_name = NULL;
+const char *program_name = wrapper; /* in case xstrdup fails */
 
 void *xmalloc (size_t num);
 char *xstrdup (const char *string);
@@ -2880,7 +2867,8 @@ char *chase_symlinks (const char *pathspec);
 int make_executable (const char *path);
 int check_executable (const char *path);
 char *strendzap (char *str, const char *pat);
-void lt_fatal (const char *message, ...);
+void lt_debugprintf (const char *file, int line, const char *fmt, ...);
+void lt_fatal (const char *file, int line, const char *message, ...);
 void lt_setenv (const char *name, const char *value);
 char *lt_extend_str (const char *orig_value, const char *add, int to_end);
 void lt_update_exe_path (const char *name, const char *value);
@@ -2932,12 +2920,10 @@ EOF
 	cat EOF
 
 #define LTWRAPPER_OPTION_PREFIX --lt-
-#define LTWRAPPER_OPTION_PREFIX_LENGTH  5
 
-static const size_t opt_prefix_len = LTWRAPPER_OPTION_PREFIX_LENGTH;
 static const char *ltwrapper_option_prefix = LTWRAPPER_OPTION_PREFIX;
-
 static const char *dumpscript_opt   = LTWRAPPER_OPTION_PREFIX dump-script;
+static const char *debug_opt= LTWRAPPER_OPTION_PREFIX debug;
 
 int
 main (int argc, char *argv[])
@@ -2954,10 +2940,13 @@ main (int argc, char *argv[])
   int i;
 
   program_name = (char *) xstrdup (base_name (argv[0]));
-  LTWRAPPER_DEBUGPRINTF (((main) argv[0]  : %s\n, argv[0]));
-  LTWRAPPER_DEBUGPRINTF (((main) program_name : %s\n, program_name));
+  newargz = XMALLOC (char *, argc + 1);
 
-  /* very simple arg parsing; don't want to rely on getopt */
+  /* very simple arg parsing; don't want to rely on getopt
+   * also, copy all non cwrapper options to newargz, except
+   * argz[0], which is handled differently
+   */
+  newargc=0;
   for (i = 1; i  argc; i++)
 {
   if (strcmp (argv[i], dumpscript_opt) == 0)
@@ -2974,18 +2963,51 @@ EOF
 	  lt_dump_script (stdout);
 	  return 0;
 	}
+  if

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-02-14 Thread Charles Wilson 
Bob Friesenhahn wrote:
 Within the bounds of technical recommendations/comments made already (by
 Ralf and perhaps others), I recommend that you commit your log-jammed
 Cygwin patches according to the 72 hour rule.  Otherwise it is unlikely
 that there will be any forward progress.  If anything becomes broken for
 some other platform, it is certain to be fixed.

I'll take that as permission only to start the 72 hour clock for the two
patches I have most recently pinged.
http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg0.html
http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg1.html

The others (and there are about 11 in total) I will repost individually,
and revisit them in sequence. I know it is difficult to review complex
patches, and several of mine are that.  So, I don't want to overload
anybody with a flood of complex patches to review under some
ticking-clock scenario.

So...just these two, for now, and nobody needs to panic.  Much.

 Development libtool has not been released for quite a long time now
 (since September 2008) and will need to undergo regression testing on a
 wide variety of platforms prior to release.

Yes, I'm afraid so.  Even gcc is using a non-released libtool (synced
with git master around 12-05-2009), so there appears to be quite some
pressure for updated features/bugfixes from the outside world.  Nothing
like the pre-2.2.x days, tho.

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-29 Thread Charles Wilson 
Charles Wilson wrote:
 Charles Wilson wrote:
 Ralf Wildenhues wrote:
 * Charles Wilson wrote on Mon, Jun 22, 2009 at 03:50:42AM CEST:
 * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src)
 [ltwrapper_debugprintf]: Renamed to...
 I think functions should still be put in (parens) in the ChangeLog
 entry, not in [brackets], according to GCS.
 [... other review comments ...]
 
 Okay, here's a followon patch to
 
 Enable runtime cwrapper debugging; add tests
 http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html
 
 The first patch, in addition to addressing various points raised in this
 thread, obsoletes the extra mingw fix:
 http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00017.html
 
 Assuming these two patches are approved, I'll squash them together into
 a single commit (and update the commit history) before pushing. I just
 figured it was easier to review, by presenting the response to the
 review comments separately.

ping...

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-15 Thread Charles Wilson 
Charles Wilson wrote:
 Ralf Wildenhues wrote:
 * Charles Wilson wrote on Mon, Jun 22, 2009 at 03:50:42AM CEST:
 * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src)
 [ltwrapper_debugprintf]: Renamed to...
 I think functions should still be put in (parens) in the ChangeLog
 entry, not in [brackets], according to GCS.
[... other review comments ...]

Okay, here's a followon patch to

Enable runtime cwrapper debugging; add tests
http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html

The first patch, in addition to addressing various points raised in this
thread, obsoletes the extra mingw fix:
http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00017.html

Assuming these two patches are approved, I'll squash them together into
a single commit (and update the commit history) before pushing. I just
figured it was easier to review, by presenting the response to the
review comments separately.

I haven't done a full test run; but I have tested a few of the old tests
and the cwrapper.at tests individually, with this change. Full test run
on both cygwin and linux in progress [*]

This sequence doesn't address the issue with regards to the /shell/
wrapper failing to pass the cwrapper tests (for instance, on linux).
I've got a followon patch to

Re: [PATCH] Add --lt-* options to shell wrapper
http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00015.html

which addresses that issue and will post it shortly, but we should
discuss that in the other thread.

[*] Testing with these two patches, plus the two shell wrapper patches
referenced above, plus the wrapper documentation patch:
http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00016.html

...
actually, the linux test already finished. Results:
Old: All 94 tests passed
New: Only two unexpected results (in particular, the cwrapper test passed)

 21: passing CXX flags through libtool   FAILED (flags.at:24)
100: Run tests with low max_cmd_len  FAILED
(cmdline_wrap.at:43)

Err.. 21 should have been skipped, because I haven't installed g++ on
the linux box yet. And 100 is just a repeat of 21.





--
Chuck
Update cwrapper and tests per review comments

* libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src):
Update comments. Initialize program_name. Eliminate _LENGTH
variables for string constants. In debug mode, print a
banner with known content before any other output.
(func_emit_cwrapperexe_src:main): Use strcmp rather than
strncmp, where safe.
(func_emit_cwrapperexe_src:lt_debugprintf): Modify signature
and implementation to conform to GCS.
(func_emit_cwrapperexe_src:lt_fatal): Ditto.
(func_emit_cwrapperexe_src:lt_error_core): Ditto.
(func_emit_cwrapperexe_src): Update all callers to lt_fatal
and lt_debugprintf.
* tests/cwrapper.at: Clarify comments. Avoid unnecessarily
copying libtool. Adjust debug tests to search for wrapper
script banner message.

diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index f09b323..4549023 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -2727,10 +2727,6 @@ func_emit_cwrapperexe_src ()
 
This wrapper executable should never be moved out of the build directory.
If it is, it will not operate correctly.
-
-   Currently, it simply execs the wrapper *script* $SHELL $output,
-   but could eventually absorb all of the scripts functionality and
-   exec $objdir/$outputname directly.
 */
 EOF
 	cat EOF
@@ -2861,7 +2857,7 @@ static int lt_debug = 1;
 static int lt_debug = 0;
 #endif
 
-const char *program_name = NULL;
+const char *program_name = wrapper; /* in case xstrdup fails */
 
 void *xmalloc (size_t num);
 char *xstrdup (const char *string);
@@ -2871,15 +2867,14 @@ char *chase_symlinks (const char *pathspec);
 int make_executable (const char *path);
 int check_executable (const char *path);
 char *strendzap (char *str, const char *pat);
-void lt_debugprintf (const char *fmt, ...);
-void lt_fatal (const char *message, ...);
+void lt_debugprintf (const char *file, int line, const char *fmt, ...);
+void lt_fatal (const char *file, int line, const char *message, ...);
 void lt_setenv (const char *name, const char *value);
 char *lt_extend_str (const char *orig_value, const char *add, int to_end);
 void lt_update_exe_path (const char *name, const char *value);
 void lt_update_lib_path (const char *name, const char *value);
 char **prepare_spawn (char **argv);
 void lt_dump_script (FILE *f);
-
 EOF
 
 	cat EOF
@@ -2925,16 +2920,10 @@ EOF
 	cat EOF
 
 #define LTWRAPPER_OPTION_PREFIX --lt-
-#define LTWRAPPER_OPTION_PREFIX_LENGTH  5
 
-static const size_t opt_prefix_len = LTWRAPPER_OPTION_PREFIX_LENGTH;
 static const char *ltwrapper_option_prefix = LTWRAPPER_OPTION_PREFIX;
-
 static const char *dumpscript_opt   = LTWRAPPER_OPTION_PREFIX dump-script;
-static const size_t dumpscript_opt_len  =

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-15 Thread Charles Wilson 
Charles Wilson wrote:
 [*] Testing with these two patches, plus the two shell wrapper patches
 referenced above, plus the wrapper documentation patch:
 http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00016.html
 
 ...
 actually, the linux test already finished. Results:
 Old: All 94 tests passed
 New: Only two unexpected results (in particular, the cwrapper test passed)
 
  21: passing CXX flags through libtool   FAILED (flags.at:24)
 100: Run tests with low max_cmd_len  FAILED
 (cmdline_wrap.at:43)
 
 Err.. 21 should have been skipped, because I haven't installed g++ on
 the linux box yet. And 100 is just a repeat of 21.

cygwin results:
Old: All 122 tests passed
New:
 48: deplib in subdir   FAILED (deplib-in-subdir.at:140)
 90: C++ exception handling FAILED (exceptions.at:254)
100: Run tests with low max_cmd_len FAILED (cmdline_wrap.at:43)

48: unrelated. one-line fix; will post later
90: not sure what's going on here, but I'm pretty sure it has nothing to
do with these changes.  I'm getting a segfault from inside libstdc++,
during exceptions_in_module(). This could be a compiler bug; this is a
relatively new compiler release on cygwin, with big changes in C++ and
exception handling...
100: dups of 48,90

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-12 Thread Ralf Wildenhues 
* Charles Wilson wrote on Mon, Jan 11, 2010 at 03:54:01AM CET:
 Ralf Wildenhues wrote:
  That's not how these ancient shells work.
 [snip long explanation]
 
 Oh, that's just...evil.  How could that EVER have been expected to work
 properly, except in the most trivial of scripts?

Good question.

  In any case, here's a quick-n-dirty set of test scripts to help narrow
  down the problem.  If someone with access to HPUX can unpack and run
  't.sh', and report back the results, we might be able to narrow down the
  cause of this problem. (Might need to change the shbang lines to use the
  same shell that's causing issues with libtool).
  
  The tests all pass with HP-UX /bin/sh and /bin/ksh.
 
 Well, that's...good, I suppose. It means that the problem isn't
 specifically in the fork/shell-func/HERE implementation, such that the
 failure ALWAYS occurs.  But instead, it means that we have a race
 condition or resource contention problem (where 'unique names' can be
 considered a resource) -- or maybe my test case wasn't sufficiently
 complex, and didn't capture the bug exposed by libtool-as-a-whole.

We have a combination of shell bugs, not all of which we can
characterize exactly, and we try to not trigger them more than
necessary.

Let's not worry about this too much for now.  If it turns out to be a
big problem in practice, we will receive bug reports.  If somebody (me)
gets overly ambitious, I'll add some printf's to find out whether we are
triggering leftover temporaries only in specific cases.  Without such
data I wouldn't bother searching any further, though.

 Unfortunately, I don't see a way to avoid this problem on HP/UX -- short
 of requiring a less brain-dead shell.

Well, even just characterizing a broken shell is a problem if you don't
know exactly what the problem is.


Bottom line of all this is merely that we always have to expect trouble
on some other systems.

For an unrelated anectode: AIX has the nice habit of caching some shared
objects (with suitable permissions) in-memory even after process
termination, such that subsequent processes using the same library are
started faster.  During Libtool testsuite run, this happens for some
libraries in the build tree.  My test hosts mostly have their trees on
NFS, which refuses to erase directories with files that are still in use
somewhere.  Result: the testsuite cannot clean up after itself.  This
frequently causes hiccups and spurious testsuite failures for Libtool.
I usually just move the toplevel testsuite.dir away and remove it a year
later, when the system is sure to have been rebooted, or ask the
operator to run slibclean at her convenience.

Cheers,
Ralf

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-10 Thread Charles Wilson 
Ralf Wildenhues wrote:
 * Charles Wilson wrote on Mon, Jun 22, 2009 at 03:50:42AM CEST:
 * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src)
 [ltwrapper_debugprintf]: Renamed to...
 
 I think functions should still be put in (parens) in the ChangeLog
 entry, not in [brackets], according to GCS.

What about C functions that are emitted in a HERE document as part of
the execution of a SH function?  That's the distinction I was trying to
draw with the variant enclosures.

How about:

* libltdl/config/ltmain.m4sh
(func_emit_cwrapperexe_src:ltwrapper_debugprintf): Renamed to...
(func_emit_cwrapperexe_src:lt_debugprintf): this. Only

 [lt_debugprintf]: this. Only print messages if lt_debug != 0.
 [file scope]: Add constants and variables to support new --lt-debug
 option. Remove LTWRAPPER_DEBUGPRINTF macro.
 [main]: Consolidate option parsing. Ensure first use of lt_debugprintf
 occurs after option parsing. Add stanza to parse for --lt-debug and
 set lt_debug variable.
 [all]: Use lt_debugprintf () instead of LTWRAPPER_DEBUGPRINTF (()).
 * tests/cwrapper.at: Add new tests for --lt-debug and -DLT_DEBUGWRAPPER.

 The testsuite additions fail on GNU/Linux (with the respective patch for
 the shell wrapper applied), for several reasons, the first two of which
 are not fixed by your patch update:
 
 - the program is actually .libs/lt-usea there, (but it might also be
   .libs/usea, or just usea on other systems),

See next item.

 - the shell wrapper outputs 'lt_argv_zero' not 'argv[0]' in its debug
   output,

Actually, the problem is this: the binary wrapper does the following
(end of long lines deleted; unimportant lines deleted):

(main) argv[0]  : ./bmp2tiff
(main) program_name : bmp2tiff
(find_executable)   : ./bmp2tiff
(check_executable)  : [snipped]
(main) found exe (before symlink chase) at : [snipped]
checking path component for symlinks: ...
...
(main) found exe (after symlink chase) at : [snipped]
(main) libtool target name: bmp2tiff.exe
(lt_setenv) setting 'BIN_SH' to 'xpg4'
(lt_setenv) setting 'DUALCASE' to '1'
(lt_update_lib_path) modifying 'PATH' by prepending [snipped]
(lt_setenv) setting 'PATH' to [snipped]
(lt_update_exe_path) modifying 'PATH' by prepending [snipped]
(lt_setenv) setting 'PATH' to[snipped]
(main) lt_argv_zero : /full/path/to/./.libs/bmp2tiff.exe
(main) newargz[0]   : /full/path/to/./.libs/bmp2tiff.exe
(main) newargz[1]   : --help


While the shell wrapper only prints the last bit (and doesn't show both
lt_argz_zero and argv[0], because we really don't have access to any
data inside the shell or child process to determine if they WERE different).

$ ./bmp2tiff --lt-debug --help
(main) lt_argv_zero : /full/path/to/.libs/bmp2tiff
(main) newargz[1]   : --help

However, these precise details are not what that test is supposed to be
checking.  We're really only trying to determine that the debug output
HAPPENS, not what it IS.  I think the right approach is to modify both
the shell wrapper and C wrapper so that the FIRST thing they output, if
--lt-debug, is some unique magic. Maybe:

libtool wrapper (GNU libtool 1.3110 2009-07-01) 2.2.7a

(or maybe)

foo:lt-foo.c:25: libtool wrapper (GNU libtool 1.3110 2009-07-01) 2.2.7a

and then modify the test to check for that, instead of worrying about
platform idiosyncratic stuff like /-vs-\, [.exe], or [lt-].  I prefer
the former, but I don't really care. See GCS discussion below.

 - the cwrapper debugging activated at compile time, tested in the second
   half of cwrapper.at, does not enable debugging for the shell wrapper.

Hmm, ok. That's easy enough to fix.

 On w32 systems, the patch changes the API of many (but not all)
 uninstalled programs generated by libtool: those which get a cwrapper.
 The semantics of when a program gets a cwrapper is currently not
 documented, but you have posted another patch for this, so let's discuss
 this with that patch.

Right.

 More nits below.
 
 Thanks.  Apologies for the immense delay.
 
 --- a/libltdl/config/ltmain.m4sh
 +++ b/libltdl/config/ltmain.m4sh
 
 @@ -2881,6 +2873,7 @@ void lt_update_exe_path (const char *name, const char 
 *value);
  void lt_update_lib_path (const char *name, const char *value);
  char **prepare_spawn (char **argv);
  void lt_dump_script (FILE *f);
 +
 
 No need for this hunk.

Ack.

  EOF
  
  cat EOF
 @@ -2932,6 +2925,10 @@ static const size_t opt_prefix_len = 
 LTWRAPPER_OPTION_PREFIX_LENGTH;
  static const char *ltwrapper_option_prefix = LTWRAPPER_OPTION_PREFIX;
  
  static const char *dumpscript_opt   = LTWRAPPER_OPTION_PREFIX 
 dump-script;
 +static const size_t dumpscript_opt_len  = LTWRAPPER_OPTION_PREFIX_LENGTH + 
 11;
 
 Why are we using these _len variables and strncmp in this code again?
 strcmp is fine and safe and portable and used already, and strncmp is
 needed only for the test of an unknown option in our domain, no?

OK. I was trying to prepare the way for a possible later improvement,
where we allow users to

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2010-01-10 Thread Charles Wilson 
Ralf Wildenhues wrote:
 That's not how these ancient shells work.
[snip long explanation]

Oh, that's just...evil.  How could that EVER have been expected to work
properly, except in the most trivial of scripts?

 Oh yeah...ping? Is the HPUX problem a blocker for these three patches,
 
 No.

Good!

 or can they be considered before solving the HPUX issue?
 
 In any case, here's a quick-n-dirty set of test scripts to help narrow
 down the problem.  If someone with access to HPUX can unpack and run
 't.sh', and report back the results, we might be able to narrow down the
 cause of this problem. (Might need to change the shbang lines to use the
 same shell that's causing issues with libtool).
 
 The tests all pass with HP-UX /bin/sh and /bin/ksh.

Well, that's...good, I suppose. It means that the problem isn't
specifically in the fork/shell-func/HERE implementation, such that the
failure ALWAYS occurs.  But instead, it means that we have a race
condition or resource contention problem (where 'unique names' can be
considered a resource) -- or maybe my test case wasn't sufficiently
complex, and didn't capture the bug exposed by libtool-as-a-whole.

...
Unfortunately, I don't see a way to avoid this problem on HP/UX -- short
of requiring a less brain-dead shell.

1. every HERE document creates a temp file, whether that HERE document
   should or should not be emitted by the script.  If the script
   contains the magic EOF...EOF incantation even if it's inside an
   excluded block:
  if /bin/false; then
cat foo -EOF
blah blah
EOF
  fi
   We may not get a file named 'foo' but we WILL get a temp file, whose
   name we do not know, with the contents 'blah blah'.
2. there are only a limited number of unique temp file names
3. Sometimes these temp files don't get cleaned up. Eventually, we will
   try to reuse the temp file name of one of these non-cleaned pieces
   of litter. Boom.

We can't even pre-emptively clean up the temp files as the first line
of the script on HP/UX -- because we'd be cleaning up the ones
produced by parsing of THIS instance of the script -- some of which we
may actually need -- not to mention possibly others needed by other
in-progress instances.  What a mess.

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-12-23 Thread Charles Wilson 
Ralf Wildenhues wrote:
 * Charles Wilson wrote on Mon, Dec 14, 2009 at 06:47:10PM CET:
 Finally, it's not clear in your message: are you saying that *existing*
 win32 changes currently in master are causing problems on HP-UX, or
 just that some of the win32 changes /in this patch/ are causing them?
 
 Something causes lots of leftover shell temp files, and they contain
 cwrapper contents.  These leftover files cause later, unrelated
 processes with same PIDs to fail when needing shell temp files as well
 (yeah the temp file naming is really crappy).  Bet it's merely some
 shell bug with here-documents in functions or so, but I haven't analyzed
 it yet.

Hm. Platforms that are not win32ish should not be emitting the cwrapper
at all.  Brainstorming without actually being able to test on HPUX, it
sounds like libtool/HPUX is creating HERE documents for some /other/
purpose, but skipping past the EOF marker until it finds one it likes,
which happens to be in the middle of the cwrapper.

Does HPUX's shell not like any of these constructs?

cat WORD
...stuff...
WORD

cat WORD
...stuff...
WORD

Maybe it doesn't know that
WORD
matches WORD in the second example?  (If that's the case, then that
particular shell is borked and we ought to detect and die...)

 As to Bob's certainly right note on a high bar for patch entry: adding
 more testsuite coverage can only help confidence in changes.  Yes, I am
 repeating myself.  (And no, this isn't particularly directed at your w32
 changes, either.)

I should hope not. One effect of /THIS/ patch is to ADD some tests.

Oh yeah...ping? Is the HPUX problem a blocker for these three patches,
or can they be considered before solving the HPUX issue?

In any case, here's a quick-n-dirty set of test scripts to help narrow
down the problem.  If someone with access to HPUX can unpack and run
't.sh', and report back the results, we might be able to narrow down the
cause of this problem. (Might need to change the shbang lines to use the
same shell that's causing issues with libtool).

--
Chuck



heredoc_test_scripts.tar.bz2
Description: Binary data

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-12-15 Thread Charles Wilson 
On Mon, 14 Dec 2009 15:06 -0800, Tim Rice wrote:
 On Mon, 14 Dec 2009, Charles Wilson wrote:
 
  Two related lines of inquiry:
  1) Under *normal* development rules -- e.g not a pre-release
  bug-fix-only phase, nor a not-quite-pre-release code slush like (I
  think) we're in right now, for 2.2.8 -- surely you aren't suggesting
  that EVERY contribution must be validated on EVERY platform, prior to
  push?  These were tested on cyg/ming and linux, so in general, during
  /normal/ development, that should be sufficient contra reveiwer
  comments, right?
  
 I would strongly advocate testing at least one System V based system.
 Non bash shells sometimes have issues.

During the release process, certainly testing on as many supported
platforms as possible is baked into the cake.  *Perhaps*, by a reviewer
or other list denizen, before a particular patch is pushed.  But it's
unreasonable to *require* every contributor to have access to multiple
platforms. Some folks who fix HP-UX might not have (or want to install)
cygwin on a win32 box. Or a mingw developer might not have any access to
a Solaris machine.

I have access to cygwin, mingw, and linux. I don't have access to SysV,
and it would be setting WAY too high a bar for contributions to say No,
thanks, we don't want your patches because you can't test them on System
Foo.

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-12-15 Thread Tim Rice 

Hi Chuck,

On Tue, 15 Dec 2009, Charles Wilson wrote:

 On Mon, 14 Dec 2009 15:06 -0800, Tim Rice wrote:
  I would strongly advocate testing at least one System V based system.
  Non bash shells sometimes have issues.
 
 During the release process, certainly testing on as many supported
 platforms as possible is baked into the cake.  *Perhaps*, by a reviewer
 or other list denizen, before a particular patch is pushed.  But it's
 unreasonable to *require* every contributor to have access to multiple
 platforms. Some folks who fix HP-UX might not have (or want to install)
 cygwin on a win32 box. Or a mingw developer might not have any access to
 a Solaris machine.
 
 I have access to cygwin, mingw, and linux. I don't have access to SysV,
 and it would be setting WAY too high a bar for contributions to say No,
 thanks, we don't want your patches because you can't test them on System
 Foo.

I did not mean to imply that it should be required.
It's just a whole lot easier to keep non portable shell/awk/sed code
from slipping through if tested on SysV.


-- 
Tim RiceMultitalents(707) 887-1469
t...@multitalents.net

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-12-12 Thread Charles Wilson 
Charles Wilson wrote:
 * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src)
 [ltwrapper_debugprintf]: Renamed to...
 [lt_debugprintf]: this. Only print messages if lt_debug != 0.
 [file scope]: Add constants and variables to support new --lt-debug
 option. Remove LTWRAPPER_DEBUGPRINTF macro.
 [main]: Consolidate option parsing. Ensure first use of lt_debugprintf
 occurs after option parsing. Add stanza to parse for --lt-debug and
 set lt_debug variable.
 [all]: Use lt_debugprintf () instead of LTWRAPPER_DEBUGPRINTF (()).
 * tests/cwrapper.at: Add new tests for --lt-debug and -DLT_DEBUGWRAPPER.
 ---
 Another fragment arising from review of
 http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00031.html
 
 Lightly tested by running tests/demo-shared.test tests/demo-make.test
 tests/demo-exec.test and cwrapper.at.

Ping x3?
http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00039.html

This has been in the cygwin distro for five months (over a year, in one
form or another), and heavily tested. It's a long patch, but
conceptually and mechanically quite simple.  The rationale was
determined via earlier on-list discussions -- I didn't just go off on a
wild tangent and do this; I was instructed that this was a better
approach than the one(s) I originally posted:

http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00036.html
http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00043.html

I'm going to be (re)raising all of my old, outstanding patches over the
next week. Some, I think, are OK for immediate push, even 'relatively
close to 2.2.8'.  Others may be too big a change to consider at this
point, and that's fine.  Just let me know if you guys think a particular
patch should be deferred until post-2.2.8 and I'll take it off the table.


This one, I think is OK for pre-2.2.8 -- what do you guys think?
OK to push?

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-12-12 Thread Charles Wilson 
Charles Wilson wrote:
 This one, I think is OK for pre-2.2.8 -- what do you guys think?
 OK to push?

I forgot: the mingw libtool distribution includes this patch, but with
the following small change. Obviously I'd merge these two together and
commit as one.

--
Chuck




diff -u a/tests/cwrapper.at b/tests/cwrapper.at
--- a/tests/cwrapper.at	2009-07-12 20:32:35.0 -0400
+++ b/tests/cwrapper.at	2009-07-13 04:42:49.39100 -0400
@@ -99,7 +99,7 @@
  [], [ignore], [ignore])
 LT_AT_EXEC_CHECK([./usea], [0], [ignore], [stderr], [--lt-debug])
 LT_AT_UNIFY_NL([stderr])
-AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: \./usea' stderr], [0], [ignore], [ignore])
+AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: .*[[\\/]]usea' stderr], [0], [ignore], [ignore])
 
 
 # Make sure wrapper debugging works, when activated at compile time.
@@ -127,7 +127,7 @@
[], [ignore], [ignore])
   LT_AT_EXEC_CHECK([./usea], [0], [ignore], [stderr], [])
   LT_AT_UNIFY_NL([stderr])
-  AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: \./usea' stderr], [0], [ignore], [ignore])
+  AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: .*[[\\/]]usea' stderr], [0], [ignore], [ignore])
 done

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-07-12 Thread Charles Wilson 
Charles Wilson wrote:
 I'll follow up this patch with another to add support for
   --lt-debug
   --lt-dump-script
 to the shell wrapper.  --lt-debug will do very little other than $ECHO
 the full path to the program executable and its arguments.
 --lt-dump-script will simply { cat $0  exit 0 }

See just-posted [PATCH] Add --lt-* options to shell wrapper

 Then I'll revise and retitle the remainder of
 [PATCH] Document and test LT_DEBUGWRAPPER cwrapper macro
 http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00031.html
 as appropriate.

See just-posted [PATCH] Document wrapper changes.

 Ping x2 ?

--
Chuck

Re: [PATCH] Enable runtime cwrapper debugging; add tests

2009-06-27 Thread Charles Wilson 
Charles Wilson wrote:
 * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src)
 [ltwrapper_debugprintf]: Renamed to...
 [lt_debugprintf]: this. Only print messages if lt_debug != 0.
 [file scope]: Add constants and variables to support new --lt-debug
 option. Remove LTWRAPPER_DEBUGPRINTF macro.
 [main]: Consolidate option parsing. Ensure first use of lt_debugprintf
 occurs after option parsing. Add stanza to parse for --lt-debug and
 set lt_debug variable.
 [all]: Use lt_debugprintf () instead of LTWRAPPER_DEBUGPRINTF (()).
 * tests/cwrapper.at: Add new tests for --lt-debug and -DLT_DEBUGWRAPPER.
 ---
 Another fragment arising from review of
 http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00031.html
 
 Lightly tested by running tests/demo-shared.test tests/demo-make.test
 tests/demo-exec.test and cwrapper.at.
 
 Ok to push?

Ping?

I'll follow up this patch with another to add support for
  --lt-debug
  --lt-dump-script
to the shell wrapper.  --lt-debug will do very little other than $ECHO
the full path to the program executable and its arguments.
--lt-dump-script will simply { cat $0  exit 0 }

Then I'll revise and retitle the remainder of
[PATCH] Document and test LT_DEBUGWRAPPER cwrapper macro
http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00031.html
as appropriate.

--
Chuck