{"thread":{"id":"14978","subject":"Patch for NO_R_TO_GCC_LINKER","startedAt":"2008-08-13T10:42:30Z","lastAt":"2008-08-13T22:12:34Z","messageCount":6,"participants":["Giovanni Funchal","Matthieu Moy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"87040","messageId":"c475e2e60808130342r452fae1cm7d08d8d2206d0468@mail.gmail.com","threadId":"14978","inReplyTo":null,"subject":"Patch for NO_R_TO_GCC_LINKER","fromName":"Giovanni Funchal","fromEmail":"gafunchal@gmail.com","sentAt":"2008-08-13T10:42:30Z","receivedAt":"2008-08-13T10:42:30Z","isPatch":false,"sender":{"key":"gafunchal@gmail.com","avatar":"https://gravatar.com/avatar/96f8068bdfb134f61682ec4a9741b71b582efa9d102055028a88643507644e9c?d=mp&s=160"},"body":"Hi,\n\nI'm proposing the patch below to add a check for linker's runtime\ndynamic library patch switch in configure script.\n\nWhat was broken? Try $ ./configure --prefix=/usr --with-curl=/usr and\nyou will see messages like \"cc: unrecognized option '-R/usr/lib'. This\nis just a message, it is NOT an error nor a warning. This can cause\nwrong versions of curl and other libraries to be linked with git, and\nproduce very very strange results!\n\nThe patch include tests for \"-R\" and \"-Wl,-rpath,\" switches in\nconfigure script and an error message if tests fail. Makefile is\nbetter, no more NO_R_TO_GCC_LINKER option. The default switch is now\n\"-Wl,-rpath,\" (which is the most common one).\n\nI tested it on ubuntu gcc 4.2.3\n\n-- Giovanni\n\ndiff --git a/Makefile b/Makefile\nindex 90c5a13..6e20b08 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -111,9 +111,8 @@ all::\n #\n # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.\n #\n-# Define NO_R_TO_GCC_LINKER if your gcc does not like \"-R/path/lib\"\n-# that tells runtime paths to dynamic libraries;\n-# \"-Wl,-rpath=/path/lib\" is used instead.\n+# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths\n+# to the linker. The default is \"-Wl,-rpath,\".\n #\n # Define USE_NSEC below if you want git to care about sub-second file mtimes\n # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and\n@@ -217,6 +216,7 @@ GITWEB_SITE_FOOTER =\n\n export prefix bindir sharedir htmldir sysconfdir\n\n+# defaults, possibly overridden by config.mak.autogen\n CC = gcc\n AR = ar\n RM = rm -f\n@@ -226,7 +226,8 @@ INSTALL = install\n RPMBUILD = rpmbuild\n TCL_PATH = tclsh\n TCLTK_PATH = wish\n-\n+LD_RUNPATH_SWITCH = -Wl,-rpath,\n+\n export TCL_PATH TCLTK_PATH\n\n # sparse is architecture-neutral, which means that we need to tell it\n@@ -689,7 +690,7 @@ ifeq ($(uname_S),NetBSD)\n \tendif\n \tBASIC_CFLAGS += -I/usr/pkg/include\n \tBASIC_LDFLAGS += -L/usr/pkg/lib\n-\tALL_LDFLAGS += -Wl,-rpath,/usr/pkg/lib\n+\tALL_LDFLAGS += $(LD_RUNPATH_SWITCH)/usr/pkg/lib\n endif\n ifeq ($(uname_S),AIX)\n \tNO_STRCASESTR=YesPlease\n@@ -781,21 +782,13 @@ ifeq ($(uname_S),Darwin)\n \tendif\n endif\n\n-ifdef NO_R_TO_GCC_LINKER\n-\t# Some gcc does not accept and pass -R to the linker to specify\n-\t# the runtime dynamic library path.\n-\tCC_LD_DYNPATH = -Wl,-rpath=\n-else\n-\tCC_LD_DYNPATH = -R\n-endif\n-\n ifdef NO_CURL\n \tBASIC_CFLAGS += -DNO_CURL\n else\n \tifdef CURLDIR\n \t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n \t\tBASIC_CFLAGS += -I$(CURLDIR)/include\n-\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl\n+\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib)\n$(LD_RUNPATH_SWITCH)$(CURLDIR)/$(lib) -lcurl\n \telse\n \t\tCURL_LIBCURL = -lcurl\n \tendif\n@@ -815,7 +808,7 @@ endif\n\n ifdef ZLIB_PATH\n \tBASIC_CFLAGS += -I$(ZLIB_PATH)/include\n-\tEXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib)\n+\tEXTLIBS += -L$(ZLIB_PATH)/$(lib) $(LD_RUNPATH_SWITCH)$(ZLIB_PATH)/$(lib)\n endif\n EXTLIBS += -lz\n\n@@ -828,7 +821,7 @@ ifndef NO_OPENSSL\n \tOPENSSL_LIBSSL = -lssl\n \tifdef OPENSSLDIR\n \t\tBASIC_CFLAGS += -I$(OPENSSLDIR)/include\n-\t\tOPENSSL_LINK = -L$(OPENSSLDIR)/$(lib) $(CC_LD_DYNPATH)$(OPENSSLDIR)/$(lib)\n+\t\tOPENSSL_LINK = -L$(OPENSSLDIR)/$(lib)\n$(LD_RUNPATH_SWITCH)$(OPENSSLDIR)/$(lib)\n \telse\n \t\tOPENSSL_LINK =\n \tendif\n@@ -845,7 +838,7 @@ endif\n ifdef NEEDS_LIBICONV\n \tifdef ICONVDIR\n \t\tBASIC_CFLAGS += -I$(ICONVDIR)/include\n-\t\tICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)\n+\t\tICONV_LINK = -L$(ICONVDIR)/$(lib) $(LD_RUNPATH_SWITCH)$(ICONVDIR)/$(lib)\n \telse\n \t\tICONV_LINK =\n \tendif\ndiff --git a/config.mak.in b/config.mak.in\nindex b776149..8a20ffe 100644\n--- a/config.mak.in\n+++ b/config.mak.in\n@@ -7,6 +7,7 @@ AR = @AR@\n TAR = @TAR@\n #INSTALL = @INSTALL@\t\t# needs install-sh or install.sh in sources\n TCLTK_PATH = @TCLTK_PATH@\n+LD_RUNPATH_SWITCH = @ld_runpath_switch@\n\n prefix = @prefix@\n exec_prefix = @exec_prefix@\ndiff --git a/configure.ac b/configure.ac\nindex 7c2856e..5debbbf 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -103,6 +103,28 @@ GIT_PARSE_WITH(tcltk))\n AC_MSG_NOTICE([CHECKS for programs])\n #\n AC_PROG_CC([cc gcc])\n+# which switch to pass runtime path to dynamic libraries to the linker\n+AC_CACHE_CHECK([if linker supports -R], ld_dashr, [\n+   SAVE_LDFLAGS=\"${LDFLAGS}\"\n+   LDFLAGS=\"${SAVE_LDFLAGS} -R /\"\n+   AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_dashr=yes], [ld_dashr=no])\n+   LDFLAGS=\"${SAVE_LDFLAGS}\"\n+])\n+if test \"$ld_dashr\" = \"yes\"; then\n+   AC_SUBST(ld_runpath_switch, [-R])\n+else\n+   AC_CACHE_CHECK([if linker supports -Wl,rpath,], ld_wl_rpath, [\n+      SAVE_LDFLAGS=\"${LDFLAGS}\"\n+      LDFLAGS=\"${SAVE_LDFLAGS} -Wl,-rpath,/\"\n+      AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_wl_rpath=yes],\n[ld_wl_rpath=no])\n+      LDFLAGS=\"${SAVE_LD_FLAGS}\"\n+   ])\n+   if test \"$ld_wl_rpath\" = \"yes\"; then\n+      AC_SUBST(ld_runpath_switch, [-Wl,-rpath,])\n+   else\n+      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])\n+   fi\n+fi\n #AC_PROG_INSTALL\t\t# needs install-sh or install.sh in sources\n AC_CHECK_TOOLS(AR, [gar ar], :)\n AC_CHECK_PROGS(TAR, [gtar tar])\n"},{"id":"87041","messageId":"vpqtzdpcfpw.fsf@bauges.imag.fr","threadId":"14978","inReplyTo":"c475e2e60808130342r452fae1cm7d08d8d2206d0468@mail.gmail.com","subject":"Re: Patch for NO_R_TO_GCC_LINKER","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-08-13T11:32:59Z","receivedAt":"2008-08-13T11:32:59Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\"Giovanni Funchal\" <gafunchal@gmail.com> writes:\n\n> Hi,\n\nHi Giovanni, glad to see you here ;-).\n\n> I'm proposing the patch below\n\nPlease, read Documentation/SubmittingPatches.\n\nI advise you to use git send-email to send your patches, this one has\n(gmail-related) whitespace damage. Read about Signed-Off-By too.\n\n> @@ -226,7 +226,8 @@ INSTALL = install\n>  RPMBUILD = rpmbuild\n>  TCL_PATH = tclsh\n>  TCLTK_PATH = wish\n> -\n> +LD_RUNPATH_SWITCH = -Wl,-rpath,\n> +\n\nWhitespace damage: the - and the + are equal (I suppose you added a\ntrailing whitespace, which you shouldn't have, and gmail stripped it).\n\n> -\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl\n> +\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib)\n> $(LD_RUNPATH_SWITCH)$(CURLDIR)/$(lib) -lcurl\n\nWhitespace damage again.\n\n> +      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])\n\nI don't think you should error out here: Git can still be compiled and\ncan still run without this support (just use $LD_LIBRARY_PATH if\nneeded). Keeping -R with a big warning would avoid having a regression\nin this case.\n\nThanks,\n\n-- \nMatthieu\n"},{"id":"87096","messageId":"7vy730n0ac.fsf@gitster.siamese.dyndns.org","threadId":"14978","inReplyTo":"c475e2e60808130342r452fae1cm7d08d8d2206d0468@mail.gmail.com","subject":"Re: Patch for NO_R_TO_GCC_LINKER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T20:10:51Z","receivedAt":"2008-08-13T20:10:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giovanni Funchal\" <gafunchal@gmail.com> writes:\n\n> diff --git a/Makefile b/Makefile\n> index 90c5a13..6e20b08 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -111,9 +111,8 @@ all::\n>  #\n>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.\n>  #\n> -# Define NO_R_TO_GCC_LINKER if your gcc does not like \"-R/path/lib\"\n> -# that tells runtime paths to dynamic libraries;\n> -# \"-Wl,-rpath=/path/lib\" is used instead.\n> +# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths\n> +# to the linker. The default is \"-Wl,-rpath,\".\n\nCan you make your change to configure.ac to minimize changes to the\nMakefile?\n\nIn this project, use of configure is strictly optional and Makefile is\nmore canonical than autoconf generated configure.  Unless absolutely\nnecessary, I'd prefer to have a solution that does _not_ change the set of\nmake variables people need to override from the command line.\n\nChanges to configure.ac so that generated script sets NO_R_TO_GCC_LINKER\nappropriately would fit the current model much better and would not break\npeople's existing setups that do not use configure.\n"},{"id":"87108","messageId":"c475e2e60808131420h5b7350abq188b3f8a53b51d98@mail.gmail.com","threadId":"14978","inReplyTo":"7vy730n0ac.fsf@gitster.siamese.dyndns.org","subject":"Re: Patch for NO_R_TO_GCC_LINKER","fromName":"Giovanni Funchal","fromEmail":"gafunchal@gmail.com","sentAt":"2008-08-13T21:20:27Z","receivedAt":"2008-08-13T21:20:27Z","isPatch":false,"sender":{"key":"gafunchal@gmail.com","avatar":"https://gravatar.com/avatar/96f8068bdfb134f61682ec4a9741b71b582efa9d102055028a88643507644e9c?d=mp&s=160"},"body":"> In this project, use of configure is strictly optional and Makefile is\n> more canonical than autoconf generated configure.  Unless absolutely\n> necessary, I'd prefer to have a solution that does _not_ change the set of\n> make variables people need to override from the command line.\n\nMy changes should not break a lot of people's setups. Most people will\nfind the new default better because it works straight on\nlinux/windows. I have tested it on ubuntu gcc 4.2.3, fedora gcc 3.2.3\nand sunos gcc 3.4.2 and only sunos require ./configure or command line\narg. In addition this should provide better support for people running\nAIX, IRIX and HP-UX which, to my best knowledge, have a different way\naround runtime paths. Although very improbable, the changes might\nindeed break some setups, but keep reading...\n\n> Changes to configure.ac so that generated script sets NO_R_TO_GCC_LINKER\n> appropriately would fit the current model much better and would not break\n> people's existing setups that do not use configure.\n\nThis is contradictory... how can changes ONLY to configure.ac make\nrpath work for people NOT using configure?\n\nTry to face it like this: current Makefile support for rpath is\nbroken. Defaults are bad and the NO_R_TO_GCC_LINKER is totally ugly\nand do not cover the case when neither \"-Wl,rpath,\" nor \"-R\" are the\nright choice. In addition if you do not test for flags support\n(running configure), you cannot have better warnings when things go\nwrong.\n\nRegards,\n-- Giovanni\n\nOn Wed, Aug 13, 2008 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Giovanni Funchal\" <gafunchal@gmail.com> writes:\n>\n>> diff --git a/Makefile b/Makefile\n>> index 90c5a13..6e20b08 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -111,9 +111,8 @@ all::\n>>  #\n>>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.\n>>  #\n>> -# Define NO_R_TO_GCC_LINKER if your gcc does not like \"-R/path/lib\"\n>> -# that tells runtime paths to dynamic libraries;\n>> -# \"-Wl,-rpath=/path/lib\" is used instead.\n>> +# LD_RUNPATH_SWITCH specifies how to pass the runtime dynamic library paths\n>> +# to the linker. The default is \"-Wl,-rpath,\".\n>\n> Can you make your change to configure.ac to minimize changes to the\n> Makefile?\n>\n> In this project, use of configure is strictly optional and Makefile is\n> more canonical than autoconf generated configure.  Unless absolutely\n> necessary, I'd prefer to have a solution that does _not_ change the set of\n> make variables people need to override from the command line.\n>\n> Changes to configure.ac so that generated script sets NO_R_TO_GCC_LINKER\n> appropriately would fit the current model much better and would not break\n> people's existing setups that do not use configure.\n>\n>\n>\n>\n"},{"id":"87114","messageId":"7vhc9olh4n.fsf@gitster.siamese.dyndns.org","threadId":"14978","inReplyTo":"c475e2e60808131420h5b7350abq188b3f8a53b51d98@mail.gmail.com","subject":"Re: Patch for NO_R_TO_GCC_LINKER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T21:50:00Z","receivedAt":"2008-08-13T21:50:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Giovanni Funchal\" <gafunchal@gmail.com> writes:\n\n> My changes should not break a lot of people's setups. Most people will\n> find the new default better because it works straight on\n> linux/windows.\n\nMy understanding is that Linux/Windows people won't be using\nNO_R_TO_GCC_LINKER.  What I was afraid of breaking was people who have\ntheir own config.mak (which is included by Makefile) to customize the way\nthe linkage works, setting NO_R_TO_GCC_LINKER appropriately.  Doesn't your\nchange to the Makefile in a way not to pay attention to the variable break\nthem?\n"},{"id":"87117","messageId":"7vbpzwlg31.fsf@gitster.siamese.dyndns.org","threadId":"14978","inReplyTo":"7vhc9olh4n.fsf@gitster.siamese.dyndns.org","subject":"Re: Patch for NO_R_TO_GCC_LINKER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T22:12:34Z","receivedAt":"2008-08-13T22:12:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> \"Giovanni Funchal\" <gafunchal@gmail.com> writes:\n>\n>> My changes should not break a lot of people's setups. Most people will\n>> find the new default better because it works straight on\n>> linux/windows.\n>\n> My understanding is that Linux/Windows people won't be using\n> NO_R_TO_GCC_LINKER.  What I was afraid of breaking was people who have\n> their own config.mak (which is included by Makefile) to customize the way\n> the linkage works, setting NO_R_TO_GCC_LINKER appropriately.  Doesn't your\n> change to the Makefile in a way not to pay attention to the variable break\n> them?\n\nIn other words, wouldn't a patch like this (based on your change to\nconfigure.ac, but I only hacked autoconf in very distant past so the\ndetails may be wrong) be with much less impact to existing users and\nachieve the same autodetection that configure.ac currently does not do?\n\nI am not sure about another aspect of your change, which changes the use\nof \"-Wl,-rpath=$(path)\" to \"-Wl,-rpath,$(path)\".  If both of them always\nwork (or neither of them and -R works), that would be great.  Otherwise,\nif we need to detect one platform that accepts only -Wl,-rpath,$(path) and\nother ones that accept only -Wl,-rpath=$(path), then it would make more\nsense to check them separately in configure.ac, and override CC_LD_DYNPATH\ndirectly, in which case Makefile needs to be modified as well.\n\n config.mak.in |    1 +\n configure.ac  |   22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/config.mak.in b/config.mak.in\nindex b776149..c079864 100644\n--- a/config.mak.in\n+++ b/config.mak.in\n@@ -7,6 +7,7 @@ AR = @AR@\n TAR = @TAR@\n #INSTALL = @INSTALL@\t\t# needs install-sh or install.sh in sources\n TCLTK_PATH = @TCLTK_PATH@\n+NO_R_TO_GCC_LINKER = @no_r_to_gcc_linker@\n \n prefix = @prefix@\n exec_prefix = @exec_prefix@\ndiff --git a/configure.ac b/configure.ac\nindex 7c2856e..a23a7f6 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -103,6 +103,28 @@ GIT_PARSE_WITH(tcltk))\n AC_MSG_NOTICE([CHECKS for programs])\n #\n AC_PROG_CC([cc gcc])\n+# which switch to pass runtime path to dynamic libraries to the linker\n+AC_CACHE_CHECK([if linker supports -R], ld_dashr, [\n+   SAVE_LDFLAGS=\"${LDFLAGS}\"\n+   LDFLAGS=\"${SAVE_LDFLAGS} -R /\"\n+   AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_dashr=yes], [ld_dashr=no])\n+   LDFLAGS=\"${SAVE_LDFLAGS}\"\n+])\n+if test \"$ld_dashr\" = \"yes\"; then\n+   AC_SUBST(no_r_to_gcc_linker, [ ])\n+else\n+   AC_CACHE_CHECK([if linker supports -Wl,rpath,], ld_wl_rpath, [\n+      SAVE_LDFLAGS=\"${LDFLAGS}\"\n+      LDFLAGS=\"${SAVE_LDFLAGS} -Wl,-rpath,/\"\n+      AC_LINK_IFELSE(AC_LANG_PROGRAM([], []), [ld_wl_rpath=yes], [ld_wl_rpath=no])\n+      LDFLAGS=\"${SAVE_LD_FLAGS}\"\n+   ])\n+   if test \"$ld_wl_rpath\" = \"yes\"; then\n+      AC_SUBST(no_r_to_gcc_linker, [YesPlease])\n+   else\n+      AC_MSG_ERROR([no linker support for runtime path to dynamic libraries])\n+   fi\n+fi\n #AC_PROG_INSTALL\t\t# needs install-sh or install.sh in sources\n AC_CHECK_TOOLS(AR, [gar ar], :)\n AC_CHECK_PROGS(TAR, [gtar tar])\n"}]}