Possible bug in Makefile when executing curl-config

Possible bug in Makefile when executing curl-config

[Raitanen, Adam]

I believe there may be a bug in the Makefile introduced by the following commit:

https://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e568ca4

The commit was merged in 2.20.0:

* The way -lcurl library gets linked has been simplified by taking
   advantage of the fact that we can just ask curl-config command how.

Unfortunately it assumes that curl-config is in the path which is not always the case. When using "--with-curl=/path/to/curl" in the configure command, the path to the actual curl-config executable is ignored and the build fails around here:

    CC http-fetch.o
make: curl-config: Command not found
    LINK git-http-fetch
http.o: In function `fill_active_slots':
/tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'
.

We were able to workaround this by forcing the correct path into the make env:

make CURL_LDFLAGS="$(/path/to/curl/curl-config --libs)".

I reproduced the problem in the latest version 2.22.0.

Thanks,
Adam

Re: Possible bug in Makefile when executing curl-config

[Jeff King]

[+cc the author of that patch]

On Mon, Jul 22, 2019 at 07:46:37PM +0000, Raitanen, Adam wrote:

> I believe there may be a bug in the Makefile introduced by the following commit:
> 
> https://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e568ca4
> 
> The commit was merged in 2.20.0:
> 
> * The way -lcurl library gets linked has been simplified by taking
>    advantage of the fact that we can just ask curl-config command how.
> 
> Unfortunately it assumes that curl-config is in the path which is not
> always the case. When using "--with-curl=/path/to/curl" in the
> configure command, the path to the actual curl-config executable is
> ignored and the build fails around here:
> 
>     CC http-fetch.o
> make: curl-config: Command not found
>     LINK git-http-fetch
> http.o: In function `fill_active_slots':
> /tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'
> .
> 
> We were able to workaround this by forcing the correct path into the make env:
> 
> make CURL_LDFLAGS="$(/path/to/curl/curl-config --libs)".
> 
> I reproduced the problem in the latest version 2.22.0.

For the case without autoconf, I think using CURL_LDFLAGS is the
intended safety valve. Though perhaps we should be falling back more
gracefully to the old behavior, like:

diff --git a/Makefile b/Makefile
index 11ccea4071..27e546bbfc 100644
--- a/Makefile
+++ b/Makefile
@@ -1343,7 +1343,7 @@ else
 ifdef CURL_LDFLAGS
 	CURL_LIBCURL += $(CURL_LDFLAGS)
 else
-	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
+	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs || echo -lcurl)
 endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X

which should work on most systems.

For your specific case, where you _do_ have curl-config but it's just
not in the PATH, then I think:

  make CURL_CONFIG=/path/to/curl-config

would be a slightly cleaner solution.

But it sounds like you _did_ use the autoconf script, but it did not
correctly set CURL_CONFIG. Do you have a config.mak.autogen file after
running ./configure, and if so, does it have an entry for CURL_CONFIG?

I'm not too familiar with our configure.ac, but it looks like
--with-curl might just point some paths for header/include files, and
not actually update the curl-config path.

-Peff

RE: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config

[Raitanen, Adam]

Yes there is a config.mak.autogen and it does not have an entry for CURL_CONFIG, although it has a correct entry for CURLDIR.

The config.log also shows it checking for curl-config and not finding it then proceeding anyway:

configure:5917: checking for curl-config
configure:5945: result: no
...

-----Original Message-----
From: Jeff King <peff@peff.net> 
Sent: Monday, July 22, 2019 5:12 PM
To: Raitanen, Adam <araitane@ciena.com>
Cc: James Knight <james.d.knight@live.com>; git@vger.kernel.org
Subject: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config

[+cc the author of that patch]

On Mon, Jul 22, 2019 at 07:46:37PM +0000, Raitanen, Adam wrote:

> I believe there may be a bug in the Makefile introduced by the following commit:
> 
> https://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e56
> 8ca4
> 
> The commit was merged in 2.20.0:
> 
> * The way -lcurl library gets linked has been simplified by taking
>    advantage of the fact that we can just ask curl-config command how.
> 
> Unfortunately it assumes that curl-config is in the path which is not 
> always the case. When using "--with-curl=/path/to/curl" in the 
> configure command, the path to the actual curl-config executable is 
> ignored and the build fails around here:
> 
>     CC http-fetch.o
> make: curl-config: Command not found
>     LINK git-http-fetch
> http.o: In function `fill_active_slots':
> /tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'
> .
> 
> We were able to workaround this by forcing the correct path into the make env:
> 
> make CURL_LDFLAGS="$(/path/to/curl/curl-config --libs)".
> 
> I reproduced the problem in the latest version 2.22.0.

For the case without autoconf, I think using CURL_LDFLAGS is the intended safety valve. Though perhaps we should be falling back more gracefully to the old behavior, like:

diff --git a/Makefile b/Makefile
index 11ccea4071..27e546bbfc 100644
--- a/Makefile
+++ b/Makefile
@@ -1343,7 +1343,7 @@ else
 ifdef CURL_LDFLAGS
 	CURL_LIBCURL += $(CURL_LDFLAGS)
 else
-	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
+	CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs || echo -lcurl)
 endif
 
 	REMOTE_CURL_PRIMARY = git-remote-http$X

which should work on most systems.

For your specific case, where you _do_ have curl-config but it's just not in the PATH, then I think:

  make CURL_CONFIG=/path/to/curl-config

would be a slightly cleaner solution.

But it sounds like you _did_ use the autoconf script, but it did not correctly set CURL_CONFIG. Do you have a config.mak.autogen file after running ./configure, and if so, does it have an entry for CURL_CONFIG?

I'm not too familiar with our configure.ac, but it looks like --with-curl might just point some paths for header/include files, and not actually update the curl-config path.

-Peff

Re: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config

[Jeff King]

On Tue, Jul 23, 2019 at 07:43:01PM +0000, Raitanen, Adam wrote:

> Yes there is a config.mak.autogen and it does not have an entry for CURL_CONFIG, although it has a correct entry for CURLDIR.
> 
> The config.log also shows it checking for curl-config and not finding it then proceeding anyway:
> 
> configure:5917: checking for curl-config
> configure:5945: result: no

OK, that makes sense from what I see in configure.ac. The "--with-curl"
flag does not interact at all with the curl-config test, but only sets
CURLDIR.

It seems like we should at least look for $CURLDIR/bin/curl-config in
such a case. But I think the fallback patch I showed earlier would
probably work for you, too (since on your system "curl-config --libs" is
likely just going to show "-lcurl" anyway).

Let's see what James says.

-Peff