threads / patch / 40373

patchMakefile: Change library order for static linking

Subject: [PATCH] Makefile: Change library order for static linking

## tl;dr

10 messages between Sep 17, 2015 and Sep 22, 2015. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Remi Pommarel· Sep 17, 2015, 21:12 UTC · lore

For static linking especially library order while linking is important. For example libssl contains symbol from libcrypto so the farmer should be linked before the latter. The global link order should be libcurl then libssl then libcrypto then libintl and finally zlib.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to Makefile +5 −5
diff --git a/Makefile b/Makefile
index 81ac5bb..26a483a 100644
--- a/Makefile
+++ b/Makefile
@@ -1030,7 +1030,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
@@ -1087,6 +1086,7 @@ else
 		endif
 	endif
 endif
+IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 
 ifdef ZLIB_PATH
 	BASIC_CFLAGS += -I$(ZLIB_PATH)/include
@@ -1117,7 +1117,7 @@ ifdef NO_OPENSSL
 	LIB_4_CRYPTO =
 else
 ifdef NEEDS_SSL_WITH_CRYPTO
-	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
+	LIB_4_CRYPTO = $(OPENSSL_LINK) -lssl -lcrypto
 else
 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
 endif
@@ -1972,10 +1972,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) \
@@ -1989,7 +1989,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
Eric Sunshine· Sep 17, 2015, 19:25 UTC · re: Remi Pommarel · lore

Re: [PATCH] Makefile: Change library order for static linking

On Thu, Sep 17, 2015 at 5:12 PM, Remi Pommarel <repk@triplefau.lt> wrote:
> For static linking especially library order while linking is important. For
> example libssl contains symbol from libcrypto so the farmer should be linked
I hope you meant: s/farmer/former/
Show 61 quoted lines
> before the latter. The global link order should be libcurl then libssl then
> libcrypto then libintl and finally zlib.
>
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  Makefile | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 81ac5bb..26a483a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1030,7 +1030,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
> @@ -1087,6 +1086,7 @@ else
>                 endif
>         endif
>  endif
> +IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
>
>  ifdef ZLIB_PATH
>         BASIC_CFLAGS += -I$(ZLIB_PATH)/include
> @@ -1117,7 +1117,7 @@ ifdef NO_OPENSSL
>         LIB_4_CRYPTO =
>  else
>  ifdef NEEDS_SSL_WITH_CRYPTO
> -       LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
> +       LIB_4_CRYPTO = $(OPENSSL_LINK) -lssl -lcrypto
>  else
>         LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
>  endif
> @@ -1972,10 +1972,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) \
> @@ -1989,7 +1989,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
Remi Pommarel· Sep 17, 2015, 21:31 UTC · re: Eric Sunshine · lore

Re: [PATCH] Makefile: Change library order for static linking

On Thu, Sep 17, 2015 at 03:25:00PM -0400, Eric Sunshine wrote:
Show 6 quoted lines
> On Thu, Sep 17, 2015 at 5:12 PM, Remi Pommarel <repk@triplefau.lt> wrote:
> > For static linking especially library order while linking is important. For
> > example libssl contains symbol from libcrypto so the farmer should be linked
> 
> I hope you meant: s/farmer/former/
> 
Oups sorry for the typo, yes I did mean former.
Junio C Hamano· Sep 17, 2015, 19:51 UTC · re: Remi Pommarel · lore

Re: [PATCH] Makefile: Change library order for static linking

Remi Pommarel <repk@triplefau.lt> writes:
> Subject: Re: [PATCH] Makefile: Change library order for static linking
s/Change/change/;

But more importantly, I think you can do much better with the same space. Perhaps like this.

    Makefile: link libcurl before openssl and crypto
> For static linking especially library order while linking is important. For
> example libssl contains symbol from libcrypto so the farmer should be linked
> before the latter. The global link order should be libcurl then libssl then
> libcrypto then libintl and finally zlib.

IIRC historically the division between ssl and crypto was messy. Especially, I am not sure if the change to NEEDS_SSL_WITH_CRYPTO in this patch is correct for platforms that require that macro defined.

Other changes in this patch looks sensible, though.
Show 57 quoted lines
>
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  Makefile | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 81ac5bb..26a483a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1030,7 +1030,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
> @@ -1087,6 +1086,7 @@ else
>  		endif
>  	endif
>  endif
> +IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
>  
>  ifdef ZLIB_PATH
>  	BASIC_CFLAGS += -I$(ZLIB_PATH)/include
> @@ -1117,7 +1117,7 @@ ifdef NO_OPENSSL
>  	LIB_4_CRYPTO =
>  else
>  ifdef NEEDS_SSL_WITH_CRYPTO
> -	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
> +	LIB_4_CRYPTO = $(OPENSSL_LINK) -lssl -lcrypto
>  else
>  	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
>  endif
> @@ -1972,10 +1972,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) \
> @@ -1989,7 +1989,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 $@ $^
Remi Pommarel· Sep 17, 2015, 21:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: Change library order for static linking

On Thu, Sep 17, 2015 at 12:51:18PM -0700, Junio C Hamano wrote:
> IIRC historically the division between ssl and crypto was messy.
> Especially, I am not sure if the change to NEEDS_SSL_WITH_CRYPTO
> in this patch is correct for platforms that require that macro
> defined.

I think it is correct as anywhere else in Makefile the order is ssl first then crypto (from Makefile:1047):

        ifdef NEEDS_SSL_WITH_CURL
                CURL_LIBCURL += -lssl
                ifdef NEEDS_CRYPTO_WITH_SSL
                        CURL_LIBCURL += -lcrypto
                endif
        endif
> 
> Other changes in this patch looks sensible, though.

If you agree with the NEED_SSL_WITH_CURL modification, I'll resend a patch with the typos corrected.

Junio C Hamano· Sep 17, 2015, 22:08 UTC · re: Remi Pommarel · lore

Re: [PATCH] Makefile: Change library order for static linking

Remi Pommarel <repk@triplefau.lt> writes:
Show 8 quoted lines
> On Thu, Sep 17, 2015 at 12:51:18PM -0700, Junio C Hamano wrote:
>> IIRC historically the division between ssl and crypto was messy.
>> Especially, I am not sure if the change to NEEDS_SSL_WITH_CRYPTO
>> in this patch is correct for platforms that require that macro
>> defined.
>
> I think it is correct as anywhere else in Makefile the order is ssl
> first then crypto (from Makefile:1047):

IIRC, NEEDS_SSL_WITH_CRYPTO was invented for people whose platform needed that to be different

597c9cc5 (Flatten tools/ directory to make build procedure simpler., 2005-09-07) has this bit:

    +ifdef NEEDS_SSL_WITH_CRYPTO
    +       LIB_4_CRYPTO = -lcrypto -lssl
    +else
    +       LIB_4_CRYPTO = -lcrypto
    +endif

That is, for most people, linking with -lcrypto was sufficient, but some people needed to grab things from -lssl when they need to do so, because things in -lcrypto referred to what was only in -lssl.

Are you on such a platform? If so can you test these both ways and make sure you are not breaking things for those on such a platform?

Remi Pommarel· Sep 18, 2015, 07:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: Change library order for static linking

On Thu, Sep 17, 2015 at 03:08:25PM -0700, Junio C Hamano wrote:
> That is, for most people, linking with -lcrypto was sufficient, but
> some people needed to grab things from -lssl when they need to do
> so, because things in -lcrypto referred to what was only in -lssl.

Oh I see what you mean, I misinterpreted what NEEDS_SSL_WITH_CRYPTO does.

The problem on static compiled target is that libcurl.a got linked with libssl symbols. The proper variable I should have used is NEEDS_SSL_WITH_CURL. But this variable is not setted on Linux and not configurable, this is why I wrongly used NEEDS_SSL_WITH_CRYPTO.

I see several ways to fix static compilation:
	1) Make NEED_SSL_WITH_CURL overridable by configure (the same
	way NEEDS_SSL_WITH_CRYPTO is). Then static target should run
	"NEED_SSL_WITH_CURL=YesPlease ./configure"
	2) Make configure know that static compilation is asked (ie
	./configure --static) and automatically set NEED_SSL_WITH_CURL.
	3) Use "curl-config --static-libs" to fill CURL_LIBCURL in
	Makefile when ones use "./configure --static".
Junio C Hamano· Sep 21, 2015, 17:09 UTC · re: Remi Pommarel · lore

Re: [PATCH] Makefile: Change library order for static linking

Remi Pommarel <repk@triplefau.lt> writes:
Show 10 quoted lines
> On Thu, Sep 17, 2015 at 03:08:25PM -0700, Junio C Hamano wrote:
>> That is, for most people, linking with -lcrypto was sufficient, but
>> some people needed to grab things from -lssl when they need to do
>> so, because things in -lcrypto referred to what was only in -lssl.
>
> Oh I see what you mean, I misinterpreted what NEEDS_SSL_WITH_CRYPTO
> does.
>
> The problem on static compiled target is that libcurl.a got linked with
> libssl symbols. 
I think I said this already, but I found these bits from your patch

- $(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) + $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)

that first uses libcurl and then libs (which has ssl bits) totally sensible. Shouldn't that be sufficient?

> The proper variable I should have used is
> NEEDS_SSL_WITH_CURL. But this variable is not setted on Linux and not
> configurable,...

Really? Anything is configurable from your config.mak, I would have thought.

Remi Pommarel· Sep 22, 2015, 20:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: Change library order for static linking

On Mon, Sep 21, 2015 at 10:09:52AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> I think I said this already, but I found these bits from your patch
> 
> -		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
> +		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
> 
> that first uses libcurl and then libs (which has ssl bits) totally
> sensible.  Shouldn't that be sufficient?

That is not sufficient on its own because -lssl is not added in $(LIBS), it is only added in $(CURL_LIBCURL) if NEEDS_SSL_WITH_CURL is set.

Show 7 quoted lines
> 
> > The proper variable I should have used is
> > NEEDS_SSL_WITH_CURL. But this variable is not setted on Linux and not
> > configurable,...
> 
> Really?  Anything is configurable from your config.mak, I would have
> thought.

You right using the parts you find sensible and adding "NEEDS_SSL_WITH_CURL=YesPlease" in config.mak makes git compile fine. So I will resubmit with only the sensible part of the patch.

I have one last question thought. Wouldn't it be nice if we had configure to autodetect the need for -lssl with libcurl ? We could make configure to check for Curl_ssl_init function symbol in libcurl, which is only present if libcurl has been compiled with openssl support, by adding something like that in configure.ac:

AC_CHECK_LIB([curl], [Curl_ssl_init],
	[NEEDS_SSL_WITH_CURL=YesPlease],
	[NEEDS_SSL_WITH_CURL=])

The thing that I'm afraid of is that checking a function that is not part of official libcurl API could be not very reliable, don't you think ?

Junio C Hamano· Sep 22, 2015, 21:30 UTC · re: Remi Pommarel · lore

Re: [PATCH] Makefile: Change library order for static linking

Remi Pommarel <repk@triplefau.lt> writes:
> I have one last question thought. Wouldn't it be nice if we had
> configure to autodetect the need for -lssl with libcurl?
Sure.  I do not think anybody tried, but it would be nice.
Show 11 quoted lines
> We could make
> configure to check for Curl_ssl_init function symbol in libcurl,
> which is only present if libcurl has been compiled with openssl support,
> by adding something like that in configure.ac:
>
> AC_CHECK_LIB([curl], [Curl_ssl_init],
> 	[NEEDS_SSL_WITH_CURL=YesPlease],
> 	[NEEDS_SSL_WITH_CURL=])
>
> The thing that I'm afraid of is that checking a function that is not part
> of official libcurl API could be not very reliable, don't you think ?
That is true.

To be bluntly honest, use of autoconf (and configure generated by it) in this project is optional, so I would not worry too much if you misidentify a version of cURL that does not need -lssl as needing one (or vice versa). As long as other parts of the resulting ./configure keeps working (read: emitting a syntactically broken shell script is not an OK offence) and if there is a way to work around the result of misidentification made by ./configure (read: config.mak can cure all gotchas made by config.mak.autogen), it would be fine to use something like the above snippet you gave us as the starting point and it will help some positive number of people. We have to start from somewhere if we want to get there.

People who find versions of libcURL that gets misidentified will send in fixes if it matters to them.

Thanks.

← back to recent threads