Patch for NO_R_TO_GCC_LINKER

Patch for NO_R_TO_GCC_LINKER

[Giovanni Funchal]

Hi,

I'm proposing the patch below to add a check for linker's runtime
dynamic library patch switch in configure script.

What was broken? Try $ ./configure --prefix=/usr --with-curl=/usr and
you will see messages like "cc: unrecognized option '-R/usr/lib'. This
is just a message, it is NOT an error nor a warning. This can cause
wrong versions of curl and other libraries to be linked with git, and
produce very very strange results!

The patch include tests for "-R" and "-Wl,-rpath," switches in
configure script and an error message if tests fail. Makefile is
better, no more NO_R_TO_GCC_LINKER option. The default switch is now
"-Wl,-rpath," (which is the most common one).

I tested it on ubuntu gcc 4.2.3

-- Giovanni

diff --git a/Makefile b/Makefile
index 90c5a13..6e20b08 100644
--- a/Makefile
+++ b/Makefile
@@ -111,9 +111,8 @@ all::
 #
 # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
 #
-# Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
-# that tells runtime paths to dynamic libraries;
-# "-Wl,-rpath=/path/lib" is used instead.
+# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths
+# to the linker. The default is "-Wl,-rpath,".
 #
 # Define USE_NSEC below if you want git to care about sub-second file mtimes
 # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
@@ -217,6 +216,7 @@ GITWEB_SITE_FOOTER =

 export prefix bindir sharedir htmldir sysconfdir

+# defaults, possibly overridden by config.mak.autogen
 CC = gcc
 AR = ar
 RM = rm -f
@@ -226,7 +226,8 @@ INSTALL = install
 RPMBUILD = rpmbuild
 TCL_PATH = tclsh
 TCLTK_PATH = wish
-
+LD_RUNPATH_SWITCH = -Wl,-rpath,
+
 export TCL_PATH TCLTK_PATH

 # sparse is architecture-neutral, which means that we need to tell it
@@ -689,7 +690,7 @@ ifeq ($(uname_S),NetBSD)
 	endif
 	BASIC_CFLAGS += -I/usr/pkg/include
 	BASIC_LDFLAGS += -L/usr/pkg/lib
-	ALL_LDFLAGS += -Wl,-rpath,/usr/pkg/lib
+	ALL_LDFLAGS += $(LD_RUNPATH_SWITCH)/usr/pkg/lib
 endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
@@ -781,21 +782,13 @@ ifeq ($(uname_S),Darwin)
 	endif
 endif

-ifdef NO_R_TO_GCC_LINKER
-	# Some gcc does not accept and pass -R to the linker to specify
-	# the runtime dynamic library path.
-	CC_LD_DYNPATH = -Wl,-rpath=
-else
-	CC_LD_DYNPATH = -R
-endif
-
 ifdef NO_CURL
 	BASIC_CFLAGS += -DNO_CURL
 else
 	ifdef CURLDIR
 		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
 		BASIC_CFLAGS += -I$(CURLDIR)/include
-		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
+		CURL_LIBCURL = -L$(CURLDIR)/$(lib)
$(LD_RUNPATH_SWITCH)$(CURLDIR)/$(lib) -lcurl
 	else
 		CURL_LIBCURL = -lcurl
 	endif
@@ -815,7 +808,7 @@ endif

 ifdef ZLIB_PATH
 	BASIC_CFLAGS += -I$(ZLIB_PATH)/include
-	EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib)
+	EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(LD_RUNPATH_SWITCH)$(ZLIB_PATH)/$(lib)
 endif
 EXTLIBS += -lz

@@ -828,7 +821,7 @@ ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
 		BASIC_CFLAGS += -I$(OPENSSLDIR)/include
-		OPENSSL_LINK = -L$(OPENSSLDIR)/$(lib) $(CC_LD_DYNPATH)$(OPENSSLDIR)/$(lib)
+		OPENSSL_LINK = -L$(OPENSSLDIR)/$(lib)
$(LD_RUNPATH_SWITCH)$(OPENSSLDIR)/$(lib)
 	else
 		OPENSSL_LINK =
 	endif
@@ -845,7 +838,7 @@ endif
 ifdef NEEDS_LIBICONV
 	ifdef ICONVDIR
 		BASIC_CFLAGS += -I$(ICONVDIR)/include
-		ICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
+		ICONV_LINK = -L$(ICONVDIR)/$(lib) $(LD_RUNPATH_SWITCH)$(ICONVDIR)/$(lib)
 	else
 		ICONV_LINK =
 	endif
diff --git a/config.mak.in b/config.mak.in
index b776149..8a20ffe 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -7,6 +7,7 @@ AR = @AR@
 TAR = @TAR@
 #INSTALL = @INSTALL@		# needs install-sh or install.sh in sources
 TCLTK_PATH = @TCLTK_PATH@
+LD_RUNPATH_SWITCH = @ld_runpath_switch@

 prefix = @prefix@
 exec_prefix = @exec_prefix@
diff --git a/configure.ac b/configure.ac
index 7c2856e..5debbbf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -103,6 +103,28 @@ GIT_PARSE_WITH(tcltk))
 AC_MSG_NOTICE([CHECKS for programs])
 #
 AC_PROG_CC([cc gcc])
+# which switch to pass runtime path to dynamic libraries to the linker
+AC_CACHE_CHECK([if linker supports -R], ld_dashr, [
+   SAVE_LDFLAGS="${LDFLAGS}"
+   LDFLAGS="${SAVE_LDFLAGS} -R /"
+   AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_dashr=yes], [ld_dashr=no])
+   LDFLAGS="${SAVE_LDFLAGS}"
+])
+if test "$ld_dashr" = "yes"; then
+   AC_SUBST(ld_runpath_switch, [-R])
+else
+   AC_CACHE_CHECK([if linker supports -Wl,rpath,], ld_wl_rpath, [
+      SAVE_LDFLAGS="${LDFLAGS}"
+      LDFLAGS="${SAVE_LDFLAGS} -Wl,-rpath,/"
+      AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_wl_rpath=yes],
[ld_wl_rpath=no])
+      LDFLAGS="${SAVE_LD_FLAGS}"
+   ])
+   if test "$ld_wl_rpath" = "yes"; then
+      AC_SUBST(ld_runpath_switch, [-Wl,-rpath,])
+   else
+      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])
+   fi
+fi
 #AC_PROG_INSTALL		# needs install-sh or install.sh in sources
 AC_CHECK_TOOLS(AR, [gar ar], :)
 AC_CHECK_PROGS(TAR, [gtar tar])

Re: Patch for NO_R_TO_GCC_LINKER

[Matthieu Moy]

"Giovanni Funchal" <gafunchal@gmail.com> writes:

> Hi,

Hi Giovanni, glad to see you here ;-).

> I'm proposing the patch below

Please, read Documentation/SubmittingPatches.

I advise you to use git send-email to send your patches, this one has
(gmail-related) whitespace damage. Read about Signed-Off-By too.

> @@ -226,7 +226,8 @@ INSTALL = install
>  RPMBUILD = rpmbuild
>  TCL_PATH = tclsh
>  TCLTK_PATH = wish
> -
> +LD_RUNPATH_SWITCH = -Wl,-rpath,
> +

Whitespace damage: the - and the + are equal (I suppose you added a
trailing whitespace, which you shouldn't have, and gmail stripped it).

> -		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
> +		CURL_LIBCURL = -L$(CURLDIR)/$(lib)
> $(LD_RUNPATH_SWITCH)$(CURLDIR)/$(lib) -lcurl

Whitespace damage again.

> +      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])

I don't think you should error out here: Git can still be compiled and
can still run without this support (just use $LD_LIBRARY_PATH if
needed). Keeping -R with a big warning would avoid having a regression
in this case.

Thanks,

-- 
Matthieu

Re: Patch for NO_R_TO_GCC_LINKER

[Junio C Hamano]

"Giovanni Funchal" <gafunchal@gmail.com> writes:

> diff --git a/Makefile b/Makefile
> index 90c5a13..6e20b08 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -111,9 +111,8 @@ all::
>  #
>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
>  #
> -# Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
> -# that tells runtime paths to dynamic libraries;
> -# "-Wl,-rpath=/path/lib" is used instead.
> +# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths
> +# to the linker. The default is "-Wl,-rpath,".

Can you make your change to configure.ac to minimize changes to the
Makefile?

In this project, use of configure is strictly optional and Makefile is
more canonical than autoconf generated configure.  Unless absolutely
necessary, I'd prefer to have a solution that does _not_ change the set of
make variables people need to override from the command line.

Changes to configure.ac so that generated script sets NO_R_TO_GCC_LINKER
appropriately would fit the current model much better and would not break
people's existing setups that do not use configure.

Re: Patch for NO_R_TO_GCC_LINKER

[Giovanni Funchal]

> In this project, use of configure is strictly optional and Makefile is
> more canonical than autoconf generated configure.  Unless absolutely
> necessary, I'd prefer to have a solution that does _not_ change the set of
> make variables people need to override from the command line.

My changes should not break a lot of people's setups. Most people will
find the new default better because it works straight on
linux/windows. I have tested it on ubuntu gcc 4.2.3, fedora gcc 3.2.3
and sunos gcc 3.4.2 and only sunos require ./configure or command line
arg. In addition this should provide better support for people running
AIX, IRIX and HP-UX which, to my best knowledge, have a different way
around runtime paths. Although very improbable, the changes might
indeed break some setups, but keep reading...

> Changes to configure.ac so that generated script sets NO_R_TO_GCC_LINKER
> appropriately would fit the current model much better and would not break
> people's existing setups that do not use configure.

This is contradictory... how can changes ONLY to configure.ac make
rpath work for people NOT using configure?

Try to face it like this: current Makefile support for rpath is
broken. Defaults are bad and the NO_R_TO_GCC_LINKER is totally ugly
and do not cover the case when neither "-Wl,rpath," nor "-R" are the
right choice. In addition if you do not test for flags support
(running configure), you cannot have better warnings when things go
wrong.

Regards,
-- Giovanni

On Wed, Aug 13, 2008 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Giovanni Funchal" <gafunchal@gmail.com> writes:
>
>> diff --git a/Makefile b/Makefile
>> index 90c5a13..6e20b08 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -111,9 +111,8 @@ all::
>>  #
>>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
>>  #
>> -# Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
>> -# that tells runtime paths to dynamic libraries;
>> -# "-Wl,-rpath=/path/lib" is used instead.
>> +# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths
>> +# to the linker. The default is "-Wl,-rpath,".
>
> Can you make your change to configure.ac to minimize changes to the
> Makefile?
>
> In this project, use of configure is strictly optional and Makefile is
> more canonical than autoconf generated configure.  Unless absolutely
> necessary, I'd prefer to have a solution that does _not_ change the set of
> make variables people need to override from the command line.
>
> Changes to configure.ac so that generated script sets NO_R_TO_GCC_LINKER
> appropriately would fit the current model much better and would not break
> people's existing setups that do not use configure.
>
>
>
>

Re: Patch for NO_R_TO_GCC_LINKER

[Junio C Hamano]

"Giovanni Funchal" <gafunchal@gmail.com> writes:

> My changes should not break a lot of people's setups. Most people will
> find the new default better because it works straight on
> linux/windows.

My understanding is that Linux/Windows people won't be using
NO_R_TO_GCC_LINKER.  What I was afraid of breaking was people who have
their own config.mak (which is included by Makefile) to customize the way
the linkage works, setting NO_R_TO_GCC_LINKER appropriately.  Doesn't your
change to the Makefile in a way not to pay attention to the variable break
them?

Re: Patch for NO_R_TO_GCC_LINKER

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Giovanni Funchal" <gafunchal@gmail.com> writes:
>
>> My changes should not break a lot of people's setups. Most people will
>> find the new default better because it works straight on
>> linux/windows.
>
> My understanding is that Linux/Windows people won't be using
> NO_R_TO_GCC_LINKER.  What I was afraid of breaking was people who have
> their own config.mak (which is included by Makefile) to customize the way
> the linkage works, setting NO_R_TO_GCC_LINKER appropriately.  Doesn't your
> change to the Makefile in a way not to pay attention to the variable break
> them?

In other words, wouldn't a patch like this (based on your change to
configure.ac, but I only hacked autoconf in very distant past so the
details may be wrong) be with much less impact to existing users and
achieve the same autodetection that configure.ac currently does not do?

I am not sure about another aspect of your change, which changes the use
of "-Wl,-rpath=$(path)" to "-Wl,-rpath,$(path)".  If both of them always
work (or neither of them and -R works), that would be great.  Otherwise,
if we need to detect one platform that accepts only -Wl,-rpath,$(path) and
other ones that accept only -Wl,-rpath=$(path), then it would make more
sense to check them separately in configure.ac, and override CC_LD_DYNPATH
directly, in which case Makefile needs to be modified as well.

 config.mak.in |    1 +
 configure.ac  |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index b776149..c079864 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -7,6 +7,7 @@ AR = @AR@
 TAR = @TAR@
 #INSTALL = @INSTALL@		# needs install-sh or install.sh in sources
 TCLTK_PATH = @TCLTK_PATH@
+NO_R_TO_GCC_LINKER = @no_r_to_gcc_linker@
 
 prefix = @prefix@
 exec_prefix = @exec_prefix@
diff --git a/configure.ac b/configure.ac
index 7c2856e..a23a7f6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -103,6 +103,28 @@ GIT_PARSE_WITH(tcltk))
 AC_MSG_NOTICE([CHECKS for programs])
 #
 AC_PROG_CC([cc gcc])
+# which switch to pass runtime path to dynamic libraries to the linker
+AC_CACHE_CHECK([if linker supports -R], ld_dashr, [
+   SAVE_LDFLAGS="${LDFLAGS}"
+   LDFLAGS="${SAVE_LDFLAGS} -R /"
+   AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_dashr=yes], [ld_dashr=no])
+   LDFLAGS="${SAVE_LDFLAGS}"
+])
+if test "$ld_dashr" = "yes"; then
+   AC_SUBST(no_r_to_gcc_linker, [ ])
+else
+   AC_CACHE_CHECK([if linker supports -Wl,rpath,], ld_wl_rpath, [
+      SAVE_LDFLAGS="${LDFLAGS}"
+      LDFLAGS="${SAVE_LDFLAGS} -Wl,-rpath,/"
+      AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_wl_rpath=yes], [ld_wl_rpath=no])
+      LDFLAGS="${SAVE_LD_FLAGS}"
+   ])
+   if test "$ld_wl_rpath" = "yes"; then
+      AC_SUBST(no_r_to_gcc_linker, [YesPlease])
+   else
+      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])
+   fi
+fi
 #AC_PROG_INSTALL		# needs install-sh or install.sh in sources
 AC_CHECK_TOOLS(AR, [gar ar], :)
 AC_CHECK_PROGS(TAR, [gtar tar])