{"thread":{"id":"36405","subject":"[PATCH 1/2] Makefile: use curl-config to determine curl flags","startedAt":"2014-04-12T23:15:01Z","lastAt":"2014-04-15T16:50:45Z","messageCount":5,"participants":["Dave Borowitz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"238803","messageId":"1397344502-23459-1-git-send-email-dborowitz@google.com","threadId":"36405","inReplyTo":null,"subject":"[PATCH 1/2] Makefile: use curl-config to determine curl flags","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-12T23:15:01Z","receivedAt":"2014-04-12T23:15:01Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"curl-config should always be installed alongside a curl distribution,\nand its purpose is to provide flags for building against libcurl, so\nuse it instead of guessing flags and dependent libraries.\n\nAllow overriding CURL_CONFIG to a custom path to curl-config, to\ncompile against a curl installation other than the first in PATH.\n\nUse this only when CURLDIR is not explicitly specified, to continue\nsupporting older builds.\n\nSigned-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Makefile | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 2128ce3..d6330bc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -34,8 +34,12 @@ all::\n # git-http-push are not built, and you cannot use http:// and https://\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+#\n # Define CURLDIR=/foo/bar if your curl header and library files are in\n-# /foo/bar/include and /foo/bar/lib directories.\n+# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,\n+# but is less robust.\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@@ -143,9 +147,11 @@ all::\n #\n # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).\n #\n-# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).\n+# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used\n+# if CURLDIR is set.\n #\n-# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).\n+# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only\n+# used if CURLDIR is set.\n #\n # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).\n #\n@@ -1121,18 +1127,23 @@ else\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\tifdef NEEDS_SSL_WITH_CURL\n+\t\t\tCURL_LIBCURL += -lssl\n+\t\t\tifdef NEEDS_CRYPTO_WITH_SSL\n+\t\t\t\tCURL_LIBCURL += -lcrypto\n+\t\t\tendif\n+\t\tendif\n+\t\tifdef NEEDS_IDN_WITH_CURL\n+\t\t\tCURL_LIBCURL += -lidn\n+\t\tendif\n \telse\n-\t\tCURL_LIBCURL = -lcurl\n-\tendif\n-\tifdef NEEDS_SSL_WITH_CURL\n-\t\tCURL_LIBCURL += -lssl\n-\t\tifdef NEEDS_CRYPTO_WITH_SSL\n-\t\t\tCURL_LIBCURL += -lcrypto\n+\t\tCURL_CONFIG ?= curl-config\n+\t\tBASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)\n+\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n+\t\tifeq \"$(CURL_LIBCURL)\" \"\"\n+\t\t\t$(error curl not detected; try setting CURLDIR)\n \t\tendif\n \tendif\n-\tifdef NEEDS_IDN_WITH_CURL\n-\t\tCURL_LIBCURL += -lidn\n-\tendif\n \n \tREMOTE_CURL_PRIMARY = git-remote-http$X\n \tREMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X\n-- \n1.9.1.423.g4596e3a\n"},{"id":"238804","messageId":"1397344502-23459-2-git-send-email-dborowitz@google.com","threadId":"36405","inReplyTo":"1397344502-23459-1-git-send-email-dborowitz@google.com","subject":"[PATCH 2/2] Makefile: allow static linking against libcurl","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-12T23:15:02Z","receivedAt":"2014-04-12T23:15:02Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Makefile | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex d6330bc..3c151d3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -37,6 +37,9 @@ all::\n # Define CURL_CONFIG to the path to a curl-config binary other than the\n # default 'curl-config'.\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@@ -1139,7 +1142,11 @@ else\n \telse\n \t\tCURL_CONFIG ?= curl-config\n \t\tBASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)\n-\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n+\t\tifdef CURL_STATIC\n+\t\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --static-libs)\n+\t\telse\n+\t\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n+\t\tendif\n \t\tifeq \"$(CURL_LIBCURL)\" \"\"\n \t\t\t$(error curl not detected; try setting CURLDIR)\n \t\tendif\n-- \n1.9.1.423.g4596e3a\n"},{"id":"238862","messageId":"xmqqioqboe50.fsf@gitster.dls.corp.google.com","threadId":"36405","inReplyTo":"1397344502-23459-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T20:22:03Z","receivedAt":"2014-04-14T20:22:03Z","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> curl-config should always be installed alongside a curl distribution,\n> and its purpose is to provide flags for building against libcurl, so\n> use it instead of guessing flags and dependent libraries.\n>\n> Allow overriding CURL_CONFIG to a custom path to curl-config, to\n> compile against a curl installation other than the first in PATH.\n>\n> Use this only when CURLDIR is not explicitly specified, to continue\n> supporting older builds.\n>\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n\nSounds logically the right thing to do.  Was there a particular\nplatform that needed this (i.e. cannot be made to work with the\nexisting CURLDIR and \"guessing flags and dependeent libraries\")\nthat may be worth mentioning in the log message?\n\n> ---\n>  Makefile | 35 +++++++++++++++++++++++------------\n>  1 file changed, 23 insertions(+), 12 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 2128ce3..d6330bc 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -34,8 +34,12 @@ all::\n>  # git-http-push are not built, and you cannot use http:// and https://\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> +#\n>  # Define CURLDIR=/foo/bar if your curl header and library files are in\n> -# /foo/bar/include and /foo/bar/lib directories.\n> +# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,\n> +# but is less robust.\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> @@ -143,9 +147,11 @@ all::\n>  #\n>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).\n>  #\n> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).\n> +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used\n> +# if CURLDIR is set.\n>  #\n> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).\n> +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only\n> +# used if CURLDIR is set.\n>  #\n>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).\n>  #\n> @@ -1121,18 +1127,23 @@ else\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\tifdef NEEDS_SSL_WITH_CURL\n> +\t\t\tCURL_LIBCURL += -lssl\n> +\t\t\tifdef NEEDS_CRYPTO_WITH_SSL\n> +\t\t\t\tCURL_LIBCURL += -lcrypto\n> +\t\t\tendif\n> +\t\tendif\n> +\t\tifdef NEEDS_IDN_WITH_CURL\n> +\t\t\tCURL_LIBCURL += -lidn\n> +\t\tendif\n>  \telse\n> -\t\tCURL_LIBCURL = -lcurl\n> -\tendif\n> -\tifdef NEEDS_SSL_WITH_CURL\n> -\t\tCURL_LIBCURL += -lssl\n> -\t\tifdef NEEDS_CRYPTO_WITH_SSL\n> -\t\t\tCURL_LIBCURL += -lcrypto\n> +\t\tCURL_CONFIG ?= curl-config\n> +\t\tBASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)\n> +\t\tCURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n> +\t\tifeq \"$(CURL_LIBCURL)\" \"\"\n> +\t\t\t$(error curl not detected; try setting CURLDIR)\n>  \t\tendif\n>  \tendif\n> -\tifdef NEEDS_IDN_WITH_CURL\n> -\t\tCURL_LIBCURL += -lidn\n> -\tendif\n>  \n>  \tREMOTE_CURL_PRIMARY = git-remote-http$X\n>  \tREMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X\n"},{"id":"238881","messageId":"CAD0k6qQJE6a5DmRnpiFpOc_MFjSn9eFyiJ2nFaL63rctvxwt5Q@mail.gmail.com","threadId":"36405","inReplyTo":"xmqqioqboe50.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2014-04-15T10:20:52Z","receivedAt":"2014-04-15T10:20:52Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Apr 14, 2014 at 4:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> curl-config should always be installed alongside a curl distribution,\n>> and its purpose is to provide flags for building against libcurl, so\n>> use it instead of guessing flags and dependent libraries.\n>>\n>> Allow overriding CURL_CONFIG to a custom path to curl-config, to\n>> compile against a curl installation other than the first in PATH.\n>>\n>> Use this only when CURLDIR is not explicitly specified, to continue\n>> supporting older builds.\n>>\n>> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n>\n> Sounds logically the right thing to do.  Was there a particular\n> platform that needed this (i.e. cannot be made to work with the\n> existing CURLDIR and \"guessing flags and dependeent libraries\")\n> that may be worth mentioning in the log message?\n\nMy end goal is to statically link git on Mac OS X (10.9) against a\nnewer version of libcurl than ships with the OS. The normal CURLDIR\napproach should work with system libcurl:\n\n$ /usr/bin/curl-config --libs\n-lcurl\n\nBut it gets a bit more complicated with a recent curl version. This\nlikely has to do with the set of enabled options; I passed flags to\n./configure based on the build script \"MacOSX-Framework\" included in\nthe curl distribution:\n$ ~/d/curl-out-7.36.0/bin/curl-config --libs\n-L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv\n-lldap -lz\n\nAnd with --static-libs there's just that much more:\n$ ~/d/curl-out-7.36.0/bin/curl-config --static-libs\n/Users/dborowitz/d/curl-out-7.36.0/lib/libcurl.a\n-Wl,-syslibroot,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk\n-arch x86_64 -Wl,-headerpad_max_install_names -framework\nCoreFoundation -framework Security -lgssapi_krb5 -lresolv -lldap -lz\n\nSo I don't think specifying NEEDS_*_WITH_CURL scales to this use case.\n\nWhile writing this up I also noticed an issue with the second patch,\nnamely that `curl-config --static-libs` is empty when curl is not\nbuilt for static linking.\n\nI will reroll with a more detailed commit message and a fix to the second patch.\n\n>> ---\n>>  Makefile | 35 +++++++++++++++++++++++------------\n>>  1 file changed, 23 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index 2128ce3..d6330bc 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -34,8 +34,12 @@ all::\n>>  # git-http-push are not built, and you cannot use http:// and https://\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>> +#\n>>  # Define CURLDIR=/foo/bar if your curl header and library files are in\n>> -# /foo/bar/include and /foo/bar/lib directories.\n>> +# /foo/bar/include and /foo/bar/lib directories.  This overrides CURL_CONFIG,\n>> +# but is less robust.\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>> @@ -143,9 +147,11 @@ all::\n>>  #\n>>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).\n>>  #\n>> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).\n>> +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).  Only used\n>> +# if CURLDIR is set.\n>>  #\n>> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).\n>> +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).  Only\n>> +# used if CURLDIR is set.\n>>  #\n>>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).\n>>  #\n>> @@ -1121,18 +1127,23 @@ else\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>> +             ifdef NEEDS_SSL_WITH_CURL\n>> +                     CURL_LIBCURL += -lssl\n>> +                     ifdef NEEDS_CRYPTO_WITH_SSL\n>> +                             CURL_LIBCURL += -lcrypto\n>> +                     endif\n>> +             endif\n>> +             ifdef NEEDS_IDN_WITH_CURL\n>> +                     CURL_LIBCURL += -lidn\n>> +             endif\n>>       else\n>> -             CURL_LIBCURL = -lcurl\n>> -     endif\n>> -     ifdef NEEDS_SSL_WITH_CURL\n>> -             CURL_LIBCURL += -lssl\n>> -             ifdef NEEDS_CRYPTO_WITH_SSL\n>> -                     CURL_LIBCURL += -lcrypto\n>> +             CURL_CONFIG ?= curl-config\n>> +             BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags)\n>> +             CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs)\n>> +             ifeq \"$(CURL_LIBCURL)\" \"\"\n>> +                     $(error curl not detected; try setting CURLDIR)\n>>               endif\n>>       endif\n>> -     ifdef NEEDS_IDN_WITH_CURL\n>> -             CURL_LIBCURL += -lidn\n>> -     endif\n>>\n>>       REMOTE_CURL_PRIMARY = git-remote-http$X\n>>       REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X\n"},{"id":"238892","messageId":"xmqqa9bmleoq.fsf@gitster.dls.corp.google.com","threadId":"36405","inReplyTo":"CAD0k6qQJE6a5DmRnpiFpOc_MFjSn9eFyiJ2nFaL63rctvxwt5Q@mail.gmail.com","subject":"Re: [PATCH 1/2] Makefile: use curl-config to determine curl flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-15T16:50:45Z","receivedAt":"2014-04-15T16:50: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> My end goal is to statically link git on Mac OS X (10.9) against a\n> newer version of libcurl than ships with the OS. The normal CURLDIR\n> approach should work with system libcurl:\n>\n> $ /usr/bin/curl-config --libs\n> -lcurl\n>\n> But it gets a bit more complicated with a recent curl version. This\n> likely has to do with the set of enabled options; I passed flags to\n> ./configure based on the build script \"MacOSX-Framework\" included in\n> the curl distribution:\n> $ ~/d/curl-out-7.36.0/bin/curl-config --libs\n> -L/Users/dborowitz/d/curl-out-7.36.0/lib -lcurl -lgssapi_krb5 -lresolv\n> -lldap -lz\n>\n> And with --static-libs there's just that much more:\n> $ ~/d/curl-out-7.36.0/bin/curl-config --static-libs\n> /Users/dborowitz/d/curl-out-7.36.0/lib/libcurl.a\n> -Wl,-syslibroot,/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk\n> -arch x86_64 -Wl,-headerpad_max_install_names -framework\n> CoreFoundation -framework Security -lgssapi_krb5 -lresolv -lldap -lz\n>\n> So I don't think specifying NEEDS_*_WITH_CURL scales to this use case.\n\nThanks.  As I said, the kosher way to learn how to link with libcURL\nis by asking curl-config about the details of options to give to the\ncompiler and the linker, so I am all for this change.\n\n> While writing this up I also noticed an issue with the second patch,\n> namely that `curl-config --static-libs` is empty when curl is not\n> built for static linking.\n>\n> I will reroll with a more detailed commit message and a fix to the second patch.\n\nI love it whenever I see the contents of the patch improved after\nspending a bit more time trying to describe the problem and the\nsolution (which is time worth spending).\n\nThanks.\n"}]}