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

[Remi Pommarel <repk@triplefau.lt>]

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