# [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 12 messages from 2013-08-05 to 2013-08-06. Participants: Brian Gernhardt, Jeremy Huddleston Sequoia, Junio C Hamano, Dave Borowitz, David Aguilar. Thread: https://gitlist.dev/t/34614 ## Brian Gernhardt, 2013-08-05 15:59 Subject: [PATCH 0/3] Fixes for OS X Message-ID: <1375718364-13824-1-git-send-email-brian@gernhardtsoftware.com> URL: https://gitlist.dev/e/1375718364-13824-1-git-send-email-brian%40gernhardtsoftware.com ``` A few changes recently broke my build on Mac 10.8, possibly because I have a more strict set of warnings/errors enabled. The first two handle minor problems with the use of APPLE_COMMON_CRYPTO, which was expanded for use in imap-send but had a couple of problems. The last is likely due to curl version skew between Dave Borowitz and myself. (see 912b2ac). There are a few notes on the patches indicating where I was less than sure about my solutions. Brian Gernhardt (3): Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 OS X: Fix redeclaration of die warning t5551: Remove header from curl cookie file Makefile | 4 +++- git-compat-util.h | 20 ++++++++++---------- t/t5551-http-fetch.sh | 6 ++---- 3 files changed, 15 insertions(+), 15 deletions(-) -- 1.8.4.rc1.384.g0976a17.dirty ``` ## Brian Gernhardt, 2013-08-05 15:59 Subject: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 Message-ID: <1375718364-13824-2-git-send-email-brian@gernhardtsoftware.com> URL: https://gitlist.dev/e/1375718364-13824-2-git-send-email-brian%40gernhardtsoftware.com In-Reply-To: <1375718364-13824-1-git-send-email-brian@gernhardtsoftware.com> ``` It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was set. But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see 3ef2bca) so make sure that the appropriate libraries are always set. Signed-off-by: Brian Gernhardt --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 82f2e22..7051956 100644 --- a/Makefile +++ b/Makefile @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO else LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto endif +ifdef APPLE_COMMON_CRYPTO + LIB_4_CRYPTO += -framework Security -framework CoreFoundation +endif endif ifdef NEEDS_LIBICONV ifdef ICONVDIR @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1 LIB_H += ppc/sha1.h else ifdef APPLE_COMMON_CRYPTO - LIB_4_CRYPTO += -framework Security -framework CoreFoundation COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL SHA1_HEADER = else -- 1.8.4.rc1.384.g0976a17.dirty ``` ## Brian Gernhardt, 2013-08-05 15:59 Subject: [PATCH 2/3] OS X: Fix redeclaration of die warning Message-ID: <1375718364-13824-3-git-send-email-brian@gernhardtsoftware.com> URL: https://gitlist.dev/e/1375718364-13824-3-git-send-email-brian%40gernhardtsoftware.com In-Reply-To: <1375718364-13824-1-git-send-email-brian@gernhardtsoftware.com> ``` compat/apple-common-crypto.h uses die() in one of its macros, but was included in git-compat-util.h before the definition of die. Fix by simply moving the relevant block after the die/error/warning declarations. Signed-off-by: Brian Gernhardt --- Not sure if this is the best place to move it to, but it's the earliest it can be in the file without causing errors. (Namely that clang has to guess what die() means in apple-common-crypto.h and guesses differently than the actual definition.) git-compat-util.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index af5f6bb..d60e28d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -129,16 +129,6 @@ #include #endif -#ifndef NO_OPENSSL -#ifdef APPLE_COMMON_CRYPTO -#include "compat/apple-common-crypto.h" -#else -#include -#include -#endif /* APPLE_COMMON_CRYPTO */ -#include -#endif /* NO_OPENSSL */ - #if defined(__MINGW32__) /* pull in Windows compatibility stuff */ #include "compat/mingw.h" @@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin extern int error(const char *err, ...) __attribute__((format (printf, 1, 2))); extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))); +#ifndef NO_OPENSSL +#ifdef APPLE_COMMON_CRYPTO +#include "compat/apple-common-crypto.h" +#else +#include +#include +#endif /* APPLE_COMMON_CRYPTO */ +#include +#endif /* NO_OPENSSL */ + /* * Let callers be aware of the constant return value; this can help * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though, -- 1.8.4.rc1.384.g0976a17.dirty ``` ## Brian Gernhardt, 2013-08-05 15:59 Subject: [PATCH 3/3] t5551: Remove header from curl cookie file Message-ID: <1375718364-13824-4-git-send-email-brian@gernhardtsoftware.com> URL: https://gitlist.dev/e/1375718364-13824-4-git-send-email-brian%40gernhardtsoftware.com In-Reply-To: <1375718364-13824-1-git-send-email-brian@gernhardtsoftware.com> ``` The URL included in the header appears to vary from curl version to curl version. Since we only care about the final few lines, only test them. However, make sure the blank line after the header is still included to make sure there are no extra cookie lines. Signed-off-by: Brian Gernhardt --- I suppose a sed invocation to strip out the URL or comments might be better, but this seemed simpler. t/t5551-http-fetch.sh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh index 287d22b..8196af1 100755 --- a/t/t5551-http-fetch.sh +++ b/t/t5551-http-fetch.sh @@ -191,9 +191,6 @@ cat >cookies.txt <expect_cookies.txt < cookies_tail.txt + test_cmp expect_cookies.txt cookies_tail.txt ' test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE -- 1.8.4.rc1.384.g0976a17.dirty ``` ## Jeremy Huddleston Sequoia, 2013-08-05 16:16 Subject: Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 Message-ID: <43C3B9F4-9E43-4BE0-B683-8AF6E2E46EC6@apple.com> URL: https://gitlist.dev/e/43C3B9F4-9E43-4BE0-B683-8AF6E2E46EC6%40apple.com In-Reply-To: <1375718364-13824-2-git-send-email-brian@gernhardtsoftware.com> ``` Thanks Brian, Reviewed-by: Jeremy Huddleston Sequoia On Aug 5, 2013, at 8:59, Brian Gernhardt wrote: > It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was > set. But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see > 3ef2bca) so make sure that the appropriate libraries are always set. > > Signed-off-by: Brian Gernhardt > --- > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 82f2e22..7051956 100644 > --- a/Makefile > +++ b/Makefile > @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO > else > LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto > endif > +ifdef APPLE_COMMON_CRYPTO > + LIB_4_CRYPTO += -framework Security -framework CoreFoundation > +endif > endif > ifdef NEEDS_LIBICONV > ifdef ICONVDIR > @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1 > LIB_H += ppc/sha1.h > else > ifdef APPLE_COMMON_CRYPTO > - LIB_4_CRYPTO += -framework Security -framework CoreFoundation > COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL > SHA1_HEADER = > else > -- > 1.8.4.rc1.384.g0976a17.dirty > ``` ## Jeremy Huddleston Sequoia, 2013-08-05 16:17 Subject: Re: [PATCH 2/3] OS X: Fix redeclaration of die warning Message-ID: <64E52D7E-518B-47B4-A048-22BF326ACEF7@apple.com> URL: https://gitlist.dev/e/64E52D7E-518B-47B4-A048-22BF326ACEF7%40apple.com In-Reply-To: <1375718364-13824-3-git-send-email-brian@gernhardtsoftware.com> ``` Thanks Brian, Reviewed-by: Jeremy Huddleston Sequoia On Aug 5, 2013, at 8:59, Brian Gernhardt wrote: > compat/apple-common-crypto.h uses die() in one of its macros, but was > included in git-compat-util.h before the definition of die. > > Fix by simply moving the relevant block after the die/error/warning > declarations. > > Signed-off-by: Brian Gernhardt > --- > > Not sure if this is the best place to move it to, but it's the earliest it can > be in the file without causing errors. (Namely that clang has to guess what > die() means in apple-common-crypto.h and guesses differently than the actual > definition.) > > git-compat-util.h | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index af5f6bb..d60e28d 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -129,16 +129,6 @@ > #include > #endif > > -#ifndef NO_OPENSSL > -#ifdef APPLE_COMMON_CRYPTO > -#include "compat/apple-common-crypto.h" > -#else > -#include > -#include > -#endif /* APPLE_COMMON_CRYPTO */ > -#include > -#endif /* NO_OPENSSL */ > - > #if defined(__MINGW32__) > /* pull in Windows compatibility stuff */ > #include "compat/mingw.h" > @@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin > extern int error(const char *err, ...) __attribute__((format (printf, 1, 2))); > extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))); > > +#ifndef NO_OPENSSL > +#ifdef APPLE_COMMON_CRYPTO > +#include "compat/apple-common-crypto.h" > +#else > +#include > +#include > +#endif /* APPLE_COMMON_CRYPTO */ > +#include > +#endif /* NO_OPENSSL */ > + > /* > * Let callers be aware of the constant return value; this can help > * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though, > -- > 1.8.4.rc1.384.g0976a17.dirty > ``` ## Junio C Hamano, 2013-08-05 17:52 Subject: Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 Message-ID: <7v4nb4kosp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4nb4kosp.fsf%40alter.siamese.dyndns.org In-Reply-To: <1375718364-13824-2-git-send-email-brian@gernhardtsoftware.com> ``` Brian Gernhardt writes: > It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was > set. But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see > 3ef2bca) so make sure that the appropriate libraries are always set. > > Signed-off-by: Brian Gernhardt > --- > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 82f2e22..7051956 100644 > --- a/Makefile > +++ b/Makefile > @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO > else > LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto > endif > +ifdef APPLE_COMMON_CRYPTO > + LIB_4_CRYPTO += -framework Security -framework CoreFoundation > +endif > endif > ifdef NEEDS_LIBICONV > ifdef ICONVDIR > @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1 > LIB_H += ppc/sha1.h > else > ifdef APPLE_COMMON_CRYPTO > - LIB_4_CRYPTO += -framework Security -framework CoreFoundation > COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL > SHA1_HEADER = > else Hmph. So the people previously tested this must have built imap-send without blk-sha1, which not just linked with these libs but also included the header file and defined the -DCOMMON_DIGEST_FOR_OPENSSL preprocessor macro. Building with blk-sha1 would not have worked for them. Now we always link with these libraries, even when building with blk-sha1. Do the COMPAT_CFLAGS and SHA1_HEADER pieces only needed when using the SHA1 digest implementation from CommonCrypto and nothing imap-send uses? ``` ## Junio C Hamano, 2013-08-05 18:00 Subject: Re: [PATCH 2/3] OS X: Fix redeclaration of die warning Message-ID: <7vwqo0j9us.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwqo0j9us.fsf%40alter.siamese.dyndns.org In-Reply-To: <1375718364-13824-3-git-send-email-brian@gernhardtsoftware.com> ``` Brian Gernhardt writes: > compat/apple-common-crypto.h uses die() in one of its macros, but was > included in git-compat-util.h before the definition of die. > > Fix by simply moving the relevant block after the die/error/warning > declarations. Puzzled. What needs fixing??? Ahh, that one is not just making #define macros, but defining static inline functions. I wonder if they need to be static inlines to be duplicated at each call sites in the first place. Wouldn't it be better to create a compat/something.c file to be linked with? > Signed-off-by: Brian Gernhardt > --- > > Not sure if this is the best place to move it to, but it's the earliest it can > be in the file without causing errors. (Namely that clang has to guess what > die() means in apple-common-crypto.h and guesses differently than the actual > definition.) > > git-compat-util.h | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index af5f6bb..d60e28d 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -129,16 +129,6 @@ > #include > #endif > > -#ifndef NO_OPENSSL > -#ifdef APPLE_COMMON_CRYPTO > -#include "compat/apple-common-crypto.h" > -#else > -#include > -#include > -#endif /* APPLE_COMMON_CRYPTO */ > -#include > -#endif /* NO_OPENSSL */ > - > #if defined(__MINGW32__) > /* pull in Windows compatibility stuff */ > #include "compat/mingw.h" > @@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin > extern int error(const char *err, ...) __attribute__((format (printf, 1, 2))); > extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))); > > +#ifndef NO_OPENSSL > +#ifdef APPLE_COMMON_CRYPTO > +#include "compat/apple-common-crypto.h" > +#else > +#include > +#include > +#endif /* APPLE_COMMON_CRYPTO */ > +#include > +#endif /* NO_OPENSSL */ > + > /* > * Let callers be aware of the constant return value; this can help > * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though, ``` ## Dave Borowitz, 2013-08-06 00:29 Subject: Re: [PATCH 3/3] t5551: Remove header from curl cookie file Message-ID: URL: https://gitlist.dev/e/CAD0k6qS7_W6iG5zCOLiEtxdMV3ujAApgDykM_fnL0qiROoUv0A%40mail.gmail.com In-Reply-To: <1375718364-13824-4-git-send-email-brian@gernhardtsoftware.com> ``` On Mon, Aug 5, 2013 at 8:59 AM, Brian Gernhardt wrote: > > The URL included in the header appears to vary from curl version to > curl version. Since we only care about the final few lines, only test > them. However, make sure the blank line after the header is still > included to make sure there are no extra cookie lines. > > Signed-off-by: Brian Gernhardt > --- > > I suppose a sed invocation to strip out the URL or comments might be better, > but this seemed simpler. > > t/t5551-http-fetch.sh | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh > index 287d22b..8196af1 100755 > --- a/t/t5551-http-fetch.sh > +++ b/t/t5551-http-fetch.sh > @@ -191,9 +191,6 @@ cat >cookies.txt < 127.0.0.1 FALSE /smart_cookies/ FALSE 0 othername othervalue > EOF > cat >expect_cookies.txt < -# Netscape HTTP Cookie File > -# http://curl.haxx.se/docs/http-cookies.html > -# This file was generated by libcurl! Edit at your own risk. > > 127.0.0.1 FALSE /smart_cookies/ FALSE 0 othername othervalue > 127.0.0.1 FALSE /smart_cookies/repo.git/info/ FALSE 0 name value > @@ -202,7 +199,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set > git config http.cookiefile cookies.txt && > git config http.savecookies true && > git ls-remote $HTTPD_URL/smart_cookies/repo.git master && > - test_cmp expect_cookies.txt cookies.txt > + tail -3 cookies.txt > cookies_tail.txt Would it make more sense to ignore comments entirely? I.e. instead of taking the tail, pipe through sed 's/#.*//'. Thanks for catching this by the way; you can probably guess that I only checked with a single curl version. > > + test_cmp expect_cookies.txt cookies_tail.txt > ' > > test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE > -- > 1.8.4.rc1.384.g0976a17.dirty > ``` ## David Aguilar, 2013-08-06 11:25 Subject: Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 Message-ID: URL: https://gitlist.dev/e/CAJDDKr7FHs0sKH9_r%3DQG9hs1Rp1_QgvqXyaiTkVLFVq0xMr8%3DA%40mail.gmail.com In-Reply-To: <7v4nb4kosp.fsf@alter.siamese.dyndns.org> ``` On Mon, Aug 5, 2013 at 10:52 AM, Junio C Hamano wrote: > Brian Gernhardt writes: > >> It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was >> set. But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see >> 3ef2bca) so make sure that the appropriate libraries are always set. >> >> Signed-off-by: Brian Gernhardt >> --- >> Makefile | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index 82f2e22..7051956 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO >> else >> LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto >> endif >> +ifdef APPLE_COMMON_CRYPTO >> + LIB_4_CRYPTO += -framework Security -framework CoreFoundation >> +endif >> endif >> ifdef NEEDS_LIBICONV >> ifdef ICONVDIR >> @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1 >> LIB_H += ppc/sha1.h >> else >> ifdef APPLE_COMMON_CRYPTO >> - LIB_4_CRYPTO += -framework Security -framework CoreFoundation >> COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL >> SHA1_HEADER = >> else > > Hmph. > > So the people previously tested this must have built imap-send > without blk-sha1, which not just linked with these libs but also > included the header file and defined > the -DCOMMON_DIGEST_FOR_OPENSSL preprocessor macro. Building with > blk-sha1 would not have worked for them. > Now we always link with these libraries, even when building with > blk-sha1. Do the COMPAT_CFLAGS and SHA1_HEADER pieces only needed > when using the SHA1 digest implementation from CommonCrypto and > nothing imap-send uses? LIB_4_CRYPTO is used by imap-send only, and these libraries are needed for the base64 git_CC_EVP_(Encode|Decode), so unconditionally adding these libraries there is correct. COMPAT_CFLAGS and SHA1_HEADER enable the common crypto SHA1 only. BLK_SHA1 provides its own SHA1 so they're not needed there. I tested the tip of da/darwin (pu) w/ and w/out BLK_SHA1. Tested-by: David Aguilar -- David ``` ## David Aguilar, 2013-08-06 11:30 Subject: Re: [PATCH 2/3] OS X: Fix redeclaration of die warning Message-ID: URL: https://gitlist.dev/e/CAJDDKr5_SU_yyWCyOHtP2nTYVe48e8-JOvR0hG5%3DWJjLBkOHWw%40mail.gmail.com In-Reply-To: <7vwqo0j9us.fsf@alter.siamese.dyndns.org> ``` On Mon, Aug 5, 2013 at 11:00 AM, Junio C Hamano wrote: > Brian Gernhardt writes: > >> compat/apple-common-crypto.h uses die() in one of its macros, but was >> included in git-compat-util.h before the definition of die. >> >> Fix by simply moving the relevant block after the die/error/warning >> declarations. > > Puzzled. What needs fixing??? > > Ahh, that one is not just making #define macros, but defining static > inline functions. > > I wonder if they need to be static inlines to be duplicated at each > call sites in the first place. Wouldn't it be better to create a > compat/something.c file to be linked with? IMO it's not worth it right now because there's only a single call site (imap-send). The moment another call site is introduced then compat/apple-common-crypto.c would be the natural home for it. -- David ``` ## Junio C Hamano, 2013-08-06 17:24 Subject: Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1 Message-ID: <7va9kuhguy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7va9kuhguy.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` David Aguilar writes: > I tested the tip of da/darwin (pu) w/ and w/out BLK_SHA1. > > Tested-by: David Aguilar Thanks. ```