* [PATCH v3 1/1] Makefile: link libcurl before libssl
@ 2015-10-05 19:24 Remi Pommarel
2015-10-05 19:41 ` Jonathan Nieder
0 siblings, 1 reply; 6+ messages in thread
From: Remi Pommarel @ 2015-10-05 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Brandon Casey, Jeff King, Jonathan Nieder,
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] Makefile: link libcurl before libssl
2015-10-05 19:24 [PATCH v3 1/1] Makefile: link libcurl before libssl Remi Pommarel
@ 2015-10-05 19:41 ` Jonathan Nieder
2015-10-05 20:16 ` Remi Pommarel
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Nieder @ 2015-10-05 19:41 UTC (permalink / raw)
To: Remi Pommarel; +Cc: git, Junio C Hamano, Brandon Casey, Jeff King
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] Makefile: link libcurl before libssl
2015-10-05 19:41 ` Jonathan Nieder
@ 2015-10-05 20:16 ` Remi Pommarel
2015-10-20 20:20 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Remi Pommarel @ 2015-10-05 20:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Brandon Casey, Jeff King
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] Makefile: link libcurl before libssl
2015-10-05 20:16 ` Remi Pommarel
@ 2015-10-20 20:20 ` Junio C Hamano
2015-10-21 16:44 ` Remi Pommarel
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2015-10-20 20:20 UTC (permalink / raw)
To: Remi Pommarel; +Cc: Jonathan Nieder, git, Brandon Casey, Jeff King
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] Makefile: link libcurl before libssl
2015-10-20 20:20 ` Junio C Hamano
@ 2015-10-21 16:44 ` Remi Pommarel
2015-10-21 16:52 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Remi Pommarel @ 2015-10-21 16:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Brandon Casey, Jeff King
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] Makefile: link libcurl before libssl
2015-10-21 16:44 ` Remi Pommarel
@ 2015-10-21 16:52 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2015-10-21 16:52 UTC (permalink / raw)
To: Remi Pommarel; +Cc: Jonathan Nieder, git, Brandon Casey, Jeff King
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-21 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-05 19:24 [PATCH v3 1/1] Makefile: link libcurl before libssl Remi Pommarel
2015-10-05 19:41 ` Jonathan Nieder
2015-10-05 20:16 ` Remi Pommarel
2015-10-20 20:20 ` Junio C Hamano
2015-10-21 16:44 ` Remi Pommarel
2015-10-21 16:52 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).