{"thread":{"id":"36527","subject":"[PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","startedAt":"2014-04-28T19:50:53Z","lastAt":"2014-04-28T21:27:18Z","messageCount":10,"participants":["Dave Borowitz","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"240019","messageId":"1398714653-1050-1-git-send-email-dborowitz@google.com","threadId":"36527","inReplyTo":null,"subject":"[PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T19:50:53Z","receivedAt":"2014-04-28T19:50:53Z","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---\n Makefile | 36 +++++++++++++++++++++++++-----------\n 1 file changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 74a929b..79b7442 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -35,7 +35,9 @@ 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, or if CURLDIR is not set,\n+# uses -lcurl with no additional library detection.\n #\n # Define CURL_STATIC to statically link libcurl.  Only applies if\n # CURL_CONFIG is used.\n@@ -1127,9 +1129,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 +1160,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":"240031","messageId":"20140428200550.GO9218@google.com","threadId":"36527","inReplyTo":"1398714653-1050-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-28T20:05:51Z","receivedAt":"2014-04-28T20:05:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Borowitz wrote:\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>  Makefile | 36 +++++++++++++++++++++++++-----------\n>  1 file changed, 25 insertions(+), 11 deletions(-)\n\nSign-off?\n\n[...]\n> +++ b/Makefile\n> @@ -35,7 +35,9 @@ 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, or if CURLDIR is not set,\n> +# uses -lcurl with no additional library detection.\n\nI'm having a little trouble parsing this but don't have any better\nsuggestion.\n\n[...]\n> @@ -1127,9 +1129,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\nTiny nit: elsewhere the makefile seems to prefer having a space before\nthe '='.\n\nIf I explicitly set CURL_LIBCURL to the empty string and CURLDIR was\nset then my setting will still override the setting below and the\nbehavior is unchanged from before this patch --- good.\n\nIf I explicitly set CURL_LIBCURL to empty and CURLDIR was unset then\nthat used to produce an error so it was an invalid configuration and\ncouldn't regress.\n\nSo this should be safe --- good.\n\n> +\telse\n> +\t\tCURL_CONFIG ?= curl-config\n\nNot about this patch, but the above '?=' should probably be plain '='\nfor consistency with the rest of the makefile's behavior wrt envvars.\n\n[...]\n> -\t\t\t\t$(error libcurl not detected; try setting CURLDIR)\n> +                                $(error libcurl not detected or not compiled with static support)\n\nWhitespace damage.\n\nExcept for the whitespace issues,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"240038","messageId":"xmqqlhupp4hx.fsf@gitster.dls.corp.google.com","threadId":"36527","inReplyTo":"1398714653-1050-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-28T20:45:45Z","receivedAt":"2014-04-28T20:45:45Z","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\nSign-off?\n\nI still think the implementation of \"If CURL_CONFIG is unset\" bit is\na bit redundant, though.\n\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\nOther than that, the remainder of the change looks correct to me.\n\nThanks.  \n"},{"id":"240039","messageId":"CAD0k6qR766hrgUtyiGzZ9KM5woknfsyUm3Mf1-Pm3M-LrL2Zhg@mail.gmail.com","threadId":"36527","inReplyTo":"20140428200550.GO9218@google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T20:46:58Z","receivedAt":"2014-04-28T20:46:58Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Dave Borowitz wrote:\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>>  Makefile | 36 +++++++++++++++++++++++++-----------\n>>  1 file changed, 25 insertions(+), 11 deletions(-)\n>\n> Sign-off?\n\nOops.\n\n> [...]\n>> +++ b/Makefile\n>> @@ -35,7 +35,9 @@ 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, or if CURLDIR is not set,\n>> +# uses -lcurl with no additional library detection.\n>\n> I'm having a little trouble parsing this but don't have any better\n> suggestion.\n\nHow about:\n\"If CURL_CONFIG is unset or points to a binary that is not found,\ndefaults to the CURLDIR behavior. If CURLDIR is not set, this means\nusing -lcurl with no additional library detection (other than\nNEEDS_*_WITH_CURL).\n\n[...]\n\n>> -                             $(error libcurl not detected; try setting CURLDIR)\n>> +                                $(error libcurl not detected or not compiled with static support)\n>\n> Whitespace damage.\n\nYes, but intentional, because Makefile parsing is weird.\n\n$ echo -e 'ifndef FOO\\n\\t$(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\nfoo' > mk1 && make -f mk1 foo\nmk1:2: *** commands commence before first target.  Stop.\n$ echo -e 'ifndef FOO\\n  $(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\nfoo' > mk2 && make -f mk2 foo\nmk2:2: *** bad things.  Stop.\n\nSee also:\nhttp://stackoverflow.com/questions/4713663/gnu-make-yields-commands-commence-before-first-target-error\n\n> Except for the whitespace issues,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks.\n"},{"id":"240040","messageId":"CAD0k6qQ+jbFqxpUfH9pm1koicpkqesCbUXxbcW_+ocgLDUOkjQ@mail.gmail.com","threadId":"36527","inReplyTo":"CAD0k6qR766hrgUtyiGzZ9KM5woknfsyUm3Mf1-Pm3M-LrL2Zhg@mail.gmail.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T20:48:48Z","receivedAt":"2014-04-28T20:48:48Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Apr 28, 2014 at 1:46 PM, Dave Borowitz <dborowitz@google.com> wrote:\n> How about:\n> \"If CURL_CONFIG is unset or points to a binary that is not found,\n> defaults to the CURLDIR behavior. If CURLDIR is not set, this means\n> using -lcurl with no additional library detection (other than\n> NEEDS_*_WITH_CURL).\n\nEven better, I'll move the blurb about CURLDIR behavior to CURLDIR.\n"},{"id":"240042","messageId":"20140428205058.GQ9218@google.com","threadId":"36527","inReplyTo":"CAD0k6qR766hrgUtyiGzZ9KM5woknfsyUm3Mf1-Pm3M-LrL2Zhg@mail.gmail.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-28T20:50:58Z","receivedAt":"2014-04-28T20:50:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dave Borowitz wrote:\n> On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Dave Borowitz wrote:\n\n>>> +++ b/Makefile\n>>> @@ -35,7 +35,9 @@ 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, or if CURLDIR is not set,\n>>> +# uses -lcurl with no additional library detection.\n>>\n>> I'm having a little trouble parsing this but don't have any better\n>> suggestion.\n>\n> How about:\n> \"If CURL_CONFIG is unset or points to a binary that is not found,\n> defaults to the CURLDIR behavior. If CURLDIR is not set, this means\n> using -lcurl with no additional library detection (other than\n> NEEDS_*_WITH_CURL).\n\nYep, that's clearer.\n\n> [...]\n>>> -                             $(error libcurl not detected; try setting CURLDIR)\n>>> +                                $(error libcurl not detected or not compiled with static support)\n>>\n>> Whitespace damage.\n>\n> Yes, but intentional, because Makefile parsing is weird.\n>\n> $ echo -e 'ifndef FOO\\n\\t$(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\n> foo' > mk1 && make -f mk1 foo\n> mk1:2: *** commands commence before first target.  Stop.\n> $ echo -e 'ifndef FOO\\n  $(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\n> foo' > mk2 && make -f mk2 foo\n> mk2:2: *** bad things.  Stop.\n\nGah.  Maybe it should be left-justified to avoid accentally breaking\nit again.\n\nThanks.\nJonathan\n"},{"id":"240043","messageId":"CAD0k6qSxBq+VAZ2CH-vR-Pyk4FjKKeJmngBU5T9Z-cwSH1LffQ@mail.gmail.com","threadId":"36527","inReplyTo":"20140428205058.GQ9218@google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T20:52:27Z","receivedAt":"2014-04-28T20:52:27Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Apr 28, 2014 at 1:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> $ echo -e 'ifndef FOO\\n\\t$(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\n>> foo' > mk1 && make -f mk1 foo\n>> mk1:2: *** commands commence before first target.  Stop.\n>> $ echo -e 'ifndef FOO\\n  $(error bad things)\\nendif\\n\\nfoo:\\n\\ttouch\n>> foo' > mk2 && make -f mk2 foo\n>> mk2:2: *** bad things.  Stop.\n>\n> Gah.  Maybe it should be left-justified to avoid accentally breaking\n> it again.\n\nI am ok with that on the basis that it is better to be ugly but\nobvious. I suspect if the Makefile were more littered with conditional\n$(error)s we would reconsider using tabs as indentation.\n\n> Thanks.\n> Jonathan\n"},{"id":"240045","messageId":"CAD0k6qRgLfgCTr=Erc=TxJ7_cnSuQLo+swv6Y=Z6-OqEzs=Hwg@mail.gmail.com","threadId":"36527","inReplyTo":"xmqqlhupp4hx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-28T20:56:14Z","receivedAt":"2014-04-28T20:56:14Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Apr 28, 2014 at 1:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> I still think the implementation of \"If CURL_CONFIG is unset\" bit is\n> a bit redundant, though.\n\nIf CURL_CONFIG is unset, then $(shell $(CURL_CONFIG) --libs) produces\n\"make: --libs: Command not found\", which may be confusing.\n"},{"id":"240047","messageId":"xmqqtx9dnp23.fsf@gitster.dls.corp.google.com","threadId":"36527","inReplyTo":"CAD0k6qR766hrgUtyiGzZ9KM5woknfsyUm3Mf1-Pm3M-LrL2Zhg@mail.gmail.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-28T21:04:36Z","receivedAt":"2014-04-28T21:04:36Z","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> On Mon, Apr 28, 2014 at 1:05 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Dave Borowitz wrote:\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>>>  Makefile | 36 +++++++++++++++++++++++++-----------\n>>>  1 file changed, 25 insertions(+), 11 deletions(-)\n>>\n>> Sign-off?\n>\n> Oops.\n\nWill forge your sign-off while queuing ;-)\n\n>\n> How about:\n> \"If CURL_CONFIG is unset or points to a binary that is not found,\n> defaults to the CURLDIR behavior. If CURLDIR is not set, this means\n> using -lcurl with no additional library detection (other than\n> NEEDS_*_WITH_CURL).\n\n... with this rephrasing.\n\n>>> -                             $(error libcurl not detected; try setting CURLDIR)\n>>> +                                $(error libcurl not detected or not compiled with static support)\n>>\n>> Whitespace damage.\n>\n> Yes, but intentional, because Makefile parsing is weird.\n\nThanks.\n"},{"id":"240051","messageId":"xmqqk3a9no09.fsf@gitster.dls.corp.google.com","threadId":"36527","inReplyTo":"20140428205058.GQ9218@google.com","subject":"Re: [PATCH] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-28T21:27:18Z","receivedAt":"2014-04-28T21:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Gah.  Maybe it should be left-justified to avoid accentally breaking\n> it again.\n\n;-).  Yes, $(error) is not the usual part of \"to build this target,\nfollow this recipe\" part.\n\nBut let's take the last round as-is and go with it for 2.0.\n\nTahnks.\n"}]}