Re: [PATCH] Ensure cwrapper compiles without warnings under -std=c99.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Charles Wilson on 5/5/2008 6:23 PM:
| 2008-05-05 Charles Wilson <...>
|
| Ensure cwrapper compiles without warnings under -std=c99.
|
Looks ok, except for these nits:
|
| -# func_emit_wrapper arg
| +# func_emit_wrapper_part1 arg
Since you provide a default, I'd show that arg is optional, as in:
# func_emit_wrapper_part1 [arg=no]
| - func_emit_wrapper_arg1=no
| + func_emit_wrapper_part1_arg1=no
| if test -n "$1" ; then
| - func_emit_wrapper_arg1=$1
| + func_emit_wrapper_part1_arg1=$1
| fi
|
| $ECHO "\
| @@ -2352,10 +2352,36 @@ else
| file=\`\$ECHO \"X\$file\" | \$Xsed -e 's%^.*/%%'\`
| file=\`ls -ld \"\$thisdir/\$file\" | ${SED} -n 's/.*-> //p'\`
|done
| +"
| +}
Is func_emit_wrapper_part1_arg1 even used? Why not just delete it?
| @@ -2658,7 +2724,8 @@ EOF
| esac
|
| cat
Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
According to Charles Wilson on 5/5/2008 6:23 PM:
| 2008-05-05 Charles Wilson <...>
|
| * libltdl/config/ltmain.m4sh (func_to_native_path):
I've become accustomed to using a 1-line summary in my ChangeLog messages
prior to the first '* file:' line; that way, the summary can be reused as
the git commit summary.
|
| I'm sorta thinking I should rename the func_to_native* functions
s/native/host/ ?
Yes, that would be appropriate. I'd like to see that, and other cleanups
below, before approving. I haven't tested yet, but the concepts look sane
by inspection.
| +func_to_native_path ()
| +{
| + func_to_native_path_result="$1"
| + if test -n "$1" ; then
| +case $host in
| + *mingw* )
| +lt_sed_naive_backslashify='s|*|\\|g;s|/|\\|g;s|\\||g'
| +case $build in
| + *mingw* ) # actually, msys
| +# awkward: cmd appends spaces to result
| +lt_sed_strip_trailing_spaces="s/[ ]*\$//"
| +func_to_native_path_tmp1=`( cmd //c echo "$1" | $SED -e
"$lt_sed_strip_trailing_spaces" ) 2>/dev/null || echo ""`
| +func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`
Some pretty long lines; is it worth trying to wrap at 80 columns?
| +;;
| + *cygwin* )
| +func_to_native_path_tmp1=`cygpath -w "$1"`
| +func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`
| +;;
| +esac
| +if test -z "$func_to_native_path_result" ; then
| + func_error "Could not determine native path corresponding to"
| + func_error " '$1'"
| + func_error "Perhaps it doesn't exist."
| + func_error "Continuing, but running uninstalled executables
may not work."
Doesn't func_error die when called? If so, how do the subsequent three
errors get printed? If not, the name seems a bit misleading...
| +func_to_native_pathlist ()
| +{
| + func_to_native_pathlist_result="$1"
| + if test -n "$1" ; then
| +case $host in
| + *mingw* )
| +lt_sed_naive_backslashify='s|*|\\|g;s|/|\\|g;s|\\||g'
| +case $build in
| + *mingw* | *cygwin* )
| +# remove leading and trailing ':' from $1. msys behavior is
| +# inconsistent here, and cygpath turns them into into '.;'
and ';.'
| +func_to_native_pathlist_tmp1="$1"
| +func_to_native_pathlist_tmp2=`echo
"$func_to_native_pathlist_tmp1" | $SED -e 's|^:*||'`
| +func_to_native_pathlist_tmp1=`echo
"$func_to_native_pathlist_tmp2" | $SED -e 's|:*$||'`
Avoid some forks - can't you combine this into one sed invocation?
Similar comment about long lines.
| +
| +static const char *dumpscript_opt = "--lt-dump-script";
| +static const char *env_set_opt = "--lt-env-set";
| + /* argument is putenv-style "foo=bar", value of foo is set to bar */
| +static const char *env_prepend_opt = "--lt-env-prepend";
| + /* argument is putenv-style "foo=bar", new value of foo is bar${foo} */
| +static const char *env_append_opt = "--lt-env-append";
| + /* argument is putenv-style "foo=bar", new value of foo is ${foo}bar */
It's a bit of a shame that we can't rely on getopt_long and get option
abbreviations; oh well.
| - cat
Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
On 6 May 2008, at 09:20, Eric Blake wrote: Doesn't func_error die when called? If so, how do the subsequent three errors get printed? If not, the name seems a bit misleading... We have func_echo, func_warn, and func_error for automatic prefixing of output, and func_fatal_error for non-returning. This allows multi line error messages with correct prefix strings to stderr like: libtool: Could not determine native path corresponding to libtool: 'mingw' libtool: Perhaps it doesn't exist. libtool: Continuing, but running uninstalled executables may not work. Cheers, Gary -- ())_. Email me: [EMAIL PROTECTED] ( '/ Read my blog: http://blog.azazil.net / )= ...and my book: http://sources.redhat.com/autobook `(_~)_ PGP.sig Description: This is a digitally signed message part
[PATCH 370] Implement lt_dlopening of only preloaded modules.
* libltdl/m4/ltdl.m4 (LTDL_INIT): Check for a libltdl that
provides lt_dladvise_preopen when deciding if installed libltdl
is 'new enough'.
* libltdl/libltdl/lt__private.h (lt__advise): Add a new
is_preload flag.
* libltdl/ltdl.c (lt_dladvise_preload): New api call to set it.
(try_dlopen): If it is set, and the search of preloaded modules
didn't return a match, don't bother searching the filesystem.
* libltdl/ltdl.h (lt_dladvise_preload): Declare it.
* doc/libtool.texi (Libltdl Interface): Document it.
* tests/lt_dladvise.at: Test it (and incidentally add some test
coverage for `libtool -dlpreopen').
* NEWS: Announce it.
---
This makes it possible to build a libltdl client with some preloaded
modules, and limit lt_dlopenadvise to open only the preloaded modules.
For example: nearly all of GNU M4 is implemented as modules, with the
core functionality preloaded by default. All module loading is
handled by lt_dlopenadvise irrespective of whether modules were
preloaded or need to be found in M4PATH. However, it is also
possible to turn off gnu extensions, such as path searching and
module loading with -G, but we still need to be able to lt_dlopen
the preloaded core modules when module loading proper has been
disabled. With this patch I can use the lt_dlodvise_preopen hint
when the driver loads the preloaded core modules, even though user
module loading from the filesystem is turned off without the need
to maintain two codepaths (load preloaded core modules even if
they can't be found in the filesystem vs load arbitrary modules
from the filesystem in gnu extended mode).
Okay to push?
ChangeLog | 17
NEWS |5 ++-
doc/libtool.texi |7 +
libltdl/libltdl/lt__private.h |1 +
libltdl/ltdl.c| 25 +-
libltdl/ltdl.h|1 +
libltdl/m4/ltdl.m4|2 +-
tests/lt_dladvise.at | 56 +
8 files changed, 98 insertions(+), 16 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 15ffae1..2a1c843 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2008-05-06 Gary V. Vaughan <[EMAIL PROTECTED]>
+
+ Implement lt_dlopening of only preloaded modules.
+ * libltdl/m4/ltdl.m4 (LTDL_INIT): Check for a libltdl that
+ provides lt_dladvise_preopen when deciding if installed libltdl
+ is 'new enough'.
+ * libltdl/libltdl/lt__private.h (lt__advise): Add a new
+ is_preload flag.
+ * libltdl/ltdl.c (lt_dladvise_preload): New api call to set it.
+ (try_dlopen): If it is set, and the search of preloaded modules
+ didn't return a match, don't bother searching the filesystem.
+ * libltdl/ltdl.h (lt_dladvise_preload): Declare it.
+ * doc/libtool.texi (Libltdl Interface): Document it.
+ * tests/lt_dladvise.at: Test it (and incidentally add some test
+ coverage for `libtool -dlpreopen').
+ * NEWS: Announce it.
+
2008-05-05 Gary V. Vaughan <[EMAIL PROTECTED]>
Fix libtoolize test failure with aclocal < 1.10.1
diff --git a/NEWS b/NEWS
index 2b113fb..3354e6c 100644
--- a/NEWS
+++ b/NEWS
@@ -2,9 +2,10 @@ NEWS - list of user-visible changes between releases of GNU
Libtool
New in 2.2.??: 2008-06-??: git version 2.2.5a, Libtool team:
-* Bug fixes:
+* New features:
- - None yet
+ - New lt_dloadvise_preload() call to set a hint that only preloadeded
+modules can be opened.
New in 2.2.4: 2008-05-04: git version 2.2.3a, Libtool team:
diff --git a/doc/libtool.texi b/doc/libtool.texi
index 31ba0c7..e694655 100644
--- a/doc/libtool.texi
+++ b/doc/libtool.texi
@@ -3816,6 +3816,13 @@ On failure, @code{lt_dladvise_resident} returns non-zero
and sets an error
message that can be retrieved with @code{lt_dlerror}.
@end deftypefun
[EMAIL PROTECTED] int lt_dladvise_preload (lt_dladvise [EMAIL PROTECTED])
+Set the @code{preload} hint on @var{advise}. Passing an @var{advise}
+parameter to @code{lt_dlopenadvise} with this hint set causes it to
+load only preloaded modules, so that if a suitable preleaded module is
+not found, @code{lt_dlopenadvise} will return @code{NULL}.
[EMAIL PROTECTED] deftypefun
+
@deftypefun int lt_dlclose (lt_dlhandle @var{handle})
Decrement the reference count on the module @var{handle}.
If it drops to zero and no other module depends on this module,
diff --git a/libltdl/libltdl/lt__private.h b/libltdl/libltdl/lt__private.h
index 4ce936d..fd6e662 100644
--- a/libltdl/libltdl/lt__private.h
+++ b/libltdl/libltdl/lt__private.h
@@ -126,6 +126,7 @@ struct lt__advise {
subsequently loaded modules. */
unsigned int is_symlocal:1; /* module symbols are only available
locally. */
+ unsigned int is_preload:1; /* only preloaded modules will be tried. */
};
/* --- ERROR HANDLING --- */
diff --git a/libltdl/ltdl.c b/libltdl/ltdl.c
index a89c6bb.
Re: [PATCH 370] Implement lt_dlopening of only preloaded modules.
Hi Gary,
a quick nit review (I haven't looked thoroughly yet):
* Gary V. Vaughan wrote on Tue, May 06, 2008 at 07:24:46PM CEST:
>
> This makes it possible to build a libltdl client with some preloaded
> modules, and limit lt_dlopenadvise to open only the preloaded modules.
> --- a/NEWS
> +++ b/NEWS
> @@ -2,9 +2,10 @@ NEWS - list of user-visible changes between releases of GNU
> Libtool
>
> New in 2.2.??: 2008-06-??: git version 2.2.5a, Libtool team:
>
> -* Bug fixes:
> +* New features:
>
> - - None yet
No need to remove the Bug fixes section, there will certainly be entries
later, no? ;-)
> --- a/doc/libtool.texi
> +++ b/doc/libtool.texi
> @@ -3816,6 +3816,13 @@ On failure, @code{lt_dladvise_resident} returns
> non-zero and sets an error
> message that can be retrieved with @code{lt_dlerror}.
> @end deftypefun
>
> [EMAIL PROTECTED] int lt_dladvise_preload (lt_dladvise [EMAIL PROTECTED])
> +Set the @code{preload} hint on @var{advise}. Passing an @var{advise}
> +parameter to @code{lt_dlopenadvise} with this hint set causes it to
> +load only preloaded modules, so that if a suitable preleaded module is
s/preleaded/preloaded/
> +not found, @code{lt_dlopenadvise} will return @code{NULL}.
> [EMAIL PROTECTED] deftypefun
> +
> --- a/libltdl/libltdl/lt__private.h
> +++ b/libltdl/libltdl/lt__private.h
> @@ -126,6 +126,7 @@ struct lt__advise {
> subsequently loaded modules. */
>unsigned int is_symlocal:1; /* module symbols are only available
> locally. */
> + unsigned int is_preload:1; /* only preloaded modules will be
> tried. */
s/is_preload/try_preload_only/ ?
> --- a/libltdl/ltdl.c
> +++ b/libltdl/ltdl.c
> @@ -1257,27 +1257,40 @@ try_dlopen (lt_dlhandle *phandle, const char
> *filename, const char *ext,
>
>if (vtable)
> {
> + char *archive_name = MALLOC (char, LT_STRLEN (name) + 3);
> *phandle = (lt_dlhandle) lt__zalloc (sizeof (struct lt__handle));
>
> - if (*phandle == NULL)
> + if ((*phandle == NULL) || (archive_name == NULL))
> {
> ++errors;
> goto cleanup;
> }
> newhandle = *phandle;
>
> - if (tryall_dlopen (&newhandle, attempt, advise, vtable) == 0)
> + /* Preloaded modules are always named according to their old
> + archive name. */
> + sprintf (archive_name, "%s.a", name);
> +
> + if (tryall_dlopen (&newhandle, archive_name, advise, vtable) == 0)
> {
> goto register_handle;
Leaking archive_name here.
> }
>
> /* If we're still here, there was no matching preloaded module,
>so put things back as we found them, and continue searching. */
> + FREE (archive_name);
> FREE (*phandle);
> newhandle = NULL;
> }
> }
>
> --- a/libltdl/m4/ltdl.m4
> +++ b/libltdl/m4/ltdl.m4
> @@ -264,7 +264,7 @@ if test "x$with_included_ltdl" != xyes; then
># decide whether there is a useful installed version we can use.
>AC_CHECK_HEADER([ltdl.h],
>[AC_CHECK_DECL([lt_dlinterface_register],
> -[AC_CHECK_LIB([ltdl], [lt_dlinterface_register],
> +[AC_CHECK_LIB([ltdl], [lt_dladvise_preload],
> [with_included_ltdl=no],
> [with_included_ltdl=yes])],
> [with_included_ltdl=yes],
I view this as a separate change. Not sure whether we should have a
policy to always require the newest available feature or not, in an
installed ltdl.
> --- a/tests/lt_dladvise.at
> +++ b/tests/lt_dladvise.at
AFAICS the test doesn't ensure that, if the platform supports
non-preloaded module, lt_dladvise_preload causes trying to load them to
fail.
Cheers,
Ralf
Re: [PATCH] Ensure cwrapper compiles without warnings under -std=c99.
Hi Charles,
In addition to Eric's review, here's some extremely picky nits:
* Charles Wilson wrote on Tue, May 06, 2008 at 02:23:05AM CEST:
> 2008-05-05 Charles Wilson <...>
>
> Ensure cwrapper compiles without warnings under -std=c99.
> * libltdl/config/ltmain.m4sh (func_emit_wrapper_part1):
> new function.
> (func_emit_wrapper_part2): new function.
> (func_emit_wrapper): delegate to new functions.
> (func_emit_cwrapperexe_src) [__CYGWIN__ && __STRICT_ANSI__]:
> ensure realpath is declared.
> (func_emit_cwrapperexe_src): declare two different strings
> to each hold part of the wrapper script content. Initialize
> using new func_emit_wrapper_partX functions.
> (func_emit_cwrapperexe_src) [main]: when emitting wrapper
> script content, use both strings.
> Reported by Yaakov Selkowitz.
> --- a/libltdl/config/ltmain.m4sh
> +++ b/libltdl/config/ltmain.m4sh
> @@ -2249,9 +2249,9 @@ func_extract_archives ()
>
>
>
> -# func_emit_wrapper arg
> +# func_emit_wrapper_part1 arg
> #
> -# emit a libtool wrapper script on stdout
> +# emit the first part of a libtool wrapper script on stdout
> # don't directly open a file because we may want to
> # incorporate the script contents within a cygwin/mingw
> # wrapper executable. Must ONLY be called from within
> @@ -2263,11 +2263,11 @@ func_extract_archives ()
> # will assume that the directory in which it is stored is
> # the '.lib' directory. This is a cygwin/mingw-specific
Isn't that .libs/_libs aka $objdir here? This is not new here, but
further below.
> # behavior.
> -func_emit_wrapper ()
> +func_emit_wrapper_part1 ()
> {
> - func_emit_wrapper_arg1=no
> + func_emit_wrapper_part1_arg1=no
> if test -n "$1" ; then
> - func_emit_wrapper_arg1=$1
> + func_emit_wrapper_part1_arg1=$1
> fi
>
> $ECHO "\
> @@ -2352,10 +2352,36 @@ else
> file=\`\$ECHO \"X\$file\" | \$Xsed -e 's%^.*/%%'\`
> file=\`ls -ld \"\$thisdir/\$file\" | ${SED} -n 's/.*-> //p'\`
>done
> +"
> +}
> +# end: func_emit_wrapper_part1
> +
> +# func_emit_wrapper_part2 arg
> +#
> +# emit the second part of a libtool wrapper script on stdout
Capitalization, period at end of the sentence.
> +# don't directly open a file because we may want to
> +# incorporate the script contents within a cygwin/mingw
> +# wrapper executable. Must ONLY be called from within
> +# func_mode_link because it depends on a number of variable
> +# set therein.
> +#
> +# arg is the value that the WRAPPER_SCRIPT_BELONGS_IN_OBJDIR
Please refer to arguments in all caps: ARG (as is done elsewhere).
> +# variable will take. If 'yes', then the emitted script
> +# will assume that the directory in which it is stored is
> +# the '.lib' directory. This is a cygwin/mingw-specific
See above (.libs).
> +# behavior.
> +func_emit_wrapper_part2 ()
> +{
> + func_emit_wrapper_part2_arg1=no
> + if test -n "$1" ; then
> + func_emit_wrapper_part2_arg1=$1
> + fi
> +
> + $ECHO "\
>
># Usually 'no', except on cygwin/mingw when embedded into
># the cwrapper.
> - WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=$func_emit_wrapper_arg1
> + WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=$func_emit_wrapper_part2_arg1
>if test \"\$WRAPPER_SCRIPT_BELONGS_IN_OBJDIR\" = \"yes\"; then
> # special case for '.'
> if test \"\$thisdir\" = \".\"; then
> @@ -2472,7 +2498,36 @@ else
> fi\
> "
> }
> -# end: func_emit_wrapper
> +# end: func_emit_wrapper_part2
> +
> +
> +# func_emit_wrapper arg
> +#
> +# emit a libtool wrapper script on stdout
> +# don't directly open a file because we may want to
> +# incorporate the script contents within a cygwin/mingw
> +# wrapper executable. Must ONLY be called from within
> +# func_mode_link because it depends on a number of variable
> +# set therein.
> +#
> +# arg is the value that the WRAPPER_SCRIPT_BELONGS_IN_OBJDIR
See above (all caps).
> +# variable will take. If 'yes', then the emitted script
> +# will assume that the directory in which it is stored is
> +# the '.lib' directory. This is a cygwin/mingw-specific
See above (.libs). I wonder whether this needs to be repeated.
> +# behavior.
> +func_emit_wrapper ()
> +{
> + func_emit_wrapper_arg1=no
> + if test -n "$1" ; then
> + func_emit_wrapper_arg1=$1
> + fi
> +
> + # split this up so that func_emit_cwrapperexe_src
> + # can call each part independently.
> + func_emit_wrapper_part1 "${func_emit_wrapper_arg1}"
> + func_emit_wrapper_part2 "${func_emit_wrapper_arg1}"
> +}
> +
Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
Hi Charles,
In addition to Eric's review:
* Charles Wilson wrote on Tue, May 06, 2008 at 02:23:05AM CEST:
>
> * libltdl/config/ltmain.m4sh (func_to_native_path):
> new function. If $host is mingw, and $build is mingw
> or cygwin, convert path to mingw native format.
> (func_to_native_pathlist): new function. Ditto, for
> :-separated pathlists.
> (func_emit_cwrapperexe_src) [__CYGWIN__ && __STRICT_ANSI__]:
> ensure putenv and setenv are declared. Define HAVE_SETENV.
> (func_emit_cwrapperexe_src) [main]: add new constants to
> hold desired PATH settings; initialize and convert to native
> mingw format using functions above. Add new command-line
> options --lt-env-set, --lt-env-prepend, and --lt-env-append.
> No longer emit wrapper script as integral part of launching
> child. Remove support for (now) unnecessary $TARGETSHELL.
> Exec actual target executable directly.
You can simplify the remaining part of the ChangeLog entry:
> (func_emit_cwrapperexe_src) [lt_setenv]: new function.
> (func_emit_cwrapperexe_src) [lt_extend_str]: new function.
> (func_emit_cwrapperexe_src) [lt_split_name_value]: new function.
> (func_emit_cwrapperexe_src) [lt_opt_process_env_set]: new function.
> (func_emit_cwrapperexe_src) [lt_opt_process_env_prepend]: new function.
> (func_emit_cwrapperexe_src) [lt_opt_process_env_append]: new function.
> (func_emit_cwrapperexe_src) [lt_update_exe_path]: new function.
> (func_emit_cwrapperexe_src) [lt_update_lib_path]: new function.
to this:
(func_emit_cwrapperexe_src) [lt_setenv]: New function.
[lt_extend_str]: New function.
[lt_split_name_value]: New function.
[lt_opt_process_env_set]: New function.
[lt_opt_process_env_prepend]: New function.
[lt_opt_process_env_append]: New function.
[lt_update_exe_path]: New function.
[lt_update_lib_path]: New function.
or even to this:
(func_emit_cwrapperexe_src) [lt_setenv, lt_extend_str]
[lt_split_name_value, lt_opt_process_env_set]
[lt_opt_process_env_prepend, lt_opt_process_env_append]
[lt_update_exe_path, lt_update_lib_path]: New functions.
More nits:
> +# func_to_native_path
> +#
> +# intended for use on "native" mingw (where libtool itself
Please capitalize, also please start the description with what the
function does, not what it is intended for:
Convert paths to build format when used with build tools.
[...]
> +# is running under the msys shell). Paths need to be converted
> +# to native format when used with native tools. Ordinarily, the
> +# (msys) shell automatically converts such things for non-msys
> +# applications it launches, but that isn't available from inside
> +# the cwrapper. Similar accommodations are necessary for $host
> +# mingw and $build cygwin. Calling this function does no harm
> +# on other $build or for other $host.
> +func_to_native_path ()
> +{
> +void
> +lt_update_exe_path (const char *name, const char *value)
> +{
> + LTWRAPPER_DEBUGPRINTF (("(lt_update_exe_path) modifying '%s' by prepending
> '%s'\n",
> + (name ? name : ""),
> + (value ? value : "")));
> +
> + if (name && *name && value && *value)
> +{
> + char *new_value = lt_extend_str (getenv (name), value, 0);
> + /* some systems can't cope with a ':'-terminated path #' */
What does that #' do here?
> + int len = strlen (new_value);
> + while (((len = strlen (new_value)) > 0) && IS_PATH_SEPARATOR
> (new_value[len-1]))
> +{
> + new_value[len-1] = '\0';
> +}
Sorry I haven't had the time to test your patches on a cross build yet.
(This isn't a veto against the patch.)
Cheers,
Ralf
Re: --with-included-ltdl infers --enable-ltdl-convenience
Hi Bob,
* Bob Friesenhahn wrote on Sun, Apr 13, 2008 at 11:25:38PM CEST:
> With latest libtool and autoconf, if the argument --with-included-ltdl is
> provided, then libtool adds a --with-included-ltdl to config.status. If
> this configuration is then re-used, autoconf warns about
> --with-included-ltdl since it is not a known supported configure option.
This patch shuts up the warning about --enable-ltdl-convenience
(I suppose the fact that you write --with-included-ltdl in the paragraph
is a typo, as you use the correct one in the Subject:). However, it
also exposes the flag to the user (via ./configure --help). I'm not
sure whether that is a good idea. What do you think?
Thanks,
Ralf
2008-05-06 Ralf Wildenhues <[EMAIL PROTECTED]>
* libltdl/m4/ltdl.m4 (_LTDL_CONVENIENCE): Add AC_ARG_ENABLE
for ltdl-convenience, to avoid configure warning for unknown
switch.
Report by Bob Friesenhahn.
diff --git a/libltdl/m4/ltdl.m4 b/libltdl/m4/ltdl.m4
index 6c277b4..b2ee9e3 100644
--- a/libltdl/m4/ltdl.m4
+++ b/libltdl/m4/ltdl.m4
@@ -95,6 +95,8 @@ m4_defun([_LTDL_CONVENIENCE],
"") enable_ltdl_convenience=yes
ac_configure_args="$ac_configure_args --enable-ltdl-convenience" ;;
esac
+AC_ARG_ENABLE([ltdl-convenience],
+ [AS_HELP_STRING([--enable-ltdl-convenience], [enable convenience libltdl])])
LIBLTDL='_LT_BUILD_PREFIX'"${lt_ltdl_dir+$lt_ltdl_dir/}libltdlc.la"
LTDLDEPS=$LIBLTDL
LTDLINCL='-I${top_srcdir}'"${lt_ltdl_dir+/$lt_ltdl_dir}"
Re: --with-included-ltdl infers --enable-ltdl-convenience
On Tue, 6 May 2008, Ralf Wildenhues wrote: This patch shuts up the warning about --enable-ltdl-convenience (I suppose the fact that you write --with-included-ltdl in the paragraph is a typo, as you use the correct one in the Subject:). However, it I don't believe that it is a typo since if --with-included-ltdl is supplied, then the configure script also sets --enable-ltdl-convenience to yes and this is inserted into the config.status file as a supplied option so that it is applied later. The --enable-ltdl-convenience option was in development libtool 2.X for almost four years. also exposes the flag to the user (via ./configure --help). I'm not sure whether that is a good idea. What do you think? It is perhaps not a great thing since it is now a secondary (non-dominant) option. In fact, I noticed that --enable-ltdl-convenience is 'yes' even if libltdl is being built as a shared library for formal installation. Bob == Bob Friesenhahn [EMAIL PROTECTED], http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/
Re: [PATCH] Ensure $OBJDUMP is defined
Ralf Wildenhues wrote: * Charles Wilson wrote on Tue, May 06, 2008 at 02:23:05AM CEST: 2008-05-05 Charles Wilson <...> Yaakov Selkowitz <...> Ensure $OBJDUMP is defined * libltdl/m4/libtool.m4 (_LT_DECL_OBJDUMP): new macro ensures that $OBJDUMP is always defined sanely. (_LT_SYS_DYNAMIC_LINKER): call it. (_LT_CHECK_MAGIC_METHOD): call it. git push at that point is fine. I usually do a git show first to visually check for stupid errors like forgetting the ChangeLog file or so. Pushed. BTW, sorry for all the duplicates. I was trying to use ssmtp and/or msmtp under cygwin, to send the git format-patch results. I waited over an hour for them to show up in my inbox (I'm used to the archives being very slow, but not actual delivery). Anyway, since I didn't get delivery of my message after an hour, I figured I must have messed up the ssmtp invocation, and tried again using msmtp. But apparently both worked. :-( -- Chuck
Re: [PATCH] Ensure cwrapper compiles without warnings under -std=c99.
Eric Blake wrote: According to Charles Wilson on 5/5/2008 6:23 PM: | -# func_emit_wrapper arg | +# func_emit_wrapper_part1 arg Since you provide a default, I'd show that arg is optional, as in: # func_emit_wrapper_part1 [arg=no] Ack. Is func_emit_wrapper_part1_arg1 even used? Why not just delete it? No, it is not (currently) used. However, that's just an artifact of where I happened to "split" the original emit function: it just so happens that the only place the original emit function used the *original* arg, ended up in the second half, after I split the emit function into two parts. I figured symmetry was easier for future maintainance, than strict adherence to the rule/pattern/guideline concerning supplying only the minimum argument set to each (sub)function called. But it doesn't matter that much to me: if you still think I should remove the superfluous argument and make the (sub) functions un-symmetric, I will. Isn't puts slightly more efficient than printf? But it doesn't matter that much to me. Not part of the scope of this patch: the old version used printf, the new version uses printf. I figured I'd get dinged for changing more than was strictly necessary to silence the observed warnings -- this was supposed to be a very simple, uncontroversial patch that could've made it in to 2.2.4... The "out of the scope of this patch" comment also applies to /all/ of Ralf's comments on this patch, but I'll address that in a separate message. Besides, we know that printf can deal with very large strings (because so far it is working with 4K+ ones, even if gcc -std=c99 *warns* about it. We don't know that puts() does. (It probably does, but we don't *know* that). -- Chuck
Re: [PATCH] Ensure cwrapper compiles without warnings under -std=c99.
Ralf Wildenhues wrote:
In addition to Eric's review, here's some extremely picky nits:
-# func_emit_wrapper arg
+# func_emit_wrapper_part1 arg
Isn't that .libs/_libs aka $objdir here? This is not new here, but
further below.
Capitalization, period at end of the sentence.
# func_emit_wrapper_part1 [ARG=no]
#
# Emit the first part of a libtool wrapper script on stdout.
# For more information, see the description associated with
# func_emit_wrapper(), below.
# func_emit_wrapper_part2 [ARG=no]
#
# Emit the second part of a libtool wrapper script on stdout.
# For more information, see the description associated with
# func_emit_wrapper(), below.
Please refer to arguments in all caps: ARG (as is done elsewhere).
See above (.libs).
# func_emit_wrapper [ARG=no]
#
# Emit a libtool wrapper script on stdout.
# Don't directly open a file because we may want to
# incorporate the script contents within a cygwin/mingw
# wrapper executable. Must ONLY be called from within
# func_mode_link because it depends on a number of variables
# set therein.
#
# ARG is the value that the WRAPPER_SCRIPT_BELONGS_IN_OBJDIR
# variable will take. If 'yes', then the emitted script
# will assume that the directory in which it is stored is
# the $objdir directory. This is a cygwin/mingw-specific
# behavior.
I wonder whether this needs to be repeated.
Yes, so did I:
http://lists.gnu.org/archive/html/libtool-patches/2008-04/msg00161.html
> I'm not sure if all the documentation needs to be duplicated for all
three functions, but...
See attached update (to be applied "on top of" previous patch.
Unresolved:
(1) whether func_emit_wrapper_part1 should even TAKE an argument
(2) whether the cwrapper src, when printing a const char*, should use
puts() in preference to printf("%s",...)
(3) from the message above, the issue listed as #3 /there/ is not
addressed by this patch, and might be a good candidate for a followup:
If compiling the cwrapper fails, libtool should notice, and at least
print some error message right away -- or error-and-die right away, and
not later when it tries to strip a non-existent file:
# we should really use a build-platform specific compiler
# here, but OTOH, the wrappers (shell script and this C one)
# are only useful if you want to execute the "real" binary.
# Since the "real" binary is built for $host, then this
# wrapper might as well be built for $host, too.
$opt_dry_run || {
$LTCC $LTCFLAGS -o $cwrapper $cwrappersource
$STRIP $cwrapper
}
(Note to self: update that comment block in the "don't call shell
wrapper from cwrapper" patch...because in the new scenario, we WANT to
use the $host compiler for the wrapper.exe)
--
Chuck
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index a2b4f9a..42e4905 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -2249,20 +2249,11 @@ func_extract_archives ()
-# func_emit_wrapper_part1 arg
+# func_emit_wrapper_part1 [ARG=no]
#
-# emit the first part of a libtool wrapper script on stdout
-# don't directly open a file because we may want to
-# incorporate the script contents within a cygwin/mingw
-# wrapper executable. Must ONLY be called from within
-# func_mode_link because it depends on a number of variable
-# set therein.
-#
-# arg is the value that the WRAPPER_SCRIPT_BELONGS_IN_OBJDIR
-# variable will take. If 'yes', then the emitted script
-# will assume that the directory in which it is stored is
-# the '.lib' directory. This is a cygwin/mingw-specific
-# behavior.
+# Emit the first part of a libtool wrapper script on stdout.
+# For more information, see the description associated with
+# func_emit_wrapper(), below.
func_emit_wrapper_part1 ()
{
func_emit_wrapper_part1_arg1=no
@@ -2356,20 +2347,11 @@ else
}
# end: func_emit_wrapper_part1
-# func_emit_wrapper_part2 arg
+# func_emit_wrapper_part2 [ARG=no]
#
-# emit the second part of a libtool wrapper script on stdout
-# don't directly open a file because we may want to
-# incorporate the script contents within a cygwin/mingw
-# wrapper executable. Must ONLY be called from within
-# func_mode_link because it depends on a number of variable
-# set therein.
-#
-# arg is the value that the WRAPPER_SCRIPT_BELONGS_IN_OBJDIR
-# variable will take. If 'yes', then the emitted script
-# will assume that the directory in which it is stored is
-# the '.lib' directory. This is a cygwin/mingw-specific
-# behavior.
+# Emit the second part of a libtool wrapper script on stdout.
+# For more information, see the description associated with
+# func_emit_wrapper(), below.
func_emit_wrapper_part2 ()
{
func_emit_wrapper_part2_arg1=no
@@ -2501,19 +2483,19 @@ fi\
# end: func_emit_wrapper_part2
-# func_emit_wrapper arg
+# func_emit_wrapper [ARG=no]
#
-# emit a libtool wrapper script on stdout
-# don't directly op
Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
Eric Blake wrote:
According to Charles Wilson on 5/5/2008 6:23 PM:
| 2008-05-05 Charles Wilson <...>
|
| * libltdl/config/ltmain.m4sh (func_to_native_path):
I've become accustomed to using a 1-line summary in my ChangeLog messages
prior to the first '* file:' line; that way, the summary can be reused as
the git commit summary.
Actually, my commit message has that, but when I edited the email for
sending, I deleted it.
|
| I'm sorta thinking I should rename the func_to_native* functions
s/native/host/ ?
Yes, that would be appropriate.
Done.
I'd like to see that, and other cleanups
below, before approving. I haven't tested yet, but the concepts look sane
by inspection.
Some pretty long lines; is it worth trying to wrap at 80 columns?
Wrapped most of them so that they were < 80, or at least much closer
to 80. With these long variable names, it's difficult, and I don't
really want to shorten them. The whole point of naming all "local"
variables after the function itself is to prevent variable collision.
But it makes for long varnames and long lines.
Doesn't func_error die when called? If so, how do the subsequent three
errors get printed? If not, the name seems a bit misleading...
See Gary's reply.
| +func_to_native_pathlist_tmp1="$1"
| +func_to_native_pathlist_tmp2=`echo
"$func_to_native_pathlist_tmp1" | $SED -e 's|^:*||'`
| +func_to_native_pathlist_tmp1=`echo
"$func_to_native_pathlist_tmp2" | $SED -e 's|:*$||'`
Avoid some forks - can't you combine this into one sed invocation?
Similar comment about long lines.
I made the change, but I haven't yet tested it. In one of these patches,
there was a /reason/ some $SED chain had to be done as separate steps,
but it might not be /this/ $SED chain...
It's a bit of a shame that we can't rely on getopt_long and get option
abbreviations; oh well.
That's a feature, not a bug. I don't want the user to have to remember
that he must use '--' if his application wants '-e'. Long option names,
in the --lt-* namespace == not likely to collide with the target app's
option set.
| -cat <
Ah, but my syntax highlighter marks "EOF" as a bright red string. \EOF
looks just like EOF.
| + target_name = (char *) xstrdup (base_name (actual_cwrapper_path));
>
Why the cast? Shouldn't xstrdup output char* already?
Yes, you're right. target_name is a renamed versions of shwrapper_name,
and it used the cast. Dunno why, but I didn't change it. Fixed now.
Also, gnulib's
base_name mallocs; if we ever make libtool's base_name match, then this
would leak memory (ie. xstrdup of gnulib's base_name is wasteful). But
your patch didn't affect base_name.
Well, yeah; if you change the behavior of the base_name() included in
the cwrapper_src, you better audit the uses of that function within the
cwrapper_src.
| + if (i+1 < argc)
Coding style: s/i+1/i + 1/
Ack.
| +{
| + lt_opt_process_env_set (argv[i+1]);
| + i++; /* don't copy */
| +}
| + else
| +{
| + lt_fatal ("%s missing required argument", env_set_opt);
| +}
Coding style: a single expression inside a control block does not need
braces.
Ack.
| + continue;
| +}
| + if (strcmp (argv[i], env_prepend_opt) == 0)
This requires --lt-env-prepend foo=bar, rather than allowing
--lt-env-prepend=foo=bar; is it worth allowing both syntax forms for
consistency with GNU coding standards? Actually, since the wrapper has
such a limited usage, I'm probably okay with the single form.
Added. (Not yet tested, but coded).
| + /* otherwise ... */
| + newargz[++newargc] = xstrdup (argv[i]);
Shouldn't you handle "--" as the end of wrapper options, in case the user
really wants to pass --lt-env-* as the first option to the wrapped
executable?
Done.
| -cat <
Actually, you've backed into a bug: when computing newargv[0], if you
trace the code, you'll see that the final component, the target exe's
name, is (effectively)
`basename $cwrapperexe .exe`.exe
If the actual target exe is lt-foo.exe, not foo.exe -- then the wrapper
is already broken. THAT is a bug I need to fix.
Before, the script wrapper name was always cwrapper name (+_TMPwrapper
or something). And libtool took care of ensuring that the script
wrapper knew if the target exe had a funky lt- name. I need to use that
same machinery here.
strcat can be inefficient if orig_value is long. Why not cache the
lengths, then use strcpy into the correct offset?
Done. Still using strcat in main() because I don't want to uglify the
code with 10 or so different length variables. However, maybe I can use
lt_extend_str instead of straight-line code (downside: extra memory
allocations, more stuff to free. Might be just as ugly...)
| + strncpy (*name, arg, len-1);
| + (*name)[len-1] = '\0';
coding style: s/len-1/len - 1/
A
Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
Ralf Wildenhues wrote: Hi Charles, You can simplify the remaining part of the ChangeLog entry: or even to this: (func_emit_cwrapperexe_src) [lt_setenv, lt_extend_str] [lt_split_name_value, lt_opt_process_env_set] [lt_opt_process_env_prepend, lt_opt_process_env_append] [lt_update_exe_path, lt_update_lib_path]: New functions. I'll try to remember to update that when I squash my topic-branch commits. More nits: +# func_to_native_path +# +# intended for use on "native" mingw (where libtool itself Please capitalize, also please start the description with what the function does, not what it is intended for: Convert paths to build format when used with build tools. [...] Done. + char *new_value = lt_extend_str (getenv (name), value, 0); + /* some systems can't cope with a ':'-terminated path #' */ What does that #' do here? See reply to Eric. Sorry I haven't had the time to test your patches on a cross build yet. (This isn't a veto against the patch.) Gary mentioned you were buried this week, and I knew this one had no chance for 2.2.4. Now, 2.2.6, maybe... -- Chuck
