threads / patch / 49740

patchbuild: link with curl-defined linker flags

Subject: [PATCH] build: link with curl-defined linker flags

## tl;dr

2 messages between Nov 2, 2018 and Nov 2, 2018. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

James Knight· Nov 2, 2018, 07:21 UTC · lore

Adjust the autotools configuration to populate libcurl-related linker flags from curl-config instead of manually populating flags based off detected features.

Originally, the configuration would check for SSL-support in the target curl library. If enabled, NEEDS_SSL_WITH_CURL would be set and used in the Makefile to append additional libraries to link against. Since the process is already depending on a curl-config utility to provide curl-related build information, adjusting the build to track the linker flags in CURL_LIBCURL and pass the configuration option into the Makefile.

Signed-off-by: James Knight <james.d.knight@live.com>
---
 Makefile         | 21 +++++++--------------
 config.mak.uname |  5 ++---
 configure.ac     | 17 +++++++----------
 3 files changed, 16 insertions(+), 27 deletions(-)
Show changes to 3 files +16 −27

Makefile, config.mak.uname, configure.ac

diff --git a/Makefile b/Makefile
index b08d5ea25..c3be87b0e 100644
--- a/Makefile
+++ b/Makefile
@@ -183,10 +183,6 @@ all::
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
 #
-# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
-#
-# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
-#
 # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
 #
 # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv.
@@ -1305,18 +1301,15 @@ else
 	ifdef CURLDIR
 		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
 		BASIC_CFLAGS += -I$(CURLDIR)/include
-		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
+		CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
 	else
-		CURL_LIBCURL = -lcurl
-	endif
-	ifdef NEEDS_SSL_WITH_CURL
-		CURL_LIBCURL += -lssl
-		ifdef NEEDS_CRYPTO_WITH_SSL
-			CURL_LIBCURL += -lcrypto
-		endif
+		CURL_LIBCURL =
 	endif
-	ifdef NEEDS_IDN_WITH_CURL
-		CURL_LIBCURL += -lidn
+
+	ifdef CURL_LDFLAGS
+		CURL_LIBCURL += $(CURL_LDFLAGS)
+	else
+		CURL_LIBCURL += -lcurl
 	endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X
diff --git a/config.mak.uname b/config.mak.uname
index 8acdeb71f..923b8fa09 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -431,8 +431,7 @@ ifeq ($(uname_S),Minix)
 	NO_NSEC = YesPlease
 	NEEDS_LIBGEN =
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
-	NEEDS_IDN_WITH_CURL = YesPlease
-	NEEDS_SSL_WITH_CURL = YesPlease
+	CURL_LDFLAGS = -lssl -lcrypto -lidn
 	NEEDS_RESOLV =
 	NO_HSTRERROR = YesPlease
 	NO_MMAP = YesPlease
@@ -458,7 +457,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	# Missdetected, hence commented out, see below.
 	#NO_CURL = YesPlease
 	# Added manually, see above.
-	NEEDS_SSL_WITH_CURL = YesPlease
+	CURL_LDFLAGS = -lssl -lcrypto
 	HAVE_LIBCHARSET_H = YesPlease
 	HAVE_STRINGS_H = YesPlease
 	NEEDS_LIBICONV = YesPlease
diff --git a/configure.ac b/configure.ac
index e11b7976a..44e8c036b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -600,17 +600,14 @@ AC_CHECK_PROG([CURL_CONFIG], [curl-config],
 
 if test $CURL_CONFIG != no; then
     GIT_CONF_SUBST([CURL_CONFIG])
-    if test -z "${NO_OPENSSL}"; then
-      AC_MSG_CHECKING([if Curl supports SSL])
-      if test $(curl-config --features|grep SSL) = SSL; then
-         NEEDS_SSL_WITH_CURL=YesPlease
-         AC_MSG_RESULT([yes])
-      else
-         NEEDS_SSL_WITH_CURL=
-         AC_MSG_RESULT([no])
-      fi
-      GIT_CONF_SUBST([NEEDS_SSL_WITH_CURL])
+
+    if test -z "$CURL_CONFIG_OPTS"; then
+        CURL_CONFIG_OPTS="--libs"
     fi
+
+    CURL_LDFLAGS=$($CURL_CONFIG $CURL_CONFIG_OPTS)
+    AC_MSG_NOTICE([Setting CURL_LDFLAGS to '$CURL_LDFLAGS'])
+    GIT_CONF_SUBST([CURL_LDFLAGS], [$CURL_LDFLAGS])
 fi
 
 fi
-- 
2.19.1
Junio C Hamano· Nov 2, 2018, 15:10 UTC · re: James Knight · lore

Re: [PATCH] build: link with curl-defined linker flags

James Knight <james.d.knight@live.com> writes:
Show 17 quoted lines
>  Makefile         | 21 +++++++--------------
>  config.mak.uname |  5 ++---
>  configure.ac     | 17 +++++++----------
>  3 files changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b08d5ea25..c3be87b0e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -183,10 +183,6 @@ all::
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
>  #
> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
> -#
> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
> -#

Hmm, because the use of autoconf -> ./configure in our build procedure is optional, wouldn't this change give regression to those of us who use these Makefile variables to configure their build, instead of relying on autoconf?

Show 11 quoted lines
> diff --git a/config.mak.uname b/config.mak.uname
> index 8acdeb71f..923b8fa09 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -431,8 +431,7 @@ ifeq ($(uname_S),Minix)
>  	NO_NSEC = YesPlease
>  	NEEDS_LIBGEN =
>  	NEEDS_CRYPTO_WITH_SSL = YesPlease
> -	NEEDS_IDN_WITH_CURL = YesPlease
> -	NEEDS_SSL_WITH_CURL = YesPlease
> +	CURL_LDFLAGS = -lssl -lcrypto -lidn

OK, as long as we describe how to update their config.mak to adjust to the new world order, the "regression" I noticed earlier is not too bad, and the way the new CURL_LDFLAGS variable drives the build is much more direct and transparent than via the old way to specify NEEDS_*_WITH_CURL to affect the build indirectly.

I think such "describe how to configure in the new world order" should go to where NEEDS_*_WITH_CURL used to be, e.g. "Define CURL_LDFLAGS to specify flags that you need to link using -lcurl; see config.mak.uname and look for the variable for examples" or something like that, perhaps.

Thanks.

← back to recent threads