threads / patch / 33823

v5, 2 partsimap-send: eliminate HMAC deprecation warnings on Mac OS X

Subject: [PATCH v5 2/2] imap-send: eliminate HMAC deprecation warnings on Mac OS X

## tl;dr

13 messages between May 15, 2013 and May 19, 2013. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

David Aguilar· May 15, 2013, 07:11 UTC · lore

[PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

Mac OS X 10.8 Mountain Lion prints warnings when building git:
	warning: 'SHA1_Init' is deprecated
	(declared at /usr/include/openssl/sha.h:121)

Silence the warnings by using the CommonCrytpo SHA-1 functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

COMMON_DIGEST_FOR_OPENSSL is defined to enable the OpenSSL compatibility macros in CommonDigest.h.

Add a NO_APPLE_COMMON_CRYPTO option to the Makefile to allow users to opt out of using this library. When defined, Git will use OpenSSL instead.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
Both of these are replacement patches "pu".
Changes from last time:

It now uses a single APPLE_COMMON_CRYPTO definition. Users can now opt-out by setting NO_APPLE_COMMON_CRYPTO.

 Makefile | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to Makefile +13 −0
diff --git a/Makefile b/Makefile
index f698c1a..8309c41 100644
--- a/Makefile
+++ b/Makefile
@@ -137,6 +137,10 @@ all::
 # specify your own (or DarwinPort's) include directories and
 # library directories by defining CFLAGS and LDFLAGS appropriately.
 #
+# Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X
+# and do not want to use Apple's CommonCrypto library.  This allows you
+# to provide your own OpenSSL library, for example from MacPorts.
+#
 # Define BLK_SHA1 environment variable to make use of the bundled
 # optimized C SHA1 routine.
 #
@@ -1054,6 +1058,9 @@ ifeq ($(uname_S),Darwin)
 			BASIC_LDFLAGS += -L/opt/local/lib
 		endif
 	endif
+	ifndef NO_APPLE_COMMON_CRYPTO
+		APPLE_COMMON_CRYPTO = YesPlease
+	endif
 	NO_REGEX = YesPlease
 	PTHREAD_LIBS =
 endif
@@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
 	LIB_H += ppc/sha1.h
 else
+ifdef APPLE_COMMON_CRYPTO
+	BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
+	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
+else
 	SHA1_HEADER = <openssl/sha.h>
 	EXTLIBS += $(LIB_4_CRYPTO)
 endif
 endif
+endif
+
 ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif

-- 
1.8.3.rc2.2.gbc955d1
David Aguilar· May 15, 2013, 07:11 UTC · re: David Aguilar · lore

Mac OS X 10.8 Mountain Lion warns that HMAC_Init() and friends are deprecated. Detect the COMMON_CRYPTO_FOR_OPENSSL definition and use CommonCrypto's HMAC functions to eliminate the warnings.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Changes since last time:

This version re-uses the existing COMMON_CRYPTO_FOR_OPENSSL define instead of tweaking the Makefile to add a new one, so it's simpler.

My previous patch had Jonathan's reviewed-by tag, but he hasn't reviewed this exact patch, so I removed it. The C macros are unchanged.

 imap-send.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to imap-send.c +10 −0
diff --git a/imap-send.c b/imap-send.c
index d9bcfb4..96012b1 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -29,8 +29,18 @@
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #else
+#ifdef COMMON_DIGEST_FOR_OPENSSL
+#include <CommonCrypto/CommonHMAC.h>
+#define HMAC_CTX CCHmacContext
+#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len)
+#define HMAC_Update CCHmacUpdate
+#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash)
+#define HMAC_CTX_cleanup
+#define EVP_md5() kCCHmacAlgMD5
+#else
 #include <openssl/evp.h>
 #include <openssl/hmac.h>
+#endif
 #include <openssl/x509v3.h>
 #endif
 

-- 
1.8.3.rc2.2.gbc955d1
Torsten Bögershausen· May 15, 2013, 17:56 UTC · re: David Aguilar · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On 2013-05-15 09.11, David Aguilar wrote:
Show 60 quoted lines
> Mac OS X 10.8 Mountain Lion prints warnings when building git:
> 
> 	warning: 'SHA1_Init' is deprecated
> 	(declared at /usr/include/openssl/sha.h:121)
> 
> Silence the warnings by using the CommonCrytpo SHA-1
> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().
> 
> COMMON_DIGEST_FOR_OPENSSL is defined to enable the OpenSSL
> compatibility macros in CommonDigest.h.
> 
> Add a NO_APPLE_COMMON_CRYPTO option to the Makefile to allow
> users to opt out of using this library.  When defined, Git will
> use OpenSSL instead.
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> Both of these are replacement patches "pu".
> 
> Changes from last time:
> 
> It now uses a single APPLE_COMMON_CRYPTO definition.
> Users can now opt-out by setting NO_APPLE_COMMON_CRYPTO.
> 
>  Makefile | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index f698c1a..8309c41 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -137,6 +137,10 @@ all::
>  # specify your own (or DarwinPort's) include directories and
>  # library directories by defining CFLAGS and LDFLAGS appropriately.
>  #
> +# Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X
> +# and do not want to use Apple's CommonCrypto library.  This allows you
> +# to provide your own OpenSSL library, for example from MacPorts.
> +#
>  # Define BLK_SHA1 environment variable to make use of the bundled
>  # optimized C SHA1 routine.
>  #
> @@ -1054,6 +1058,9 @@ ifeq ($(uname_S),Darwin)
>  			BASIC_LDFLAGS += -L/opt/local/lib
>  		endif
>  	endif
> +	ifndef NO_APPLE_COMMON_CRYPTO
> +		APPLE_COMMON_CRYPTO = YesPlease
> +	endif
>  	NO_REGEX = YesPlease
>  	PTHREAD_LIBS =
>  endif
> @@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
>  	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
>  	LIB_H += ppc/sha1.h
>  else
> +ifdef APPLE_COMMON_CRYPTO
> +	BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
> +	SHA1_HEADER = <CommonCrypto/CommonDigest.h>

Would it make sense to replace APPLE_COMMON_CRYPTO with COMMON_DIGEST_FOR_OPENSSL ?

In the spirit of other Makefile-defines becoming Compiler defines,
a random picked example:
ifdef NO_STRTOULL
	COMPAT_CFLAGS += -DNO_STRTOULL
endif
/Torsten
Eric Sunshine· May 17, 2013, 06:18 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Wed, May 15, 2013 at 1:56 PM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 23 quoted lines
> On 2013-05-15 09.11, David Aguilar wrote:
>> +     ifndef NO_APPLE_COMMON_CRYPTO
>> +             APPLE_COMMON_CRYPTO = YesPlease
>> +     endif
>>       NO_REGEX = YesPlease
>>       PTHREAD_LIBS =
>>  endif
>> @@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
>>       LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
>>       LIB_H += ppc/sha1.h
>>  else
>> +ifdef APPLE_COMMON_CRYPTO
>> +     BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>> +     SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>
> Would it make sense to replace APPLE_COMMON_CRYPTO
> with COMMON_DIGEST_FOR_OPENSSL ?
>
> In the spirit of other Makefile-defines becoming Compiler defines,
> a random picked example:
> ifdef NO_STRTOULL
>         COMPAT_CFLAGS += -DNO_STRTOULL
> endif

Not necessarily. Unlike NO_STRTOULL and cousins, COMMON_DIGEST_FOR_OPENSSL is not a Git build tweak; it is merely a (public) implementation detail of the Apple header [1] to magically associate OpenSSL digest functions with CommonCrypto counterparts. It's not the only such macro recognized by the Apple headers. For instance, COMMON_DIGEST_FOR_RFC_1321 magically associates legacy MD5 digest functions with CommonCrypto counterparts.

Further, as Junio noted elsewhere, David is using CommonCrypto for HMAC replacements, not just for digest replacements, so a Makefile knob with DIGEST in its name is not really appropriate. More generally, David would like to find CommonCrypto replacements for all the OpenSSL functionality, so a Makefile knob named after DIGEST is too specific.

These considerations motivated the original suggestion for a single Git Makefile knob to enable/disable, as a unit, all CommonCrypto replacements. Such a knob would naturally have COMMON_CRYPTO as part of its name.

[1]: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/include/CommonCrypto/CommonDigest.h
-- ES
David Aguilar· May 17, 2013, 08:21 UTC · re: Eric Sunshine · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Thu, May 16, 2013 at 11:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 46 quoted lines
> On Wed, May 15, 2013 at 1:56 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> On 2013-05-15 09.11, David Aguilar wrote:
>>> +     ifndef NO_APPLE_COMMON_CRYPTO
>>> +             APPLE_COMMON_CRYPTO = YesPlease
>>> +     endif
>>>       NO_REGEX = YesPlease
>>>       PTHREAD_LIBS =
>>>  endif
>>> @@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
>>>       LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
>>>       LIB_H += ppc/sha1.h
>>>  else
>>> +ifdef APPLE_COMMON_CRYPTO
>>> +     BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>>> +     SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>>
>> Would it make sense to replace APPLE_COMMON_CRYPTO
>> with COMMON_DIGEST_FOR_OPENSSL ?
>>
>> In the spirit of other Makefile-defines becoming Compiler defines,
>> a random picked example:
>> ifdef NO_STRTOULL
>>         COMPAT_CFLAGS += -DNO_STRTOULL
>> endif
>
> Not necessarily. Unlike NO_STRTOULL and cousins,
> COMMON_DIGEST_FOR_OPENSSL is not a Git build tweak; it is merely a
> (public) implementation detail of the Apple header [1] to magically
> associate OpenSSL digest functions with CommonCrypto counterparts.
> It's not the only such macro recognized by the Apple headers. For
> instance, COMMON_DIGEST_FOR_RFC_1321 magically associates legacy MD5
> digest functions with CommonCrypto counterparts.
>
> Further, as Junio noted elsewhere, David is using CommonCrypto for
> HMAC replacements, not just for digest replacements, so a Makefile
> knob with DIGEST in its name is not really appropriate. More
> generally, David would like to find CommonCrypto replacements for all
> the OpenSSL functionality, so a Makefile knob named after DIGEST is
> too specific.
>
> These considerations motivated the original suggestion for a single
> Git Makefile knob to enable/disable, as a unit, all CommonCrypto
> replacements. Such a knob would naturally have COMMON_CRYPTO as part
> of its name.
>
> [1]: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/include/CommonCrypto/CommonDigest.h
This is a nice justification for taking v5 of this series over v6.

Sorry for all the churn in this series, Junio. I wrote v5 so I certainly felt it was a good idea at the time, and I feel bad for not having waited longer before sending out v6 (which is what was eventually queued in "pu").

Do you have advice on how we should proceed? :sigh: sorry for wasting so much maintainer time on this series already. If you need any resends or anything please let me know. This time I'll wait for a strong opinion before firing off patches.

My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where we should have stopped painting. Hindsight is 20/20. Luckily it never left "pu". -- David

Junio C Hamano· May 17, 2013, 16:53 UTC · re: David Aguilar · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

David Aguilar <davvid@gmail.com> writes:
Show 8 quoted lines
> Do you have advice on how we should proceed?  :sigh: sorry for wasting
> so much maintainer time on this series already.  If you need any
> resends or anything please let me know.  This time I'll wait for a
> strong opinion before firing off patches.
>
> My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where
> we should have stopped painting.  Hindsight is 20/20.  Luckily it
> never left "pu".
I could do this easily:
    $ git checkout da/darwin ;# b72ac20a6f73b
    $ git format-patch --stdout -2 |
      sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
    $ git checkout HEAD^^ ;# 29de20504e
    $ git am P.mbox
    $ git diff da/darwin HEAD ;# sanity check
    $ git log da/darwin.. ;# sanity check
    $ git branch -f da/darwin
if you nicely ask ;-)
David Aguilar· May 17, 2013, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Fri, May 17, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>
>> Do you have advice on how we should proceed?  :sigh: sorry for wasting
>> so much maintainer time on this series already.  If you need any
>> resends or anything please let me know.  This time I'll wait for a
>> strong opinion before firing off patches.
>>
>> My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where
>> we should have stopped painting.  Hindsight is 20/20.  Luckily it
>> never left "pu".
>
> I could do this easily:
>
>     $ git checkout da/darwin ;# b72ac20a6f73b
>     $ git format-patch --stdout -2 |
>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>     $ git checkout HEAD^^ ;# 29de20504e
>     $ git am P.mbox
>     $ git diff da/darwin HEAD ;# sanity check
>     $ git log da/darwin.. ;# sanity check
>     $ git branch -f da/darwin
>
> if you nicely ask ;-)
gitster please ;-)

-- David

Eric Sunshine· May 17, 2013, 17:29 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>
>> Do you have advice on how we should proceed?  :sigh: sorry for wasting
>> so much maintainer time on this series already.  If you need any
>> resends or anything please let me know.  This time I'll wait for a
>> strong opinion before firing off patches.
>>
>> My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where
>> we should have stopped painting.  Hindsight is 20/20.  Luckily it
>> never left "pu".
>
> I could do this easily:
>
>     $ git checkout da/darwin ;# b72ac20a6f73b
>     $ git format-patch --stdout -2 |
>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>     $ git checkout HEAD^^ ;# 29de20504e
>     $ git am P.mbox
>     $ git diff da/darwin HEAD ;# sanity check
>     $ git log da/darwin.. ;# sanity check
>     $ git branch -f da/darwin

That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which needs to be defined before <CommonCrypto/CommonDigest.h> is included.

Junio C Hamano· May 17, 2013, 17:57 UTC · re: Eric Sunshine · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 25 quoted lines
> On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Aguilar <davvid@gmail.com> writes:
>>
>>> Do you have advice on how we should proceed?  :sigh: sorry for wasting
>>> so much maintainer time on this series already.  If you need any
>>> resends or anything please let me know.  This time I'll wait for a
>>> strong opinion before firing off patches.
>>>
>>> My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where
>>> we should have stopped painting.  Hindsight is 20/20.  Luckily it
>>> never left "pu".
>>
>> I could do this easily:
>>
>>     $ git checkout da/darwin ;# b72ac20a6f73b
>>     $ git format-patch --stdout -2 |
>>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>>     $ git checkout HEAD^^ ;# 29de20504e
>>     $ git am P.mbox
>>     $ git diff da/darwin HEAD ;# sanity check
>>     $ git log da/darwin.. ;# sanity check
>>     $ git branch -f da/darwin
>
> That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which
> needs to be defined before <CommonCrypto/CommonDigest.h> is included.

It probably is a good catch, but I'll stop reading patches and start today's integration cycle (and will tag 1.8.3-rc3).

At this point, I think the best would be for you to reroll these two patches, then after David reviews it and I'd re-queue the result with

	... original commit message ...
        Reorginized use of Makefile variables and C preprosessor
        macros with help by Eric Sunshine.
	Signed-off-by: David
        Signed-off-by: Eric
        Signed-off-by: Me
or something.  Can you two help that?
David Aguilar· May 18, 2013, 00:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Fri, May 17, 2013 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 45 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> David Aguilar <davvid@gmail.com> writes:
>>>
>>>> Do you have advice on how we should proceed?  :sigh: sorry for wasting
>>>> so much maintainer time on this series already.  If you need any
>>>> resends or anything please let me know.  This time I'll wait for a
>>>> strong opinion before firing off patches.
>>>>
>>>> My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where
>>>> we should have stopped painting.  Hindsight is 20/20.  Luckily it
>>>> never left "pu".
>>>
>>> I could do this easily:
>>>
>>>     $ git checkout da/darwin ;# b72ac20a6f73b
>>>     $ git format-patch --stdout -2 |
>>>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>>>     $ git checkout HEAD^^ ;# 29de20504e
>>>     $ git am P.mbox
>>>     $ git diff da/darwin HEAD ;# sanity check
>>>     $ git log da/darwin.. ;# sanity check
>>>     $ git branch -f da/darwin
>>
>> That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which
>> needs to be defined before <CommonCrypto/CommonDigest.h> is included.
>
> It probably is a good catch, but I'll stop reading patches and start
> today's integration cycle (and will tag 1.8.3-rc3).
>
> At this point, I think the best would be for you to reroll these two
> patches, then after David reviews it and I'd re-queue the result
> with
>
>         ... original commit message ...
>
>         Reorginized use of Makefile variables and C preprosessor
>         macros with help by Eric Sunshine.
>
>         Signed-off-by: David
>         Signed-off-by: Eric
>         Signed-off-by: Me
>
> or something.  Can you two help that?
Thanks Eric and Junio.  I looked over the patches and they look good.
Signed-off-by: David Aguilar <davvid@gmail.com>

Cheers, -- David

Junio C Hamano· May 19, 2013, 06:26 UTC · re: David Aguilar · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

David Aguilar <davvid@gmail.com> writes:
> Thanks Eric and Junio.  I looked over the patches and they look good.

Are you sure about that? It seemed to me that it was breaking everybody that is not on MacOS X --- did I misread the patch?

David Aguilar· May 19, 2013, 21:51 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Sat, May 18, 2013 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> David Aguilar <davvid@gmail.com> writes:
>
>> Thanks Eric and Junio.  I looked over the patches and they look good.
>
> Are you sure about that?  It seemed to me that it was breaking
> everybody that is not on MacOS X --- did I misread the patch?

Gah, correct. I've now tested v8 which Eric just sent out. It worked fine for me, with and without NO_APPLE_COMMON_CRYPTO.

Thanks, -- David

Eric Sunshine· May 17, 2013, 17:40 UTC · re: David Aguilar · lore

Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

On Fri, May 17, 2013 at 4:21 AM, David Aguilar <davvid@gmail.com> wrote:
Show 47 quoted lines
> On Thu, May 16, 2013 at 11:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, May 15, 2013 at 1:56 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>>> On 2013-05-15 09.11, David Aguilar wrote:
>>>> +     ifndef NO_APPLE_COMMON_CRYPTO
>>>> +             APPLE_COMMON_CRYPTO = YesPlease
>>>> +     endif
>>>>       NO_REGEX = YesPlease
>>>>       PTHREAD_LIBS =
>>>>  endif
>>>> @@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
>>>>       LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
>>>>       LIB_H += ppc/sha1.h
>>>>  else
>>>> +ifdef APPLE_COMMON_CRYPTO
>>>> +     BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>>>> +     SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>>>
>>> Would it make sense to replace APPLE_COMMON_CRYPTO
>>> with COMMON_DIGEST_FOR_OPENSSL ?
>>>
>>> In the spirit of other Makefile-defines becoming Compiler defines,
>>> a random picked example:
>>> ifdef NO_STRTOULL
>>>         COMPAT_CFLAGS += -DNO_STRTOULL
>>> endif
>>
>> Not necessarily. Unlike NO_STRTOULL and cousins,
>> COMMON_DIGEST_FOR_OPENSSL is not a Git build tweak; it is merely a
>> (public) implementation detail of the Apple header [1] to magically
>> associate OpenSSL digest functions with CommonCrypto counterparts.
>> It's not the only such macro recognized by the Apple headers. For
>> instance, COMMON_DIGEST_FOR_RFC_1321 magically associates legacy MD5
>> digest functions with CommonCrypto counterparts.
>>
>> Further, as Junio noted elsewhere, David is using CommonCrypto for
>> HMAC replacements, not just for digest replacements, so a Makefile
>> knob with DIGEST in its name is not really appropriate. More
>> generally, David would like to find CommonCrypto replacements for all
>> the OpenSSL functionality, so a Makefile knob named after DIGEST is
>> too specific.
>>
>> These considerations motivated the original suggestion for a single
>> Git Makefile knob to enable/disable, as a unit, all CommonCrypto
>> replacements. Such a knob would naturally have COMMON_CRYPTO as part
>> of its name.
>
> This is a nice justification for taking v5 of this series over v6.

You will consider this bike-shedding (I don't), but the above also is good justification for revising your HMAC patch to _not_ rely on COMMON_DIGEST_FOR_OPENSSL, which is an implementation detail of your SHA patch, rather than a proper build knob.

Similar to NO_STRTOULL and cousins, you should have a #define (such as NO_APPLE_COMMON_CRYPTO or NO_COMMON_CRYPTO) which is consulted by your HMAC patch and any future patches you submit to map CommonCrypto counterparts to OpenSSL functions. The fact that you also must #define COMMON_DIGEST_FOR_OPENSSL for the SHA patch is just an implementation detail of that one patch; it is not relevant to the other patches.

-- ES

← back to recent threads