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

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

[Remi Pommarel]

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(-)

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

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

[Jonathan Nieder]

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?

> --- 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 =

[...]
> @@ -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

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

[Remi Pommarel]

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.

> 
> > --- 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.

> > @@ -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.

> >  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

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

[Junio C Hamano]

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)?

Thanks.

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

[Remi Pommarel]

On Tue, Oct 20, 2015 at 01:20:18PM -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

Remi Pommarel <repk@triplefau.lt> writes:

> 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.