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

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

[Eric Sunshine]

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/

> 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

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

[Junio C Hamano]

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.


>
> 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 $@ $^

[PATCH] Makefile: Change library order for static linking

[Remi Pommarel]

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

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

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

[Remi Pommarel]

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.

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

[Remi Pommarel]

On Thu, Sep 17, 2015 at 03:25:00PM -0400, Eric Sunshine wrote:
> 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.

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

[Junio C Hamano]

Remi Pommarel <repk@triplefau.lt> writes:

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

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

[Remi Pommarel]

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

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

[Junio C Hamano]

Remi Pommarel <repk@triplefau.lt> writes:

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

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

[Remi Pommarel]

On Mon, Sep 21, 2015 at 10:09:52AM -0700, Junio C Hamano wrote:
> 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.

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

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

[Junio C Hamano]

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.

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