threads / patch / 40486

v3Makefile: link libcurl before libssl

Subject: [PATCH v3 1/1] Makefile: link libcurl before libssl

## tl;dr

6 messages between Oct 5, 2015 and Oct 21, 2015. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Remi Pommarel· Oct 5, 2015, 19:24 UTC · lore

For static linking especially library order while linking is important. For example, libcurl wants symbols for libssl when building http-push, http-fetch and remote-curl. So for these programs libcurl has to be linked before libssl.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to Makefile +4 −4
diff --git a/Makefile b/Makefile
index ce0cfe2..59a0747 100644
--- a/Makefile
+++ b/Makefile
@@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H
 endif
 
 IMAP_SEND_BUILDDEPS =
-IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
 ifdef NO_CURL
 	BASIC_CFLAGS += -DNO_CURL
@@ -1086,6 +1085,7 @@ else
 		endif
 	endif
 endif
+IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
 ifdef ZLIB_PATH
 	BASIC_CFLAGS += -I$(ZLIB_PATH)/include
@@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
 
 git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(CURL_LIBCURL)
+		$(CURL_LIBCURL) $(LIBS)
 git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
 git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \
@@ -1988,7 +1988,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 
 $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
+		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^

-- 
2.0.1
Jonathan Nieder· Oct 5, 2015, 19:41 UTC · re: Remi Pommarel · lore

Re: [PATCH v3 1/1] Makefile: link libcurl before libssl

Remi Pommarel wrote:
> For static linking especially library order while linking is important. For

Please be more explicit for the uninitiated: during static linking, the library that needs symbols comes before the library providing those symbols.

> example, libcurl wants symbols for libssl when building http-push, http-fetch
> and remote-curl. So for these programs libcurl has to be linked before libssl.

Based on experience with NEEDS_CRYPTO_WITH_SSL vs NEEDS_SSL_WITH_CRYPTO, these kinds of dependencies can be platform-specific. But a little quick thought reveals that libssl is never going to depend on libcurl and libcurl always needs libssl. Good.

Based on this dependency, shouldn't CURL_LIBCURL always include -lssl when statically linking? How does this relate to NEEDS_SSL_WITH_CURL?

Show 7 quoted lines
> --- a/Makefile
> +++ b/Makefile
> @@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H
>  endif
>  
>  IMAP_SEND_BUILDDEPS =
> -IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)

To protect against a value that might leak in from the environment, this should say

	IMAP_SEND_LDFLAGS =
[...]
Show 6 quoted lines
> @@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
>  
>  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS) $(CURL_LIBCURL)
> +		$(CURL_LIBCURL) $(LIBS)
What happens in the NEEDS_SSL_WITH_CURL=Yes case?
>  git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> +		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
Same question.

I guess the general principle is that $(LIBS) should always go at the end. (That would make sense to me.)

Ideally this would be two patches:
 - one putting $(LIBS) at the end everywhere, which is the simple part of the change
 - a second doing some appropriate thing to turn on NEEDS_SSL_WITH_CURL when appropriate
   or something
Sensible?

Thanks, Jonathan

Remi Pommarel· Oct 5, 2015, 20:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH v3 1/1] Makefile: link libcurl before libssl

On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:
> Remi Pommarel wrote:
[...]
> Based on this dependency, shouldn't CURL_LIBCURL always include -lssl when
> statically linking?  How does this relate to NEEDS_SSL_WITH_CURL?

In fact libcurl will only need -lssl when the curl library has been statically compiled with ssl support. That is why NEEDS_SSL_WITH_CURL is useful.

Show 15 quoted lines
> 
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1029,7 +1029,6 @@ ifdef HAVE_ALLOCA_H
> >  endif
> >  
> >  IMAP_SEND_BUILDDEPS =
> > -IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
> 
> To protect against a value that might leak in from the environment, this
> should say
> 
> 	IMAP_SEND_LDFLAGS =
> 
> [...]
Oups my bad.
Show 9 quoted lines
> > @@ -1971,10 +1971,10 @@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
> >  
> >  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
> >  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> > -		$(LIBS) $(CURL_LIBCURL)
> > +		$(CURL_LIBCURL) $(LIBS)
> 
> What happens in the NEEDS_SSL_WITH_CURL=Yes case?
>

In the NEEDS_SSL_WITH_CURL=YesPlease case, $(CURL_LIBCURL) will be "-lcurl -lssl" and $(LIBS) will have "... -lz". That's important to put $(LIBS) at the end because libcurl will also need symbols from zlib.

Maybe taking the -lssl example here could be misleading. In fact, putting $(LIBS) at the end really fix the libcurl's need for zlib and libintl.

Show 17 quoted lines
> >  git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)
> >  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> > -		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> > +		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
> 
> Same question.
> 
> I guess the general principle is that $(LIBS) should always go at the
> end.  (That would make sense to me.)
> 
> Ideally this would be two patches:
> 
>  - one putting $(LIBS) at the end everywhere, which is the simple part of the change
>  - a second doing some appropriate thing to turn on NEEDS_SSL_WITH_CURL when appropriate
>    or something
> 
> Sensible?

Agreed, that is why I have sent another patch here http://marc.info/?l=git&m=144312206220612. This patch make ./configure to autodetect for libcurl's need for -lssl.

Thanks
-- 
Rémi
Junio C Hamano· Oct 20, 2015, 20:20 UTC · re: Remi Pommarel · lore

Re: [PATCH v3 1/1] Makefile: link libcurl before libssl

Remi Pommarel <repk@triplefau.lt> writes:
Show 11 quoted lines
> On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:
> ...
>> To protect against a value that might leak in from the environment, this
>> should say
>> 
>> 	IMAP_SEND_LDFLAGS =
>> 
>> [...]
>
> Oups my bad.
> ...

So, what's the status of this patch and other two patches (I consider them as a three-patch series)?

Thanks.
Remi Pommarel· Oct 21, 2015, 16:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 1/1] Makefile: link libcurl before libssl

On Tue, Oct 20, 2015 at 01:20:18PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> Remi Pommarel <repk@triplefau.lt> writes:
> 
> > On Mon, Oct 05, 2015 at 12:41:34PM -0700, Jonathan Nieder wrote:
> > ...
> >> To protect against a value that might leak in from the environment, this
> >> should say
> >> 
> >> 	IMAP_SEND_LDFLAGS =
> >> 
> >> [...]
> >
> > Oups my bad.
> > ...
> 
> So, what's the status of this patch and other two patches (I
> consider them as a three-patch series)?

So I have to fix the non initialized variable and to rephrase a litle bit the description for this patch. Taking libssl as an example is misleading, zlib is more appropriate. I'll resend another version shortly.

For patch "[PATCH v3 1/1] Makefile: make curl-config path configurable" it has been reviewed by Jonathan Nieder with a litle modification to be squashed in. I can resend a squashed in version if it is easier for you.

Sorry for the delay.
Thanks
Junio C Hamano· Oct 21, 2015, 16:52 UTC · re: Remi Pommarel · lore

Re: [PATCH v3 1/1] Makefile: link libcurl before libssl

Remi Pommarel <repk@triplefau.lt> writes:
Show 12 quoted lines
> On Tue, Oct 20, 2015 at 01:20:18PM -0700, Junio C Hamano wrote:
>> 
>> So, what's the status of this patch and other two patches (I
>> consider them as a three-patch series)?
>
> So I have to fix the non initialized variable and to rephrase a litle
> bit the description for this patch. Taking libssl as an example is
> misleading, zlib is more appropriate. I'll resend another version shortly.
>
> For patch "[PATCH v3 1/1] Makefile: make curl-config path configurable"
> it has been reviewed by Jonathan Nieder with a litle modification to be
> squashed in. I can resend a squashed in version if it is easier for you.
Yes, please.
> Sorry for the delay.

No need to say sorry; people do their part at their own pace and that is perfectly fine. I just wanted to make sure that some sign of this topic not being forgotten would stay as I expunge older messages from my mailbox ;-)

Thanks.

← back to recent threads