[PATCH 1/7] syntax-check: fix violations and implement sc_useless_quotes_in_case.

2011-11-21 Thread Gary V. Vaughan 
More syntax-check inspired goodness to make our shell code more
readable and maintainable.  There's nothing controversial in here,
except perhaps a seemingly large amount of code churn for a
relatively small gain in each case, however the code definitely
*does* need all the help it can get to be maintainable without
severe hair loss, and even a journey of 1000 miles starts with a
single step, so I'll push the whole series in 72 hours or so as
usual.

By the end of this series, we've made some good progress in
unifying the style of new and old shell code alike, and since it's
all hooked into syntax-check, it'll take a concerted effort to
regress the aspects of style I've tackled, since make distcheck
will now fail if new violations are added in future. But, making
some of the older stuff not look like spaghetti is a much much
bigger task than these few patches are able to achieve.

Feedback welcome.

Contrary to popular belief, Bourne shell does not resplit case
expressions after expansion, so if there are no shell unquoted
shell metacharacters or whitespace, the quotes are useless.
* cfg.mk (sc_useless_quotes_in_case): New syntax-check rule to
ensure we don't reintroduce useless quoted case expressions.
* build-aux/ltmain.m4sh, m4/libtool.m4, tests/bindir.at,
tests/darwin.at, tests/defs.m4sh, tests/demo-hardcode.test,
tests/demo-nopic.test, tests/link-2.test, tests/quote.test,
tests/sysroot.at: Remove spurious quotes.

Signed-off-by: Gary V. Vaughan g...@gnu.org
---
 build-aux/ltmain.m4sh|   12 ++--
 cfg.mk   |8 
 m4/libtool.m4|2 +-
 tests/bindir.at  |4 ++--
 tests/darwin.at  |2 +-
 tests/defs.m4sh  |2 +-
 tests/demo-hardcode.test |4 ++--
 tests/demo-nopic.test|6 +++---
 tests/link-2.test|2 +-
 tests/quote.test |8 
 tests/sysroot.at |2 +-
 11 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 5e8fb25..ee93a21 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -461,7 +461,7 @@ func_lalib_unsafe_p ()
for lalib_p_l in 1 2 3 4
do
read lalib_p_line
-   case $lalib_p_line in
+   case $lalib_p_line in
\#\ Generated\ by\ *$PACKAGE* ) lalib_p=yes; break;;
esac
done
@@ -568,7 +568,7 @@ func_resolve_sysroot ()
 # store the result into func_replace_sysroot_result.
 func_replace_sysroot ()
 {
-  case $lt_sysroot:$1 in
+  case $lt_sysroot:$1 in
   ?*:$lt_sysroot*)
 func_stripname $lt_sysroot '' $1
 func_replace_sysroot_result==$func_stripname_result
@@ -3625,7 +3625,7 @@ main (int argc, char *argv[])
   if (STREQ (argv[i], dumpscript_opt))
{
 EOF
-   case $host in
+   case $host in
  *mingw* | *cygwin* )
# make stdout use unix line endings
echo   setmode(1,_O_BINARY);
@@ -5796,7 +5796,7 @@ func_mode_link ()
  if test -z $libdir  test $linkmode = prog; then
func_fatal_error only libraries may -dlpreopen a convenience 
library: \`$lib'
  fi
- case $host in
+ case $host in
# special handling for platforms with PE-DLLs.
*cygwin* | *mingw* | *cegcc* )
  # Linker will automatically link against shared library if both
@@ -5896,7 +5896,7 @@ func_mode_link ()
# We need to hardcode the library path
if test -n $shlibpath_var  test -z $avoidtemprpath ; then
  # Make sure the rpath contains only unique directories.
- case $temp_rpath: in
+ case $temp_rpath: in
  *$absdir:*) ;;
  *) func_append temp_rpath $absdir: ;;
  esac
@@ -8765,7 +8765,7 @@ func_mode_uninstall ()
  done
  test -n $old_library  func_append rmfiles  $odir/$old_library
 
- case $opt_mode in
+ case $opt_mode in
  clean)
case  $library_names  in
* $dlname *) ;;
diff --git a/cfg.mk b/cfg.mk
index 6a1748c..4bc32a7 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -60,6 +60,14 @@ sc_trailing_blank-non-rfc3676:
halt='found trailing blank(s)'  \
  $(_sc_search_regexp)
 
+# Avoid useless quotes around case arguments such as:
+#   case $foo in ...
+exclude_file_name_regexp--sc_useless_quotes_in_case = ^cfg.mk$$
+sc_useless_quotes_in_case:
+   @prohibit='case [^  ]*[   ][  ]*in'  \
+   halt='found spurious quotes around case argument'   \
+ $(_sc_search_regexp)
+
 # List syntax-check exempted files.
 exclude_file_name_regexp--sc_bindtextdomain = ^tests/.*demo[0-9]*/.*\.c$$
 exclude_file_name_regexp--sc_error_message_uppercase = \
diff --git a/m4/libtool.m4 b/m4/libtool.m4
index a9e20cf..ec00e81 100644
--- a/m4/libtool.m4
+++ b/m4/libtool.m4
@@ -1198,7 +1198,7 @@

[PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.

2011-11-21 Thread Gary V. Vaughan 
Some modernization of the legacy testsuite.
* tests/sh.test: Remove.
* Makefile.am (COMMON_TESTS): Adjust.
* cfg.mk (sc_libtool_m4_cc_basename, sc_prohibit_bracket_as_test)
(sc_prohibit_nested_quotes, sc_prohibit_set_dummy_without_shift)
(sc_prohibit_set_minus_minus, sc_prohibit_test_binary_operators)
(sc_prohibit_test_dollar, sc_prohibit_test_minus_e)
(sc_prohibit_test_unary_operators, sc_prohibit_test_X)
(sc_prohibit_Xsed_withou_X, sc_require_function_nl_brace):
Functionally identical tests to what used to be performed by
sh.test, only with coverage of all files.
* bootstrap, build-aux/edit-readme-alpha,
build-aux/extract-trace, build-aux/getopt.m4sh,
build-aux/ltmain.m4sh, configure.ac, m4/libtool.m4, m4/ltdl.m4,
tests/bindir.at, tests/configure-iface.at, tests/cwrapper.at,
tests/darwin.at, tests/defs.m4sh, tests/demo-hardcode.test,
tests/dlloader-api.at, tests/exceptions.at,
tests/getopt-m4sh.at, tests/lalib-syntax.at, tests/link-2.test,
tests/link-order2.at, tests/loadlibrary.at,
tests/lt_dladvise.at, tests/lt_dlexit.at, tests/lt_dlopen_a.at,
tests/lt_dlopenext.at, tests/need_lib_prefix.at,
tests/nonrecursive.at, tests/recursive.at, tests/resident.at,
tests/standalone.at, tests/static.at, tests/stresstest.at,
tests/subproject.at, tests/sysroot.at, tests/tagtrace.test,
tests/testsuite.at: Fix violations of the new syntax checks.

Signed-off-by: Gary V. Vaughan g...@gnu.org
---
 Makefile.am |3 +-
 bootstrap   |   21 ---
 build-aux/edit-readme-alpha |4 +-
 build-aux/extract-trace |6 +-
 build-aux/getopt.m4sh   |2 +-
 build-aux/ltmain.m4sh   |2 +-
 cfg.mk  |  104 +
 configure.ac|2 +-
 m4/libtool.m4   |   50 
 m4/ltdl.m4  |2 +-
 tests/bindir.at |   10 ++--
 tests/configure-iface.at|8 +-
 tests/cwrapper.at   |2 +-
 tests/darwin.at |2 +-
 tests/defs.m4sh |4 +-
 tests/demo-hardcode.test|6 +-
 tests/dlloader-api.at   |2 +-
 tests/exceptions.at |2 +-
 tests/getopt-m4sh.at|2 +-
 tests/lalib-syntax.at   |2 +-
 tests/link-2.test   |2 +-
 tests/link-order2.at|8 +-
 tests/loadlibrary.at|2 +-
 tests/lt_dladvise.at|4 +-
 tests/lt_dlexit.at  |2 +-
 tests/lt_dlopen_a.at|2 +-
 tests/lt_dlopenext.at   |4 +-
 tests/need_lib_prefix.at|2 +-
 tests/nonrecursive.at   |4 +-
 tests/recursive.at  |4 +-
 tests/resident.at   |2 +-
 tests/sh.test   |  134 ---
 tests/standalone.at |4 +-
 tests/static.at |2 +-
 tests/stresstest.at |2 +-
 tests/subproject.at |4 +-
 tests/sysroot.at|4 +-
 tests/tagtrace.test |4 +-
 tests/testsuite.at  |4 +-
 39 files changed, 200 insertions(+), 230 deletions(-)
 delete mode 100755 tests/sh.test

diff --git a/Makefile.am b/Makefile.am
index 2c6cf81..31d286e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -557,7 +557,7 @@ changelog   = $(distdir)/ChangeLog
 # date is updated to the following year.
 changelog_start_date = 2011-01-01
 $(changelog): FORCE
-   $(AM_V_GEN)if test -d $(srcdir)/.git; then \
+   $(AM_V_GEN)if test -d '$(srcdir)/.git'; then \
  $(gitlog_to_changelog) --amend=$(git_log_fix) \
  --since=$(changelog_start_date)  '$@T'; \
  rm -f '$@'; mv '$@T' '$@'; \
@@ -874,7 +874,6 @@ COMMON_TESTS = \
tests/nomode.test \
tests/objectlist.test \
tests/quote.test \
-   tests/sh.test \
tests/suffix.test \
tests/tagtrace.test \
tests/cdemo-static.test \
diff --git a/bootstrap b/bootstrap
index 9a9e94b..4dbdf72 100755
--- a/bootstrap
+++ b/bootstrap
@@ -738,20 +738,20 @@ func_clean_unused_macros ()
 
   # We use `ls|grep' instead of `ls *.m4' to avoid exceeding
   # command line length limits in some shells.
-  for file in `cd $macro_dir  ls -1 |grep '\.m4$'`; do
+  for file in `cd $macro_dir  ls -1 |grep '\.m4$'`; do
 
# Remove a macro file when aclocal.m4 does not m4_include it...
 func_grep_q 'm4_include([[]'$macro_dir/$file'])' $aclocal_m4s \
-|| test ! -f $gnulib_path/m4/$file || {
+|| test ! -f $gnulib_path/m4/$file || {
 
   # ...and there is an identical file in gnulib...
-  if func_cmp_s $gnulib_path/m4/$file $macro_dir/$file; then
+  if func_cmp_s $gnulib_path/m4/$file $macro_dir/$file; then
 
 # ...and it's not in the precious list (`echo' is needed
 # here to squash whitespace for the match expression).
 case  `echo $gnulib_precious`  in
   * $file *) ;;
-  *) rm -f

[PATCH 7/7] syntax-check: fix violations and implement sc_prohibit_sed_s_comma.

2011-11-21 Thread Gary V. Vaughan 
I like to name temporary directories that I will remove shortly
with two leading commas so that they sort lexicographically at
the top of `ls' output.  Now, `./configure
--prefix=`pwd`/,,inst' works again, for the first time in
several years.
* cfg.mk (sc_prohibit_sed_s_comma): Comma is too common a
character to use routinely as the separator for sed
substitutions on file paths and other variables determined by
the user, causing bugs like the one I describe above.  Make sure
we don't accidentally reintroduce any comma separators in
future.
* Makefile.am, bootstrap, bootstrap.conf, build-aux/extract-trace,
build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
build-aux/git-log-fix, build-aux/ltmain.m4sh, libtoolize.m4sh,
m4/libtool.m4, m4/ltdl.m4, tests/cdemo-undef.test,
tests/cmdline_wrap.at, tests/darwin.at, tests/defs.m4sh,
tests/getopt-m4sh.at, tests/install.at, tests/libtoolize.at,
tests/mdemo/Makefile.am, tests/need_lib_prefix.at,
tests/sysroot.at, tests/tagdemo-undef.test, tests/testsuite.at:
Try to use `|' as the default separator wherever possible,
otherwise something else that doesn't occur in the substitution
expression.
* NEWS: Updated.

Signed-off-by: Gary V. Vaughan g...@gnu.org
---
 Makefile.am|   88 
 NEWS   |1 +
 bootstrap  |   10 ++--
 bootstrap.conf |6 +-
 build-aux/extract-trace|   10 ++--
 build-aux/general.m4sh |   16 
 build-aux/git-hooks/commit-msg |4 +-
 build-aux/git-log-fix  |   48 +++---
 build-aux/ltmain.m4sh  |6 +-
 cfg.mk |   10 +
 libtoolize.m4sh|   62 ++--
 m4/libtool.m4  |   16 
 m4/ltdl.m4 |2 +-
 tests/cdemo-undef.test |2 +-
 tests/cmdline_wrap.at  |2 +-
 tests/darwin.at|2 +-
 tests/defs.m4sh|2 +-
 tests/getopt-m4sh.at   |2 +-
 tests/install.at   |2 +-
 tests/libtoolize.at|   14 +++---
 tests/mdemo/Makefile.am|2 +-
 tests/need_lib_prefix.at   |2 +-
 tests/sysroot.at   |2 +-
 tests/tagdemo-undef.test   |2 +-
 tests/testsuite.at |4 +-
 25 files changed, 164 insertions(+), 153 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 31d286e..680ae3e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -52,7 +52,7 @@ LT_M4SH   = $(M4SH) -B 
'$(srcdir)/$(m4sh_dir)'
 lt__cd = CDPATH=$${ZSH_VERSION+.}$(PATH_SEPARATOR)  cd
 
 git_version_gen = '$(SHELL)' '$(aux_dir)/git-version-gen' '.tarball-version'
-rebuild = rebuild=:; revision=`$(lt__cd) $(srcdir)  $(git_version_gen) | sed 
's,-.*$$,,g'`
+rebuild = rebuild=:; revision=`$(lt__cd) $(srcdir)  $(git_version_gen) | sed 
's|-.*$$||g'`
 
 
 # -- #
@@ -108,18 +108,18 @@ EXTRA_DIST += $(extract_trace) $(libtoolize_in) 
$(libtoolize_m4sh) \
 ## because they must be static in distributed files, and not accidentally
 ## changed by configure running on the build machine.
 bootstrap_edit  = $(SED) \
- -e 's,@MACRO_VERSION\@,$(VERSION),g' \
- -e s,@MACRO_REVISION\@,$$revision,g \
- -e s,@MACRO_SERIAL\@,$$serial,g \
- -e 's,@PACKAGE\@,$(PACKAGE),g' \
- -e 's,@PACKAGE_BUGREPORT\@,$(PACKAGE_BUGREPORT),g' \
- -e 's,@PACKAGE_URL\@,$(PACKAGE_URL),g' \
- -e 's,@PACKAGE_NAME\@,$(PACKAGE_NAME),g' \
- -e s,@package_revision\@,$$revision,g \
- -e 's,@PACKAGE_STRING\@,$(PACKAGE_NAME) $(VERSION),g' \
- -e 's,@PACKAGE_TARNAME\@,$(PACKAGE),g' \
- -e 's,@PACKAGE_VERSION\@,$(VERSION),g' \
- -e 's,@VERSION\@,$(VERSION),g'
+ -e 's|@MACRO_VERSION\@|$(VERSION)|g' \
+ -e s|@MACRO_REVISION\@|$$revision|g \
+ -e s|@MACRO_SERIAL\@|$$serial|g \
+ -e 's|@PACKAGE\@|$(PACKAGE)|g' \
+ -e 's|@PACKAGE_BUGREPORT\@|$(PACKAGE_BUGREPORT)|g' \
+ -e 's|@PACKAGE_URL\@|$(PACKAGE_URL)|g' \
+ -e 's|@PACKAGE_NAME\@|$(PACKAGE_NAME)|g' \
+ -e s|@package_revision\@|$$revision|g \
+ -e 's|@PACKAGE_STRING\@|$(PACKAGE_NAME) $(VERSION)|g' \
+ -e 's|@PACKAGE_TARNAME\@|$(PACKAGE)|g' \
+ -e 's|@PACKAGE_VERSION\@|$(VERSION)|g' \
+ -e 's|@VERSION\@|$(VERSION)|g'
 
 ## ltmain.sh needs some additional editing to remove unsubstituted
 ## variable defaulting lines, because ltmain.sh never gets passed
@@ -210,8 +210,8 @@ $(lt_Makefile_am): $(ltdl_mk)
  }; \
  '$(SED)' -n '/^.. DO NOT REMOVE THIS LINE -- /,$$p' \
  '$(ltdl_mk)' \
-   |'$(SED)' -e 's,libltdl_,,; s,libltdl/,,; s,:

Re: [PATCH 1/7] syntax-check: fix violations and implement sc_useless_quotes_in_case.

2011-11-21 Thread Eric Blake 
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 Contrary to popular belief, Bourne shell does not resplit case
 expressions after expansion, so if there are no shell unquoted
 shell metacharacters or whitespace, the quotes are useless.
 @@ -568,7 +568,7 @@ func_resolve_sysroot ()
  # store the result into func_replace_sysroot_result.
  func_replace_sysroot ()
  {
 -  case $lt_sysroot:$1 in
 +  case $lt_sysroot:$1 in
?*:$lt_sysroot*)

Likewise in the pattern expression; you could further change this to:

case $lt_sysroot:$1 in
  ?*:$lt_sysroot*)

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.

2011-11-21 Thread Stefano Lattarini 
Hi Gary.  Just a quick nit (I haven't looked at the whole
series, and not even at the whole patch in fact; sorry).

On Monday 21 November 2011, Gary V wrote:
 for file
  do
 -  test -f $file || touch $file
 +  test -f $file || touch $file
  
What's the point of quoting file after `test -f' it it remains
unquoted after `touch'?

Regards,
  Stefano

Re: [PATCH 4/7] syntax-check: fix violations and implement sc_prohibit_bare_basename.

2011-11-21 Thread Stefano Lattarini 
Hi Gary.  Again, few quick nits here, probably incomplete.

On Monday 21 November 2011, Gary V wrote:
 * cfg.mk (sc_prohibit_bare_basename, sc_prohibit_basename_with_sed):
 Make sure not to go back to using occasional `|$basename' or
 `|$dirname' syntax.
 * build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
 build-aux/ltmain.m4sh, build-aux/options-parser,
 tests/fcdemo-conf.test, tests/fcdemo-shared.test,
 tests/fcdemo-static.test, tests/libtoolize.at: Fix violations.
 
 Signed-off-by: Gary V. Vaughan g...@gnu.org
 
 diff --git a/build-aux/general.m4sh b/build-aux/general.m4sh
 index 1f44535..790f4e0 100644
 --- a/build-aux/general.m4sh
 +++ b/build-aux/general.m4sh
 @@ -70,15 +70,15 @@ lt_nl='
  '
  IFS= $lt_nl
  
 -dirname='s,/[^/]*$,,'
 -basename='s,^.*/,,'
 +dirname='s|/[^/]*$||'
 +basename='s|^.*/||'

What's the point of this change?  If it's only stylistic, shouldn't it
be done in a separate patch?

  # func_dirname file append nondir_replacement
  # Compute the dirname of FILE.  If nonempty, add APPEND to the result,
  # otherwise set result to NONDIR_REPLACEMENT.
  func_dirname ()
  {
 -func_dirname_result=`$ECHO ${1} | $SED $dirname`
 +func_dirname_result=`$ECHO ${1} |$SED $dirname`

Ditto, and for other similar changes as well.


 diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
 index 02ff034..b367ddd 100644
 --- a/build-aux/ltmain.m4sh
 +++ b/build-aux/ltmain.m4sh
 @@ -3042,7 +3042,7 @@ func_extract_archives ()
 $RM 
 unfat-$$/${darwin_base_archive}-${darwin_arch}/${darwin_base_archive}
   done # $darwin_arches
  ## Okay now we've a bunch of thin objects, gotta fatten them up 
 :)
 - darwin_filelist=`find unfat-$$ -type f ... -print | $SED -e 
 $basename | sort -u`
 + darwin_filelist=`find unfat-$$ -type f ... -print | $SED $basename 
 | sort -u`

Why this quote removal?  It seems gratuitous -- even dangerous,
since `$basename' contains shell wildcards (`*' at least).

Regards,
  Stefano

Re: [PATCH 5/7] syntax-check: fix violations and implement sc_useless_braces_in_variable_derefs.

2011-11-21 Thread Eric Blake 
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 Until now, libtool sources have used braced variable names
 seemingly at random! Almost always the braces are just noise, so
 remove all the unnecessary ones.
 * cfg.mk (sc_useless_braces_in_variable_derefs): New syntax
 check rule to ensure we only reintroduce braced variable
 dereferences if they are followed by a valid variable name
 character.
 build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
 build-aux/ltmain.m4sh, build-aux/options-parser, configure.ac,
 libltdl/configure.ac, m4/libtool.m4, m4/ltdl.m4,
 m4/ltoptions.m4, tests/defs.m4sh, tests/demo-nopic.test,
 tests/depdemo/configure.ac, tests/flags.at, tests/link.test,
 tests/objectlist.test, tests/quote.test, tests/static.at: Remove
 spurious braces.
 
 +++ b/build-aux/ltmain.m4sh

 @@ -152,7 +152,7 @@ exec_cmd=
  # Append VALUE to the end of shell variable VAR.
  func_append ()
  {
 -eval ${1}=\$${1}\${2}
 +eval $1=\$$1\$2

Not necessarily correct.  One of the reasons for using ${1} in any m4
code that comprises the m4_define() definition of a macro is that $1 is
replaced by an argument by m4 in expanding the macro, while ${1} is
preserved unchanged through m4 expansion to be saved for the resulting
shell code.  I fear that your global search-and-replace may have been
too eager in m4-related files, but haven't read it closely to check for
sure; even if it didn't, the stylistic convention of ${1} for shell
expansion vs. $1 for m4 expansion made the file slightly easier to maintain.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.

2011-11-21 Thread Eric Blake 
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 To safely use a non-literal first argument to `test', you must
 always prepend a literal non-`-' character, but often the second
 operand is a constant that doesn't begin with a `-' already, so
 always use `test a = $b' instead of noisy `test X$b = Xa'.

Not true.

test a = $b

is just as likely to trigger improper evaluation in buggy test(1)
implementations as:

test $b = a

If you cannot guarantee the contents of $b, then you MUST prefix both
sides of the comparison with x or X.  Conversely, if you CAN guarantee
the contents of $b (for example, if you did b=$?, then you KNOW that b
is a numeric tring with no problematic characters), then you might as
well use the more idiomatic comparison of variable to constant.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.

2011-11-21 Thread Stefano Lattarini 
Hi Gary.  Few more random nits...

On Monday 21 November 2011, Gary V wrote:
 To safely use a non-literal first argument to `test', you must
 always prepend a literal non-`-' character, but often the second
 operand is a constant that doesn't begin with a `-' already, so
 always use `test a = $b' instead of noisy `test X$b = Xa'.

This seems back-bending to me, and slightly unclear to read.  Also,
it goes against the (unofficial) conventions of autoconf, which is
to use either `test x$b = xa' or `test x$b = Xa'.

Also ...

  # Bootstrap this package from checked-out sources.
  # Written by Gary V. Vaughan, 2010
 @@ -1760,7 +1760,7 @@ func_ifcontains ()
;;
  esac
 
 -test $_G_status -eq 0 || exit $_G_status
 +test 0 -eq $_G_status || exit $_G_status
  }
... changes like this seems overly paranoid, in case $_G_status is
expected (as I surmise it is) to be a non-negative integer.  And
if this assumption stps to hold dur to a bug in your code, you are
going to be bitten by much worse problem anyway:

 # $shell is either Solaris 1 0or ATT ksh, Solaris 10 XPG4 sh, or
 # zsh 4.3.12.
 $ $shell -c 'exit t; echo foo'; echo status = $?
 status = 0

Regards,
  Stefano

Re: [PATCH 7/7] syntax-check: fix violations and implement sc_prohibit_sed_s_comma.

2011-11-21 Thread Eric Blake 
On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 I like to name temporary directories that I will remove shortly
 with two leading commas so that they sort lexicographically at
 the top of `ls' output.  Now, `./configure
 --prefix=`pwd`/,,inst' works again, for the first time in
 several years.

 Try to use `|' as the default separator wherever possible,
 otherwise something else that doesn't occur in the substitution
 expression.

I'm in favor of this one.  In particular, one of the reasons that
autoconf documents | as superior to , is that | has to be shell-quoted
to be used, while , does not, which means a user is more likely to have
a filename containing comma than they are to have a filename containing
a pipe.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/7] syntax-check: fix violations and implement sc_useless_quotes_in_assignment.

2011-11-21 Thread Roumen Petrov 

Gary V. Vaughan wrote:
[SNIP]

diff --git a/bootstrap.conf b/bootstrap.conf
index 6f0f0c3..c3491b5 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -77,13 +77,13 @@ gnulib_modules='

  # Extra gnulib files that are not in modules, which override files of
  # the same name installed by other bootstrap tools.
-gnulib_non_module_files=$gnulib_non_module_files'
+gnulib_non_module_files=$gnulib_non_module_files'
  doc/COPYINGv2
  doc/fdl.texi
   

[SNIP]
hmm, origin is only with end apostrophe .

Next one is


 # Extend the existing usage message
-usage_message=$usage_message'
+usage_message=$usage_message'

.
May be exist more places. I did not check rest of the file.


Roumen

Re: [PATCH 2/7] syntax-check: fix violations and implement sc_useless_quotes_in_assignment.

2011-11-21 Thread Eric Blake 
On 11/21/2011 01:59 PM, Roumen Petrov wrote:
 Gary V. Vaughan wrote:
 [SNIP]
 diff --git a/bootstrap.conf b/bootstrap.conf
 index 6f0f0c3..c3491b5 100644
 --- a/bootstrap.conf
 +++ b/bootstrap.conf
 @@ -77,13 +77,13 @@ gnulib_modules='

   # Extra gnulib files that are not in modules, which override files of
   # the same name installed by other bootstrap tools.
 -gnulib_non_module_files=$gnulib_non_module_files'
 +gnulib_non_module_files=$gnulib_non_module_files'
   doc/COPYINGv2
   doc/fdl.texi

 [SNIP]
 hmm, origin is only with end apostrophe .

That's not a problem.  It's merely changing:

var=$expanded'literal'

to the equivalent

var=$expanded'literal'

where the literal portion includes newline characters.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[PATCH 8/7] syntax-check: fix violations and implement sc_useless_quotes_in_case_branch.

2011-11-21 Thread Gary V. Vaughan 
Hi Eric,

On 21 Nov 2011, at 23:15, Eric Blake wrote:
 On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 Contrary to popular belief, Bourne shell does not resplit case
 expressions after expansion, so if there are no shell unquoted
 shell metacharacters or whitespace, the quotes are useless.
 @@ -568,7 +568,7 @@ func_resolve_sysroot ()
 # store the result into func_replace_sysroot_result.
 func_replace_sysroot ()
 {
 -  case $lt_sysroot:$1 in
 +  case $lt_sysroot:$1 in
   ?*:$lt_sysroot*)
 
 Likewise in the pattern expression; you could further change this to:
 
 case $lt_sysroot:$1 in
  ?*:$lt_sysroot*)

Good call, although narrowing the search down to eliminate false positives
is a lot trickier in this case!

I'm adding the following changeset to this series.  Thanks!


Contrary to popular belief, Bourne shell does not resplit case
branch expressions after expansion, so if there are no unquoted
shell metacharacters or whitespace, the quotes are useless.
* cfg.mk (sc_useless_quotes_in_case_branch): New syntax-check
rule to ensure we don't reintroduce useless quoted case branch
expressions.
* bootstrap, build-aux/ltmain.m4sh, tests/quote.test: Remove
spurious quotes.

Signed-off-by: Gary V. Vaughan g...@gnu.org
---
 bootstrap |2 +-
 build-aux/ltmain.m4sh |   26 +-
 cfg.mk|   16 
 tests/quote.test  |8 
 4 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/bootstrap b/bootstrap
index afdde66..49685f2 100755
--- a/bootstrap
+++ b/bootstrap
@@ -1782,7 +1782,7 @@ func_append_u ()
 _G_delim=`expr $2 : '\(.\)'`
 
 case $_G_delim$_G_current_value$_G_delim in
-  *$2$_G_delim*) ;;
+  *$2$_G_delim*) ;;
   *) func_append $@ ;;
 esac
 }
diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 8955838..5cd201f 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -569,7 +569,7 @@ func_resolve_sysroot ()
 func_replace_sysroot ()
 {
   case $lt_sysroot:$1 in
-  ?*:$lt_sysroot*)
+  ?*:$lt_sysroot*)
 func_stripname $lt_sysroot '' $1
 func_replace_sysroot_result='='$func_stripname_result
 ;;
@@ -895,7 +895,7 @@ func_to_tool_file ()
   $debug_cmd
 
   case ,$2, in
-*,$to_tool_file_cmd,*)
+*,$to_tool_file_cmd,*)
   func_to_tool_file_result=$1
   ;;
 *)
@@ -4836,12 +4836,12 @@ func_mode_link ()
*-*-cygwin* | *-*-mingw* | *-*-pw32* | *-*-os2* | *-cegcc*)
  testbindir=`$ECHO $dir | $SED 's*/lib$*/bin*'`
  case :$dllsearchpath: in
- *:$dir:*) ;;
+ *:$dir:*) ;;
  ::) dllsearchpath=$dir;;
  *) func_append dllsearchpath :$dir;;
  esac
  case :$dllsearchpath: in
- *:$testbindir:*) ;;
+ *:$testbindir:*) ;;
  ::) dllsearchpath=$testbindir;;
  *) func_append dllsearchpath :$testbindir;;
  esac
@@ -5895,7 +5895,7 @@ func_mode_link ()
if test -n $shlibpath_var  test -z $avoidtemprpath ; then
  # Make sure the rpath contains only unique directories.
  case $temp_rpath: in
- *$absdir:*) ;;
+ *$absdir:*) ;;
  *) func_append temp_rpath $absdir: ;;
  esac
fi
@@ -6122,7 +6122,7 @@ func_mode_link ()
 
if test -n $add_shlibpath; then
  case :$compile_shlibpath: in
- *:$add_shlibpath:*) ;;
+ *:$add_shlibpath:*) ;;
  *) func_append compile_shlibpath $add_shlibpath: ;;
  esac
fi
@@ -6136,7 +6136,7 @@ func_mode_link ()
 test yes != $hardcode_minus_L 
 test yes = $hardcode_shlibpath_var; then
case :$finalize_shlibpath: in
-   *:$libdir:*) ;;
+   *:$libdir:*) ;;
*) func_append finalize_shlibpath $libdir: ;;
esac
  fi
@@ -6156,7 +6156,7 @@ func_mode_link ()
  add=-l$name
elif test yes = $hardcode_shlibpath_var; then
  case :$finalize_shlibpath: in
- *:$libdir:*) ;;
+ *:$libdir:*) ;;
  *) func_append finalize_shlibpath $libdir: ;;
  esac
  add=-l$name
@@ -7307,7 +7307,7 @@ EOF
else
  # Just accumulate the unique libdirs.
  case 
$hardcode_libdir_separator$hardcode_libdirs$hardcode_libdir_separator in
- 
*$hardcode_libdir_separator$libdir$hardcode_libdir_separator*)
+ *$hardcode_libdir_separator$libdir$hardcode_libdir_separator*)
;;
  *)
func_append hardcode_libdirs 
$hardcode_libdir_separator$libdir
@@ -8034,7 +8034,7 @@ EOF
else
  # Just accumulate the unique libdirs.
  case 
$hardcode_libdir_separator$hardcode_libdirs$hardcode_libdir_separator in
-

Re: [PATCH 3/7] tests: migrate tests/sh.test checks to syntax-checks.

2011-11-21 Thread Gary V. Vaughan 
Hi Stefano,

On 22 Nov 2011, at 02:52, Stefano Lattarini wrote:
 Hi Gary.  Just a quick nit (I haven't looked at the whole
 series, and not even at the whole patch in fact; sorry).

No apologies necessary, every little helps!  Thank you.

 On Monday 21 November 2011, Gary V wrote:
for file
 do
 -  test -f $file || touch $file
 +  test -f $file || touch $file
 
 What's the point of quoting file after `test -f' it it remains
 unquoted after `touch'?

Even though we know there is no whitespace in $file because of the
for loop, there still might be other shell meta-characters in there.
All uses of $file (including a bunch in the following lines) should
be quoted correctly, but that is another patch.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)

Re: [PATCH 4/7] syntax-check: fix violations and implement sc_prohibit_bare_basename.

2011-11-21 Thread Gary V. Vaughan 
On 22 Nov 2011, at 02:59, Stefano Lattarini wrote:
 Hi Gary.  Again, few quick nits here, probably incomplete.

Hi Stefano,

Thanks for taking a look again.

 On Monday 21 November 2011, Gary V wrote:
 * cfg.mk (sc_prohibit_bare_basename, sc_prohibit_basename_with_sed):
 Make sure not to go back to using occasional `|$basename' or
 `|$dirname' syntax.
 * build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
 build-aux/ltmain.m4sh, build-aux/options-parser,
 tests/fcdemo-conf.test, tests/fcdemo-shared.test,
 tests/fcdemo-static.test, tests/libtoolize.at: Fix violations.
 
 Signed-off-by: Gary V. Vaughan g...@gnu.org
 
 diff --git a/build-aux/general.m4sh b/build-aux/general.m4sh
 index 1f44535..790f4e0 100644
 --- a/build-aux/general.m4sh
 +++ b/build-aux/general.m4sh
 @@ -70,15 +70,15 @@ lt_nl='
 '
 IFS= $lt_nl
 
 -dirname='s,/[^/]*$,,'
 -basename='s,^.*/,,'
 +dirname='s|/[^/]*$||'
 +basename='s|^.*/||'
 
 What's the point of this change?  If it's only stylistic, shouldn't it
 be done in a separate patch?

That leaked out of the prohibit_sed_s_comma patch later in the series, so
I've moved it back to the correct patch.  Thanks.

 # func_dirname file append nondir_replacement
 # Compute the dirname of FILE.  If nonempty, add APPEND to the result,
 # otherwise set result to NONDIR_REPLACEMENT.
 func_dirname ()
 {
 -func_dirname_result=`$ECHO ${1} | $SED $dirname`
 +func_dirname_result=`$ECHO ${1} |$SED $dirname`
 
 Ditto, and for other similar changes as well.

Not sure how that happened, maybe cut and pasting between various definitions.
Also fixed.


 diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
 index 02ff034..b367ddd 100644
 --- a/build-aux/ltmain.m4sh
 +++ b/build-aux/ltmain.m4sh
 @@ -3042,7 +3042,7 @@ func_extract_archives ()
$RM 
 unfat-$$/${darwin_base_archive}-${darwin_arch}/${darwin_base_archive}
  done # $darwin_arches
 ## Okay now we've a bunch of thin objects, gotta fatten them up 
 :)
 -darwin_filelist=`find unfat-$$ -type f ... -print | $SED -e 
 $basename | sort -u`
 +darwin_filelist=`find unfat-$$ -type f ... -print | $SED $basename 
 | sort -u`
 
 Why this quote removal?  It seems gratuitous -- even dangerous,
 since `$basename' contains shell wildcards (`*' at least).

Yikes!  Well that's embarrassing.  Nicely caught, thanks.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)

Re: [PATCH 5/7] syntax-check: fix violations and implement sc_useless_braces_in_variable_derefs.

2011-11-21 Thread Gary V. Vaughan 
Hi Eric,

Thanks for the feedback.

On 22 Nov 2011, at 03:05, Eric Blake wrote:

 On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 Until now, libtool sources have used braced variable names
 seemingly at random! Almost always the braces are just noise, so
 remove all the unnecessary ones.
 * cfg.mk (sc_useless_braces_in_variable_derefs): New syntax
 check rule to ensure we only reintroduce braced variable
 dereferences if they are followed by a valid variable name
 character.
 build-aux/general.m4sh, build-aux/git-hooks/commit-msg,
 build-aux/ltmain.m4sh, build-aux/options-parser, configure.ac,
 libltdl/configure.ac, m4/libtool.m4, m4/ltdl.m4,
 m4/ltoptions.m4, tests/defs.m4sh, tests/demo-nopic.test,
 tests/depdemo/configure.ac, tests/flags.at, tests/link.test,
 tests/objectlist.test, tests/quote.test, tests/static.at: Remove
 spurious braces.
 
 +++ b/build-aux/ltmain.m4sh
 
 @@ -152,7 +152,7 @@ exec_cmd=
 # Append VALUE to the end of shell variable VAR.
 func_append ()
 {
 -eval ${1}=\$${1}\${2}
 +eval $1=\$$1\$2
 
 Not necessarily correct.  One of the reasons for using ${1} in any m4
 code that comprises the m4_define() definition of a macro is that $1 is
 replaced by an argument by m4 in expanding the macro, while ${1} is
 preserved unchanged through m4 expansion to be saved for the resulting
 shell code.  I fear that your global search-and-replace may have been
 too eager in m4-related files, but haven't read it closely to check for
 sure; even if it didn't, the stylistic convention of ${1} for shell
 expansion vs. $1 for m4 expansion made the file slightly easier to maintain.

I went back and forth on this myself.

In the end, I didn't want to exclude .m4sh tests from the syntax check
because there's a ton of our messiest shell code in those files, which
is most of the reason for adding the new syntax-checks in the first place.

While it's true that the shell braces do prevent M4 from substituting its
own positional parameters, the vast majority of uses in our .m4sh code are
wrapped in M4SH_VERBATIM([[...]]), so they are left unexpanded for the
shell even with the braces removed.  In the handful of cases where we had
previously saved positional parameters from being prematurely expanded by
M4 using braces, again I'm reluctant to exclude the whole file from the
syntax-check, so I used quadrigraphs instead to keep all the tools happy :)

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)

Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.

2011-11-21 Thread Gary V. Vaughan 
Hi Eric,

On 22 Nov 2011, at 03:07, Eric Blake wrote:

 On 11/21/2011 07:47 AM, Gary V. Vaughan wrote:
 To safely use a non-literal first argument to `test', you must
 always prepend a literal non-`-' character, but often the second
 operand is a constant that doesn't begin with a `-' already, so
 always use `test a = $b' instead of noisy `test X$b = Xa'.
 
 Not true.
 
 test a = $b
 
 is just as likely to trigger improper evaluation in buggy test(1)
 implementations as:
 
 test $b = a

:-o  For real?  On non-museum pieces?

I tested a bunch of /bin/sh, bash, ksh and zsh versions that I have
easy access to, and for all of them, the only way to upset test with
leading hyphens is in the first argument.

 If you cannot guarantee the contents of $b, then you MUST prefix both
 sides of the comparison with x or X.  Conversely, if you CAN guarantee
 the contents of $b (for example, if you did b=$?, then you KNOW that b
 is a numeric tring with no problematic characters), then you might as
 well use the more idiomatic comparison of variable to constant.

I don't suppose you can point me at a shell that chokes or fails on:

   test a != -b || echo bug

?  Or at least some reliable documentation that shows we're not dealing
with outdated dogma here?

I'll postpone pushing this patch until we get to the bottom of the
issue though.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)

Re: [PATCH 6/7] syntax-check: fix violations and implement sc_prohibit_test_const_follows_var.

2011-11-21 Thread Gary V. Vaughan 
Hi Stefano,

On 22 Nov 2011, at 03:13, Stefano Lattarini wrote:
 Hi Gary.  Few more random nits...

Thanks ;)

 On Monday 21 November 2011, Gary V wrote:
 To safely use a non-literal first argument to `test', you must
 always prepend a literal non-`-' character, but often the second
 operand is a constant that doesn't begin with a `-' already, so
 always use `test a = $b' instead of noisy `test X$b = Xa'.
 
 This seems back-bending to me, and slightly unclear to read.  Also,
 it goes against the (unofficial) conventions of autoconf, which is
 to use either `test x$b = xa' or `test x$b = Xa'.

I was unable to find any shells that choke on:

  test a != -b || echo bug

Where it's easy to upset test with:

  test -b != a

 Also ...
 
 # Bootstrap this package from checked-out sources.
 # Written by Gary V. Vaughan, 2010
 @@ -1760,7 +1760,7 @@ func_ifcontains ()
   ;;
 esac
 
 -test $_G_status -eq 0 || exit $_G_status
 +test 0 -eq $_G_status || exit $_G_status
 }
 ... changes like this seems overly paranoid, in case $_G_status is
 expected (as I surmise it is) to be a non-negative integer.  And
 if this assumption stps to hold dur to a bug in your code, you are
 going to be bitten by much worse problem anyway:

Well, in addition to saving a few characters of typing, and being
consistent with other uses of test after this patch, it also prevents
the syntax-check from triggering.

I certainly wouldn't expect any difference in behaviour either way,
even on buggy shells/test implementations.

 # $shell is either Solaris 1 0or ATT ksh, Solaris 10 XPG4 sh, or
 # zsh 4.3.12.
 $ $shell -c 'exit t; echo foo'; echo status = $?
 status = 0

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)

Re: inter-library-dependency, how?

2011-11-21 Thread Bert Wesarg 

Hi,



Hi Nick,

thanks for the quick response.

Am 09.09.2011 18:01, schrieb Nick Bowler:

On 2011-09-09 17:56 +0200, Christian Rössel wrote:

I want to build a program that depends on a libtool library that depends
on a non-libtool library that needs rpath information to be found a 
runtime.

My Makefile.am looks like this:
lib_LTLIBRARIES= libfoo.la
libfoo_la_SOURCES  = libfoo.c  libfoo.h
libfoo_la_CPPFLAGS = -I/opt/packages/papi/4.1.2.1/include
libfoo_la_LDFLAGS  = -L/opt/packages/papi/4.1.2.1/lib
libfoo_la_LIBADD   = -lpapi

bin_PROGRAMS = foo
foo_SOURCES  = foo.c
foo_LDADD= libfoo.la

[...]

Building and linking succeeds, but trying to run ./foo leads to
./foo: error while loading shared libraries: libpapi.so: cannot open
shared object file: No such file or directory

The library is in the location specified by libfoo_la_LDFLAGS (.a and
.so). But the rpath /opt/packages/papi/4.1.2.1/lib is not available. How
do I get it into foo without specifying it as
foo_LDFLAGS = -Wl,-rpath /opt/packages/papi/4.1.2.1/lib?

Looking at 
https://www.gnu.org/software/libtool/manual/libtool.html#Link-mode,

   -R libdir
   If output-file is a program, add libdir to its run-time path. If
   output-file is a library, add -Rlibdir to its dependency_libs, so
   that, whenever the library is linked into a program, libdir will
   be added to its run-time path.

So it seems that adding -R/opt/packages/papi/4.1.2.1/lib to
libfoo_la_LDFLAGS would do the trick?

Well, like the documentations says, -R/opt/packages/papi/4.1.2.1/lib is
added to the dependency_libs:
dependency_libs=' -R/opt/packages/papi/4.1.2.1/lib
-L/opt/packages/papi/4.1.2.1/lib -lpapi'

But when I link libfoo.la into foo, nothing is added to foo's runpath:
libtool: link: gcc -g -O2 -o foo foo.o  ./.libs/libfoo.a
-L/opt/packages/papi/4.1.2.1/lib -lpapi

So, the problem remains. Any help appreciated.


Christian and I independently solved this with the following fix:

diff --git i/libltdl/config/ltmain.m4sh w/libltdl/config/ltmain.m4sh
index ca67c8a..f79ab5b 100644 libltdl/config/ltmain.m4sh
--- i/libltdl/config/ltmain.m4sh
+++ w/libltdl/config/ltmain.m4sh
@@ -6391,8 +6391,7 @@ func_mode_link ()
# Pragmatically, this seems to cause very few problems in
# practice:
case $deplib in
-   -L*) new_libs=$deplib $new_libs ;;
-   -R*) ;;
+   -L*|-R*) new_libs=$deplib $new_libs ;;
*)
  # And here is the reason: when a library appears more
  # than once as an explicit dependence of a library, or

With this patch, the -R flag inside the dependency_libs list is honored 
and appropriately added to the link command.


Is there anything wrong with this approach. If not we propose this patch 
for inclusion. A proper ChangeLog and Signed-of-by line will follow 
than. I will also try to transform Christians example into an test than.


Thanks.

Bert



Thanks,



___
https://lists.gnu.org/mailman/listinfo/libtool

Re: inter-library-dependency, how?

2011-11-21 Thread Bob Friesenhahn 

On Mon, 21 Nov 2011, Bert Wesarg wrote:

Christian and I independently solved this with the following fix:

diff --git i/libltdl/config/ltmain.m4sh w/libltdl/config/ltmain.m4sh
index ca67c8a..f79ab5b 100644 libltdl/config/ltmain.m4sh
--- i/libltdl/config/ltmain.m4sh
+++ w/libltdl/config/ltmain.m4sh
@@ -6391,8 +6391,7 @@ func_mode_link ()
   # Pragmatically, this seems to cause very few problems in
   # practice:
   case $deplib in
-   -L*) new_libs=$deplib $new_libs ;;
-   -R*) ;;
+   -L*|-R*) new_libs=$deplib $new_libs ;;
   *)
 # And here is the reason: when a library appears more
 # than once as an explicit dependence of a library, or

With this patch, the -R flag inside the dependency_libs list is honored and 
appropriately added to the link command.


Is there anything wrong with this approach. If not we propose this patch for 
inclusion. A proper ChangeLog and Signed-of-by line will follow than. I will 
also try to transform Christians example into an test than.


Yes, I think that there is something wrong with this approach.  The -R 
option should not be used to find libraries at link time.  Libraries 
present in the run-time linker search path at link time are not 
necessarily the correct ones to be used for linking.  Run-time linker 
search path and link-time linker search path are totally different 
things.


Bob
--
Bob Friesenhahn
bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,http://www.GraphicsMagick.org/

___
https://lists.gnu.org/mailman/listinfo/libtool

Re: linking problems on SL6

2011-11-21 Thread Peter O'Gorman 

Hi,

On 11/19/2011 01:03 AM, Adam Mercer wrote:

Hi

In building a development snapshot of one of my projects, to a custom
path, on SL6 I am running into what appears to be a linking problem.
The libtool command used to link the library is as follows:

libtool: link: gcc -std=gnu99 -shared  -fPIC -DPIC .libs/Aggregation.o
.libs/FrameCache.o .libs/FrameCalibration.o .libs/FrameData.o
.libs/FrameSeries.o .libs/FrameStream.o .libs/LALFrameIO.o
.libs/LALFrameVCSInfo.o .libs/LowLatencyData.o -Wl,-rpath
-Wl,/usr/lib64 -Wl,-rpath
-Wl,/usr1/ram/lalsuite/lal/packages/support/src/.libs -Wl,-rpath
-Wl,/usr1/ram/lalsuite/lal/lib/.libs -Wl,-rpath -Wl,/usr/lib64
-Wl,-rpath -Wl,/home/ram/lalsuite/lib64 /usr/lib64/libFrame.so
../../lal/packages/support/src/.libs/liblalsupport.so -lz
../../lal/lib/.libs/liblal.so -lgsl -lgslcblas -lfftw3 -lfftw3f -lm
-O2   -Wl,-soname -Wl,liblalframe.so.0 -o .libs/liblalframe.so.0.2.0


Are these -Wl,-rpath flags coming from libtool? They seem to be there 
already on the libtool command line. Could you post this snippet of your 
makefile.am, please?




The problem is that there is the current release version, installed
from the release RPM, in /usr and because of the -Wl,-rpath
-Wl,/usr/lib64 on the link line this older version is getting picked
up in preference to the more recent development version in
/home/ram/lalsuite. Why would libtool be passing /usr/lib64 to the
compiler? On Debian Squeeze, using the same git tag, the build
succeeds so I am led to believe that this is an issue with the
autotools on SL6 but I'm not sure where to start looking. Has any got
any suggestions?


Libtool has issues with multilib systems. Someone had a patch at some 
point that correctly figured out the multilib library paths using 
ldconfig, but I've long forgotten who. You can set 
lt_cv_sys_lib_dlsearch_path_spec and lt_cv_sys_lib_search_path_spec at 
configure time if libtool is getting 
sys_lib_dlsearch_path_spec/sys_lib_search_path_spec wrong (check the 
generated libtool script for ^sys_lib_dlsearch_path_spec and 
^sys_lib_search_path_spec to see what it's guessing).


Debian/Red Hat systems do multilib differently, one has lib32 and lib, 
the other lib and lib64, I wouldn't be surprised if someone has a linux 
system with lib32 lib and lib64 (like IRIX!).


Peter




___
https://lists.gnu.org/mailman/listinfo/libtool

Re: linking problems on SL6

2011-11-21 Thread Adam Mercer 
On Mon, Nov 21, 2011 at 12:21, Peter O'Gorman pe...@pogma.com wrote:

Peter

 Are these -Wl,-rpath flags coming from libtool? They seem to be there
 already on the libtool command line. Could you post this snippet of your
 makefile.am, please?

I am not setting these myself and they don't seem to be coming from
any library dependencies. The section of the Makefile.am that deals
with the library is essentially:

INCLUDES = -I$(top_builddir)/include
LDADD = liblalframe.la

lalframeincludedir = $(includedir)/lal
lalframeinclude_HEADERS = \
Aggregation.h \
FrameCache.h \
FrameCalibration.h \
FrameData.h \
FrameStream.h \
LALFrameConfig.h \
LALFrameIO.h \
LALFrameL.h \
LALFrameVCSInfo.h \
LowLatencyData.h

lib_LTLIBRARIES = liblalframe.la
liblalframe_la_SOURCES = \
Aggregation.c \
FrameCache.c \
FrameCalibration.c \
FrameData.c \
FrameSeries.c \
FrameStream.c \
LALFrameIO.c \
LALFrameVCSInfo.c \
LowLatencyData.c

noinst_HEADERS = \
FrameSeriesRead_source.c \
FrameSeriesWrite_source.c

liblalframe_la_LDFLAGS = -version-info $(LIBVERSION)

This is not the complete Makefile.am, the rest is stuff for test
codes, documentation, etc...

 Libtool has issues with multilib systems. Someone had a patch at some point
 that correctly figured out the multilib library paths using ldconfig, but
 I've long forgotten who. You can set lt_cv_sys_lib_dlsearch_path_spec and
 lt_cv_sys_lib_search_path_spec at configure time if libtool is getting
 sys_lib_dlsearch_path_spec/sys_lib_search_path_spec wrong (check the
 generated libtool script for ^sys_lib_dlsearch_path_spec and
 ^sys_lib_search_path_spec to see what it's guessing).

I have been looking at those variables, they are set as:

# Compile-time system search path for libraries.
sys_lib_search_path_spec=/usr/lib/gcc/x86_64-redhat-linux/4.4.5
/usr/lib64 /lib64 

# Run-time system search path for libraries.
sys_lib_dlsearch_path_spec=/lib /usr/lib /usr/lib64/R/lib
/usr/lib/atlas-3dnow /usr/lib/atlas-sse /usr/lib/atlas-sse2
/usr/lib/atlas-sse3 /usr/lib64/atlas /usr/lib64/mysql
/usr/lib64/octave-3.2.4 /usr/lib64/root 

sys_lib_search_path_spec seems to be what I'd expect, but not
sys_lib_dlsearch_path_spec. If I edit this to contain /lib64 and
/usr/lib64 then the -Wl,-rpath -Wl,/usr/lib64 flags aren't passed
and the library links correctly.

I've been looking through libtool.m4, specifically where is sets
sys_lib_dlsearch_path_spec, and the output seems to be correct given
the code but I'm not very familier with the libtool internals so I
could be mistaken.

Setting lt_cv_sys_lib_dlsearch_path_spec=/lib64 /usr/lib64 at
configure time results in -Wl,-rpath -Wl,/usr/lib64 not being passed
and the correct libraries linked. So this is a workaround but I'd like
to understand why these flags are being added in the first place.

Cheers

Adam

___
https://lists.gnu.org/mailman/listinfo/libtool

Re: linking problems on SL6

2011-11-21 Thread Peter O'Gorman 

On 11/21/2011 03:22 PM, Adam Mercer wrote:

Setting lt_cv_sys_lib_dlsearch_path_spec=/lib64 /usr/lib64 at
configure time results in -Wl,-rpath -Wl,/usr/lib64 not being passed
and the correct libraries linked. So this is a workaround but I'd like
to understand why these flags are being added in the first place.


Glad it works around it. The problem is libtool brokenness, most vendors 
patch around it in their packaged libtool, e.g.


http://pkgs.fedoraproject.org/gitweb/?p=libtool.git;a=blob;f=libtool-2.2.10-rpath.patch;h=d0d6d82178642658e6aca5a28d36813158980ca3;hb=HEAD

Peter

___
https://lists.gnu.org/mailman/listinfo/libtool

Re: inter-library-dependency, how?

2011-11-21 Thread Bert Wesarg 
On Mon, Nov 21, 2011 at 16:31, Bob Friesenhahn
bfrie...@simple.dallas.tx.us wrote:
 On Mon, 21 Nov 2011, Bert Wesarg wrote:

 Christian and I independently solved this with the following fix:

 diff --git i/libltdl/config/ltmain.m4sh w/libltdl/config/ltmain.m4sh
 index ca67c8a..f79ab5b 100644 libltdl/config/ltmain.m4sh
 --- i/libltdl/config/ltmain.m4sh
 +++ w/libltdl/config/ltmain.m4sh
 @@ -6391,8 +6391,7 @@ func_mode_link ()
           # Pragmatically, this seems to cause very few problems in
           # practice:
           case $deplib in
 -           -L*) new_libs=$deplib $new_libs ;;
 -           -R*) ;;
 +           -L*|-R*) new_libs=$deplib $new_libs ;;
           *)
             # And here is the reason: when a library appears more
             # than once as an explicit dependence of a library, or

 With this patch, the -R flag inside the dependency_libs list is honored
 and appropriately added to the link command.

 Is there anything wrong with this approach. If not we propose this patch
 for inclusion. A proper ChangeLog and Signed-of-by line will follow than. I
 will also try to transform Christians example into an test than.

 Yes, I think that there is something wrong with this approach.  The -R
 option should not be used to find libraries at link time.  Libraries present
 in the run-time linker search path at link time are not necessarily the
 correct ones to be used for linking.  Run-time linker search path and
 link-time linker search path are totally different things.

The -R option is not used by libtool to find libraries at link-time,
it tells libtool to add this path to the targets run-time linker
search path. I.e. it passes -Wl,-rpath -Wl,path to the linker.

Thus, this patch does what the documentation says:

https://www.gnu.org/software/libtool/manual/libtool.html#Link-mode

Bert


 Bob

___
https://lists.gnu.org/mailman/listinfo/libtool