{"thread":{"id":"51510","subject":"Possible bug in Makefile when executing curl-config","startedAt":"2019-07-22T20:09:29Z","lastAt":"2019-07-23T20:16:52Z","messageCount":4,"participants":["Raitanen, Adam","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379157","messageId":"DM6PR04MB47939645A97D099241AD25F0D0C40@DM6PR04MB4793.namprd04.prod.outlook.com","threadId":"51510","inReplyTo":null,"subject":"Possible bug in Makefile when executing curl-config","fromName":"Raitanen, Adam","fromEmail":"araitane@ciena.com","sentAt":"2019-07-22T19:46:37Z","receivedAt":"2019-07-22T20:09:29Z","isPatch":false,"sender":{"key":"araitane@ciena.com","avatar":null},"body":"I believe there may be a bug in the Makefile introduced by the following commit:\n\nhttps://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e568ca4\n\nThe commit was merged in 2.20.0:\n\n* The way -lcurl library gets linked has been simplified by taking\n   advantage of the fact that we can just ask curl-config command how.\n\nUnfortunately 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:\n\n    CC http-fetch.o\nmake: curl-config: Command not found\n    LINK git-http-fetch\nhttp.o: In function `fill_active_slots':\n/tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'\n.\n\nWe were able to workaround this by forcing the correct path into the make env:\n\nmake CURL_LDFLAGS=\"$(/path/to/curl/curl-config --libs)\".\n\nI reproduced the problem in the latest version 2.22.0.\n\nThanks,\nAdam\n\n"},{"id":"379159","messageId":"20190722211219.GB31664@sigill.intra.peff.net","threadId":"51510","inReplyTo":"DM6PR04MB47939645A97D099241AD25F0D0C40@DM6PR04MB4793.namprd04.prod.outlook.com","subject":"Re: Possible bug in Makefile when executing curl-config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-22T21:12:19Z","receivedAt":"2019-07-22T21:12:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc the author of that patch]\n\nOn Mon, Jul 22, 2019 at 07:46:37PM +0000, Raitanen, Adam wrote:\n\n> I believe there may be a bug in the Makefile introduced by the following commit:\n> \n> https://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e568ca4\n> \n> The commit was merged in 2.20.0:\n> \n> * The way -lcurl library gets linked has been simplified by taking\n>    advantage of the fact that we can just ask curl-config command how.\n> \n> Unfortunately it assumes that curl-config is in the path which is not\n> always the case. When using \"--with-curl=/path/to/curl\" in the\n> configure command, the path to the actual curl-config executable is\n> ignored and the build fails around here:\n> \n>     CC http-fetch.o\n> make: curl-config: Command not found\n>     LINK git-http-fetch\n> http.o: In function `fill_active_slots':\n> /tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'\n> .\n> \n> We were able to workaround this by forcing the correct path into the make env:\n> \n> make CURL_LDFLAGS=\"$(/path/to/curl/curl-config --libs)\".\n> \n> I reproduced the problem in the latest version 2.22.0.\n\nFor the case without autoconf, I think using CURL_LDFLAGS is the\nintended safety valve. Though perhaps we should be falling back more\ngracefully to the old behavior, like:\n\ndiff --git a/Makefile b/Makefile\nindex 11ccea4071..27e546bbfc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1343,7 +1343,7 @@ else\n ifdef CURL_LDFLAGS\n \tCURL_LIBCURL += $(CURL_LDFLAGS)\n else\n-\tCURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)\n+\tCURL_LIBCURL += $(shell $(CURL_CONFIG) --libs || echo -lcurl)\n endif\n \n \tREMOTE_CURL_PRIMARY = git-remote-http$X\n\nwhich should work on most systems.\n\nFor your specific case, where you _do_ have curl-config but it's just\nnot in the PATH, then I think:\n\n  make CURL_CONFIG=/path/to/curl-config\n\nwould be a slightly cleaner solution.\n\nBut it sounds like you _did_ use the autoconf script, but it did not\ncorrectly set CURL_CONFIG. Do you have a config.mak.autogen file after\nrunning ./configure, and if so, does it have an entry for CURL_CONFIG?\n\nI'm not too familiar with our configure.ac, but it looks like\n--with-curl might just point some paths for header/include files, and\nnot actually update the curl-config path.\n\n-Peff\n"},{"id":"379184","messageId":"DM6PR04MB479387667E7EDEF2A27BFCA2D0C70@DM6PR04MB4793.namprd04.prod.outlook.com","threadId":"51510","inReplyTo":"20190722211219.GB31664@sigill.intra.peff.net","subject":"RE: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config","fromName":"Raitanen, Adam","fromEmail":"araitane@ciena.com","sentAt":"2019-07-23T19:43:01Z","receivedAt":"2019-07-23T20:05:18Z","isPatch":false,"sender":{"key":"araitane@ciena.com","avatar":null},"body":"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.\n\nThe config.log also shows it checking for curl-config and not finding it then proceeding anyway:\n\nconfigure:5917: checking for curl-config\nconfigure:5945: result: no\n...\n\n-----Original Message-----\nFrom: Jeff King <peff@peff.net> \nSent: Monday, July 22, 2019 5:12 PM\nTo: Raitanen, Adam <araitane@ciena.com>\nCc: James Knight <james.d.knight@live.com>; git@vger.kernel.org\nSubject: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config\n\n[+cc the author of that patch]\n\nOn Mon, Jul 22, 2019 at 07:46:37PM +0000, Raitanen, Adam wrote:\n\n> I believe there may be a bug in the Makefile introduced by the following commit:\n> \n> https://github.com/git/git/commit/23c4bbe28e61974577164db09cbd1d1c7e56\n> 8ca4\n> \n> The commit was merged in 2.20.0:\n> \n> * The way -lcurl library gets linked has been simplified by taking\n>    advantage of the fact that we can just ask curl-config command how.\n> \n> Unfortunately it assumes that curl-config is in the path which is not \n> always the case. When using \"--with-curl=/path/to/curl\" in the \n> configure command, the path to the actual curl-config executable is \n> ignored and the build fails around here:\n> \n>     CC http-fetch.o\n> make: curl-config: Command not found\n>     LINK git-http-fetch\n> http.o: In function `fill_active_slots':\n> /tmp/git-2.21.0/http.c:1385: undefined reference to `curl_easy_cleanup'\n> .\n> \n> We were able to workaround this by forcing the correct path into the make env:\n> \n> make CURL_LDFLAGS=\"$(/path/to/curl/curl-config --libs)\".\n> \n> I reproduced the problem in the latest version 2.22.0.\n\nFor 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:\n\ndiff --git a/Makefile b/Makefile\nindex 11ccea4071..27e546bbfc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1343,7 +1343,7 @@ else\n ifdef CURL_LDFLAGS\n \tCURL_LIBCURL += $(CURL_LDFLAGS)\n else\n-\tCURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)\n+\tCURL_LIBCURL += $(shell $(CURL_CONFIG) --libs || echo -lcurl)\n endif\n \n \tREMOTE_CURL_PRIMARY = git-remote-http$X\n\nwhich should work on most systems.\n\nFor your specific case, where you _do_ have curl-config but it's just not in the PATH, then I think:\n\n  make CURL_CONFIG=/path/to/curl-config\n\nwould be a slightly cleaner solution.\n\nBut 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?\n\nI'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.\n\n-Peff\n"},{"id":"379186","messageId":"20190723201649.GA4995@sigill.intra.peff.net","threadId":"51510","inReplyTo":"DM6PR04MB479387667E7EDEF2A27BFCA2D0C70@DM6PR04MB4793.namprd04.prod.outlook.com","subject":"Re: [**EXTERNAL**] Re: Possible bug in Makefile when executing curl-config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-23T20:16:50Z","receivedAt":"2019-07-23T20:16:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 23, 2019 at 07:43:01PM +0000, Raitanen, Adam wrote:\n\n> 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.\n> \n> The config.log also shows it checking for curl-config and not finding it then proceeding anyway:\n> \n> configure:5917: checking for curl-config\n> configure:5945: result: no\n\nOK, that makes sense from what I see in configure.ac. The \"--with-curl\"\nflag does not interact at all with the curl-config test, but only sets\nCURLDIR.\n\nIt seems like we should at least look for $CURLDIR/bin/curl-config in\nsuch a case. But I think the fallback patch I showed earlier would\nprobably work for you, too (since on your system \"curl-config --libs\" is\nlikely just going to show \"-lcurl\" anyway).\n\nLet's see what James says.\n\n-Peff\n"}]}