{"thread":{"id":"38514","subject":"[PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","startedAt":"2015-02-06T09:35:31Z","lastAt":"2015-02-06T22:01:28Z","messageCount":6,"participants":["Kyle J. McKay","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255667","messageId":"f2a35ed9fba20d48ef5736ee4564101@74d39fa044aa309eaea14b9f57fe79c","threadId":"38514","inReplyTo":null,"subject":"[PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-06T09:35:31Z","receivedAt":"2015-02-06T09:35:31Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"MAC_OS_X_VERSION_MIN_REQUIRED may be defined by the builder to a\nspecific version in order to produce compatible binaries for a\nparticular system. Blindly defining it to MAC_OS_X_VERSION_10_6\nis bad.\n\nAdditionally MAC_OS_X_VERSION_10_6 will not be defined on older\nsystems and should AvailabilityMacros.h be included on such as\nsystem an error will result. However, using the explicit value\nof 1060 (which is what MAC_OS_X_VERSION_10_6 is defined to) does\nnot solve the problem.\n\nThe changes that introduced stepping on MAC_OS_X_VERSION_MIN were\nmade in b195aa00 (git-compat-util: suppress unavoidable\nApple-specific deprecation warnings) to avoid deprecation\nwarnings.\n\nInstead of blindly setting MAC_OS_X_VERSION_MIN to 1060 change\nthe definition of DEPRECATED_ATTRIBUTE to empty to avoid the\nwarnings. This preserves any MAC_OS_X_VERSION_MIN_REQUIRED\nsetting while avoiding the warnings as intended by b195aa00.\n\nSigned-off-by: Kyle J. McKay \n---\n git-compat-util.h | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex eb9b0ff3..0efd32c4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -212,12 +212,15 @@ extern char *gitbasename(char *);\n #endif\n \n #ifndef NO_OPENSSL\n+#ifdef __APPLE__\n #define __AVAILABILITY_MACROS_USES_AVAILABILITY 0\n-#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6\n+#include \n+#undef DEPRECATED_ATTRIBUTE\n+#define DEPRECATED_ATTRIBUTE\n+#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n+#endif\n #include \n #include \n-#undef MAC_OS_X_VERSION_MIN_REQUIRED\n-#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n #ifdef NO_HMAC_CTX_CLEANUP\n #define HMAC_CTX_cleanup HMAC_cleanup\n #endif\n--\n"},{"id":"255668","messageId":"CAPig+cT0-ftZZyRORx-W2_Nit6XdgrpiyGS=pRjGtHoz1jW+Kg@mail.gmail.com","threadId":"38514","inReplyTo":"f2a35ed9fba20d48ef5736ee4564101@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-06T10:00:59Z","receivedAt":"2015-02-06T10:00:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 6, 2015 at 4:35 AM, Kyle J. McKay wrote:\n> MAC_OS_X_VERSION_MIN_REQUIRED may be defined by the builder to a\n> specific version in order to produce compatible binaries for a\n> particular system. Blindly defining it to MAC_OS_X_VERSION_10_6\n> is bad.\n>\n> Additionally MAC_OS_X_VERSION_10_6 will not be defined on older\n> systems and should AvailabilityMacros.h be included on such as\n> system an error will result. However, using the explicit value\n> of 1060 (which is what MAC_OS_X_VERSION_10_6 is defined to) does\n> not solve the problem.\n>\n> The changes that introduced stepping on MAC_OS_X_VERSION_MIN were\n> made in b195aa00 (git-compat-util: suppress unavoidable\n> Apple-specific deprecation warnings) to avoid deprecation\n> warnings.\n>\n> Instead of blindly setting MAC_OS_X_VERSION_MIN to 1060 change\n> the definition of DEPRECATED_ATTRIBUTE to empty to avoid the\n> warnings. This preserves any MAC_OS_X_VERSION_MIN_REQUIRED\n> setting while avoiding the warnings as intended by b195aa00.\n>\n> Signed-off-by: Kyle J. McKay \n\nTested on 10.10.2 (Yosemite) and 10.5.8 (Snow Leopard).\n\nReviewed-by: Eric Sunshine \n\nMore below...\n\n> ---\n> git-compat-util.h | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index eb9b0ff3..0efd32c4 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -212,12 +212,15 @@ extern char *gitbasename(char *);\n> #endif\n>\n> #ifndef NO_OPENSSL\n> +#ifdef __APPLE__\n> #define __AVAILABILITY_MACROS_USES_AVAILABILITY 0\n> -#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6\n> +#include \n> +#undef DEPRECATED_ATTRIBUTE\n> +#define DEPRECATED_ATTRIBUTE\n> +#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n> +#endif\n> #include \n> #include \n> -#undef MAC_OS_X_VERSION_MIN_REQUIRED\n> -#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n\nDEPRECATED_ATTRIBUTE is a fairly generic name. Do we want to be extra\ncareful and #undef it here to avoid potential unintended interactions\nwith other #includes (Apple or not)?\n\n #ifdef __APPLE__\n #undef DEPRECATED_ATTRIBUTE\n #endif\n\nOn the other hand, we've already mucked with it, so #undef may be superfluous.\n\n> #ifdef NO_HMAC_CTX_CLEANUP\n> #define HMAC_CTX_cleanup HMAC_cleanup\n> #endif\n> --\n"},{"id":"255675","messageId":"28F603A7-610B-4711-9D2F-CD9866C96A74@gmail.com","threadId":"38514","inReplyTo":"CAPig+cT0-ftZZyRORx-W2_Nit6XdgrpiyGS=pRjGtHoz1jW+Kg@mail.gmail.com","subject":"Re: [PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-06T19:47:51Z","receivedAt":"2015-02-06T19:47:51Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 6, 2015, at 02:00, Eric Sunshine wrote:\n> On Fri, Feb 6, 2015 at 4:35 AM, Kyle J. McKay \n> wrote:\n>> #ifndef NO_OPENSSL\n>> +#ifdef __APPLE__\n>> #define __AVAILABILITY_MACROS_USES_AVAILABILITY 0\n>> -#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6\n>> +#include \n>> +#undef DEPRECATED_ATTRIBUTE\n>> +#define DEPRECATED_ATTRIBUTE\n>> +#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n>> +#endif\n>> #include \n>> #include \n>> -#undef MAC_OS_X_VERSION_MIN_REQUIRED\n>> -#undef __AVAILABILITY_MACROS_USES_AVAILABILITY\n>\n> DEPRECATED_ATTRIBUTE is a fairly generic name. Do we want to be extra\n> careful and #undef it here to avoid potential unintended interactions\n> with other #includes (Apple or not)?\n\nThe new patch only mucks with it when we have #ifdef __APPLE__ and \npretty much any apple code is going to end up including the \navailability stuff sooner or later which manipulates \nDEPRECATED_ATTRIBUTE. Essentially that makes \"DEPRECATED_ATTRIBUTE\" a \nreserved macro name on __APPLE__.\n\n>\n> #ifdef __APPLE__\n> #undef DEPRECATED_ATTRIBUTE\n> #endif\n\nIf we do that, won't the compiler then helpfully supply a \"0\" when the \nmacro is used? Or is that only when an undefined macro is used inside \nan #if or #elif ?\n\nThat would break all later declarations that use it.\n\n> On the other hand, we've already mucked with it, so #undef may be \n> superfluous.\n\nActually I just tested it. If we #undef it we could end up producing \nthese:\n\n error: syntax error before ‘DEPRECATED_ATTRIBUTE’\n\nSo I think it needs to stay #define'd to nothing to be safe in case \nanything later on ends up including stuff that uses it. The worst \nthat happens is you don't see some deprecation warnings that you \notherwise would. It won't make any functions available that shouldn't \nbe or make unavailable functions that should be.\n\n-Kyle"},{"id":"255676","messageId":"xmqq7fvuhkl3.fsf@gitster.dls.corp.google.com","threadId":"38514","inReplyTo":"28F603A7-610B-4711-9D2F-CD9866C96A74@gmail.com","subject":"Re: [PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-06T20:05:28Z","receivedAt":"2015-02-06T20:05:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> Actually I just tested it. If we #undef it we could end up producing \n> these:\n>\n> error: syntax error before DEPRECATED_ATTRIBUTE\n>\n> So I think it needs to stay #define'd to nothing to be safe in case \n> anything later on ends up including stuff that uses it.\n\nDoesn't the fact that your test failed indicates that it is not jsut\n\"to be safe in case\" but is required for correctness?\n\nThe first hit for \"MAC_OS_X_VERSION_MIN_REQUIRED\" was this:\n\n https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/AvailabilityMacros.h\n\nwhich marks quite a many macros to that value. I do not know what\nchanges they make to openssl/*.h (which is included just after the\nabove header is included, but I would imagine that is where the\nAVAILABLE_MAC_OS_X_VERSION_XXX_AND_LATER_BUT_DEPRECATED_IN_MAC_OS_VERSION_YYY\nmacros are checked and annoying warnings that are being squelched by\nthe previous change are given?\n"},{"id":"255680","messageId":"1FC680E6-2258-4DBB-802C-6FB62D29C378@gmail.com","threadId":"38514","inReplyTo":"xmqq7fvuhkl3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-06T20:43:26Z","receivedAt":"2015-02-06T20:43:26Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 6, 2015, at 12:05, Junio C Hamano wrote:\n\n> \"Kyle J. McKay\" writes:\n>\n>> Actually I just tested it. If we #undef it we could end up producing\n>> these:\n>>\n>> error: syntax error before DEPRECATED_ATTRIBUTE\n>>\n>> So I think it needs to stay #define'd to nothing to be safe in case\n>> anything later on ends up including stuff that uses it.\n>\n> Doesn't the fact that your test failed indicates that it is not jsut\n> \"to be safe in case\" but is required for correctness?\n>\n> The first hit for \"MAC_OS_X_VERSION_MIN_REQUIRED\" was this:\n>\n> https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/AvailabilityMacros.h\n>\n> which marks quite a many macros to that value. I do not know what\n> changes they make to openssl/*.h (which is included just after the\n> above header is included, but I would imagine that is where the\n> AVAILABLE_MAC_OS_X_VERSION_XXX_AND_LATER_BUT_DEPRECATED_IN_MAC_OS_VERSION_YYY\n> macros are checked and annoying warnings that are being squelched by\n> the previous change are given?\n\nYes.\n\nAlthough Eric didn't specify exactly where when he suggested adding \nthis:\n\nOn Feb 6, 2015, at 02:00, Eric Sunshine wrote:\n> #ifdef __APPLE__\n> #undef DEPRECATED_ATTRIBUTE\n> #endif\n\n\nI took the suggestion to be after the openssl/*.h headers are included \nwhich would avoid the error of having DEPRECATED_ATTRIBUTE be #undef'd \nfor them. But, even math.h can end up including AvailabilityMacros.h, \nso I think #undef'ing DEPRECATED_ATTRIBUTE after the openssl/*.h \nheaders are included would be unsafe in general. While we might \nhappen to get away with that today, if say compat/apple-common- \ncrypto.h changes in the future (or for that matter any sequence of \nincludes in other files or any headers in the Apple SDK) we could \nstart seeing the error.\n\nTLDR; yeah, DEPRECATED_ATTRIBUTE needs to remain defined to nothing.\n\n-Kyle\n"},{"id":"255682","messageId":"CAPig+cTRC5AFKVYx4bwtY1-vYe38KNU+OdYgxmCm-LN8P+4cYg@mail.gmail.com","threadId":"38514","inReplyTo":"1FC680E6-2258-4DBB-802C-6FB62D29C378@gmail.com","subject":"Re: [PATCH] git-compat-util: do not step on MAC_OS_X_VERSION_MIN_REQUIRED","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-06T22:01:28Z","receivedAt":"2015-02-06T22:01:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 6, 2015 at 3:43 PM, Kyle J. McKay wrote:\n> On Feb 6, 2015, at 12:05, Junio C Hamano wrote:\n>> \"Kyle J. McKay\" writes:\n>>> So I think it needs to stay #define'd to nothing to be safe in case\n>>> anything later on ends up including stuff that uses it.\n>>\n>> Doesn't the fact that your test failed indicates that it is not jsut\n>> \"to be safe in case\" but is required for correctness?\n>>\n>> [...] I do not know what\n>> changes they make to openssl/*.h (which is included just after the\n>> above header is included, but I would imagine that is where the\n>> AVAILABLE_MAC_OS_X_VERSION_XXX_AND_LATER_BUT_DEPRECATED_IN_MAC_OS_VERSION_YYY\n>> macros are checked and annoying warnings that are being squelched by\n>> the previous change are given?\n>\n> Yes.\n>\n> Although Eric didn't specify exactly where when he suggested adding this:\n>\n> On Feb 6, 2015, at 02:00, Eric Sunshine wrote:\n>>\n>> #ifdef __APPLE__\n>> #undef DEPRECATED_ATTRIBUTE\n>> #endif\n>\n> I took the suggestion to be after the openssl/*.h headers are included which\n> would avoid the error of having DEPRECATED_ATTRIBUTE be #undef'd for them.\n> But, even math.h can end up including AvailabilityMacros.h, so I think\n> #undef'ing DEPRECATED_ATTRIBUTE after the openssl/*.h headers are included\n> would be unsafe in general. While we might happen to get away with that\n> today, if say compat/apple-common-crypto.h changes in the future (or for\n> that matter any sequence of includes in other files or any headers in the\n> Apple SDK) we could start seeing the error.\n>\n> TLDR; yeah, DEPRECATED_ATTRIBUTE needs to remain defined to nothing.\n\nI agree with this analysis. An alternative would be to revert b195aa00\n(and not apply this patch), but then we risk having legitimate\nGit-related compilation warnings lost in the noise of the useless\nApple deprecation warnings. Given the glacial pace at which Apple\nheaders changes, and considering the rapid pace of change in Git, it\nstill seems the lesser evil to suppress the useless warnings Apple\nthrust upon us when they deprecated OpenSSL in its entirety without\nproviding replacements. It's unfortunate that the DEPRECATED_ATTRIBUTE\n#define will bleed beyond the OpenSSL #includes and potentially allow\nus to miss some future Apple deprecations, however, given the\nshortcomings of b195aa00, the proposed patch seems to be the best we\ncan do.\n"}]}