{"thread":{"id":"40486","subject":"[PATCH v3 1/1] Makefile: link libcurl before libssl","startedAt":"2015-10-05T19:24:03Z","lastAt":"2015-10-21T16:52:05Z","messageCount":6,"participants":["Remi Pommarel","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":1},"messages":[{"id":"271153","messageId":"1444073043-24618-1-git-send-email-repk@triplefau.lt","threadId":"40486","inReplyTo":null,"subject":"[PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Remi Pommarel","fromEmail":"repk@triplefau.lt","sentAt":"2015-10-05T19:24:03Z","receivedAt":"2015-10-05T19:24:03Z","isPatch":true,"sender":{"key":"repk@triplefau.lt","avatar":"https://avatars.githubusercontent.com/u/6833759?v=4"},"body":"For static linking especially library order while linking is important. For\nexample, libcurl wants symbols for libssl when building http-push, http-fetch\nand remote-curl. So for these programs libcurl has to be linked before libssl.\n\nSigned-off-by: Remi Pommarel <repk@triplefau.lt>\n---\n Makefile | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ce0cfe2..59a0747 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H\n endif\n \n IMAP_SEND_BUILDDEPS =\n-IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n \n ifdef NO_CURL\n \tBASIC_CFLAGS += -DNO_CURL\n@@ -1086,6 +1085,7 @@ else\n \t\tendif\n \tendif\n endif\n+IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n \n ifdef ZLIB_PATH\n \tBASIC_CFLAGS += -I$(ZLIB_PATH)/include\n@@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n \n git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-\t\t$(LIBS) $(CURL_LIBCURL)\n+\t\t$(CURL_LIBCURL) $(LIBS)\n git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-\t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n+\t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n \n git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \\\n@@ -1988,7 +1988,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)\n \n $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n-\t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n+\t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n \n $(LIB_FILE): $(LIB_OBJS)\n \t$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^\n-- \n2.0.1\n"},{"id":"271155","messageId":"20151005194134.GD11993@google.com","threadId":"40486","inReplyTo":"1444073043-24618-1-git-send-email-repk@triplefau.lt","subject":"Re: [PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-10-05T19:41:34Z","receivedAt":"2015-10-05T19:41:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Remi Pommarel wrote:\n\n> For static linking especially library order while linking is important. For\n\nPlease be more explicit for the uninitiated: during static linking, the\nlibrary that needs symbols comes before the library providing those symbols.\n\n> example, libcurl wants symbols for libssl when building http-push, http-fetch\n> and remote-curl. So for these programs libcurl has to be linked before libssl.\n\nBased on experience with NEEDS_CRYPTO_WITH_SSL vs NEEDS_SSL_WITH_CRYPTO,\nthese kinds of dependencies can be platform-specific.  But a little quick\nthought reveals that libssl is never going to depend on libcurl and libcurl\nalways needs libssl.  Good.\n\nBased on this dependency, shouldn't CURL_LIBCURL always include -lssl when\nstatically linking?  How does this relate to NEEDS_SSL_WITH_CURL?\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H\n>  endif\n>  \n>  IMAP_SEND_BUILDDEPS =\n> -IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n\nTo protect against a value that might leak in from the environment, this\nshould say\n\n\tIMAP_SEND_LDFLAGS =\n\n[...]\n> @@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n>  \n>  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> -\t\t$(LIBS) $(CURL_LIBCURL)\n> +\t\t$(CURL_LIBCURL) $(LIBS)\n\nWhat happens in the NEEDS_SSL_WITH_CURL=Yes case?\n\n>  git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> -\t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n> +\t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n\nSame question.\n\nI guess the general principle is that $(LIBS) should always go at the\nend.  (That would make sense to me.)\n\nIdeally this would be two patches:\n\n - one putting $(LIBS) at the end everywhere, which is the simple part of the change\n - a second doing some appropriate thing to turn on NEEDS_SSL_WITH_CURL when appropriate\n   or something\n\nSensible?\n\nThanks,\nJonathan\n"},{"id":"271160","messageId":"20151005201619.GA386@cruxbox","threadId":"40486","inReplyTo":"20151005194134.GD11993@google.com","subject":"Re: [PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Remi Pommarel","fromEmail":"repk@triplefau.lt","sentAt":"2015-10-05T20:16:19Z","receivedAt":"2015-10-05T20:16:19Z","isPatch":true,"sender":{"key":"repk@triplefau.lt","avatar":"https://avatars.githubusercontent.com/u/6833759?v=4"},"body":"On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:\n> Remi Pommarel wrote:\n\n[...]\n\n> Based on this dependency, shouldn't CURL_LIBCURL always include -lssl when\n> statically linking?  How does this relate to NEEDS_SSL_WITH_CURL?\n\nIn fact libcurl will only need -lssl when the curl library has been\nstatically compiled with ssl support. That is why NEEDS_SSL_WITH_CURL is\nuseful.\n\n> \n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H\n> >  endif\n> >  \n> >  IMAP_SEND_BUILDDEPS =\n> > -IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)\n> \n> To protect against a value that might leak in from the environment, this\n> should say\n> \n> \tIMAP_SEND_LDFLAGS =\n> \n> [...]\n\nOups my bad.\n\n> > @@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)\n> >  \n> >  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)\n> >  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> > -\t\t$(LIBS) $(CURL_LIBCURL)\n> > +\t\t$(CURL_LIBCURL) $(LIBS)\n> \n> What happens in the NEEDS_SSL_WITH_CURL=Yes case?\n> \n\nIn the NEEDS_SSL_WITH_CURL=YesPlease case, $(CURL_LIBCURL) will be\n\"-lcurl -lssl\" and $(LIBS) will have \"... -lz\". That's important to put\n$(LIBS) at the end because libcurl will also need symbols from zlib.\n\nMaybe taking the -lssl example here could be misleading. In fact, putting\n$(LIBS) at the end really fix the libcurl's need for zlib and libintl.\n\n> >  git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)\n> >  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n> > -\t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n> > +\t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n> \n> Same question.\n> \n> I guess the general principle is that $(LIBS) should always go at the\n> end.  (That would make sense to me.)\n> \n> Ideally this would be two patches:\n> \n>  - one putting $(LIBS) at the end everywhere, which is the simple part of the change\n>  - a second doing some appropriate thing to turn on NEEDS_SSL_WITH_CURL when appropriate\n>    or something\n> \n> Sensible?\n\nAgreed, that is why I have sent another patch here\nhttp://marc.info/?l=git&m=144312206220612. This patch make ./configure\nto autodetect for libcurl's need for -lssl.\n\nThanks\n\n-- \nRémi\n"},{"id":"271993","messageId":"xmqqsi5570n1.fsf@gitster.mtv.corp.google.com","threadId":"40486","inReplyTo":"20151005201619.GA386@cruxbox","subject":"Re: [PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T20:20:18Z","receivedAt":"2015-10-20T20:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remi Pommarel <repk@triplefau.lt> writes:\n\n> On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:\n> ...\n>> To protect against a value that might leak in from the environment, this\n>> should say\n>> \n>> \tIMAP_SEND_LDFLAGS =\n>> \n>> [...]\n>\n> Oups my bad.\n> ...\n\nSo, what's the status of this patch and other two patches (I\nconsider them as a three-patch series)?\n\nThanks.\n"},{"id":"272040","messageId":"20151021164406.GC13308@cruxbox","threadId":"40486","inReplyTo":"xmqqsi5570n1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Remi Pommarel","fromEmail":"repk@triplefau.lt","sentAt":"2015-10-21T16:44:06Z","receivedAt":"2015-10-21T16:44:06Z","isPatch":true,"sender":{"key":"repk@triplefau.lt","avatar":"https://avatars.githubusercontent.com/u/6833759?v=4"},"body":"On Tue, Oct 20, 2015 at 01:20:18PM -0700, Junio C Hamano wrote:\n> Remi Pommarel <repk@triplefau.lt> writes:\n> \n> > On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:\n> > ...\n> >> To protect against a value that might leak in from the environment, this\n> >> should say\n> >> \n> >> \tIMAP_SEND_LDFLAGS =\n> >> \n> >> [...]\n> >\n> > Oups my bad.\n> > ...\n> \n> So, what's the status of this patch and other two patches (I\n> consider them as a three-patch series)?\n\nSo I have to fix the non initialized variable and to rephrase a litle\nbit the description for this patch. Taking libssl as an example is\nmisleading, zlib is more appropriate. I'll resend another version shortly.\n\nFor patch \"[PATCH v3 1/1] Makefile: make curl-config path configurable\"\nit has been reviewed by Jonathan Nieder with a litle modification to be\nsquashed in. I can resend a squashed in version if it is easier for you.\n\nSorry for the delay.\n\nThanks\n"},{"id":"272041","messageId":"xmqqy4ew5fm2.fsf@gitster.mtv.corp.google.com","threadId":"40486","inReplyTo":"20151021164406.GC13308@cruxbox","subject":"Re: [PATCH v3 1/1] Makefile: link libcurl before libssl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-21T16:52:05Z","receivedAt":"2015-10-21T16:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remi Pommarel <repk@triplefau.lt> writes:\n\n> On Tue, Oct 20, 2015 at 01:20:18PM -0700, Junio C Hamano wrote:\n>> \n>> So, what's the status of this patch and other two patches (I\n>> consider them as a three-patch series)?\n>\n> So I have to fix the non initialized variable and to rephrase a litle\n> bit the description for this patch. Taking libssl as an example is\n> misleading, zlib is more appropriate. I'll resend another version shortly.\n>\n> For patch \"[PATCH v3 1/1] Makefile: make curl-config path configurable\"\n> it has been reviewed by Jonathan Nieder with a litle modification to be\n> squashed in. I can resend a squashed in version if it is easier for you.\n\nYes, please.\n\n> Sorry for the delay.\n\nNo need to say sorry; people do their part at their own pace and\nthat is perfectly fine.  I just wanted to make sure that some sign\nof this topic not being forgotten would stay as I expunge older\nmessages from my mailbox ;-)\n\nThanks.\n"}]}