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

From: Junio C Hamano <gitster@pobox.com>
To: James Knight <james.d.knight@live.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH v2] build: link with curl-defined linker flags
Date: Mon, 05 Nov 2018 10:22:48 +0900	[thread overview]
Message-ID: <xmqqbm74l3mf.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <SN4PR0701MB3679953BCD417500EC2A267BA0C80@SN4PR0701MB3679.namprd07.prod.outlook.com> (James Knight's message of "Sat, 3 Nov 2018 05:12:11 +0000")

James Knight <james.d.knight@live.com> writes:

> Changes v1 -> v2:
>  - Improved support for detecting curl linker flags when not using a
>     configure-based build (mentioned by Junio C Hamano).
>  - Adding a description on how to explicitly use the CURL_LDFLAGS
>     define when not using configure (suggested by Junio C Hamano).
>
> The original patch made a (bad) assumption that builds would always
> invoke ./configure before attempting to build Git. To support a
> make-invoked build, CURL_LDFLAGS can also be populated using the defined
> curl-config utility. This change also comes with the assumption that
> since both ./configure and Makefile are using curl-config to determine
> aspects of the build, it should be also fine to use the same utility to
> provide the linker flags to compile against (please indicate so if this
> is another bad assumption). With this change, the explicit configuration
> of CURL_LDFLAGS inside config.mak.uname for Minix and NONSTOP_KERNEL
> have been dropped.

Will replace; thanks.

>  Makefile         | 30 +++++++++++++++---------------
>  config.mak.uname |  3 ---
>  configure.ac     | 17 +++++++----------
>  3 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b08d5ea258c69a78745dfa73fe698c11d021858a..36da17dc1f9b1a70c9142604afe989f1eb8ee87f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -59,6 +59,13 @@ all::
>  # Define CURL_CONFIG to curl's configuration program that prints information
>  # about the library (e.g., its version number).  The default is 'curl-config'.
>  #
> +# Define CURL_LDFLAGS to specify flags that you need to link when using libcurl,
> +# if you do not want to rely on the libraries provided by CURL_CONFIG.  The
> +# default value is a result of `curl-config --libs`.  An example value for
> +# CURL_LDFLAGS is as follows:
> +#
> +#     CURL_LDFLAGS=-lcurl
> +#
>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>  # not built, and you cannot push using http:// and https:// transports (dumb).
>  #
> @@ -183,10 +190,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,20 +1308,17 @@ 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
> -	endif
> -	ifdef NEEDS_IDN_WITH_CURL
> -		CURL_LIBCURL += -lidn
> +		CURL_LIBCURL =
>  	endif
>  
> +ifdef CURL_LDFLAGS
> +	CURL_LIBCURL += $(CURL_LDFLAGS)
> +else
> +	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
> +endif
> +
>  	REMOTE_CURL_PRIMARY = git-remote-http$X
>  	REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X
>  	REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> diff --git a/config.mak.uname b/config.mak.uname
> index 8acdeb71fdab3b3e8e3c536110eb6de13f14e8ff..19e6633040b1db4a148d7b33c4e9d374fe7f87ba 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -431,8 +431,6 @@ ifeq ($(uname_S),Minix)
>  	NO_NSEC = YesPlease
>  	NEEDS_LIBGEN =
>  	NEEDS_CRYPTO_WITH_SSL = YesPlease
> -	NEEDS_IDN_WITH_CURL = YesPlease
> -	NEEDS_SSL_WITH_CURL = YesPlease
>  	NEEDS_RESOLV =
>  	NO_HSTRERROR = YesPlease
>  	NO_MMAP = YesPlease
> @@ -458,7 +456,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	# Missdetected, hence commented out, see below.
>  	#NO_CURL = YesPlease
>  	# Added manually, see above.
> -	NEEDS_SSL_WITH_CURL = YesPlease
>  	HAVE_LIBCHARSET_H = YesPlease
>  	HAVE_STRINGS_H = YesPlease
>  	NEEDS_LIBICONV = YesPlease
> diff --git a/configure.ac b/configure.ac
> index e11b7976ab1c93d8ccec2e499d0093db42551059..44e8c036b6ec417e95ca4e5c2861785900d8634c 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