{"thread":{"id":"36528","subject":"[PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","startedAt":"2014-04-28T21:01:23Z","lastAt":"2014-04-30T17:26:12Z","messageCount":7,"participants":["Dave Borowitz","Junio C Hamano","Jonathan Nieder","Erik Faye-Lund"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"240046","messageId":"1398718883-5630-1-git-send-email-dborowitz@google.com","threadId":"36528","inReplyTo":null,"subject":"[PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T21:01:23Z","receivedAt":"2014-04-28T21:01:23Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"The original implementation of CURL_CONFIG support did not match the\noriginal behavior of using -lcurl when CURLDIR was not set. This broke\nimplementations that were lacking curl-config but did have libcurl\ninstalled along system libraries, such as MSysGit. In other words, the\nassumption that curl-config is always installed was incorrect.\n\nInstead, if CURL_CONFIG is empty or returns an empty result (e.g. due\nto curl-config being missing), use the old behavior of falling back to\n-lcurl.\n\nSigned-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Makefile | 41 ++++++++++++++++++++++++++++-------------\n 1 file changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 74a929b..81e8214 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -35,14 +35,17 @@ all::\n # transports (neither smart nor dumb).\n #\n # Define CURL_CONFIG to the path to a curl-config binary other than the\n-# default 'curl-config'.\n+# default 'curl-config'.  If CURL_CONFIG is unset or points to a binary that\n+# is not found, defaults to the CURLDIR behavior.\n #\n # Define CURL_STATIC to statically link libcurl.  Only applies if\n # CURL_CONFIG is used.\n #\n # Define CURLDIR=/foo/bar if your curl header and library files are in\n-# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,\n-# but is less robust.\n+# /foo/bar/include and /foo/bar/lib directories.  This overrides\n+# CURL_CONFIG, but is less robust.  If not set, and CURL_CONFIG is not set,\n+# uses -lcurl with no additional library detection (other than\n+# NEEDS_*_WITH_CURL).\n #\n # Define NO_EXPAT if you do not have expat installed.  git-http-push is\n # not built, and you cannot push using http:// and https:// transports (dumb).\n@@ -1127,9 +1130,27 @@ ifdef NO_CURL\n \tREMOTE_CURL_NAMES =\n else\n \tifdef CURLDIR\n-\t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n-\t\tBASIC_CFLAGS += -I$(CURLDIR)/include\n-\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl\n+\t\tCURL_LIBCURL =\n+\telse\n+\t\tCURL_CONFIG = curl-config\n+\t\tifeq \"$(CURL_CONFIG)\" \"\"\n+\t\t\tCURL_LIBCURL =\n+\t\telse\n+\t\t\tCURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)\n+\t\tendif\n+\tendif\n+\n+\tifeq \"$(CURL_LIBCURL)\" \"\"\n+\t\tifdef CURL_STATIC\n+$(error \"CURL_STATIC must be used with CURL_CONFIG\")\n+\t\tendif\n+\t\tifdef CURLDIR\n+\t\t\t# Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n+\t\t\tBASIC_CFLAGS += -I$(CURLDIR)/include\n+\t\t\tCURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl\n+\t\telse\n+\t\t\tCURL_LIBCURL = -lcurl\n+\t\tendif\n \t\tifdef NEEDS_SSL_WITH_CURL\n \t\t\tCURL_LIBCURL += -lssl\n \t\t\tifdef NEEDS_CRYPTO_WITH_SSL\n@@ -1140,17 +1161,11 @@ else\n \t\t\tCURL_LIBCURL += -lidn\n \t\tendif\n \telse\n-\t\tCURL_CONFIG ?= curl-config\n \t\tBASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)\n \t\tifdef CURL_STATIC\n \t\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs)\n \t\t\tifeq \"$(CURL_LIBCURL)\" \"\"\n-\t\t\t\t$(error libcurl not detected or not compiled with static support)\n-\t\t\tendif\n-\t\telse\n-\t\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n-\t\t\tifeq \"$(CURL_LIBCURL)\" \"\"\n-\t\t\t\t$(error libcurl not detected; try setting CURLDIR)\n+$(error libcurl not detected or not compiled with static support)\n \t\t\tendif\n \t\tendif\n \tendif\n-- \n1.9.1.423.g4596e3a\n"},{"id":"240052","messageId":"xmqqfvkxnnqu.fsf@gitster.dls.corp.google.com","threadId":"36528","inReplyTo":"1398718883-5630-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-28T21:32:57Z","receivedAt":"2014-04-28T21:32:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> The original implementation of CURL_CONFIG support did not match the\n> original behavior of using -lcurl when CURLDIR was not set. This broke\n> implementations that were lacking curl-config but did have libcurl\n> installed along system libraries, such as MSysGit. In other words, the\n> assumption that curl-config is always installed was incorrect.\n>\n> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due\n> to curl-config being missing), use the old behavior of falling back to\n> -lcurl.\n>\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n> ---\n\nThanks. Will pick this version up.\n"},{"id":"240088","messageId":"20140428233016.GR9218@google.com","threadId":"36528","inReplyTo":"1398718883-5630-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-28T23:30:16Z","receivedAt":"2014-04-28T23:30:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Borowitz wrote:\n\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n> ---\n>  Makefile | 41 ++++++++++++++++++++++++++++-------------\n>  1 file changed, 28 insertions(+), 13 deletions(-)\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for the quick turnaround.\n"},{"id":"240301","messageId":"CABPQNSYDD7g3nOwb2ZaOQ9M9gQnjzQyKP4Zo-i8p4o-s30bk1Q@mail.gmail.com","threadId":"36528","inReplyTo":"1398718883-5630-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-04-30T13:04:51Z","receivedAt":"2014-04-30T13:04:51Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Apr 28, 2014 at 11:01 PM, Dave Borowitz <dborowitz@google.com> wrote:\n> The original implementation of CURL_CONFIG support did not match the\n> original behavior of using -lcurl when CURLDIR was not set. This broke\n> implementations that were lacking curl-config but did have libcurl\n> installed along system libraries, such as MSysGit. In other words, the\n> assumption that curl-config is always installed was incorrect.\n>\n> Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due\n> to curl-config being missing), use the old behavior of falling back to\n> -lcurl.\n>\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n> ---\n>  Makefile | 41 ++++++++++++++++++++++++++++-------------\n>  1 file changed, 28 insertions(+), 13 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 74a929b..81e8214 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -35,14 +35,17 @@ all::\n>  # transports (neither smart nor dumb).\n>  #\n>  # Define CURL_CONFIG to the path to a curl-config binary other than the\n> -# default 'curl-config'.\n> +# default 'curl-config'.  If CURL_CONFIG is unset or points to a binary that\n> +# is not found, defaults to the CURLDIR behavior.\n>  #\n>  # Define CURL_STATIC to statically link libcurl.  Only applies if\n>  # CURL_CONFIG is used.\n>  #\n>  # Define CURLDIR=/foo/bar if your curl header and library files are in\n> -# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,\n> -# but is less robust.\n> +# /foo/bar/include and /foo/bar/lib directories.  This overrides\n> +# CURL_CONFIG, but is less robust.  If not set, and CURL_CONFIG is not set,\n> +# uses -lcurl with no additional library detection (other than\n> +# NEEDS_*_WITH_CURL).\n\nThis is wrong, no? With CURL_CONFIG not set, it currently *does* run\ncurl-config, see below.\n\n>  #\n>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is\n>  # not built, and you cannot push using http:// and https:// transports (dumb).\n> @@ -1127,9 +1130,27 @@ ifdef NO_CURL\n>         REMOTE_CURL_NAMES =\n>  else\n>         ifdef CURLDIR\n> -               # Try \"-Wl,-rpath=$(CURLDIR)/$(lib)\" in such a case.\n> -               BASIC_CFLAGS += -I$(CURLDIR)/include\n> -               CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl\n> +               CURL_LIBCURL =\n> +       else\n> +               CURL_CONFIG = curl-config\n> +               ifeq \"$(CURL_CONFIG)\" \"\"\n> +                       CURL_LIBCURL =\n> +               else\n> +                       CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)\n> +               endif\n\nDoesn't that definition just define CURL_CONFIG unconditionally? How\nare the first condition ever supposed to get triggered?\n\n$ make\nmake: curl-config: Command not found\nGIT_VERSION = 1.9.2.462.gf3f11fa\nmake: curl-config: Command not found\n    * new build flags\n    * new link flags\n    * new prefix flags\n    GEN common-cmds.h\n...\n\nYuck.\n"},{"id":"240310","messageId":"xmqqk3a6hmv6.fsf@gitster.dls.corp.google.com","threadId":"36528","inReplyTo":"CABPQNSYDD7g3nOwb2ZaOQ9M9gQnjzQyKP4Zo-i8p4o-s30bk1Q@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-30T15:13:01Z","receivedAt":"2014-04-30T15:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> This is wrong, no? With CURL_CONFIG not set, it currently *does* run\n> curl-config, see below.\n> ...\n>>         ifdef CURLDIR\n>> +               CURL_LIBCURL =\n>> +       else\n>> +               CURL_CONFIG = curl-config\n>> +               ifeq \"$(CURL_CONFIG)\" \"\"\n>> +                       CURL_LIBCURL =\n>> +               else\n>> +                       CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs)\n>> +               endif\n>\n> Doesn't that definition just define CURL_CONFIG unconditionally? How\n> are the first condition ever supposed to get triggered?\n>\n> $ make\n> make: curl-config: Command not found\n> GIT_VERSION = 1.9.2.462.gf3f11fa\n> make: curl-config: Command not found\n>     * new build flags\n>     * new link flags\n>     * new prefix flags\n>     GEN common-cmds.h\n> ...\n>\n> Yuck.\n\nAn earlier iteration of the patch used \"CURL_CONFIG ?= curl-config\",\nbut that would not have been much different:\n\n        $ cat >Makefile <<\\EOF\n        CURL_CONFIG ?= curl-config\n        ifeq \"$(CURL_CONFIG)\" \"\"\n                X=Empty\n        else\n                X=NotEmpty\n        endif\n        ifdef CURL_CONFIG\n                Z=Defined\n        else\n                Z=Undefined\n        endif\n        all::\n                @echo \"$(X) $(Z) CURL_CONFIG=<$(CURL_CONFIG)>\"\n        EOF\n        $ make\n        NotEmpty Defined CURL_CONFIG=<curl-config>\n        $ make CURL_CONFIG=\"\"\n        Empty Undefined CURL_CONFIG=<>\n        $ CURL_CONFIG=\"\" make\n        Empty Undefined CURL_CONFIG=<>\n\nAs the first one (the default) will still use curl-config and\npassing an explicit CURL_CONFIG=\"\" on the command line would be the\nonly way to squelch this unpleasantness.  If you change\n\n\tCURL_CONFIG ?= curl-config\n\nto\n\n\tCURL_CONFIG = curl-config\n\nin the above illustration, the first two would be the same result as\nabove, and the last one will behave the same as the first one---an\nenvironment set to empty is still protected from the default defined\nin the Makefile.\n\nI think something along the lines of \n\n\tifdef CURLDIR\n        \tCURL_LIBCURL =\n\telse\n\t\tCURL_CONFIG = curl-config\n\t\tCURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)\n\tfi\n\nmay be the right way to write this?\n\nNote that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty\nwould barf when $(CURL_CONFIG) expands to an empty string.\n"},{"id":"240316","messageId":"CABPQNSZVFWhP+oTYpU2HeenJXwMsDNQGegwYZVb=bZ2+BRz3-w@mail.gmail.com","threadId":"36528","inReplyTo":"xmqqk3a6hmv6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-04-30T16:05:35Z","receivedAt":"2014-04-30T16:05:35Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> I think something along the lines of\n>\n>         ifdef CURLDIR\n>                 CURL_LIBCURL =\n>         else\n>                 CURL_CONFIG = curl-config\n>                 CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)\n>         fi\n>\n> may be the right way to write this?\n>\n> Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty\n> would barf when $(CURL_CONFIG) expands to an empty string.\n\nThere's still the fact that msysGit does *not* need CURLDIR, but\ndoesn't have curl-config either. So I think this one will also\ncomplain for us.\n"},{"id":"240338","messageId":"xmqqzjj2g24r.fsf@gitster.dls.corp.google.com","threadId":"36528","inReplyTo":"CABPQNSZVFWhP+oTYpU2HeenJXwMsDNQGegwYZVb=bZ2+BRz3-w@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-30T17:26:12Z","receivedAt":"2014-04-30T17:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> I think something along the lines of\n>>\n>>         ifdef CURLDIR\n>>                 CURL_LIBCURL =\n>>         else\n>>                 CURL_CONFIG = curl-config\n>>                 CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' 2>/dev/null)\n>>         fi\n>>\n>> may be the right way to write this?\n>>\n>> Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty\n>> would barf when $(CURL_CONFIG) expands to an empty string.\n>\n> There's still the fact that msysGit does *not* need CURLDIR, but\n> doesn't have curl-config either. So I think this one will also\n> complain for us.\n\nEven with 2>/dev/null?\n"}]}